Re: Is it correct to update db state in control file as "shutting down" during end-of-recovery checkpoint?

At Thu, 27 Jan 2022 10:47:20 -0800, Nathan Bossart <nathandbossart@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