On Fri, Oct 13, 2023 at 3:29 AM Michael Paquier <mich...@paquier.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
v9-0001-During-online-checkpoints-insert-XLOG_CHECKPOINT_.patch
Description: Binary data
