Re: Allow pg_archivecleanup to remove backup history files

On 2023-05-24 10:26, Michael Paquier wrote:
> 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

Thanks for reviewing!
Updated patches according to your comment.

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION