Re: Possible corruption by CreateRestartPoint at promotion

On Wed, Apr 27, 2022 at 02:16:01PM +0900, Michael Paquier wrote:
> On Tue, Apr 26, 2022 at 08:26:09PM -0700, Nathan Bossart wrote:
>> On Wed, Apr 27, 2022 at 10:43:53AM +0900, Kyotaro Horiguchi wrote:
>>> +        ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
>>> +        ControlFile->minRecoveryPointTLI = 0;
>>> +
>>> +        /* also update local copy */
>>> +        LocalMinRecoveryPoint = InvalidXLogRecPtr;
>>> +        LocalMinRecoveryPointTLI = 0;
>> 
>> Should this be handled by the code that changes the control file state to
>> DB_IN_PRODUCTION instead?  It looks like this is ordinarily done in the
>> next checkpoint.  It's not clear to me why it is done this way.
> 
> Anyway, that would be the work of the end-of-recovery checkpoint
> requested at the end of StartupXLOG() once a promotion happens or of
> the checkpoint requested by PerformRecoveryXLogAction() in the second
> case, no?  So, I don't quite see why we need to update
> minRecoveryPoint and minRecoveryPointTLI in the control file here, as
> much as this does not have to be part of the end-of-recovery code
> that switches the control file to DB_IN_PRODUCTION.

+1. We probably don't need to reset minRecoveryPoint here.

> -   if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY &&
> -       ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
> -   {
> 7ff23c6 has removed the last call to CreateCheckpoint() outside the
> checkpointer, meaning that there is one less concurrent race to worry
> about, but I have to admit that this change, to update the control
> file's checkPoint and checkPointCopy even if we don't check after
> ControlFile->checkPointCopy.redo < lastCheckPoint.redo would make the
> code less robust in ~14.  So I am questioning whether a backpatch
> is actually worth the risk here.

IMO we should still check this before updating ControlFile to be safe.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com