Re: New WAL record to detect the checkpoint redo location

On Fri, Oct 13, 2023 at 3:29 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> Now looking at 0002, where you should be careful about the code
> indentation or koel will complain.

Fixed in the attached version.

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

Cool.

> 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.

I don't quite understand what you're saying here. If you're suggesting
a potential improvement, can you be a bit more clear and explicit
about what the suggestion is?

> + /*
> + * 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.

I have adjusted the comment in CreateCheckPoint to hopefully address
this concern. I don't understand what you mean about
ReserveXLogSwitch(), though.

--
Robert Haas
EDB: http://www.enterprisedb.com