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