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