Feedback-ID: i0fe9450f:Fastmail
Date: Fri, 10 Nov 2023 13:37:00 +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: <ZU2zbPwGwy5ammRm@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>
 <ZUnzS_yCM0lPXMat@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="EjUR43iDrfg31goy"
Content-Disposition: inline
In-Reply-To: <ZUnzS_yCM0lPXMat@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/ZU2zbPwGwy5ammRm%40paquier.xyz>
Precedence: bulk


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

On Tue, Nov 07, 2023 at 05:20:27PM +0900, Michael Paquier wrote:
> On Mon, Nov 06, 2023 at 05:39:02PM -0400, David Steele wrote:
> I've retested today, and miss the failure.  I'll let you know if I see
> this again.

I've done a few more dozen runs, and still nothing.  I am wondering
what this disturbance was.

>> If we set these fields where backup_label was renamed, the logic would n=
ot
>> 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.
>=20
> 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.
>=20
>> Thomas included this change in his pg_basebackup changes so I did the sa=
me.
>> Maybe wait a bit before we split this out? Seems like a pretty small
>> change...
>=20
> 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?

The split still looks worth doing seen from here, so I am switching
the patch as WoA for now.

>>> 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 alignmen=
t,
>> 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.
>=20
> 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.

+    * backupFromStandby indicates that the backup was taken on a standby. =
It is
+    * require to initialize recovery and set to false afterwards.
s/require/required/.

The term "backup recovery", that we've never used in the tree until
now as far as I know.  Perhaps this recovery method should just be
referred as "recovery from backup"?

By the way, there is another thing that this patch has forgotten: the
SQL functions that display data from the control file.  Shouldn't
pg_control_recovery() be extended with the new fields?  These fields
may be less critical than the other ones related to recovery, but I
suspect that showing them can become handy at least for debugging and
monitoring purposes.

Something in this area is that backupRecoveryRequired is the switch
controlling if the fields set by the recovery initialization.  Could
it be actually useful to leave the other fields as they are and only
reset backupRecoveryRequired before the first control file update?
This would leave a trace of the backup history directly in the control
file.

What about pg_resetwal and RewriteControlFile()?  Shouldn't these
recovery fields be reset as well?

git diff --check is complaining a bit.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmVNs2wACgkQnvQgOdby
QH3c/A//RUzgm2dUyeEklMGQX3k74oUx0MsuwACUXdFXPH1//ybPk9t+Iqjcc7zZ
wPBsgwHhufY/ZegHlwiTUYZfhrAHzHfEFN4N831qVFI4UAxaFbhHRkAFgCfgDkh3
8O0XlLe54+LxMLD5m6iDIf+WFbJb2dG2hHShskpyXXO0HpblRMZvubJfm0DaRVkv
rZCuwG0/G1MhYP+FAEuvV5xNp4awTfJaOovWqcJX/t+myinEDViVluiL/sYjqai1
Qi6MvQ+lxoQyVzsSU6k+wzdLookOWQ5oH8jU37uC6oqxSh4BB7BC8xSkmc3nD9FN
6yAJdMBCcQoM+WEhsSiEi+VRAXgXWOBM02XYySqbIfQ3eE7BWwUWR4yh7i4wSYFn
fRPGjA//BIxfBJst9cOELjCKzIXQGfk0wCvBapbw+ggxRUkn4xkTM8le2f69sbyx
N3rRMvLhRaxGSR7toseU/jEiA9lTHwupfy2sBMlwkvRgiJID4h58+rh+p9YPYyf2
Ox6wf1bvVdKdcgZ7e6/yvQdY1l3yp6bnFxdjrCNtNCrui4Lr6dWYwo7u5BfnfDeu
K8n9fuSwXRiI5Q6FSpBFG7KlepeQ/tKQxm4MPnrfRzlHXETH5VjJo1WtFoLoQf00
sV5JNovRMkl9afv54AOOKqOH+pYbz9sBucwqZYLpejIScC1iwdw=
=3uLD
-----END PGP SIGNATURE-----

--EjUR43iDrfg31goy--