At Thu, 27 Jan 2022 10:47:20 -0800, Nathan Bossart <nathandboss...@gmail.com> wrote in > 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.
Putting aside the readyness of the patch, I think what the patch intends is to avoid starnge state transitions happen during end-of-recovery checkpoint. So, I'm confused... End-of-recovery checkpoint is requested as CHECKPOINT_WAIT, which seems to me to mean the state is always DB_IN_ARCHIVE_RECOVERY while the checkpoint is running? If correct, if server is killed druing the end-of-recovery checkpoint, the state stays DB_IN_ARCHIVE_RECOVERY instead of DB_SHUTDOWNING or DB_SHUTDOWNED. AFAICS there's no differece between the first two at next startup. I dont' think DB_SHUTDOWNED case is not worth considering here. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
