On Thu, Jan 27, 2022 at 02:06:40PM +0900, Michael Paquier wrote: > So, I have been checking this idea in details, and spotted what looks > like one issue in CreateRestartPoint(), as of: > /* > * Update pg_control, using current time. Check that it still shows > * DB_IN_ARCHIVE_RECOVERY state and an older checkpoint, else do > nothing; > * this is a quick hack to make sure nothing really bad happens if > somehow > * we get here after the end-of-recovery checkpoint. > */ > LWLockAcquire(ControlFileLock, LW_EXCLUSIVE); > if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY && > ControlFile->checkPointCopy.redo < lastCheckPoint.redo) > > This change increases the window making this code path reachable if an > end-of-recovery checkpoint is triggered but not finished at the end of > recovery (possible of course at the end of crash recovery, but > DB_IN_ARCHIVE_RECOVERY is also possible when !IsUnderPostmaster with a > promotion request), before updating ControlFile->checkPointCopy at the > end of the checkpoint because the state could still be > DB_IN_ARCHIVE_RECOVERY. The window is wider the longer the > end-of-recovery checkpoint. And this would be the case of an instance > restarted, when a restart point is created.
I wonder if this is actually a problem in practice. IIUC all of the values updated in this block should be reset at the end of the end-of-recovery checkpoint. Is the intent of the quick hack to prevent those updates after an end-of-recovery checkpoint completes, or is it trying to block them after one begins? It looks like the control file was being updated to DB_SHUTDOWNING at the beginning of end-of-recovery checkpoints when that change was first introduced (2de48a8), so I agree that we'd better be careful with this change. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com/
