On Mon, Oct 30, 2023 at 12:47:41PM -0700, Andres Freund wrote:
> I think the problem with these variables is that they're a really messy state
> machine - something this patch doesn't meaningfully improve IMO.
Okay. Yes, this is my root issue as well. We're at the stage where
we should reduce the possible set of combinations and assumptions
we're inventing because people can do undocumented stuff, then perhaps
refactor the code on top of that (say, if one combination with too
booleans is not possible, switch to a three-state enum rather than 2
bools, etc).
>> This configuration was possible when recovering from a base backup taken
>> by pg_basebackup without -R. Note that the documentation requires at
>> least to set recovery.signal to restore from a backup, but the startup
>> process was not making this policy explicit.
>
> Maybe I just didn't check the right place, but from I saw, this, at most, is
> implied, rather than explicitly stated.
See the doc reference here:
https://www.postgresql.org/message-id/ZUBM6BNQnEh7lzIM@paquier.xyz
So it kind of implies it, still also mentions restore_command. It's
like Schrödinger's cat, yes and no at the same time.
> With -X ... we have all the necessary WAL locally, how does the workload on
> the primary matter? If you pass --no-slot, pg_basebackup might fail to fetch
> the necessary wal, but then you'd also have gotten an error.
>
> [...]
>
> Right now running pg_basebackup with -X stream, without --write-recovery-conf,
> gives you a copy of a cluster that will come up correctly as a distinct
> instance.
>
> [...]
>
> I also just don't think that it's always desirable to create a new timeline.
Yeah. Another argument I was mentioning to Robert is that we may want
to just treat the case where you have a backup_label without any
signal files just the same as crash recovery, replaying all the local
pg_wal/, and nothing else. For example, something like the attached
should make sure that InArchiveRecovery=true should never be set if
ArchiveRecoveryRequested is not set.
The attached would still cause redo to complain on a "WAL ends before
end of online backup" if not all the WAL is here (reason behind the
tweak of 010_pg_basebackup.pl, but the previous tweak to pg_rewind's
008_min_recovery_point.pl is not required here).
Attached is the idea I had in mind, in terms of code, FWIW.
--
Michael