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(at)gmail(dot)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