Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 18 Mar 2026 13:53:51 +0900
From: Michael Paquier <michael@paquier.xyz>
To: David Steele <david@pgbackrest.org>
Cc: Haibo Yan <tristan.yim@gmail.com>,
	Pg Hackers <pgsql-hackers@postgresql.org>,
	Heikki Linnakangas <hlinnaka@iki.fi>,
	Robert Haas <robertmhaas@gmail.com>,
	Andres Freund <andres@anarazel.de>,
	Fujii Masao <masao.fujii@gmail.com>
Subject: Re: Return pg_control from pg_backup_stop().
Message-ID: <abov3-Kre8ntiL-S@paquier.xyz>
References: <edc09339-3a4c-4195-8c06-7aa2953ca5be@pgbackrest.org>
 <d388dba5-dc68-4dab-9b8d-ca301956e85b@pgbackrest.org>
 <8b8aa673-fcef-4e14-a05d-0885283ef1b8@pgbackrest.org>
 <d21f3c7c-563c-49db-8983-faa73ed4369f@pgbackrest.org>
 <f74cab02-5ebb-439c-975b-29d019eed787@pgbackrest.org>
 <17DC1346-0CDE-4E39-B110-3D6FB0797AC6@gmail.com>
 <feb68313-3257-4d9d-8b69-6468e506a61a@pgbackrest.org>
 <7F7B289B-F94F-42C2-9E54-6A689C0D64BB@gmail.com>
 <aboDQSoywyKGGNCr@paquier.xyz>
 <1800c83c-264a-4183-9da5-ac78e25849a8@pgbackrest.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="8V4C4ROzhKllyeSn"
Content-Disposition: inline
In-Reply-To: <1800c83c-264a-4183-9da5-ac78e25849a8@pgbackrest.org>
Archived-At: 
 <https://www.postgresql.org/message-id/abov3-Kre8ntiL-S%40paquier.xyz>
Precedence: bulk


--8V4C4ROzhKllyeSn
Content-Type: multipart/mixed; boundary="wCHQKHAnDpDjn8nr"
Content-Disposition: inline


--wCHQKHAnDpDjn8nr
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 18, 2026 at 04:05:24AM +0000, David Steele wrote:
> On 3/18/26 08:43, Michael Paquier wrote:
>> I was wondering if we should have an assertion at least to cross-check
>> that the contents we store in shared memory never go out-of-sync with
>> the on-disk contents, in the shape of a USE_ASSERT_CHECKING block that
>> calls get_controlfile() and memcmp()'s the contents between shmem and
>> the on-disk file, while the LWLock is taken.  We ship the control file
>> last on purpose, one reason being backups taken from standbys, so that
>> may be sensible to do.
>=20
> As far as I can see this should always be true -- I audited all the
>=20
> LWLockAcquire(ControlFileLock, LW_EXCLUSIVE)
>=20
> sections and the file is always saved once if is updated. Let me see if I
> can add this check without too much pain, e.g. an additional parameter.

This matches with my reads of the code.  The attached check, that can
be applied on top of your patches, passes under check-world.

>> Another property of the new control file flag that is implied in the
>> implementation but not documented is that we should never check for
>> backupLabelRequired when a backup_label is gone.
>=20
> I'm not sure what you mean here? That's exactly when we do want to check =
as
> below:

Sorry for the confusion, I meant that "we should never check for
backupLabelRequired when we have a backup_label".

> Using the pg_control copy from pg_backup_stop() is entirely optional and
> nothing is broken if vendors decide not to use it. This can be demonstrat=
ed
> by applying the 01 patch without 02. In that case the tests in
> 042_low_level_backup continue to run. You can also apply 01 and 02 and
> revert the test changes in 042_low_level_backup and that works, too.

FWIW, after a second look I am actually wondering if 0002 is safe at
all.  The contents of the control file are fetched after we are done
with do_pg_backup_stop(), and there could be a bunch of activity that
happens between the end of do_pg_backup_stop() and the
backup_control_file() call, where the control file could be updated
more and interfere with the recovery startup for some of its fields?
GUC parameter updates that may touch the control file are one thing
popping into mind.
--
Michael

--wCHQKHAnDpDjn8nr
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename=0001-Add-consistency-check-for-shmem-and-on-disk-control-.patch
Content-Transfer-Encoding: quoted-printable

=46rom de5b9b00867e39e73dc99a1ee61a814cead1ab7d Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Wed, 18 Mar 2026 13:44:01 +0900
Subject: [PATCH] Add consistency check for shmem and on-disk control file

---
 src/backend/access/transam/xlog.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam=
/xlog.c
index c8ea1ea49d24..0f8516fcb563 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9606,6 +9606,17 @@ backup_control_file(uint8_t *controlFile)
=20
 	LWLockAcquire(ControlFileLock, LW_SHARED);
 	memcpy(controlFile, ControlFile, sizeof(ControlFileData));
+
+#ifdef USE_ASSERT_CHECKING
+	{
+		bool crc_ok;
+		ControlFileData *dataDisk =3D get_controlfile(DataDir, &crc_ok);
+
+		Assert(crc_ok &&
+			   memcmp(dataDisk, ControlFile, sizeof(ControlFileData)) =3D=3D 0);
+	}
+#endif
+
 	LWLockRelease(ControlFileLock);
=20
 	controlData->backupLabelRequired =3D true;
--=20
2.53.0


--wCHQKHAnDpDjn8nr--

--8V4C4ROzhKllyeSn
Content-Type: application/pgp-signature; name=signature.asc

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

iQIzBAEBCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmm6L98ACgkQnvQgOdby
QH2/rA/5AWNGGXbgqfq1qK16GrHHkQ7H1CwFvHNvH8QTjed2q8mXZ6eNxuSVCsEV
6DchchNjT8KNAcapcUIibPl/KWTFxTLIImkoB8GDbtDZo7QgYDYcnaqhscva4eJP
+vQ3E0s0RWGo+tbZawWdP8DGnn3B/2oRmvH+HvmNyIRT+YJ3l857fAZrOFHBPT9r
vNZGzcpN9WWcSI9c14PowZBrlX7QGZvZ5lGb34iOjfySJPPGi1lri9pznuRHjV9f
Y/drLQ6OojdBZz07RJrO3purCZK0xZyGTZNMEmoM8i8Rx4DbCoSEfAiEzCFUdkv3
S+KzXUvLyj8XU0StXMBrB3p8QiG5Vr6oZc3QgNVPo8i62HwrbX/yOCZcTbK7Aqsh
ONNmdMA0yn9bfd4OvcQofEr/XGkM2sFiPrFygfxIqzcftC7Oa/cS7Ivtdlw7+H+1
ati3Uy+rx3McO7kUA6OBtyF611Rx6/xE7HQ+nJGbtu9LfNN3zV+dsKu3jrZyNAP4
mZyXIm4xaBdr8bMMA2L7FFZzUNIMb0nrBkZfosWGMkusShepu+l5n9mI3NF2DOpf
11mLlP37WN8w0OivoDgWWNrCTtnZ0KHnPPPHwU7D0GvMgeWQAKEGM9Om89odi8Uk
Y+eaKU/WU89Lra2D5bAKPbObDwZIQElVVQTEehTBD0zWR2YGFjg=
=2mr4
-----END PGP SIGNATURE-----

--8V4C4ROzhKllyeSn--