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(at)clear-code(dot)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