On Sun, Dec 11, 2011 at 9:52 AM, Gabriele Bartolini <gabriele.bartol...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers