From: Nathan Bossart <nathandbossart@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Cc: masao.fujii@oss.nttdata.com
Subject: Re: Possible corruption by CreateRestartPoint at promotion
Date: Wed, 27 Apr 2022 11:09:45 -0700
Message-ID: <20220427180945.GA3222843@nathanxps13> (raw)
In-Reply-To: <YmjRkYo2Ce+mO5WO@paquier.xyz>
References: <20220316.102444.2193181487576617583.horikyota.ntt@gmail.com>
<20220426183349.GA3002960@nathanxps13>
<20220427.104353.691219597727808197.horikyota.ntt@gmail.com>
<20220427032609.GA3220948@nathanxps13>
<YmjRkYo2Ce+mO5WO@paquier.xyz>
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
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: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, michael@paquier.xyz, horikyota.ntt@gmail.com, pgsql-hackers@lists.postgresql.org, masao.fujii@oss.nttdata.com
Subject: Re: Possible corruption by CreateRestartPoint at promotion
In-Reply-To: <20220427180945.GA3222843@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox