Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 31 Oct 2023 10:15:21 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Andres Freund <andres@anarazel.de>
Cc: Robert Haas <robertmhaas@gmail.com>, David Steele <david@pgmasters.net>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	pgsql-hackers@lists.postgresql.org, zxwsbg12138@gmail.com,
	david.zhang@highgo.ca
Subject: Re: Requiring recovery.signal or standby.signal when recovering with
 a backup_label
Message-ID: <ZUBVKfL6FR6NOQyt@paquier.xyz>
References: <ZArVOMifjzE7f8W7@paquier.xyz>
 <20230928.125851.2051261991058034791.horikyota.ntt@gmail.com>
 <4f94351c-24e2-cf32-82d3-1e1bf89a0f5e@pgmasters.net>
 <ZRYTa6Qs9Ecseux1@paquier.xyz>
 <d0ce5b28-a14c-9c08-2505-751c5ff50c0d@pgmasters.net>
 <ZSzQG3IF960zQe3-@paquier.xyz>
 <ZTtlTut2uGQa1EgE@paquier.xyz>
 <0f25d737-64c8-4ef3-9873-f2d4f44b1431@pgmasters.net>
 <ZT9Wgn8lz2pETGb9@paquier.xyz>
 <20231030194741.achmawmgheibz73i@awork3.anarazel.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="PRq/Cvx5bM4PInlW"
Content-Disposition: inline
In-Reply-To: <20231030194741.achmawmgheibz73i@awork3.anarazel.de>
Archived-At: <https://www.postgresql.org/message-id/ZUBVKfL6FR6NOQyt%40paquier.xyz>
Precedence: bulk


--PRq/Cvx5bM4PInlW
Content-Type: multipart/mixed; boundary="kgdqikpN8DcwisnN"
Content-Disposition: inline


--kgdqikpN8DcwisnN
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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 s=
tate
> 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.
>=20
> 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=F6dinger'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 fe=
tch
> the necessary wal, but then you'd also have gotten an error.
>
> [...]
>=20
> 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.
>
> [...]
>=20
> I also just don't think that it's always desirable to create a new timeli=
ne.

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=3Dtrue 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

--kgdqikpN8DcwisnN
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0001-Force-crash-recovery-with-backup_label-and-no-.signa.patch"
Content-Transfer-Encoding: quoted-printable

=46rom fbd9fa407c04799ad4401d3ba5c6b67a0d922631 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
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 =3D false;
 bool		InArchiveRecovery =3D false;
@@ -595,13 +597,22 @@ InitWalRecovery(ControlFileData *ControlFile, bool *w=
asShutdown_ptr,
 		List	   *tablespaces =3D NIL;
=20
 		/*
-		 * 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=3Dstream, for example.
 		 */
-		InArchiveRecovery =3D true;
-		if (StandbyModeRequested)
-			EnableStandbyMode();
+		if (ArchiveRecoveryRequested)
+		{
+			InArchiveRecovery =3D true;
+			if (StandbyModeRequested)
+				EnableStandbyMode();
+		}
=20
 		/*
 		 * 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);
 }
=20
 /*
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_base=
backup/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 =3D PostgreSQL::Test::Cluster->new('replica');
=20
 	# Recover main data directory
-	$node2->init_from_backup($node, 'tarbackup2', tar_program =3D> $tar);
+	$node2->init_from_backup($node, 'tarbackup2', tar_program =3D> $tar,
+	    has_restoring =3D> 1);
=20
 	# Recover tablespace into a new directory (not where it was!)
 	my $repTsDir =3D "$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 =3D '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>
=20
--=20
2.42.0


--kgdqikpN8DcwisnN--

--PRq/Cvx5bM4PInlW
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmVAVSkACgkQnvQgOdby
QH3LeQ/+OZ6m4+TeWefj3/IM+gX+LJWeDWAkyspxGm8S59Jd89NCu1TJw+Tu0hvk
4WL7sIUS8ojJ4lWnJmV6QoJ7oaZAxMz6hopBjuvbF1AzBX7LaeGG03Xf59Jf4yvT
vrbn9uGkfyD4LsSsLPD+hq5A+iTF68Db+NPTd2xtbEVCO2wf9ARSjkyORX47iN6d
prvduuNRBf8vRA4ht4EceuqnObkoghdImgm3mZd/j11A23NkBRdacK7c11g/+3qH
HPgj65EkNPw6VmtDc5FNjdeWlUPfMhhlTkfGKN9XPcJBePZg6aElNa8Xjawk0N+C
Nk8L0ne6uCc+eb0onJ+6FVtlwMB/1cibAwacAdPsAVBgDFK1H8HUdoelU5LWO07M
O2tNxwgzHaInnRX6r5Of0bRIFIAMj+ltt4g82s11oOzfrQhkiS/KnsuQkh9KWLB8
isawjrlJvD957TGbFDVXzMQvZIUyjKVm2Utc1oWkpX+j3L6/uvZHkRNm9MeZu16V
U6VaLwr0LDRk7kfBIi6kTHCP0OYMi+UGFWhSQPV9o/8A5IK4toZhZTWIbzdW+UQ4
syBAd4Ve/c9lr0SdEUC3eNxgvnks5UzrAkLnw50H7XBy+OVi2MtNNpmF3/cy8H7S
c98av6qwPHYdt8ouKhIgY7YNTsgJHxcPUOCihpUsZv74KH5wvNQ=
=InJw
-----END PGP SIGNATURE-----

--PRq/Cvx5bM4PInlW--