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

On Wed, Jan 26, 2022 at 09:57:59AM +0900, Michael Paquier wrote:
> Yeah, that sounds like a good compromise. At least, I find the whole
> a bit easier to follow.

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.

> Heikki was planning to commit a large refactoring of xlog.c, so we'd
> better wait for that to happen before concluding here.

I have checked that as well, and both are independent.
--
Michael