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

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
switch a bit the order of the actions taken at the beginning of the
startup process and enforce ArchiveRecoveryRequested when we find a
backup_label so as we always go through archive recovery as all the
code paths of the startup process assume that. As far as I can see,
we have never assumed that the combination of ArchiveRecoveryRequested
= false and InArchiveRecovery = true should be a valid one. With
HEAD, it is actually possible to do much nastier things, actually,
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).

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

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