Horton

Thread

Commits

  1. Fix control file update done in restartpoints still running after promotion

  1. Possible corruption by CreateRestartPoint at promotion

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-03-16T01:24:44Z

    Hello,  (Cc:ed Fujii-san)

This is a diverged topic from [1], which is summarized as $SUBJECT.

To recap:

While discussing on additional LSNs in checkpoint log message,
Fujii-san pointed out [2] that there is a case where
CreateRestartPoint leaves unrecoverable database when concurrent
promotion happens. That corruption is "fixed" by the next checkpoint
so it is not a severe corruption.

AFAICS since 9.5, no check(/restart)pionts won't run concurrently with
restartpoint [3].  So I propose to remove the code path as attached.

regards.


[1] https://www.postgresql.org/message-id/20220316.091913.806120467943749797.horikyota.ntt%40gmail.com

[2] https://www.postgresql.org/message-id/7bfad665-db9c-0c2a-2604-9f54763c5f9e%40oss.nttdata.com

[3] https://www.postgresql.org/message-id/20220222.174401.765586897814316743.horikyota.ntt%40gmail.com

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  2. Re: Possible corruption by CreateRestartPoint at promotion

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-03-16T08:36:18Z

    Just for the record.

An instance of the corruption showed up in this mailing list [1].

[1] https://www.postgresql.org/message-id/flat/9EB4CF63-1107-470E-B5A4-061FB9EF8CC8%40outlook.com

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  3. Re: Possible corruption by CreateRestartPoint at promotion

    Nathan Bossart <nathandbossart@gmail.com> — 2022-04-26T18:33:49Z

    On Wed, Mar 16, 2022 at 10:24:44AM +0900, Kyotaro Horiguchi wrote:
> While discussing on additional LSNs in checkpoint log message,
> Fujii-san pointed out [2] that there is a case where
> CreateRestartPoint leaves unrecoverable database when concurrent
> promotion happens. That corruption is "fixed" by the next checkpoint
> so it is not a severe corruption.

I suspect we'll start seeing this problem more often once end-of-recovery
checkpoints are removed [0].  Would you mind creating a commitfest entry
for this thread?  I didn't see one.

> AFAICS since 9.5, no check(/restart)pionts won't run concurrently with
> restartpoint [3].  So I propose to remove the code path as attached.

Yeah, this "quick hack" has been around for some time (2de48a8), and I
believe much has changed since then, so something like what you're
proposing is probably the right thing to do.

>  	/* Also update the info_lck-protected copy */
>  	SpinLockAcquire(&XLogCtl->info_lck);
> -	XLogCtl->RedoRecPtr = lastCheckPoint.redo;
> +	XLogCtl->RedoRecPtr = RedoRecPtr;
>  	SpinLockRelease(&XLogCtl->info_lck);
>  
>  	/*
> @@ -6984,7 +6987,10 @@ CreateRestartPoint(int flags)
>  	/* Update the process title */
>  	update_checkpoint_display(flags, true, false);
>  
> -	CheckPointGuts(lastCheckPoint.redo, flags);
> +	CheckPointGuts(RedoRecPtr, flags);

I don't understand the purpose of these changes.  Are these related to the
fix, or is this just tidying up?

[0] https://postgr.es/m/CA%2BTgmoY%2BSJLTjma4Hfn1sA7S6CZAgbihYd%3DKzO6srd7Ut%3DXVBQ%40mail.gmail.com

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  4. Re: Possible corruption by CreateRestartPoint at promotion

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-04-27T01:43:53Z

    At Tue, 26 Apr 2022 11:33:49 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in 
> On Wed, Mar 16, 2022 at 10:24:44AM +0900, Kyotaro Horiguchi wrote:
> > While discussing on additional LSNs in checkpoint log message,
> > Fujii-san pointed out [2] that there is a case where
> > CreateRestartPoint leaves unrecoverable database when concurrent
> > promotion happens. That corruption is "fixed" by the next checkpoint
> > so it is not a severe corruption.
> 
> I suspect we'll start seeing this problem more often once end-of-recovery
> checkpoints are removed [0].  Would you mind creating a commitfest entry
> for this thread?  I didn't see one.

I'm not sure the patch makes any change here, because restart points
don't run while crash recovery, since no checkpoint records seen
during a crash recovery.  Anyway the patch doesn't apply anymore so
rebased, but only the one for master for the lack of time for now.

> > AFAICS since 9.5, no check(/restart)pionts won't run concurrently with
> > restartpoint [3].  So I propose to remove the code path as attached.
> 
> Yeah, this "quick hack" has been around for some time (2de48a8), and I
> believe much has changed since then, so something like what you're
> proposing is probably the right thing to do.

Thanks for checking!

> >  	/* Also update the info_lck-protected copy */
> >  	SpinLockAcquire(&XLogCtl->info_lck);
> > -	XLogCtl->RedoRecPtr = lastCheckPoint.redo;
> > +	XLogCtl->RedoRecPtr = RedoRecPtr;
> >  	SpinLockRelease(&XLogCtl->info_lck);
> >  
> >  	/*
> > @@ -6984,7 +6987,10 @@ CreateRestartPoint(int flags)
> >  	/* Update the process title */
> >  	update_checkpoint_display(flags, true, false);
> >  
> > -	CheckPointGuts(lastCheckPoint.redo, flags);
> > +	CheckPointGuts(RedoRecPtr, flags);
> 
> I don't understand the purpose of these changes.  Are these related to the
> fix, or is this just tidying up?

The latter, since the mixed use of two not-guaranteed-to-be-same
variables at the same time for the same purpose made me perplexed (but
I feel the change can hardly incorporated alone). However, you're
right that it is irrelevant to the fix, so removed including other
instances of the same.

> [0] https://postgr.es/m/CA%2BTgmoY%2BSJLTjma4Hfn1sA7S6CZAgbihYd%3DKzO6srd7Ut%3DXVBQ%40mail.gmail.com

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  5. Re: Possible corruption by CreateRestartPoint at promotion

    Nathan Bossart <nathandbossart@gmail.com> — 2022-04-27T03:26:09Z

    On Wed, Apr 27, 2022 at 10:43:53AM +0900, Kyotaro Horiguchi wrote:
> At Tue, 26 Apr 2022 11:33:49 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in 
>> I suspect we'll start seeing this problem more often once end-of-recovery
>> checkpoints are removed [0].  Would you mind creating a commitfest entry
>> for this thread?  I didn't see one.
> 
> I'm not sure the patch makes any change here, because restart points
> don't run while crash recovery, since no checkpoint records seen
> during a crash recovery.  Anyway the patch doesn't apply anymore so
> rebased, but only the one for master for the lack of time for now.

Thanks for the new patch!  Yeah, it wouldn't affect crash recovery, but
IIUC Robert's patch also applies to archive recovery.

> +	/* Update pg_control */
> +	LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
> +
>  	/*
>  	 * Remember the prior checkpoint's redo ptr for
>  	 * UpdateCheckPointDistanceEstimate()
>  	 */
>  	PriorRedoPtr = ControlFile->checkPointCopy.redo;

nitpick: Why move the LWLockAcquire() all the way up here?

> +	Assert (PriorRedoPtr < RedoRecPtr);

I think this could use a short explanation.

> +	 * Ensure minRecoveryPoint is past the checkpoint record while archive
> +	 * recovery is still ongoing.  Normally, this will have happened already
> +	 * while writing out dirty buffers, but not necessarily - e.g. because no
> +	 * buffers were dirtied.  We do this because a non-exclusive base backup
> +	 * uses minRecoveryPoint to determine which WAL files must be included in
> +	 * the backup, and the file (or files) containing the checkpoint record
> +	 * must be included, at a minimum. Note that for an ordinary restart of
> +	 * recovery there's no value in having the minimum recovery point any
> +	 * earlier than this anyway, because redo will begin just after the
> +	 * checkpoint record.

nitpick: Since exclusive backup mode is now removed, we don't need to
specify that the base backup is non-exclusive.

> +		/*
> +		 * Aarchive recovery has ended. Crash recovery ever after should
> +		 * always recover to the end of WAL
> +		 */
> +		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.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  6. Re: Possible corruption by CreateRestartPoint at promotion

    Michael Paquier <michael@paquier.xyz> — 2022-04-27T05:16:01Z

    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:
>> At Tue, 26 Apr 2022 11:33:49 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in 
>>> I suspect we'll start seeing this problem more often once end-of-recovery
>>> checkpoints are removed [0].  Would you mind creating a commitfest entry
>>> for this thread?  I didn't see one.
>> 
>> I'm not sure the patch makes any change here, because restart points
>> don't run while crash recovery, since no checkpoint records seen
>> during a crash recovery.  Anyway the patch doesn't apply anymore so
>> rebased, but only the one for master for the lack of time for now.
> 
> Thanks for the new patch!  Yeah, it wouldn't affect crash recovery, but
> IIUC Robert's patch also applies to archive recovery.
> 
>> +	/* Update pg_control */
>> +	LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
>> +
>>  	/*
>>  	 * Remember the prior checkpoint's redo ptr for
>>  	 * UpdateCheckPointDistanceEstimate()
>>  	 */
>>  	PriorRedoPtr = ControlFile->checkPointCopy.redo;
> 
> nitpick: Why move the LWLockAcquire() all the way up here?

Yeah, that should not be necessary.  InitWalRecovery() is the only
place outside the checkpointer that would touch this field, but that
happens far too early in the startup sequence to matter with the
checkpointer.

>> +	Assert (PriorRedoPtr < RedoRecPtr);
> 
> I think this could use a short explanation.

That's just to make sure that the current redo LSN is always older
than the one prior that.  It does not seem really necessary to me to
add that.

>> +		/*
>> +		 * Aarchive recovery has ended. Crash recovery ever after should
>> +		 * always recover to the end of WAL
>> +		 */

s/Aarchive/Archive/.

>> +		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.

-   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.
--
Michael

  7. Re: Possible corruption by CreateRestartPoint at promotion

    Nathan Bossart <nathandbossart@gmail.com> — 2022-04-27T18:09:45Z

    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

  8. Re: Possible corruption by CreateRestartPoint at promotion

    Michael Paquier <michael@paquier.xyz> — 2022-04-28T00:12:13Z

    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.
--
Michael

  9. Re: Possible corruption by CreateRestartPoint at promotion

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-04-28T02:39:38Z

    At Wed, 27 Apr 2022 14:16:01 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> 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:
> >> At Tue, 26 Apr 2022 11:33:49 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in 
> >>> I suspect we'll start seeing this problem more often once end-of-recovery
> >>> checkpoints are removed [0].  Would you mind creating a commitfest entry
> >>> for this thread?  I didn't see one.
> >> 
> >> I'm not sure the patch makes any change here, because restart points
> >> don't run while crash recovery, since no checkpoint records seen
> >> during a crash recovery.  Anyway the patch doesn't apply anymore so
> >> rebased, but only the one for master for the lack of time for now.
> > 
> > Thanks for the new patch!  Yeah, it wouldn't affect crash recovery, but
> > IIUC Robert's patch also applies to archive recovery.
> > 
> >> +	/* Update pg_control */
> >> +	LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
> >> +
> >>  	/*
> >>  	 * Remember the prior checkpoint's redo ptr for
> >>  	 * UpdateCheckPointDistanceEstimate()
> >>  	 */
> >>  	PriorRedoPtr = ControlFile->checkPointCopy.redo;
> > 
> > nitpick: Why move the LWLockAcquire() all the way up here?
> 
> Yeah, that should not be necessary.  InitWalRecovery() is the only
> place outside the checkpointer that would touch this field, but that
> happens far too early in the startup sequence to matter with the
> checkpointer.

Yes it is not necessary. I just wanted to apparently ensure not to
access ControlFile outside ControlFileLoc.  So I won't be opposed to
reverting it since, as you say, it is *actuall* safe..

> >> +	Assert (PriorRedoPtr < RedoRecPtr);
> > 
> > I think this could use a short explanation.
> 
> That's just to make sure that the current redo LSN is always older
> than the one prior that.  It does not seem really necessary to me to
> add that.

Just after we call UpdateCheckPointDistanceEstimate(RedoRecPtr -
PriorRedoPtr). Don't we really need any safeguard against giving a
wrap-arounded (in other words, treamendously large) value to the
function?  Actually it doesn't seem to happen now, but I don't
confident that that never ever happens.

That being said, I'm a minority here, so removed it.

> >> +		/*
> >> +		 * Aarchive recovery has ended. Crash recovery ever after should
> >> +		 * always recover to the end of WAL
> >> +		 */
> 
> s/Aarchive/Archive/.

Oops! Fixed.

> >> +		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

Eventually the work is done by StartupXLOG().  So we don't need to do
that at all even in CreateCheckPoint().  If we expect that the
end-of-recovery checkpoint clears it, that won't happen if if the last
restart point takes so long time that the end-of-recovery checkpoint
request is ignored. If DB_IN_ARCHIVE_RECOVERY ended while the restart
point is running, it is highly possible that the end-of-recovery
checkpoint trigger is ignored. In that case the values are cleard at
the next checkpoint.

In short, if we want to reset them at so-called end-of-recovery
checkpoint, we should do that also in CreateRecoveryPoint.

So, it is not removed in this version.

> 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.
> 
> -   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

Sure.  In very early stage the reasoning to rmove the code was it.
And the rason for proposing to back-patch the same to older versions
is basing on the further investigation, and I'm not fully confident
that for the earlier versions.

> 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.

Agreed.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  10. Re: Possible corruption by CreateRestartPoint at promotion

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-04-28T02:43:57Z

    At Thu, 28 Apr 2022 09:12:13 +0900, Michael Paquier <michael@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?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  11. Re: Possible corruption by CreateRestartPoint at promotion

    Michael Paquier <michael@paquier.xyz> — 2022-04-28T06:49:42Z

    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@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

  12. Re: Possible corruption by CreateRestartPoint at promotion

    Michael Paquier <michael@paquier.xyz> — 2022-05-06T10:58:43Z

    On Thu, Apr 28, 2022 at 03:49:42PM +0900, Michael Paquier wrote:
> 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).

And I have spent a bit of this stuff to finish with the attached.  It
will be a plus to get that done on HEAD for beta1, so I'll try to deal
with it on Monday.  I am still a bit stressed about the back branches
as concurrent checkpoints are possible via CreateCheckPoint() from the
startup process (not the case of HEAD), but the stable branches will
have a new point release very soon so let's revisit this choice there
later.  v6 attached includes a TAP test, but I don't intend to include
it as it is expensive.
--
Michael

  13. Re: Possible corruption by CreateRestartPoint at promotion

    Nathan Bossart <nathandbossart@gmail.com> — 2022-05-06T15:52:45Z

    On Fri, May 06, 2022 at 07:58:43PM +0900, Michael Paquier wrote:
> And I have spent a bit of this stuff to finish with the attached.  It
> will be a plus to get that done on HEAD for beta1, so I'll try to deal
> with it on Monday.  I am still a bit stressed about the back branches
> as concurrent checkpoints are possible via CreateCheckPoint() from the
> startup process (not the case of HEAD), but the stable branches will
> have a new point release very soon so let's revisit this choice there
> later.  v6 attached includes a TAP test, but I don't intend to include
> it as it is expensive.

I was looking at other changes in this area (e.g., 3c64dcb), and now I'm
wondering if we actually should invalidate the minRecoveryPoint when the
control file no longer indicates archive recovery.  Specifically, what
happens if a base backup of a newly promoted standby is used for a
point-in-time restore?  If the checkpoint location is updated and all
previous segments have been recycled/removed, it seems like the
minRecoveryPoint might point to a missing segment.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  14. Re: Possible corruption by CreateRestartPoint at promotion

    Michael Paquier <michael@paquier.xyz> — 2022-05-06T23:43:27Z

    On Fri, May 06, 2022 at 08:52:45AM -0700, Nathan Bossart wrote:
> I was looking at other changes in this area (e.g., 3c64dcb), and now I'm
> wondering if we actually should invalidate the minRecoveryPoint when the
> control file no longer indicates archive recovery.  Specifically, what
> happens if a base backup of a newly promoted standby is used for a
> point-in-time restore?  If the checkpoint location is updated and all
> previous segments have been recycled/removed, it seems like the
> minRecoveryPoint might point to a missing segment.

A new checkpoint is enforced at the beginning of the backup which
would update minRecoveryPoint and minRecoveryPointTLI, while we don't
a allow a backup to finish if it began on a standby has just promoted
in-between.
--
Michael

  15. Re: Possible corruption by CreateRestartPoint at promotion

    Michael Paquier <michael@paquier.xyz> — 2022-05-09T00:24:06Z

    On Fri, May 06, 2022 at 07:58:43PM +0900, Michael Paquier wrote:
> And I have spent a bit of this stuff to finish with the attached.  It
> will be a plus to get that done on HEAD for beta1, so I'll try to deal
> with it on Monday.  I am still a bit stressed about the back branches
> as concurrent checkpoints are possible via CreateCheckPoint() from the
> startup process (not the case of HEAD), but the stable branches will
> have a new point release very soon so let's revisit this choice there
> later.  v6 attached includes a TAP test, but I don't intend to include
> it as it is expensive.

Okay, applied this one on HEAD after going back-and-forth on it for
the last couple of days.  I have found myself shaping the patch in
what looks like its simplest form, by applying the check based on an
older checkpoint to all the fields updated in the control file, with
the check on DB_IN_ARCHIVE_RECOVERY applying to the addition of
DB_SHUTDOWNED_IN_RECOVERY (got initialially surprised that this was
having side effects on pg_rewind) and the minRecoveryPoint
calculations.

Now, it would be nice to get a test for this stuff, and we are going
to need something cheaper than what's been proposed upthread.  This
comes down to the point of being able to put a deterministic stop
in a restart point while it is processing, meaning that we need to
interact with one of the internal routines of CheckPointGuts().  One
fancy way to do so would be to forcibly take a LWLock to stuck the
restart point until it is released.  Using a SQL function for that
would be possible, if not artistic.  Perhaps we don't need such a
function though, if we could stuck arbitrarily the internals of a 
checkpoint?  Any ideas?
--
Michael

  16. Re: Possible corruption by CreateRestartPoint at promotion

    Michael Paquier <michael@paquier.xyz> — 2022-05-16T03:47:29Z

    On Mon, May 09, 2022 at 09:24:06AM +0900, Michael Paquier wrote:
> Okay, applied this one on HEAD after going back-and-forth on it for
> the last couple of days.  I have found myself shaping the patch in
> what looks like its simplest form, by applying the check based on an
> older checkpoint to all the fields updated in the control file, with
> the check on DB_IN_ARCHIVE_RECOVERY applying to the addition of
> DB_SHUTDOWNED_IN_RECOVERY (got initialially surprised that this was
> having side effects on pg_rewind) and the minRecoveryPoint
> calculations.

It took me some time to make sure that this would be safe, but done
now for all the stable branches.

> Now, it would be nice to get a test for this stuff, and we are going
> to need something cheaper than what's been proposed upthread.  This
> comes down to the point of being able to put a deterministic stop
> in a restart point while it is processing, meaning that we need to
> interact with one of the internal routines of CheckPointGuts().  One
> fancy way to do so would be to forcibly take a LWLock to stuck the
> restart point until it is released.  Using a SQL function for that
> would be possible, if not artistic.  Perhaps we don't need such a
> function though, if we could stuck arbitrarily the internals of a 
> checkpoint?  Any ideas?

One thing that we could do here is to resurrect the patch that adds
support for stop points in the code..
--
Michael