Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 7 Nov 2023 17:20:27 +0900
From: Michael Paquier <michael@paquier.xyz>
To: David Steele <david@pgmasters.net>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Add recovery to pg_control and remove backup_label
Message-ID: <ZUnzS_yCM0lPXMat@paquier.xyz>
References: <2daf8adc-8db7-4204-a7f2-a7e94e2bfa4b@pgmasters.net>
 <330b8277-e755-428e-b30c-eb9ed9cafd58@pgmasters.net>
 <ZUiJMJc6sRDoGJTJ@paquier.xyz>
 <46c300b9-96a1-449c-8ecd-9003bd02cb6b@pgmasters.net>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="puzT2WXCe0GYUjfO"
Content-Disposition: inline
In-Reply-To: <46c300b9-96a1-449c-8ecd-9003bd02cb6b@pgmasters.net>
Archived-At: 
 <https://www.postgresql.org/message-id/ZUnzS_yCM0lPXMat%40paquier.xyz>
Precedence: bulk


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

On Mon, Nov 06, 2023 at 05:39:02PM -0400, David Steele wrote:
> On 11/6/23 02:35, Michael Paquier wrote:
>> The patch is failing the recovery test 039_end_of_wal.pl.  Could you
>> look at the failure?
>=20
> I'm not seeing this failure, and CI seems happy [1]. Can you give details=
 of
> the error message?

I've retested today, and miss the failure.  I'll let you know if I see
this again.

>> +        /* Clear fields used to initialize recovery */
>> +        ControlFile->backupCheckPoint =3D InvalidXLogRecPtr;
>> +        ControlFile->backupStartPointTLI =3D 0;
>> +        ControlFile->backupRecoveryRequired =3D false;
>> +        ControlFile->backupFromStandby =3D false;
>>=20
>> These variables in the control file are cleaned up when the
>> backup_label file was read previously, but backup_label is renamed to
>> backup_label.old a bit later than that.  Your logic looks correct seen
>> from here, but shouldn't these variables be set much later, aka just
>> *after* UpdateControlFile().  This gap between the initialization of
>> the control file and the in-memory reset makes the code quite brittle,
>> IMO.

Yeah, sorry, there's a think from me here.  I meant to reset these
variables just before the UpdateControlFile() after InitWalRecovery()
in UpdateControlFile(), much closer to it.

> If we set these fields where backup_label was renamed, the logic would not
> be exactly the same since pg_control won't be updated until the next time
> through the loop. Since the fields should be updated before
> UpdateControlFile() I thought it made sense to keep all the updates
> together.
>=20
> Overall I think it is simpler, and we don't need to acquire a lock on
> ControlFile.

What you are proposing is the same as what we already do for
backupEndRequired or backupStartPoint in the control file when
initializing recovery, so objection withdrawn.

> Thomas included this change in his pg_basebackup changes so I did the sam=
e.
> Maybe wait a bit before we split this out? Seems like a pretty small
> change...

Seems like a pretty good argument for refactoring that now, and let
any other patches rely on it.  Would you like to send a separate
patch?

>> Actually, grouping backupStartPointTLI and minRecoveryPointTLI should
>> reduce more the size with some alignment magic, no?
>=20
> I thought about this, but it seemed to me that existing fields had been
> positioned to make the grouping logical rather than to optimize alignment,
> e.g. minRecoveryPointTLI. Ideally that would have been placed near
> backupEndRequired (or vice versa). But if the general opinion is to
> rearrange for alignment, I'm OK with that.

I've not tested, but it looks like moving backupStartPointTLI after
backupEndPoint should shave 8 bytes, if you want to maintain a more
coherent group for the LSNs.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmVJ80sACgkQnvQgOdby
QH3KdRAAmYRK7gFL1cTTrmIKa2l93jIrIbSiNQToD4IM26EM2y6gSXSuWymauHYW
1Z8NrGulf1QvpxbU1R4ea4I4UH7LFJumR9mj7nTmk4aZmMJmvjP4Ll24II5MmAny
fIe0RFH79goIOV90tuNtOqOUKIJvpc49CGt+DaGFpvc6ZJDbMSTSaBIqjman4lPz
lU3A+szuFCEg7Mq8k37eU6D57KirRm0bqZMqtN2xaR9oB9/o8xgD58rMvtF6W8ra
CBoo2LHYMLnEM8bUgYadKhuEkxwgQ2Tr65Dzit3tnau6g/3mpuc0lL++qqaM9Hh6
Pi4eT6q5IocflWAUy2jiajUxJANgU/iBTDtsXkJU1/EbeDgFu5geeyb3mYnpkRFI
Z28sIBBd63Mj2PpM3r8bEJayH7lgHZeHFZpfyZZ02e7KQzrYiWfKkFE7qkjrVQ0O
6TGHBemfdfJu8bCZJH2mOP9y+fPNN/139kCjba84tU6ZpbVESycLP/6cSLa0futH
uFM0RQwt/rYc0PLxx82rtZW4JqAycaJ/8bUF4qniPfuUc7Y9w4oZnBfYwnGsIIPR
Gr3NmpaiLtT+ifcLKoGUSSjXIssirNIvliqclx/DyGTOrZtR8kp/pljl7GBT/bM5
18UuzNz+Sf+TtOOfxhogZLtEIQ04Rp8Cgy64F/CQM4bbr/j3j+w=
=NeRz
-----END PGP SIGNATURE-----

--puzT2WXCe0GYUjfO--