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

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/