Re: Possible corruption by CreateRestartPoint at promotion

On Thu, Apr 28, 2022 at 11:43:57AM +0900, Kyotaro Horiguchi wrote:
> At Thu, 28 Apr 2022 09:12:13 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
>> On Wed, Apr 27, 2022 at 11:09:45AM -0700, Nathan Bossart wrote:
>>> On Wed, Apr 27, 2022 at 02:16:01PM +0900, Michael Paquier wrote:
>>>> -   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.
>> 
>> Sure.  Fine by me to play it safe.
> 
> Why do we consider concurrent check/restart points here while we don't
> consider the same for ControlFile->checkPointCopy?

I am not sure what you mean here.  FWIW, I am translating the
suggestion of Nathan to split the existing check in
CreateRestartPoint() that we are discussing here into two if blocks,
instead of just one:
- Move the update of checkPoint and checkPointCopy into its own if
block, controlled only by the check on
(ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
- Keep the code updating minRecoveryPoint and minRecoveryPointTLI
mostly as-is, but just do the update when the control file state is
DB_IN_ARCHIVE_RECOVERY.  Of course we need to keep the check on
(minRecoveryPoint < lastCheckPointEndPtr).

v5 is mostly doing that.
--
Michael