At Thu, 25 May 2023 23:51:18 +0900, torikoshia <torikos...@oss.nttdata.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
