Re: pg_rewind WAL segments deletion pitfall

Hi Sutou,

Thank you for picking it up!

On Fri, 12 Jul 2024 at 09:24, Sutou Kouhei <kou@clear-code.com> wrote:

Here are my review comments:
>
> @@ -217,6 +221,26 @@ findLastCheckpoint(const char *datadir, XLogRecPtr
> forkptr, int tliIndex,
> +                       char            xlogfname[MAXFNAMELEN];
> +
> +                       tli = xlogreader->seg.ws_tli;
> +                       segno = xlogreader->seg.ws_segno;
> +
> +                       snprintf(xlogfname, MAXPGPATH, XLOGDIR "/");
> +                       XLogFileName(xlogfname + strlen(xlogfname),
> +                                                xlogreader->seg.ws_tli,
> +                                                xlogreader->seg.ws_segno,
> WalSegSz);
> +
> +                       /*
> +                        * Make sure pg_rewind doesn't remove this file,
> because it is
> +                        * required for postgres to start after rewind.
> +                        */
> +                       insert_keepwalhash_entry(xlogfname);
>
> MAXFNAMELEN is 64 and MAXPGPATH is 1024. strlen(XLOGDIR "/")
> is 7 because XLOGDIR is "pg_wal". So xlogfname has enough
> size but snprintf(xlogfname, MAXPGPATH) is wrong usage.
> (And XLogFileName() uses snprintf(xlogfname, MAXFNAMELEN)
> internally.)
>

Nice catch!

I don't think we need another buffer here, just need to use MAXFNAMELEN,
because strlen("pg_wal/$wal_filename") + 1 = 32 perfectly fits into 64
bytes.

The new version is attached.

Regards,
--
Alexander Kukushkin