Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 18 Mar 2026 10:43:29 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Haibo Yan <tristan.yim@gmail.com>
Cc: David Steele <david@pgbackrest.org>,
	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: <aboDQSoywyKGGNCr@paquier.xyz>
References: <1248c005-b693-494a-8d7a-68bc7d482679@pgbackrest.org>
 <fe9a5423-c36d-43ad-9884-dee4c3b7dc6d@pgbackrest.org>
 <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>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="ZW23AjjBRkQnF7f4"
Content-Disposition: inline
In-Reply-To: <7F7B289B-F94F-42C2-9E54-6A689C0D64BB@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/aboDQSoywyKGGNCr%40paquier.xyz>
Precedence: bulk


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

On Tue, Mar 17, 2026 at 11:50:29AM -0700, Haibo Yan wrote:
> Thank you for the clarification. I have now read the code, and
> overall it looks good to me. I only had one very small comment.

(Bottom-posting note from above, please be careful.)

> I was just wondering whether the following might be slightly clearer:
> ```
> memset(controlFile, 0, PG_CONTROL_FILE_SIZE);
> memcpy(controlFile, ControlFile, sizeof(ControlFileData));
> ```
>=20
> I do not think this is a real issue, though.

         {
             ControlFile->backupStartPoint =3D checkPoint.redo;
             ControlFile->backupEndRequired =3D backupEndRequired;
+            ControlFile->backupLabelRequired =3D false;

It sounds like it is going to be important to document the reason why
the flag is reset here (aka we don't need the backup_label file
anymore).

+backup_control_file(uint8_t *controlFile)
+{
+    ControlFileData *controlData =3D ((ControlFileData *)controlFile);
+
+    memset(controlFile + sizeof(ControlFileData), 0,
+           PG_CONTROL_FILE_SIZE - sizeof(ControlFileData));
+
+    LWLockAcquire(ControlFileLock, LW_SHARED);
+    memcpy(controlFile, ControlFile, sizeof(ControlFileData));
+    LWLockRelease(ControlFileLock);
+
+    controlData->backupLabelRequired =3D true;
+
+    INIT_CRC32C(controlData->crc);
+    COMP_CRC32C(controlData->crc, controlFile, offsetof(ControlFileData, c=
rc));
+    FIN_CRC32C(controlData->crc);

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.

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.  Actually, the flag
is reset in InitWalRecovery() in the initial steps of recovery, and
the backup_label file is removed much later in StartupXLOG() just
*after* UpdateControlFile() to minimize the window where the contents
of the control file and the backup_label file is removed are
out-of-sync.  This window means that if we crash between the
completion of UpdateControlFile() and the durable_rename() we could
have a flag reset with a backup_label still around.  On restart,
recovery would fail, requiring a manual modification of the control
file, at least.  It sounds to me that this implementation detail
should be documented clearly.

Finally, here is a general opinion.  I like this patch, and it is
basically risk-free for base backups taken with the replication
protocol as we update the control file with the new flag set
on-the-fly.=20

Now, I am worried about backups that use pg_stop_backup().  Changing
backup APIs has always been a very sensitive area, and this is going
to require operators to update backup tools so as the control file
received as a result of pg_stop_backup() is copied, at the cost of
getting a failure if they don't do so.  I will *not* proceed with this
change without a clear approval from some more committers or senior
hackers that they like this change (approach previously suggested by
Andres, actually, for what I can see).  I am adding in CC a few
committers who have commented on this set of proposals and who have
touched the recovery code in the last few years, for awareness.
The timing is what it is, and we are at the end of a release cycle.
Let's see if we can reach a consensus of some kind.
--
Michael

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

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

iQIzBAEBCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmm6A0EACgkQnvQgOdby
QH2bGA//ctPKCNU/9CWuz/GUhwYavjHaGhhMp9+TpdCvlhODUieTNy810sDWc40s
UCmF8TxrIxSQPoPyQ2kE7hCHYH4hXO1IeMCqc/83MxU9VQnnGMzhmue+MFpsfBIp
xOuMdSTwYl6mJ73oDe6Lo2IBbq77yPA9QwDmdTGKOkj6pU/kiI0pRlfaaaAiC0rV
5sFXgea0Vz4nbqOLKXBqs/uhaRo0i1qWWdtsxsl+hDTRPze6pFZffD9Ntwp0m4Fi
TbezTInfbowoZebMJFkw5JxY3AMeQu219BR79EZNBJ6nrKw/EfvV2VlEB4QNPYxD
XnUPvWaHBClfdF3mP8oyEdBqglLz6wwMdGfc7UpYDWJxtAH9p71MEQOOeCs4jw5I
8xdd5FUdty52sOeMrCW1pMyXgheYGlnroM7PkxoBHN3KLQ0VM/tV2gugOgwkUqGN
S8gJVKB7Trx0fsX0LQ63iZl3bdzQRzRxQWYUNwnfRQtpY2iagIEUr/e5hY7XEF9y
n0Hrjeq2uXsIikcqW/syvRBsvJS+aNXu5HOCzf3uL3ZAe+JAoFBY+ZVj+MuArbK/
P8kRKGlNnigd6r9p5H4z4DPkFX+LU+LyDATM5W8B3YqiEa1KaRyIimPh3QxHbsf+
UuVNU6OuBLMmD1oF8DWtovK6kCGw97iwbH3+95jr8cJBbDN+CEM=
=rsfG
-----END PGP SIGNATURE-----

--ZW23AjjBRkQnF7f4--