On Thu, Apr 28, 2022 at 11:43:57AM +0900, Kyotaro Horiguchi wrote:
> At Thu, 28 Apr 2022 09:12:13 +0900, Michael Paquier <mich...@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
signature.asc
Description: PGP signature
