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.bartolini@2ndQuadrant.it | www.2ndQuadrant.it
Attachments:
add-dry-run-option-pgarchivecleanup.patchtext/plain; name=add-dry-run-option-pgarchivecleanup.patch; x-mac-creator=0; x-mac-type=0Download
*** a/contrib/pg_archivecleanup/pg_archivecleanup.c
--- b/contrib/pg_archivecleanup/pg_archivecleanup.c
***************
*** 36,41 **** const char *progname;
--- 36,42 ----
/* Options and defaults */
bool debug = false; /* are we debugging? */
+ bool dryrun = false; /* are we performing a dry-run operation? */
char *archiveLocation; /* where to find the archive? */
char *restartWALFileName; /* the file from which we can restart restore */
***************
*** 123,128 **** CleanupPriorWALFiles(void)
--- 124,136 ----
fprintf(stderr, "%s: removing file \"%s\"\n",
progname, WALFilePath);
+ if (dryrun)
+ {
+ /* Prints the name of the file and
+ * skips the actual removal of the file */
+ fprintf(stdout, "%s\n", WALFilePath);
+ continue;
+ }
rc = unlink(WALFilePath);
if (rc != 0)
{
***************
*** 205,210 **** usage(void)
--- 213,219 ----
printf(" %s [OPTION]... ARCHIVELOCATION OLDESTKEPTWALFILE\n", progname);
printf("\nOptions:\n");
printf(" -d generates debug output (verbose mode)\n");
+ printf(" -n shows the names of the files that would have been removed (dry-run)\n");
printf(" --help show this help, then exit\n");
printf(" --version output version information, then exit\n");
printf("\n"
***************
*** 241,253 **** main(int argc, char **argv)
}
}
! while ((c = getopt(argc, argv, "d")) != -1)
{
switch (c)
{
case 'd': /* Debug mode */
debug = true;
break;
default:
fprintf(stderr, "Try \"%s --help\" for more information.\n", progname);
exit(2);
--- 250,265 ----
}
}
! while ((c = getopt(argc, argv, "dn")) != -1)
{
switch (c)
{
case 'd': /* Debug mode */
debug = true;
break;
+ case 'n': /* Dry-Run mode */
+ dryrun = true;
+ break;
default:
fprintf(stderr, "Try \"%s --help\" for more information.\n", progname);
exit(2);
*** a/doc/src/sgml/pgarchivecleanup.sgml
--- b/doc/src/sgml/pgarchivecleanup.sgml
***************
*** 98,103 **** pg_archivecleanup: removing file "archive/00000001000000370000000E"
--- 98,112 ----
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>-n</option></term>
+ <listitem>
+ <para>
+ Print the names of the files that would have been removed on <filename>stdout</> (performs a dry run).
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</para>
On Sun, Dec 11, 2011 at 15:52, 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.
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.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
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.bartolini@2ndQuadrant.it | www.2ndQuadrant.it
On Sun, Jan 15, 2012 at 3:02 PM, Gabriele Bartolini
<gabriele.bartolini@2ndquadrant.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@gmail.com> wrote:
On Sun, Jan 15, 2012 at 3:02 PM, Gabriele Bartolini
<gabriele.bartolini@2ndquadrant.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@gmail.com> wrote:
On Sun, Jan 15, 2012 at 5:05 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote:
On Sun, Jan 15, 2012 at 3:02 PM, Gabriele Bartolini
<gabriele.bartolini@2ndquadrant.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@gmail.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@commandprompt.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.bartolini@2ndQuadrant.it | www.2ndQuadrant.it
Attachments:
add-dry-run-option-pgarchivecleanup-v2.patchtext/plain; name=add-dry-run-option-pgarchivecleanup-v2.patch; x-mac-creator=0; x-mac-type=0Download
*** a/contrib/pg_archivecleanup/pg_archivecleanup.c
--- b/contrib/pg_archivecleanup/pg_archivecleanup.c
***************
*** 36,41 **** const char *progname;
--- 36,42 ----
/* Options and defaults */
bool debug = false; /* are we debugging? */
+ bool dryrun = false; /* are we performing a dry-run operation? */
char *archiveLocation; /* where to find the archive? */
char *restartWALFileName; /* the file from which we can restart restore */
***************
*** 119,124 **** CleanupPriorWALFiles(void)
--- 120,137 ----
{
snprintf(WALFilePath, MAXPGPATH, "%s/%s",
archiveLocation, xlde->d_name);
+
+ if (dryrun)
+ {
+ /* Prints the name of the file and
+ * skips the actual removal of the file */
+ fprintf(stdout, "%s\n", WALFilePath);
+ if (debug)
+ fprintf(stderr, "%s: dry-run mode suggests removing file \"%s\"\n",
+ progname, WALFilePath);
+ continue;
+ }
+
if (debug)
fprintf(stderr, "%s: removing file \"%s\"\n",
progname, WALFilePath);
***************
*** 205,210 **** usage(void)
--- 218,224 ----
printf(" %s [OPTION]... ARCHIVELOCATION OLDESTKEPTWALFILE\n", progname);
printf("\nOptions:\n");
printf(" -d generates debug output (verbose mode)\n");
+ printf(" -n shows the names of the files that would have been removed (dry-run)\n");
printf(" --help show this help, then exit\n");
printf(" --version output version information, then exit\n");
printf("\n"
***************
*** 241,253 **** main(int argc, char **argv)
}
}
! while ((c = getopt(argc, argv, "d")) != -1)
{
switch (c)
{
case 'd': /* Debug mode */
debug = true;
break;
default:
fprintf(stderr, "Try \"%s --help\" for more information.\n", progname);
exit(2);
--- 255,270 ----
}
}
! while ((c = getopt(argc, argv, "dn")) != -1)
{
switch (c)
{
case 'd': /* Debug mode */
debug = true;
break;
+ case 'n': /* Dry-Run mode */
+ dryrun = true;
+ break;
default:
fprintf(stderr, "Try \"%s --help\" for more information.\n", progname);
exit(2);
*** a/doc/src/sgml/pgarchivecleanup.sgml
--- b/doc/src/sgml/pgarchivecleanup.sgml
***************
*** 98,103 **** pg_archivecleanup: removing file "archive/00000001000000370000000E"
--- 98,112 ----
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>-n</option></term>
+ <listitem>
+ <para>
+ Print the names of the files that would have been removed on <filename>stdout</> (performs a dry run).
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</para>
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.bartolini@2ndQuadrant.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@commandprompt.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.bartolini@2ndquadrant.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.
--
Álvaro Herrera <alvherre@commandprompt.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.
Beautiful! :)
Thank you very much.
Ciao,
Gabriele
--
Gabriele Bartolini - 2ndQuadrant Italia
PostgreSQL Training, Services and Support
gabriele.bartolini@2ndQuadrant.it | www.2ndQuadrant.it