Subject: Re: Requiring recovery.signal or standby.signal when recovering with a backup_label

From: Michael Paquier <[email-masked]>
Date: Mon, 16 Oct 2023 14:54:35 +0900

On Sat, Oct 14, 2023 at 03:45:33PM -0400, David Steele wrote:
> On 9/28/23 19:59, Michael Paquier wrote:
> OK, I have now reviewed and tested the patch and it looks good to me. I
> stopped short of marking this RfC since there are other reviewers in the
> mix.

Thanks for the review.  Yes, I am wondering if other people would
chime in here.  It doesn't feel like this has gathered enough
opinions.  Now this thread has been around for many months, and we've
done quite a few changes in the backup APIs in the last few years with
few users complaining back about them..

> I dislike that we need to repeat:
> 
> OwnLatch(&XLogRecoveryCtl->recoveryWakeupLatch);
> 
> But I see the logic behind why you did it and there's no better way to do it
> as far as I can see.

The main point is that there is no meaning in setting the latch until
the backup_label file is read because if ArchiveRecoveryRequested is
*not* set the startup process would outright fail as of the lack of
[recovery|standby].signal.

>> Another idea I had was to force the creation of recovery.signal by
>> pg_basebackup even if -R is not used.  All the reports we've seen with
>> people getting confused came from pg_basebackup that enforces no
>> configuration.
> 
> This change makes it more obvious if configuration is missing (since you'll
> get an error), however +1 for adding this to pg_basebackup.

Looking at the streaming APIs of pg_basebackup, it looks like this
would be a matter of using bbstreamer_inject_file() to inject an empty
file into the stream.  Still something seems to be off once
compression methods are involved..  Hmm.  I am not sure.  Well, this
could always be done as a patch independant of this one, under a
separate discussion.  There are extra arguments about whether it would
be a good idea to add a recovery.signal even when taking a backup from
a standby, and do that only in 17~.

>> A last thing, that had better be covered in a separate thread and
>> patch, is about validateRecoveryParameters().  These days, I'd like to
>> think that it may be OK to lift at least the restriction on
>> restore_command being required if we are doing recovery to ease the
>> case of self-contained backups (aka the case where all the WAL needed
>> to reach a consistent point is in pg_wal/ or its tarball)
> 
> Hmmm, I'm not sure about this. I'd prefer users set
> restore_command=/bin/false explicitly to fetch WAL from pg_wal by default if
> that's what they really intend.

It wouldn't be the first time we break compatibility in this area, so
perhaps you are right and keeping this requirement is fine, even if it
requires one extra step when recovering a self-contained backup
generated by pg_basebackup.  At least this forces users to look at
their setup and check if something is wrong.  We'd likely finish with
a few "bug" reports, as well :D
--
Michael

---

From: Bowen Shi <[email-masked]>
Date: Mon, 16 Oct 2023 06:21:07 +0000

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

It looks good to me.

I have reviewed the code and tested the patch with basic check-world test an pgbench test (metioned in https://www.postgresql.org/message-id/flat/ZQtzcH2lvo8leXEr%40paquier.xyz#cc5ed83e0edc0b9a1c1305f08ff7a335). 

Another reviewer has also approved it, so I change the status to RFC.

The new status of this patch is: Ready for Committer

---

From: Laurenz Albe <[email-masked]>
Date: Mon, 16 Oct 2023 17:48:43 +0200

On Mon, 2023-10-16 at 14:54 +0900, Michael Paquier wrote:
> opinions.

I don't have strong feelings either way.  If you have backup_label
but no signal file, starting PostgreSQL may succeed (if the WAL
with the checkpoint happens to be in pg_wal) or it may fail with
an error message.  There is no danger of causing damage unless you
remove backup_label, right?

I cannot think of a use case where you use such a configuration on
purpose, and the current error message is more crypric than a plain
"you must have a signal file to start from a backup", so perhaps
your patch is a good idea.

Yours,
Laurenz Albe

---

From: Michael Paquier <[email-masked]>
Date: Tue, 17 Oct 2023 08:21:39 +0900

> remove backup_label, right?

A bit more happens currently if you have a backup_label with no signal
files, unfortunately, because this causes some startup states to not
be initialized.  See around here:
https://www.postgresql.org/message-id/Y/Q/[email-masked]
https://www.postgresql.org/message-id/Y/v0c+3W89NBT/[email-masked]

> your patch is a good idea.

I hope so.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Fri, 27 Oct 2023 16:22:54 +0900

> a standby, and do that only in 17~.

Hmm.  On this specific point, it would actually be much simpler to
force recovery.signal to be in the contents streamed to a BASE_BACKUP.
This does not step on your proposal at [1], though, because you'd
still require a .signal file for recovery as far as I understand :/ 

[1]: https://www.postgresql.org/message-id/[email-masked]

Would folks be OK to move on with the patch of this thread at the end?
I am attempting a last-call kind of thing.
--
Michael

---

From: David Steele <[email-masked]>
Date: Fri, 27 Oct 2023 09:31:10 -0400

> force recovery.signal to be in the contents streamed to a BASE_BACKUP.

That sounds like the right plan to me. Nice and simple.

> [1]: https://www.postgresql.org/message-id/[email-masked]

Yes.

> I am attempting a last-call kind of thing.

I'm still +1 for the patch as it stands.

Regards,
-David

---

From: Michael Paquier <[email-masked]>
Date: Mon, 30 Oct 2023 16:08:50 +0900

> That sounds like the right plan to me. Nice and simple.

I'll tackle that in a separate thread with a patch registered for the
upcoming CF of November.

> I'm still +1 for the patch as it stands.

I have been reviewing the patch, and applied portions of it as of
dc5bd388 and 1ffdc03c and they're quite independent pieces.  After
that, the remaining bits of the patch to change the behavior is now
straight-forward.  I have written a commit message for it, while on
it, as per the attached.
--
Michael
From 26a8432fe3ab8426e7797d85d19b0fe69d3384c9 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Mon, 30 Oct 2023 16:02:52 +0900
Subject: [PATCH v4] Require recovery.signal or standby.signal when reading a
 backup_file

Historically, the startup process uses two static variables to control
if archive recovery should happen, when either recovery.signal or
standby.signal are defined in the data folder at the beginning of
recovery:
- ArchiveRecoveryRequested, set to "true" if either .signal file exists.
This controls if archive recovery has been requested.
- InArchiveRecovery, that controls if recovery should switch to archive
mode during recovery.  It is initially "false" but switched to "true"
during recovery for two cases:
-- Just after finding and reading a valid backup_label file, close to
the beginning of recovery.
-- When no backup_label file is found, it would be set to "false" to
force crash recovery, where all the WAL located in pg_wal/ is read
first.  When all the local WAL records have been consumed, it is
changed to "true" to switch to archive recovery mode, equivalent to a
base backup.

Now comes the fact that recovery has an old logical problem: when a
backup_label is found but no .signal file is defined, the startup
process finishes in a state where ArchiveRecoveryRequested is "false"
but InArchiveRecovery is "true".  This is proving to cause various
issues, that got worse since restart points can run during crash
recovery for the benefit of being able to start and stop instances
without doing crash recovery from its initial point (7ff23c6d277d):
- Some standby states needed by archive recovery would not be set,
causing correctness issues, like assertion failures during recovery.
- recoveryWakeupLatch would not be set.
- Some GUCs are messed up, hot_standby that depends on
ArchiveRecoveryRequested.

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.  In most cases, one would
have been able to complete recovery, but that's a matter of luck,
really, as it depends on the workload of the origin server.  This has
the advantage of simplifying the logic for the archive recovery case, as
InArchiveRecovery now requires ArchiveRecoveryRequested to be set.
Recovering a self-contained backup now requires a recovery.signal, with
a restore_command set in postgresql.conf (or related conf file).

One test of pg_basebackup and one test of pg_rewind need to be tweaked
to avoid the FATAL introduced by this patch when a base_backup is found
without a .signal file, even if the intention of both is to have a
recovery.signal set.

Reviewed-by: David Steele, Bowen Shi
Discussion: https://postgr.es/m/[email-masked]
Discussion: https://postgr.es/m/Y/Q/[email-masked]
Discussion: https://postgr.es/m/Y/v0c+3W89NBT/[email-masked]
---
 src/backend/access/transam/xlogrecovery.c     | 22 ++++++++++++++++---
 src/bin/pg_basebackup/t/010_pg_basebackup.pl  |  3 ++-
 src/bin/pg_rewind/t/008_min_recovery_point.pl |  1 +
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index c61566666a..0088dfec2e 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -125,14 +125,18 @@ static TimeLineID curFileTLI;
 
 /*
  * When ArchiveRecoveryRequested is set, archive recovery was requested,
- * ie. signal files were present.  When InArchiveRecovery is set, we are
- * currently recovering using offline XLOG archives.  These variables are only
- * valid in the startup process.
+ * ie. signal files or backup_label were present.  When InArchiveRecovery is
+ * set, we are currently recovering using offline XLOG archives.  These
+ * variables are only valid in the startup process.  Note that the presence of
+ * a backup_label file requires the presence of one of the two .signal files
+ * to enforce archive recovery.
  *
  * When ArchiveRecoveryRequested is true, but InArchiveRecovery is false, we're
  * currently performing crash recovery using only XLOG files in pg_wal, but
  * will switch to using offline XLOG archives as soon as we reach the end of
  * WAL in pg_wal.
+ *
+ * InArchiveRecovery should never be set without ArchiveRecoveryRequested.
  */
 bool		ArchiveRecoveryRequested = false;
 bool		InArchiveRecovery = false;
@@ -594,6 +598,12 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
 	{
 		List	   *tablespaces = NIL;
 
+		if (!ArchiveRecoveryRequested)
+			ereport(FATAL,
+					(errmsg("could not find recovery.signal or standby.signal when recovering with backup_label"),
+					 errhint("If you are restoring from a backup, touch \"%s/recovery.signal\" or \"%s/standby.signal\" and add required recovery options.",
+							 DataDir, DataDir)));
+
 		/*
 		 * Archive recovery was requested, and thanks to the backup label
 		 * file, we know how far we need to replay to reach consistency. Enter
@@ -1591,6 +1601,12 @@ ShutdownWalRecovery(void)
 	 */
 	if (ArchiveRecoveryRequested)
 		DisownLatch(&XLogRecoveryCtl->recoveryWakeupLatch);
+
+	/*
+	 * InArchiveRecovery should never have been set without
+	 * ArchiveRecoveryRequested.
+	 */
+	Assert(ArchiveRecoveryRequested || !InArchiveRecovery);
 }
 
 /*
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index b9f5e1266b..b9e54f4562 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -392,7 +392,8 @@ SKIP:
 	my $node2 = PostgreSQL::Test::Cluster->new('replica');
 
 	# Recover main data directory
-	$node2->init_from_backup($node, 'tarbackup2', tar_program => $tar);
+	$node2->init_from_backup($node, 'tarbackup2', tar_program => $tar,
+	    has_restoring => 1);
 
 	# Recover tablespace into a new directory (not where it was!)
 	my $repTsDir = "$tempdir/tblspc1replica";
diff --git a/src/bin/pg_rewind/t/008_min_recovery_point.pl b/src/bin/pg_rewind/t/008_min_recovery_point.pl
index d4c89451e6..1cff5b7019 100644
--- a/src/bin/pg_rewind/t/008_min_recovery_point.pl
+++ b/src/bin/pg_rewind/t/008_min_recovery_point.pl
@@ -152,6 +152,7 @@ move(
 	"$tmp_folder/node_2-postgresql.conf.tmp",
 	"$node_2_pgdata/postgresql.conf");
 
+$node_2->append_conf('standby.signal', '');
 $node_2->start;
 
 # Check contents of the test tables after rewind. The rows inserted in node 3
-- 
2.42.0

---

From: Roberto Mello <[email-masked]>
Date: Mon, 30 Oct 2023 10:32:28 -0600

> straight-forward.  I have written a commit message for it, while on

A suggestion for the hint message in an effort to improve readability:

"If you are restoring from a backup, ensure \"%s/recovery.signal\" or
\"%s/standby.signal\" is present and add required recovery options."

I realize the original use of "touch" is a valid shortcut for what I
suggest above, however that will be less clear for the not-so-un*x-inclined
users of Postgres, while for some it'll be downright confusing, IMHO. It
also provides the advantage of being crystal clear on what needs to be done
to fix the problem.

Roberto

---

From: Robert Haas <[email-masked]>
Date: Mon, 30 Oct 2023 13:55:13 -0400

> straight-forward.  I have written a commit message for it, while on

I would encourage some caution here.

In a vacuum, I'm in favor of this, and for the same reasons as you,
namely, that the huge pile of Booleans that we use to control recovery
is confusing, and it's difficult to make sure that all the code paths
are adequately tested, and I think some of the things that actually
work here are not documented.

But in practice, I think there is a possibility of something like this
backfiring very hard. Notice that the first two people who commented
on the thread saw the error and immediately removed backup_label even
though that's 100% wrong. It shows how utterly willing users are to
remove backup_label for any reason or no reason at all. If we convert
cases where things would have worked into cases where people nuke
backup_label and then it appears to work, we're going to be worse off
in the long run, no matter how crazy the idea of removing backup_label
may seem to us.

Also, Andres just recently mentioned to me that he uses this procedure
of starting a server with a backup_label but no recovery.signal or
standby.signal file regularly, and thinks other people do too. I was
surprised, since I've never done that, except maybe when I was a noob
and didn't have a clue. But Andres is far from a noob.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Andres Freund <[email-masked]>
Date: Mon, 30 Oct 2023 12:47:41 -0700

Hi,

> standby.signal are defined in the data folder at the beginning of

I think the problem with these variables is that they're a really messy state
machine - something this patch doesn't meaningfully improve IMO.

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

> In most cases, one would have been able to complete recovery, but that's a
> matter of luck, really, as it depends on the workload of the origin server.

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.

I agree with Robert that this would be a good error check on a green field,
but that I am less convinced it's going to help more than hurt now.

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.

With this change applied, you need to know that the way to avoid the existing
FATAL about restore_command at startup (when recovery.signal exists but
restore_command isn't set)) is to is to set "restore_command = false",
something we don't explain anywhere afaict. We should lessen the need to ever
use restore_command, not increase it.

It also seems risky to have people get used to restore_command = false,
because that effectively disables detection of other timelines etc. But, this
method does force a new timeline - which will be the same on each clone of the
database...

I also just don't think that it's always desirable to create a new timeline.

Greetings,

Andres Freund

---

From: Michael Paquier <[email-masked]>
Date: Tue, 31 Oct 2023 09:40:08 +0900

> I would encourage some caution here.

Thanks for chiming here.

> work here are not documented.

Yep, same feeling here.

> in the long run, no matter how crazy the idea of removing backup_label

As far as I know, there's one paragraph in the docs that implies this
mode without giving an actual hint that this may be OK or not, so
shrug:
https://www.postgresql.org/docs/devel/continuous-archiving.html#BACKUP-TIPS
"As with base backups, the easiest way to produce a standalone hot
backup is to use the pg_basebackup tool. If you include the -X
parameter when calling it, all the write-ahead log required to use the
backup will be included in the backup automatically, and no special
action is required to restore the backup."

And a few lines down we imply to use restore_command, something that
we check is set only if recovery.signal is set.  See additionally
validateRecoveryParameters(), where the comments imply that
InArchiveRecovery would be set only when there's a restore command.

As you're telling me, and I've considered that as an option as well,
perhaps we should just consider the presence of a backup_label file
with no .signal files as a synonym of crash recovery?  In the recovery
path, currently the essence of the problem is when we do
InArchiveRecovery=true, but ArchiveRecoveryRequested=false, meaning
that it should do archive recovery but we don't want it, and that does
not really make sense.  The rest of the code sort of implies that this
is not a suported combination.  So basically, my suggestion here, is
to just replay WAL up to the end of what's in your local pg_wal/ and
hope for the best, without TLI jumps, except that we'd do nothing.
Doing a pg_basebackup -X stream followed by a restart would work fine
with that, because all the WAL is here.

A point of contention is if we'd better be stricter about satisfying
backupEndPoint in such a case, but the redo code only wants to do
something here when ArchiveRecoveryRequested is set (aka there's a
.signal file set), and we would not want a TLI jump at the end of
recovery, so I don't see an argument with caring about backupEndPoint
in this case.

At the end, I'm OK as long as ArchiveRecoveryRequested=false
InArchiveRecovery=true does not exist anymore, because it's much
easier to get what's going on with the redo path, IMHO.

(I have a patch at hand to show the idea, will post it with a reply to
Andres' message.)

> and didn't have a clue. But Andres is far from a noob.

At this stage, that's basically at your own risk, as the code thinks
it's OK to force what's basically archive-recovery-without-being-it.
So it basically works, but it can also easily backfire, as well..
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Tue, 31 Oct 2023 09:42:18 +0900

> also provides the advantage of being crystal clear on what needs to be done

Indeed, "touch" may be better in this path if we'd throw an ERROR to
enforce a given policy, and that's more consistent with the rest of
the area.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Tue, 31 Oct 2023 10:15:21 +0900

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

> implied, rather than explicitly stated.

See the doc reference here:
https://www.postgresql.org/message-id/[email-masked]

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.

> [...]
> [...]

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
From fbd9fa407c04799ad4401d3ba5c6b67a0d922631 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Tue, 31 Oct 2023 10:14:01 +0900
Subject: [PATCH] Force crash recovery with backup_label and no .signal files

---
 src/backend/access/transam/xlogrecovery.c    | 29 ++++++++++++++++----
 src/bin/pg_basebackup/t/010_pg_basebackup.pl |  3 +-
 doc/src/sgml/backup.sgml                     |  4 ++-
 3 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index c61566666a..de5787d7e8 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -133,6 +133,8 @@ static TimeLineID curFileTLI;
  * currently performing crash recovery using only XLOG files in pg_wal, but
  * will switch to using offline XLOG archives as soon as we reach the end of
  * WAL in pg_wal.
+ *
+ * InArchiveRecovery should never be set without ArchiveRecoveryRequested.
  */
 bool		ArchiveRecoveryRequested = false;
 bool		InArchiveRecovery = false;
@@ -595,13 +597,22 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
 		List	   *tablespaces = NIL;
 
 		/*
-		 * Archive recovery was requested, and thanks to the backup label
-		 * file, we know how far we need to replay to reach consistency. Enter
-		 * archive recovery directly.
+		 * If archive recovery was requested, and we know how far we need to
+		 * replay to reach consistency thanks to the backup label file, then
+		 * enter archive recovery directly in this case.
+		 *
+		 * If archive recovery was not requested, then do crash recovery and
+		 * replay all the local WAL.  This still checks that all the WAL up
+		 * to backupEndRequired has been replayed.  This case is useful when
+		 * restoring from a standalone base backup, taken with pg_basebackup
+		 * --wal-method=stream, for example.
 		 */
-		InArchiveRecovery = true;
-		if (StandbyModeRequested)
-			EnableStandbyMode();
+		if (ArchiveRecoveryRequested)
+		{
+			InArchiveRecovery = true;
+			if (StandbyModeRequested)
+				EnableStandbyMode();
+		}
 
 		/*
 		 * When a backup_label file is present, we want to roll forward from
@@ -1591,6 +1602,12 @@ ShutdownWalRecovery(void)
 	 */
 	if (ArchiveRecoveryRequested)
 		DisownLatch(&XLogRecoveryCtl->recoveryWakeupLatch);
+
+	/*
+	 * InArchiveRecovery should never have been set without
+	 * ArchiveRecoveryRequested.
+	 */
+	Assert(ArchiveRecoveryRequested || !InArchiveRecovery);
 }
 
 /*

 	# Recover tablespace into a new directory (not where it was!)
 	my $repTsDir = "$tempdir/tblspc1replica";
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 8cb24d6ae5..5ba7a284cf 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -1380,7 +1380,9 @@ restore_command = 'cp /mnt/server/archivedir/%f %p'
       tool. If you include the <literal>-X</literal> parameter when calling
       it, all the write-ahead log required to use the backup will be
       included in the backup automatically, and no special action is
-      required to restore the backup.
+      required to restore the backup. Restoring a standalone backup is
+      equivalent to crash recovery, replaying all the WAL stored in
+      <filename>pg_wal</filename> up to its end.
      </para>
     </sect3>
 
-- 
2.42.0

---

From: Robert Haas <[email-masked]>
Date: Tue, 31 Oct 2023 08:28:07 -0400

> action is required to restore the backup."

I see your point, but that's way too subtle. As far as I know, the
only actually-documented procedure for restoring is this one:

https://www.postgresql.org/docs/current/continuous-archiving.html#BACKUP-PITR-RECOVERY

That procedure actually is badly in need of some updating, IMHO,
because close to half of it is about moving your existing database
cluster out of the way, which may or may not be needed in the case of
any particular backup restore. Also, it unconditionally mentions
creating recovery.signal, with no mention of standby.signal. And
certainly not with neither. It also gives zero motivation for actually
doing this and says nothing useful about backup_label.

Both recovery.signal and standby.signal are documented in
https://www.postgresql.org/docs/current/runtime-config-wal.html#RUNTIME-CONFIG-WAL-ARCHIVE-RECOVERY
but you'd have no real reason to look in a list of GUCs for
information about a file on disk. recovery.signal but not
standby.signal is mentioned in
https://www.postgresql.org/docs/current/warm-standby.html but nowhere
that I can find do we explicitly talk about running with at least one
of them.

> hope for the best, without TLI jumps, except that we'd do nothing.

This sentence seems to be incomplete.

But I was not saying we should treat the case where we have a
backup_label file like crash recovery. The real question here is why
we don't treat it fully like archive recovery. I don't know off-hand
what is different if I start the server with both backup_label and
recovery.signal vs. if I start it with only backup_label, but I
question whether there should be any difference at all.

> recovery, so I don't see an argument with caring about backupEndPoint

This is a bit hard for me to understand, but I disagree strongly with
the idea that we should ever ignore a backup end point if we have one.

---

From: Michael Paquier <[email-masked]>
Date: Wed, 1 Nov 2023 08:39:17 +0900

> that I can find do we explicitly talk about running with at least one

Point 7. of what you quote says to use one?  True that this needs a
refresh, and perhaps a bit fat warning about the fact that these are
required if you want to fetch WAL from other sources than the local
pg_wal/.  Perhaps there may be a point of revisiting the default
behavior of recovery_target_timeline in this case, I don't know.

> This sentence seems to be incomplete.

I've re-read it, and it looks OK to me.  What I mean with this
paragraph are two things:
- Remove InArchiveRecovery=true and ArchiveRecoveryRequested=false as
a possible combination in the code.
- Treat backup_label with no .signal file as the same as crash
recovery, that:
-- Does no TLI jump at the end of recovery.
-- Expects all the WAL to be in pg_wal/.

> we don't treat it fully like archive recovery.

Timeline jump at the end of recovery?  Archive recovery forces a TLI
jump by default at the end of redo if there's a signal file, and some
users may not want a TLI jump by default?

> I don't know off-hand

Perhaps we could do that, but note that backup_label is renamed to
backup_label.old at the beginning of redo.  The code has historically
always enforced InArchiveRecovery=true when there's a backup label,
and InArchiveRecovery=false where there is no backup label, so we
don't get the same recovery behavior if a cluster is restarted while
it was still performing recovery.  I don't quite see how it is
possible to make this code simpler without enforcing a policy to take
care of this inconsistency.  I've listed two of them on this thread:
- Force the presence of a .recovery file when there is a
backup_label, to force archive recovery.
- Force crash recovery if there are no signal files but a
backup_label, then a restart of a cluster that began a restore while
it processed a backup would be confused: should it do crash recovery
or archive recovery?

My guess, based on what I read from the feedback of this thread, is
that it could be more helpful to do the second thing, not the third
one, because this is better with standalone backups: no TLI jumps and
restore happens with all the local WAL in pg_wal/, without any GUCs to
control how recovery should run.

You are suggesting a third, hybrid, approach.  Now note we have always
checked for signal files before the backup_label.  Recovery GUCs are
checked only if there's one of the two signal files.  It seems to me
that what you are suggesting would make the code a bit harder to
follow, actually, and more inconsistent with stable branches because
we would need to check the control file contents *before* checking for
the .signal files or backup_label to be able to see if archive
recovery *should* happen, depending on if there's a backupEndPoint.

> the idea that we should ever ignore a backup end point if we have one.

Actually, while experimenting yesterday before sending my reply to
you, I have noticed that redo cares about backupEndPoint even if you
force crash recovery when there's only a backup_label file.  There's a
case in pg_basebackup that would fail, but that's accidental, AFAIK:
https://www.postgresql.org/message-id/ZUBVKfL6FR6NOQyt%40paquier.xyz

See in StartupXLOG(), around the comment "complain if we did not roll
forward far enough to reach".  This complains if archive recovery has
been requested *or* if we retrieved a backup end LSN from the
backup_label.
--
Michael

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Thu, 02 Nov 2023 11:03:35 +0900 (JST)

At Wed, 1 Nov 2023 08:39:17 +0900, Michael Paquier <[email-masked]> wrote in 
> been requested *or* if we retrieved a backup end LSN from the

Please note that backupStartPoint is not reset even when reaching the
backup end point during crash recovery. If backup_label enforces
archive recovery, I think this point won't be an issue as you
mentioned. For the record, my earlier proposal aimed to detect
reaching the end point even during crash recovery.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: Robert Haas <[email-masked]>
Date: Wed, 8 Nov 2023 13:16:58 -0500

> behavior of recovery_target_timeline in this case, I don't know.

I don't really know what to say to this -- sure, point 7 of
"Recovering Using a Continuous Archive Backup" says to use
recovery.signal. But as I said in the preceding paragraph, it doesn't
say either "use recovery.signal or standby.signal". Nor does it or
anything else in the documentation explain under what circumstances
you're allowed to have neither. So the whole thing is very unclear.

> I've re-read it, and it looks OK to me.

Well, the sentence ends with "except that we'd do nothing" and I don't
know what that means. It would make sense to me if it said "except
that we'd do nothing about <whatever>" or "except that we'd do nothing
instead of <something>" but as you've written it basically seems to
boil down to "my suggestion is to replay WAL except do nothing" which
makes no sense. If you replay WAL, you're not doing nothing.

> users may not want a TLI jump by default?

Uggh. I don't know what to think about that. I bet some people do want
that, but that makes it pretty easy to end up with multiple copies of
the same cluster running on the same TLI, too, which is not a thing
that you really want to have happen.

At the end of the day, I'm coming around to the view that the biggest
problem here is the documentation. Nobody can really know what's
supposed to work right now because the documentation doesn't say which
things you are and are not allowed to do and what results you should
expect in each case. If it did, it would be easier to discuss possible
behavior changes. Right now, it's hard to change any code at all,
because there's no list of supported scenarios, so you can't tell
whether a potential change affects a scenario that somebody thinks
should work, or only cases that nobody can possibly care about. It's
sort of possible to reason your way through that, to an extent, but
it's pretty hard. The fact that I didn't know that starting from a
backup with neither recovery.signal nor standby.signal was a thing
that anybody did or cared about is good evidence of that.

I'm coming to the understanding that we have four supported scenarios.
One, no backup_label, no recovery.signal, and no standby.signal.
Hence, replay WAL until the end, then start up. Two, backup_label
exists but neither recovery.signal nor standby.signal does. As before,
but if I understand correctly, now we can check that we reached the
backup end location. Three, recovery.signal exists, with or without
backup_label. Now we create a new TLI at the end of recovery, and
also, now can fetch WAL that is not present in pg_wal using
primary_conninfo or restore_command. In fact, I think we may prefer to
do that over using WAL we have locally, but I'm not quite sure about
that. Fourth, standby.signal exists, with or without backup_label. As
the previous scenario, but now when we reach the end of WAL we wait
for more to appear instead of ending recovery. I have a feeling this
is not quite an exhaustive list of differences between the various
modes, and I'm not even sure that it lists all of the things someone
might try to do. Thoughts?

I also feel like the terminology here sometimes obscures more than it
illuminates. For instance, it seems like ArchiveRecoveryRequested
really means "are any signal files present?" while InArchiveRecovery
means "are we fetching WAL from outside pg_wal rather than using
what's in pg_wal?". But these are not obvious from the names, and
sometimes we have additional variables with overlapping meanings, like
readSource, which indicates whether we're reading from pg_wal, the
archive, or the walreceiver, and yet is probably not redundant with
InArchiveRecovery. In any event, I think that we need to start with
the question of what behavior(s) we want to expose to users, and then
back into the question of what internal variables and states need to
exist in order to support that behavior. We cannot start by deciding
what variables we'd like to get rid of and then trying to justify the
resulting behavior changes on the grounds that they simplify the code.
Users aren't going to like that, hackers aren't going to like that,
and the resulting behavior probably won't be anything great.

---

From: Michael Paquier <[email-masked]>
Date: Thu, 9 Nov 2023 12:04:19 +0900

> makes no sense. If you replay WAL, you're not doing nothing.

Sure, sorry for the confusion.  By "we'd do nothing", I mean precirely
"to take no specific action related to archive recovery and recovery
parameters at the end of recovery", meaning that a combination of
backup_label with no signal file would be the same as crash recovery,
replaying WAL up to the end of what can be found in pg_wal/, and only
that.

> that you really want to have happen.

Andres has mentioned upthread that this is something he's been using
to quickly be able to clone a cluster.  I would not recommend doing
that, personally, but if that's useful in some cases, well, why not.

> that anybody did or cared about is good evidence of that.

That's one problem, not all of it, because the code takes extra
assumptions around that.

> and the resulting behavior probably won't be anything great.

Note as well that InArchiveRecovery is set when there's a
backup_label, but that the code would check for the existence of a
restore_command only if a signal file exists.  That's strange, but if
people have been relying on this behavior, so be it.

At this stage, it looks pretty clear to me that there's no consensus
on what to do, and nobody's happy with the proposal of this thread, so
I am going to mark it as rejected.
--
Michael