Re: pg_rewind WAL segments deletion pitfall

At Wed, 28 Sep 2022 10:09:05 +0200, Polina Bungina <bungina(at)gmail(dot)com> wrote in
> On Tue, Sep 27, 2022 at 9:50 AM Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
> wrote:
>
> > Regarding the the patch, pg_rewind starts reading segments from the
> > divergence point back to the nearest checkpoint, then moves foward
> > during rewinding. So, the fact that SimpleXLogPageRead have read a
> > segment suggests that the segment is required during the next startup.
> > So I don't think we need to move around the keepWalSeg flag. All
> > files that are wanted while rewinding should be preserved
> > unconditionally.
> >
>
> I am probably not getting this right but as far as I see SimpleXLogPageRead
> is called at most 3 times during pg_rewind run:
> 1. From readOneRecord to determine the end-of-WAL on the target by reading
> the last shutdown checkpoint record/minRecoveryPoint on it
> 2. From findLastCheckpoint to find last common checkpoint (here it
> indeed reads all the segments that are required during the startup, hence
> the keepWalSeg flag set to true)
> 3. From extractPageMap to extract all the pages modified after the fork
> (here we also read all the segments that should be kept but also the ones
> further, until the target's end record. Doesn't seem we should
> unconditionally preserve them all).
> Am I missing something?

No. You're right. I have to admit that I was confused at the time X(,
sorry for that. Those extra files are I believe harmless but of
course it's preferable to avoid them. So the keepWalSeg is useful.

So the latest version become very similar to v1 in that the both have
keepWalSeg flag. The difference is the need of the file name hash. I
still think that it's better if we don't need the additional file
hash. If we move out the bare code in v2 added to
SimpleXLogPageRead(), then name it "preserve_file(char *filepath)",
the code become more easy to read.

+ if (private->keepWalSeg)
+ {
+ /* the caller told us to preserve this file for future use */
+ snprintf(xlogfpath, MAXPGPATH, XLOGDIR "/%s", xlogfname);
+ preserve_file(xlogfpath);
+ }

Instead, I think we should add a comment here:

@@ -192,6 +195,7 @@ findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, int tliIndex,

> private.tliIndex = tliIndex;
> private.restoreCommand = restoreCommand;
++ /*
++ * WAL files read during searching for the last checkpoint are required
++ * by the next startup recovery of the target cluster.
++ */
>+ private.keepWalSeg = true;

What do you think about the above?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center