Re: Allow pg_archivecleanup to remove backup history files - Mailing list pgsql-hackers

At Wed, 21 Jun 2023 23:41:33 +0900, torikoshia <torikoshia@oss.nttdata.com> wrote in 
> --v10-0002-Preliminary-refactoring-for-a-subsequent-patch.patch
> + * Also we skip backup history files when --clean-backup-history
> +        * is not specified.
> +        */
> + if (!IsXLogFileName(walfile) && !IsPartialXLogFileName(walfile))
> +           continue;
> 
> I think this comment should be located in 0003.

Auch! Right.

> Attached updated patches.

Thanks!


0001:

Looks good to me.

0002:

+         * Check file name.
+         *
+         * We skip files which are not WAL file or partial WAL file.

There's no need to spend this many lines describing this, and it's
suggested to avoid restating what the code does. I think a comment
might not be necessary. But if we decide to have one, it could look
like this:

>        /* we're only removing specific types of files */

Other than that, it looks good to me.


0003:
+       <para>
+         Remove backup history files.

I might be overthinking it, but I'd phrase it as, "Remove backup
history files *as well*.".  (The --help message describes the same
thing using "including".)



+         For details about backup history file, please refer to the <xref linkend="backup-base-backup"/>.

We usually write this as simple as "See <xref...> for details (of the
backup history files)" or "Refer to <xref..> for more information
(about the backup history files)." or such like... (I think)



+bool        cleanBackupHistory = false;    /* remove files including
+                                                 * backup history files */

The indentation appears to be inconsistent.


-         * 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.)
+         * Truncation is essentially harmless,  because we check the file
+         * format including the length immediately after this.
+         * (In principle, one could use a 1000-character additional_ext
+         * and get trouble.)
          */
         strlcpy(walfile, xlde->d_name, MAXPGPATH);
         TrimExtension(walfile, additional_ext);

The revised comment seems to drift from the original point. Except for
a potential exception by a too-long addition_ext, the original comment
asserted that the name truncation was safe since it wouldn't affect
the files we're removing. In other words, it asserted that the
filenames to be removed won't be truncated and any actual truncation
wouldn't lead to accidental deletions.

Hence, I think we should adjust the comment to maintain its original
point, and extend it to include backup history files. A possible
revision could be (very simple):

>         * Truncation is essentially harmless, because we skip names of length
>         * longer than the length of backup history file. (In principle, one
>         * could use a 1000-character additional_ext and get trouble.)


Regarding the TAP test, it checks that the --clean-backup-history does
indeed remove backup history files. However, it doesn't check that
this doesn't occur if the option isn't specified. Shouldn't we test
for the latter scenario as well?


+sub get_walfiles
+{
<snip..>
+
+create_files(get_walfiles(@walfiles_with_gz));

The new function get_walfiels() just increases the line count without
cutting any lines. The following changes are sufficient and easier to
read (at least for me).

>        open my $file, '>', "$tempdir/$fn->{name}";

>    foreach my $fn (map {$_->{name}} @walfiles_with_gz)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center