Re: Allow pg_archivecleanup to remove backup history files - Mailing list pgsql-hackers

From Michael Paquier
Subject Re: Allow pg_archivecleanup to remove backup history files
Date
Msg-id ZG1nq13v411y4TFL@paquier.xyz
Whole thread Raw
In response to Re: Allow pg_archivecleanup to remove backup history files  (torikoshia <torikoshia@oss.nttdata.com>)
Responses Re: Allow pg_archivecleanup to remove backup history files
List pgsql-hackers
On Mon, May 22, 2023 at 06:24:49PM +0900, torikoshia wrote:
> Thanks for your advice, attached patches.

0001 looks OK, thanks!

+    Remove files including backup history file.

This could be reworded as "Remove backup history files.", I assume.

+         Note that when <replaceable>oldestkeptwalfile</replaceable> is a backup history file,
+         specified file is kept and only preceding WAL files and backup history files are removed.

The same thing is described at the bottom of the documentation, so it
does not seem necessary here.

-       printf(_("  -d, --debug               generate debug output (verbose mode)\n"));
-       printf(_("  -n, --dry-run             dry run, show the names of the files that would be removed\n"));
-       printf(_("  -V, --version             output version information, then exit\n"));
-       printf(_("  -x --strip-extension=EXT  clean up files if they have this extension\n"));
-       printf(_("  -?, --help                show this help, then exit\n"));
+       printf(_("  -d, --debug                 generate debug output (verbose mode)\n"));
+       printf(_("  -n, --dry-run               dry run, show the names of the files that would be removed\n"));
+       printf(_("  -b, --clean-backup-history  clean up files including backup history files\n"));
+       printf(_("  -V, --version               output version information, then exit\n"));
+       printf(_("  -x --strip-extension=EXT    clean up files if they have this extension\n"));
+       printf(_("  -?, --help                  show this help, then exit\n"));

Perhaps this indentation had better be adjusted in 0001, no big deal
either way.

-       ok(!-f "$tempdir/$walfiles[0]",
-               "$test_name: first older WAL file was cleaned up");
+       if (grep {$_ eq '-x.gz'} @options) {
+               ok(!-f "$tempdir/$walfiles[0]",
+                       "$test_name: first older WAL file with .gz was cleaned up");
+       } else {
+               ok(-f "$tempdir/$walfiles[0]",
+                       "$test_name: first older WAL file with .gz was not cleaned up");
+       }
[...]
-       ok(-f "$tempdir/$walfiles[2]",
-               "$test_name: restartfile was not cleaned up");
+       if (grep {$_ eq '-b'} @options) {
+               ok(!-f "$tempdir/$walfiles[2]",
+                       "$test_name: Backup history file was cleaned up");
+       } else {
+               ok(-f "$tempdir/$walfiles[2]",
+                       "$test_name: Backup history file was not cleaned up");
+       }

That's over-complicated, caused by the fact that all the tests rely on
the same list of WAL files to create, aka @walfiles defined at the
beginning of the script.  Wouldn't it be cleaner to handle the fake
file creations with more than one global list, before each command
run?  The list of files to be created could just be passed down as an
argument of run_check(), for example, before cleaning up the contents
for the next command.
--
Michael

Attachment

pgsql-hackers by date:

Previous
From: Michael Paquier
Date:
Subject: Re: unnecessary #include "pg_getopt.h"?
Next
From: Michael Paquier
Date:
Subject: Re: Docs: Encourage strong server verification with SCRAM