Re: Allow pg_archivecleanup to remove backup history files

At Thu, 25 May 2023 23:51:18 +0900, torikoshia <torikoshia(at)oss(dot)nttdata(dot)com> wrote in
> Updated patches according to your comment.

+ printf(_(" -x --strip-extension=EXT clean up files if they have this extension\n"));

Perhaps a comma is needed after "-x". 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".

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.

+ 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)))

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..

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center