Date: Wed, 27 Apr 2022 11:09:45 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	pgsql-hackers@lists.postgresql.org, masao.fujii@oss.nttdata.com
Subject: Re: Possible corruption by CreateRestartPoint at promotion
Message-ID: <20220427180945.GA3222843@nathanxps13>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YmjRkYo2Ce+mO5WO@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/20220427180945.GA3222843%40nathanxps13>
Precedence: bulk

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