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

At Tue, 21 Feb 2023 12:51:51 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Wed, Feb 01, 2023 at 07:32:52AM -0800, Andres Freund wrote:
> > I might be missing something, but I suspect the problem here is that we
> > shouldn't have been creating a restart point. Afaict, the setup
> > instructions provided don't configure a recovery.signal, so we'll just
> > perform crash recovery.
> > 
> > And I don't think it'd ever make sense to create a restart point during
> > crash recovery?
> 
> A restart point should never be created during crash recovery until we
> reach a consistent state, because there could be a risk of seeing
> inconsistent pages, for one, no?   SwitchIntoArchiveRecovery() would

I'm not sure. Anyway the on-disk pages are always inconsistent
regardless of type of recovery being running or whether recovery is
being executed at all.

> be the portion of the code doing the switch from crash to archive
> recovery if it was requested, as called in the callback to read
> records in ReadRecord().
> 
> > Except that in this case, it's not pure crash recovery, it's restoring
> > from a backup label. Due to which it actually might make sense to create
> > restart points?
> 
> Hmm.  I'd like to think that these days requesting replay from a
> backup_label file without a recovery.signal or a standby.signal is
> just asking for trouble.  Still, the case of the OP, by just having a
> backup label, is equivalent to restoring from a self-contained backup,
> which is something that should work on its own even if there is no
> recovery.signal.
> 
> > If you're doing PITR or such you don't really gain
> > anything by doing checkpoints until you've reached consistency, unless
> > you want to optimize for the case that you might need to start/stop the
> > instance multiple times?
> 
> Yes, that could help in some cases.  The PITR target could be far away

According to Thomas' initial mail that started the discussion about
that change, one of the goals is to let crash recovery have a "fast"
mode just like archive recovery already has. It's not just limited to
end-of-recovery checkpoint.

> from the consistent point, and the user could have just copied WAL
> segments in pg_wal/ rather than relying on a recovery.signal with at
> least a restore_command.  That would be fancy, still being able to
> start/stop and retry targets may make sense..
> 
> So, yes, it seems to me that the correct answer here would be just to
> skip restart points as long as SharedRecoveryState is in
> RECOVERY_STATE_CRASH, because the checkpointer relies on
> RecoveryInProgress() and it cannot make the difference between crash
> recovery and archive recovery.  But now the checkpointer is started
> during recovery to ease the end-of-recovery checkpoint process.

If we prevent restartpoints during crash recovery, why do we need to
launch checkpointer at such an early stage? In PG15 the comment for
SendPostmasterSignal(PMSIGNAL_RECOVERY_STARTED) was missing the
follwoing description, that means we are no longer supposed to have
this restriction.

>                                                   We don't bother during
>         * crash recovery as restartpoints can only be performed during
>         * archive recovery.  And we'd like to keep crash recovery simple, to
>         * avoid introducing bugs that could affect you when recovering after
>         * crash.

The follwoing is the corresponding commit.

> commit 7ff23c6d277d1d90478a51f0dd81414d343f3850
> Author: Thomas Munro <tmunro@postgresql.org>
> Date:   Mon Aug 2 17:32:20 2021 +1200
> 
>     Run checkpointer and bgwriter in crash recovery.
>     
>     Start up the checkpointer and bgwriter during crash recovery (except in
>     --single mode), as we do for replication.  This wasn't done back in
>     commit cdd46c76 out of caution.  Now it seems like a better idea to make
>     the environment as similar as possible in both cases.  There may also be
>     some performance advantages.

Wouldn't just previnting restartpoints during crash recovery mean
making this change inneffective?  If something wrong actually happens
when restartpoints happen during crash recovery, shouldn't we identify
the cause and let restartpoints run safely during crash recovery?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center