Re: Possible corruption by CreateRestartPoint at promotion

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
Cc: nathandbossart(at)gmail(dot)com, pgsql-hackers(at)lists(dot)postgresql(dot)org, masao(dot)fujii(at)oss(dot)nttdata(dot)com
Subject: Re: Possible corruption by CreateRestartPoint at promotion
Date: 2022-04-28 06:49:42
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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(at)paquier(dot)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.

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Pavel Stehule 2022-04-28 08:16:41 json_query - redundant result
Previous Message Laurenz Albe 2022-04-28 06:48:36 Re: Multi-Master Logical Replication