Re: BUG #17744: Fail Assert while recoverying from pg_basebackup

At Mon, 27 Feb 2023 09:08:19 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in
> On Sun, Feb 26, 2023 at 09:38:01AM +1300, Thomas Munro wrote:
> > Yeah, it's all a bit confusing. That particular fact is the reason we
> > can't just rely on RECOVERY_STATE_CRASH to suppress restart points in
> > RecoveryRestartPoint(), which was my first naive idea before I started
> > working through scenarios... Another thing that is a little confusing
> > is that commit 7ff23c6 didn't change the behaviour of hot standbys in
> > this respect: they already started the checkpointer at the start of
> > recovery, not at the start of hot standby. Which leads to Bowen's
> > suggestion to make recovery-from-backup work the same.
>
> FWIW, I think that 7ff23c6 and this thread's issue are just a victim
> of the decision taken in abf5c5c9 (basically mentioned upthread) where
> it was decided that we should initialize for hot standby depending on
> ArchiveRecoveryRequested rather than InArchiveRecovery, as of this
> part of the commit:
> @@ -5243,7 +5344,7 @@ StartupXLOG(void)
> * control file and we've established a recovery snapshot from a
> * running-xacts WAL record.
> */
> - if (InArchiveRecovery && EnableHotStandby)
> + if (ArchiveRecoveryRequested && EnableHotStandby)
> {
> TransactionId *xids;
> int nxids;
>
> At the end, I would suggest something the attached patch, where we
> like missing the setup of recoveryWakeupLatch, etc. One of my test
> scripts has been doing something like that, actually, because there is
> no need to run an interrupted pgbench concurrently with pg_basebackup
> to trigger the issue:
> - Setup a primary, with archiving.
> - Take a base backup of the primary, it has an end-of-recovery record,
> already. Do not start it yet.
> - Run pgbench and interrupt it a few times.
> - Copy the contents of the archives to the previous base backup's
> pg_wal/ (Critical!).
> - Start the base backup, causing the backup to replay all the WAL it
> can find, and enjoy the show as consistency is reached as of the end
> of the previous base backup (one can reload some recovery parameters,
> as well).

In my understanding, the approach differs slightly from the OP's
case. While server reaches consistency state early on using this
steps, in the OP's case it takes much longer time to reach
consistency, to the point where a checkpoint occurs before it is
reached. It seems that the approach taken by the patch does not work
in the OP's case, where (I guess that) hot-standby is disabled.

> Note that this makes the backup_file read happen slightly earlier. I'd
> like to think that we should enforce the decision of having at least a
> recovery.signal in 17~ if we find a backup_label with a proper
> validation of validateRecoveryParameters(). Another side effect is
> that this actually changes the behavior reported on this thread by
> making the end-of-recovery *switch* to a new TLI rather than
> continuing the existing timeline once recovery is finished, so this
> actually makes the base backup recovery much more predictible. It is
> true that this could influence some recovery flows, but this
> combination is actually leads to more consistency at recovery, and a
> backup_label is all about archive recovery, so..

As you know, pg_basebackup has the -R option. It seems to me that the
existence means we conciously allowed the backup to start without
archive recovery. The OP's operation (if it is correct) involves
taking a base backup and treating it as a literal backup, meaning it
begins its life on the origial timeline. This change prevents an
operation that we may have implicitly allowed (at least, that is my
understanding).

> > Then for master I think we should (in later patches, for 16 or maybe
> > 17) consider relaxing that. There doesn't seem to be a technical
> > reason why a very long running PITR shouldn't benefit from restart
> > points, and that would be in line with the direction of making
> > recovery modes more similar, but given we/I missed this subtlety in
> > the past, the more conservative choice seems more appropriate for 15.
>
> If you wish to be more conservative and prevent checkpoints to run in
> cases where they were not running before 14 and lift this restriction
> in 17~, that would be fine by me, but I am not convinced that we
> really need that. As far as I can see, 7ff23c6 is kind of right.

Thus, I slightly object to let backup_label to enforce archive
recovery. And rather prefer to suppress checkpoints before reaching
consistency(?).

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center