public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: Kyotaro Horiguchi <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: Possible corruption by CreateRestartPoint at promotion
Date: Thu, 28 Apr 2022 15:49:42 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<20220427180945.GA3222843@nathanxps13>
<[email protected]>
<[email protected]>
On Thu, Apr 28, 2022 at 11:43:57AM +0900, Kyotaro Horiguchi wrote:
> At Thu, 28 Apr 2022 09:12:13 +0900, Michael Paquier <[email protected]> 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
Attachments:
[application/pgp-signature] signature.asc (833B, ../[email protected]/2-signature.asc)
download
view thread (12+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Possible corruption by CreateRestartPoint at promotion
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox