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
signature.asc
Description: PGP signature
