Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 1 Nov 2023 08:39:17 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Robert Haas <robertmhaas@gmail.com>
Cc: David Steele <david@pgmasters.net>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	pgsql-hackers@lists.postgresql.org, zxwsbg12138@gmail.com,
	david.zhang@highgo.ca, Andres Freund <andres@anarazel.de>
Subject: Re: Requiring recovery.signal or standby.signal when recovering with
 a backup_label
Message-ID: <ZUGQJQjV3QqH0rRd@paquier.xyz>
References: <4f94351c-24e2-cf32-82d3-1e1bf89a0f5e@pgmasters.net>
 <ZRYTa6Qs9Ecseux1@paquier.xyz>
 <d0ce5b28-a14c-9c08-2505-751c5ff50c0d@pgmasters.net>
 <ZSzQG3IF960zQe3-@paquier.xyz>
 <ZTtlTut2uGQa1EgE@paquier.xyz>
 <0f25d737-64c8-4ef3-9873-f2d4f44b1431@pgmasters.net>
 <ZT9Wgn8lz2pETGb9@paquier.xyz>
 <CA+TgmoaHhzE=2FZCQPoa5D5x1Ks3SmbwqNzgYRKiuxhO2kQnng@mail.gmail.com>
 <ZUBM6BNQnEh7lzIM@paquier.xyz>
 <CA+TgmoYmWhN4u4zxmdYciaHJZdqkGKzpn==sbSOeRP8PJFSvoA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="6adJR9BzAyG1b4om"
Content-Disposition: inline
In-Reply-To: <CA+TgmoYmWhN4u4zxmdYciaHJZdqkGKzpn==sbSOeRP8PJFSvoA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZUGQJQjV3QqH0rRd%40paquier.xyz>
Precedence: bulk


--6adJR9BzAyG1b4om
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Oct 31, 2023 at 08:28:07AM -0400, Robert Haas wrote:
> On Mon, Oct 30, 2023 at 8:40=E2=80=AFPM Michael Paquier <michael@paquier.=
xyz> wrote:
>> As far as I know, there's one paragraph in the docs that implies this
>> mode without giving an actual hint that this may be OK or not, so
>> shrug:
>> https://www.postgresql.org/docs/devel/continuous-archiving.html#BACKUP-T=
IPS
>> "As with base backups, the easiest way to produce a standalone hot
>> backup is to use the pg_basebackup tool. If you include the -X
>> parameter when calling it, all the write-ahead log required to use the
>> backup will be included in the backup automatically, and no special
>> action is required to restore the backup."
>=20
> I see your point, but that's way too subtle. As far as I know, the
> only actually-documented procedure for restoring is this one:
> https://www.postgresql.org/docs/current/continuous-archiving.html#BACKUP-=
PITR-RECOVERY
>=20
> That procedure actually is badly in need of some updating, IMHO,
> because close to half of it is about moving your existing database
> cluster out of the way, which may or may not be needed in the case of
> any particular backup restore. Also, it unconditionally mentions
> creating recovery.signal, with no mention of standby.signal. And
> certainly not with neither. It also gives zero motivation for actually
> doing this and says nothing useful about backup_label.
>=20
> Both recovery.signal and standby.signal are documented in
> https://www.postgresql.org/docs/current/runtime-config-wal.html#RUNTIME-C=
ONFIG-WAL-ARCHIVE-RECOVERY
> but you'd have no real reason to look in a list of GUCs for
> information about a file on disk. recovery.signal but not
> standby.signal is mentioned in
> https://www.postgresql.org/docs/current/warm-standby.html but nowhere
> that I can find do we explicitly talk about running with at least one
> of them.

Point 7. of what you quote says to use one?  True that this needs a
refresh, and perhaps a bit fat warning about the fact that these are
required if you want to fetch WAL from other sources than the local
pg_wal/.  Perhaps there may be a point of revisiting the default
behavior of recovery_target_timeline in this case, I don't know.

>> As you're telling me, and I've considered that as an option as well,
>> perhaps we should just consider the presence of a backup_label file
>> with no .signal files as a synonym of crash recovery?  In the recovery
>> path, currently the essence of the problem is when we do
>> InArchiveRecovery=3Dtrue, but ArchiveRecoveryRequested=3Dfalse, meaning
>> that it should do archive recovery but we don't want it, and that does
>> not really make sense.  The rest of the code sort of implies that this
>> is not a suported combination.  So basically, my suggestion here, is
>> to just replay WAL up to the end of what's in your local pg_wal/ and
>> hope for the best, without TLI jumps, except that we'd do nothing.
>
> This sentence seems to be incomplete.

I've re-read it, and it looks OK to me.  What I mean with this
paragraph are two things:
- Remove InArchiveRecovery=3Dtrue and ArchiveRecoveryRequested=3Dfalse as
a possible combination in the code.
- Treat backup_label with no .signal file as the same as crash
recovery, that:
-- Does no TLI jump at the end of recovery.
-- Expects all the WAL to be in pg_wal/.

> But I was not saying we should treat the case where we have a
> backup_label file like crash recovery. The real question here is why
> we don't treat it fully like archive recovery.

Timeline jump at the end of recovery?  Archive recovery forces a TLI
jump by default at the end of redo if there's a signal file, and some
users may not want a TLI jump by default?

> I don't know off-hand
> what is different if I start the server with both backup_label and
> recovery.signal vs. if I start it with only backup_label, but I
> question whether there should be any difference at all.

Perhaps we could do that, but note that backup_label is renamed to
backup_label.old at the beginning of redo.  The code has historically
always enforced InArchiveRecovery=3Dtrue when there's a backup label,
and InArchiveRecovery=3Dfalse where there is no backup label, so we
don't get the same recovery behavior if a cluster is restarted while
it was still performing recovery.  I don't quite see how it is
possible to make this code simpler without enforcing a policy to take
care of this inconsistency.  I've listed two of them on this thread:
- Force the presence of a .recovery file when there is a
backup_label, to force archive recovery.
- Force crash recovery if there are no signal files but a
backup_label, then a restart of a cluster that began a restore while
it processed a backup would be confused: should it do crash recovery
or archive recovery?

My guess, based on what I read from the feedback of this thread, is
that it could be more helpful to do the second thing, not the third
one, because this is better with standalone backups: no TLI jumps and
restore happens with all the local WAL in pg_wal/, without any GUCs to
control how recovery should run.

You are suggesting a third, hybrid, approach.  Now note we have always
checked for signal files before the backup_label.  Recovery GUCs are
checked only if there's one of the two signal files.  It seems to me
that what you are suggesting would make the code a bit harder to
follow, actually, and more inconsistent with stable branches because
we would need to check the control file contents *before* checking for
the .signal files or backup_label to be able to see if archive
recovery *should* happen, depending on if there's a backupEndPoint.

>> A point of contention is if we'd better be stricter about satisfying
>> backupEndPoint in such a case, but the redo code only wants to do
>> something here when ArchiveRecoveryRequested is set (aka there's a
>> .signal file set), and we would not want a TLI jump at the end of
>> recovery, so I don't see an argument with caring about backupEndPoint
>> in this case.
>=20
> This is a bit hard for me to understand, but I disagree strongly with
> the idea that we should ever ignore a backup end point if we have one.

Actually, while experimenting yesterday before sending my reply to
you, I have noticed that redo cares about backupEndPoint even if you
force crash recovery when there's only a backup_label file.  There's a
case in pg_basebackup that would fail, but that's accidental, AFAIK:
https://www.postgresql.org/message-id/ZUBVKfL6FR6NOQyt%40paquier.xyz

See in StartupXLOG(), around the comment "complain if we did not roll
forward far enough to reach".  This complains if archive recovery has
been requested *or* if we retrieved a backup end LSN from the
backup_label.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmVBkCUACgkQnvQgOdby
QH0yYxAAj42HQwXycoODjOAM5l9RMNisyPJACRvx7/1IVSsdEp1G9nJAYocI/1Yo
PSJ6PYFyub207oxFa6ObPuDWXNyM5nOSZmUXX4+jTi29e+o/DI4SnRqGuYJYi7v2
uTH6RtjytfL/reS1N+BiM1enThcDT7UiHK8cyhrxvNUt8sdBIO4WhPg3vlxJ5W1r
ptKdIEYtJxlybYV5cA1c5byHuPvXxrf3Ua5inqpg5ijM636xUc0AwmOFWh59g+MA
EM40mdMVRoCKyyK/GeBP6J8w6kTx49IlgV0fsbqsUaJqndpKzzy7ZcGrZI4DF+4R
IVrceZefiKMzcEfkJ2mHydJgPzJYFnUQMjmPrNvWie2w4mNS2L71933ugdyKxTG3
wV1bXC6/nMRYZEsICYrU5gf34St4UoXT5eE5INV3LbIAIeNP6hucc2JeeZrFPLpf
hcmJbQdQ6oSAQdChfL0PfMHugpp+DJtKWMbaNyhl10BOqQ11JZ6q4onb3BgOA+R4
f6F9j6BdnVlQkhC6DSIAZD9eJGg69b84e/sU75JiHRuA0QOhYbpvIbJJcvlXDSOw
6rBSiB6cAoF8UFgX3pTYdQuPCcnbdaMM7r/LUYgvB97jRBfaJn8eOitK/wcKkkq+
sr/6PKzkyeqPwLA2ETkk9VElkpP8vTea0jTkZmn1UWUXks+839k=
=nLJ2
-----END PGP SIGNATURE-----

--6adJR9BzAyG1b4om--