Re: Dry-run mode for pg_archivecleanup

Hi guys,

I have added the '-n' option to pg_archivecleanup which performs a
dry-run and outputs the names of the files to be removed to stdout
(making possible to pass the list via pipe to another process).

Please find attached the small patch. I submit it to the CommitFest.

Thanks,
Gabriele

--
Gabriele Bartolini - 2ndQuadrant Italia
PostgreSQL Training, Services and Support
gabriele(dot)bartolini(at)2ndQuadrant(dot)it | www.2ndQuadrant.it

On Sun, Dec 11, 2011 at 15:52, Gabriele Bartolini
<gabriele(dot)bartolini(at)2ndquadrant(dot)it> wrote:
> Hi guys,
>
>  I have added the '-n' option to pg_archivecleanup which performs a dry-run
> and outputs the names of the files to be removed to stdout (making possible
> to pass the list via pipe to another process).
>
>  Please find attached the small patch. I submit it to the CommitFest.

Sounds like a useful feature. But you added it to the wrong commitfest
;) I've moved it to the proper one (2012-01).

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Sun, Dec 11, 2011 at 9:52 AM, Gabriele Bartolini
<gabriele(dot)bartolini(at)2ndquadrant(dot)it> wrote:
> Hi guys,
>
>  I have added the '-n' option to pg_archivecleanup which performs a dry-run
> and outputs the names of the files to be removed to stdout (making possible
> to pass the list via pipe to another process).
>
>  Please find attached the small patch. I submit it to the CommitFest.

Hi Gabriele,

I have signed on to review this patch for the 2012-01 CF. The patch
applies cleanly, includes the necessary documentation, and implements
a useful feature.

I think the actual debugging line:

+ fprintf(stdout, "%s\n", WALFilePath);

might need to be tweaked. First, it's printing to stdout, and I think
pg_archivecleanup intentionally sends all its output to stderr, so
that it may show up in the postmaster log. (I expect the dry-run mode
would often be used to test out an archive_cleanup_command, instead of
only in stand-alone mode, where stdout would be fine.)

Also, I think the actual message should be something a little more
descriptive than just the WALFilePath. In debug mode,
pg_archivecleanup prints out a message like:

fprintf(stderr, "%s: removing file \"%s\"\n",
progname, WALFilePath);

I think we'd want to print something similar, i.e. "would remove file ...".

Oh, and I think the "removing file... " debug message above should not
be printed in dryrun-mode, lest we confuse the admin.

Other than that, everything looks good.

Josh

Hi Josh,

Il 15/01/12 01:13, Josh Kupershmidt ha scritto:
> I have signed on to review this patch for the 2012-01 CF. The patch
> applies cleanly, includes the necessary documentation, and implements
> a useful feature.
Thank you for dedicating your time to this matter.
> I think the actual debugging line:
>
> + fprintf(stdout, "%s\n", WALFilePath);
My actual intention was to have the filename as output of the command,
in order to easily "pipe" it to another script. Hence my first choice
was to use the stdout channel, considering also that pg_archivecleanup
in dry-run mode is harmless and does not touch the content of the directory.
> Oh, and I think the "removing file... " debug message above should not
> be printed in dryrun-mode, lest we confuse the admin.
Yes, I agree with you here.

Let me know what you think about my reasons for the stdout channel, then
I will send v2.

Thanks again!

Ciao,
Gabriele

--
Gabriele Bartolini - 2ndQuadrant Italia
PostgreSQL Training, Services and Support
gabriele(dot)bartolini(at)2ndQuadrant(dot)it | www.2ndQuadrant.it

On Sun, Jan 15, 2012 at 3:02 PM, Gabriele Bartolini
<gabriele(dot)bartolini(at)2ndquadrant(dot)it> wrote:

> My actual intention was to have the filename as output of the command, in
> order to easily "pipe" it to another script. Hence my first choice was to
> use the stdout channel, considering also that pg_archivecleanup in dry-run
> mode is harmless and does not touch the content of the directory.

Oh, right - I should have re-read your initial email before diving
into the patch. That all makes sense given your intended purpose. I
guess your goal of constructing some simple way to pass the files
which would be removed on to another script is a little different than
what I initially thought the patch would be useful for, namely as a
testing/debugging aid for an admin.

Perhaps both goals could be met by making use of '--debug' together
with '--dry-run'. If they are both on, then an additional message like
"pg_archivecleanup: would remove file ... " would be printed to
stderr, along with just the filename printed to stdout you already
have.

Josh

On Sun, Jan 15, 2012 at 5:05 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
> On Sun, Jan 15, 2012 at 3:02 PM, Gabriele Bartolini
> <gabriele(dot)bartolini(at)2ndquadrant(dot)it> wrote:
>
>> My actual intention was to have the filename as output of the command, in
>> order to easily "pipe" it to another script. Hence my first choice was to
>> use the stdout channel, considering also that pg_archivecleanup in dry-run
>> mode is harmless and does not touch the content of the directory.
>
> Oh, right - I should have re-read your initial email before diving
> into the patch. That all makes sense given your intended purpose. I
> guess your goal of constructing some simple way to pass the files
> which would be removed on to another script is a little different than
> what I initially thought the patch would be useful for, namely as a
> testing/debugging aid for an admin.
>
> Perhaps both goals could be met by making use of '--debug' together
> with '--dry-run'. If they are both on, then an additional message like
> "pg_archivecleanup: would remove file ... " would be printed to
> stderr, along with just the filename printed to stdout you already
> have.

This email thread seems to have trailed off without reaching a
conclusion. The patch is marked as Waiting on Author in the
CommitFest application, but I'm not sure that's accurate. Can we try
to nail this down?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Jan 27, 2012 at 9:47 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sun, Jan 15, 2012 at 5:05 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
>> On Sun, Jan 15, 2012 at 3:02 PM, Gabriele Bartolini
>> <gabriele(dot)bartolini(at)2ndquadrant(dot)it> wrote:
>>
>>> My actual intention was to have the filename as output of the command, in
>>> order to easily "pipe" it to another script. Hence my first choice was to
>>> use the stdout channel, considering also that pg_archivecleanup in dry-run
>>> mode is harmless and does not touch the content of the directory.
>>
>> Oh, right - I should have re-read your initial email before diving
>> into the patch. That all makes sense given your intended purpose. I
>> guess your goal of constructing some simple way to pass the files
>> which would be removed on to another script is a little different than
>> what I initially thought the patch would be useful for, namely as a
>> testing/debugging aid for an admin.
>>
>> Perhaps both goals could be met by making use of '--debug' together
>> with '--dry-run'. If they are both on, then an additional message like
>> "pg_archivecleanup: would remove file ... " would be printed to
>> stderr, along with just the filename printed to stdout you already
>> have.
>
> This email thread seems to have trailed off without reaching a
> conclusion.  The patch is marked as Waiting on Author in the
> CommitFest application, but I'm not sure that's accurate.  Can we try
> to nail this down?

Perhaps my last email was a bit wordy. The only real change I am
suggesting for Gabriele's patch is that the message printed to stderr
when debug + dryrun are activated be changed to "would remove file
..." from "removing file", i.e around line 124:

if (debug)
fprintf(stderr, "%s: %s file \"%s\"\n",
progname, (dryrun ? "would remove" : "removing"),
WALFilePath);

Other than that little quibble, I thought the patch was fine.

Josh

Excerpts from Josh Kupershmidt's message of vie ene 27 19:43:51 -0300 2012:
> On Fri, Jan 27, 2012 at 9:47 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> > This email thread seems to have trailed off without reaching a
> > conclusion.  The patch is marked as Waiting on Author in the
> > CommitFest application, but I'm not sure that's accurate.  Can we try
> > to nail this down?
>
> Perhaps my last email was a bit wordy. The only real change I am
> suggesting for Gabriele's patch is that the message printed to stderr
> when debug + dryrun are activated be changed to "would remove file
> ..." from "removing file", i.e around line 124:
>
> if (debug)
> fprintf(stderr, "%s: %s file \"%s\"\n",
> progname, (dryrun ? "would remove" : "removing"),
> WALFilePath);
>
> Other than that little quibble, I thought the patch was fine.

If you do that, please make sure you use two complete separate strings
instead of building one from spare parts. This bit is missing the _()
stuff though.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hi Robert,

sorry for the delay.

Il 27/01/12 15:47, Robert Haas ha scritto:
> This email thread seems to have trailed off without reaching a
> conclusion. The patch is marked as Waiting on Author in the CommitFest
> application, but I'm not sure that's accurate. Can we try to nail this
> down?
Here is my final version which embeds comments from Josh. I have also
added debug information to be printed in case '-d' is given.

I will update the CommitFest website.

Thanks,
Gabriele

--
Gabriele Bartolini - 2ndQuadrant Italia
PostgreSQL Training, Services and Support
gabriele(dot)bartolini(at)2ndQuadrant(dot)it | www.2ndQuadrant.it

Hi Alvaro,

Il 27/01/12 23:55, Alvaro Herrera ha scritto:
> If you do that, please make sure you use two complete separate strings
> instead of building one from spare parts. This bit is missing the _()
> stuff though.
Currently pg_archivecleanup does not comply with localisation standards.
I can put this in the todo list however. Let me know what you think.

Cheers,
Gabriele

--
Gabriele Bartolini - 2ndQuadrant Italia
PostgreSQL Training, Services and Support
gabriele(dot)bartolini(at)2ndQuadrant(dot)it | www.2ndQuadrant.it

Excerpts from Gabriele Bartolini's message of mar ene 31 12:31:58 -0300 2012:
> Hi Alvaro,
>
> Il 27/01/12 23:55, Alvaro Herrera ha scritto:
> > If you do that, please make sure you use two complete separate strings
> > instead of building one from spare parts. This bit is missing the _()
> > stuff though.
> Currently pg_archivecleanup does not comply with localisation standards.
> I can put this in the todo list however. Let me know what you think.

Sorry, for some reason I thought this was pg_basebackup. Programs in
contrib don't have localisation support, so it doesn't matter much.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Tue, Jan 31, 2012 at 10:29 AM, Gabriele Bartolini
<gabriele(dot)bartolini(at)2ndquadrant(dot)it> wrote:

> Here is my final version which embeds comments from Josh. I have also added
> debug information to be printed in case '-d' is given.

Looks fine; will mark Ready For Committer.

Josh

Excerpts from Gabriele Bartolini's message of mar ene 31 12:29:51 -0300 2012:
> Hi Robert,
>
> sorry for the delay.
>
> Il 27/01/12 15:47, Robert Haas ha scritto:
> > This email thread seems to have trailed off without reaching a
> > conclusion. The patch is marked as Waiting on Author in the CommitFest
> > application, but I'm not sure that's accurate. Can we try to nail this
> > down?
> Here is my final version which embeds comments from Josh. I have also
> added debug information to be printed in case '-d' is given.

FWIW I committed this last week, though I changed the debug message
wording slightly -- hope that's OK; it can be adjusted of course.

http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b2e431a4db81a735d1474c4d1565a20b835878c9

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Ciao Alvaro,

Il 06/02/12 16:02, Alvaro Herrera ha scritto:
> FWIW I committed this last week, though I changed the debug message
> wording slightly -- hope that's OK; it can be adjusted of course.
>
> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b2e431a4db81a735d1474c4d1565a20b835878c9
>

Beautiful! :)

Thank you very much.

Ciao,
Gabriele

--
Gabriele Bartolini - 2ndQuadrant Italia
PostgreSQL Training, Services and Support
gabriele(dot)bartolini(at)2ndQuadrant(dot)it | www.2ndQuadrant.it