Re: Allow pg_archivecleanup to remove backup history files

On 2023-05-26 10:07, Kyotaro Horiguchi wrote:
Thanks for your review!

> + printf(_(" -x --strip-extension=EXT clean up files if they have
> this extension\n"));
>
> Perhaps a comma is needed after "-x".

That's an oversight. Modified it.

> The apparent inconsistency
> between the long name and the description is perplexing. I think it
> might be more suitable to reword the descrition to "ignore this
> extension while identifying files for cleanup" or something along
> those lines..., and then name the option as "--ignore-extension".

I used 'strip' since it is used in the manual as below:

| -x extension
| Provide an extension that will be stripped from all file names before
deciding if they should be deleted

I think using the same verb both in long name option and in the manual
is natural.
How about something like this?

| printf(_(" -x, --strip-extension=EXT strip this extention before
identifying files fo clean up\n"));

> The patch leaves the code.
>
>> * Truncation is essentially harmless, because we skip names of
>> * length other than XLOG_FNAME_LEN. (In principle, one could use
>> * a 1000-character additional_ext and get trouble.)
>> */
>> strlcpy(walfile, xlde->d_name, MAXPGPATH);
>> TrimExtension(walfile, additional_ext);
>
> The comment is no longer correct about the file name length.

Yeah.
considering parital WAL, it would be not correct even before applying
the patch.
I modifiied the comments as below:

| * Truncation is essentially harmless, because we check the file
| * format including the length immediately after this.

> + if (!IsXLogFileName(walfile) && !IsPartialXLogFileName(walfile) &&
> + (!cleanBackupHistory || !IsBackupHistoryFileName(walfile)))
> + continue;
>
> I'm not certain about the others, but I see this a tad tricky to
> understand at first glance. Here's how I would phrase it. (A nearby
> comment might require a tweak if we go ahead with this change.)
>
> if (!(IsXLogFileName(walfile) || IsPartialXLogFileName(walfile) ||
> (cleanBackupHistory && IsBackupHistoryFileName(walfile))))
>
> or
>
> if (!IsXLogFileName(walfile) && !IsPartialXLogFileName(walfile) &&
> !(cleanBackupHistory && IsBackupHistoryFileName(walfile)))

Thanks for the suggestion, I used the second one.

> By the way, this patch reduces the nesting level. If we go that
> direction, the following structure could be reworked similarly, and I
> believe it results in a more standard structure.
>
> if ((xldir = opendir(archiveLocation)) != NULL)
> {
> ...
> }
> else
> pg_fatal("could not open archive location..

Agreed. Attached 0003 patch for this.

On 2023-05-26 14:19, Michael Paquier wrote:
> On Thu, May 25, 2023 at 11:51:18PM +0900, torikoshia wrote:
>> Updated patches according to your comment.
>
> - ok(!-f "$tempdir/$walfiles[1]",
> - "$test_name: second older WAL file was cleaned up");
> - ok(-f "$tempdir/$walfiles[2]",
> + ok(!-f "$tempdir/@$walfiles[1]",
> + "$test_name: second older WAL/backup history file was cleaned
> up");
> + ok(-f "$tempdir/@$walfiles[2]",
>
> This is still a bit confusing, because as designed the test has a
> dependency on the number of elements present in the list, and the
> description of the test may not refer to what's actually used (the
> second element of each list is either a WAL segment or a backup
> history file). I think that I would just rewrite that so as we have a
> list of WAL segments in an array with the expected result associated
> to each one of them. Basically, something like that:
> my @wal_segments = (
> { name => "SEGMENT1", present => 0 },
> { name => "BACKUPFILE1", present => 1 },
> { name => "SEGMENT2", present => 0 });
>
> Then the last part of run_check() would loop through all the elements
> listed.

Thanks.
Update the patch according to the advice.
I also changed the parameter of run_check() from specifying extension of
oldestkeptwalfile to oldestkeptwalfile itself.

--
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION