Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 16 Oct 2023 14:54:35 +0900
From: Michael Paquier <michael@paquier.xyz>
To: David Steele <david@pgmasters.net>
Cc: 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: <ZSzQG3IF960zQe3-@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>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="2x2s7IhQdRv6lQ2H"
Content-Disposition: inline
In-Reply-To: <d0ce5b28-a14c-9c08-2505-751c5ff50c0d@pgmasters.net>
Archived-At: <https://www.postgresql.org/message-id/ZSzQG3IF960zQe3-%40paquier.xyz>
Precedence: bulk


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

On Sat, Oct 14, 2023 at 03:45:33PM -0400, David Steele wrote:
> On 9/28/23 19:59, Michael Paquier wrote:
> OK, I have now reviewed and tested the patch and it looks good to me. I
> stopped short of marking this RfC since there are other reviewers in the
> mix.

Thanks for the review.  Yes, I am wondering if other people would
chime in here.  It doesn't feel like this has gathered enough
opinions.  Now this thread has been around for many months, and we've
done quite a few changes in the backup APIs in the last few years with
few users complaining back about them..

> I dislike that we need to repeat:
>=20
> OwnLatch(&XLogRecoveryCtl->recoveryWakeupLatch);
>=20
> But I see the logic behind why you did it and there's no better way to do=
 it
> as far as I can see.

The main point is that there is no meaning in setting the latch until
the backup_label file is read because if ArchiveRecoveryRequested is
*not* set the startup process would outright fail as of the lack of
[recovery|standby].signal.

>> Another idea I had was to force the creation of recovery.signal by
>> pg_basebackup even if -R is not used.  All the reports we've seen with
>> people getting confused came from pg_basebackup that enforces no
>> configuration.
>=20
> This change makes it more obvious if configuration is missing (since you'=
ll
> get an error), however +1 for adding this to pg_basebackup.

Looking at the streaming APIs of pg_basebackup, it looks like this
would be a matter of using bbstreamer_inject_file() to inject an empty
file into the stream.  Still something seems to be off once
compression methods are involved..  Hmm.  I am not sure.  Well, this
could always be done as a patch independant of this one, under a
separate discussion.  There are extra arguments about whether it would
be a good idea to add a recovery.signal even when taking a backup from
a standby, and do that only in 17~.

>> A last thing, that had better be covered in a separate thread and
>> patch, is about validateRecoveryParameters().  These days, I'd like to
>> think that it may be OK to lift at least the restriction on
>> restore_command being required if we are doing recovery to ease the
>> case of self-contained backups (aka the case where all the WAL needed
>> to reach a consistent point is in pg_wal/ or its tarball)
>=20
> Hmmm, I'm not sure about this. I'd prefer users set
> restore_command=3D/bin/false explicitly to fetch WAL from pg_wal by defau=
lt if
> that's what they really intend.

It wouldn't be the first time we break compatibility in this area, so
perhaps you are right and keeping this requirement is fine, even if it
requires one extra step when recovering a self-contained backup
generated by pg_basebackup.  At least this forces users to look at
their setup and check if something is wrong.  We'd likely finish with
a few "bug" reports, as well :D
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmUs0BsACgkQnvQgOdby
QH1fjQ//YSwlwVzGkH32vEg3+u0PC5mrBPcdRPmKSCAihkOBTyy3hclWIqtHi3sC
WHAxZgi/zlpOH6mOk7M41MOwoyrFgmgl8cv1I8psgGH4+S4VKgAD57RSFEzkdWBg
gt+rOTEUaw+jcbcUiLDJOIlGWaf+iN3Vy+rIChFxEHTVspKlXStnbVUajFw5Z84E
ibA/cFA9dvuSNfIpP4B3r8cUNC4ooxM8H2moisxk+2biopzHwbkKGLbIMlvbcQH/
9TQsfTH9ojJjkEBp/y49UPg2MIWGHaiQ2saKHrigbPXdFMOOzxuEyAGddBoZXAlb
o7eXbMMqzum5v4s2iCZeT25jrZgJgfPshwP9lpc0Dvi551/cdeQFEHz/61ic1IUj
VLW+gNhq/Zk+0URybVKJ+VOWrCzhiybVyFlpV8hMqh3s7mfR7TjbShSJI7X21vT7
apbe5ybNvjzV/psIPlRNhl+PTdUiDVhVCjqER1evjaHl2rLxUuF82OBUssomqSWm
exw/Azd12WUGzrxPltUV/JP/jTW6Z9Q+tsyECA3mTNNxd+0bhDNg40ZvfxAdM4Kk
f9HUXHR6//SCyBquHjXueAfgRctk4MZQEmx7pS4Ul6FuBpb7g2ZRPFUqclVH5Xsc
1l8xWK8Bs3GHoGzsaXCBOB1MgkuqLeewaHDCOpy1tctv5wANcvs=
=mt8O
-----END PGP SIGNATURE-----

--2x2s7IhQdRv6lQ2H--