Re: New WAL record to detect the checkpoint redo location

On Tue, Oct 10, 2023 at 02:43:34PM -0400, Robert Haas wrote:
> Here's a new patch set. I think I've incorporated the performance
> fixes that you've suggested so far into this version. I also adjusted
> a couple of other things:

Now looking at 0002, where you should be careful about the code
indentation or koel will complain.

> - After further study of a point previously raised by Amit, I adjusted
> CreateCheckPoint slightly to call WALInsertLockAcquireExclusive
> significantly later than before. I think that there's no real reason
> to do it so early and that the current coding is probably just a
> historical leftover, but it would be good to have some review here.

This makes the new code call LocalSetXLogInsertAllowed() and what we
set for checkPoint.PrevTimeLineID after taking the insertion locks,
which should be OK.

> - I added a cross-check that when starting redo from a checkpoint
> whose redo pointer points to an earlier LSN that the checkpoint
> itself, the record we read from that LSN must an XLOG_CHECKPOINT_REDO
> record.

I've mentioned as well a test in pg_walinspect after one of the
checkpoints generated there, but what you do here is enough for the
online case.

+       /*
+        * XLogInsertRecord will have updated RedoRecPtr, but we need to copy
+        * that into the record that will be inserted when the checkpoint is
+        * complete.
+        */
+       checkPoint.redo = RedoRecPtr;

For online checkpoints, a very important point is that
XLogCtl->Insert.RedoRecPtr is also updated in XLogInsertRecord().
Perhaps that's worth an addition?  I was a bit confused first that we
do the following for shutdown checkpoints:
RedoRecPtr = XLogCtl->Insert.RedoRecPtr = checkPoint.redo;

Then repeat this pattern for non-shutdown checkpoints a few lines down
without touching the copy of the redo LSN in XLogCtl->Insert, because
of course we don't hold the WAL insert locks in an exclusive fashion
here:
checkPoint.redo = RedoRecPtr;

My point is that this is not only about RedoRecPtr, but also about
XLogCtl->Insert.RedoRecPtr here.  The comment in ReserveXLogSwitch()
says that.
--
Michael