On Sun, Dec 11, 2011 at 9:52 AM, Gabriele Bartolini
<gabriele.bartolini@2ndquadrant.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
