Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 29 Jun 2026 14:47:58 +0900
From: Michael Paquier <michael@paquier.xyz>
To: JoongHyuk Shin <sjh910805@gmail.com>
Cc: assam258@gmail.com,
	=?iso-8859-1?Q?=C1lvaro?= Herrera <alvherre@kurilemu.de>,
	Scott Ray <scott@scottray.io>, Fujii Masao <masao.fujii@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC
 assign hooks
Message-ID: <akIHDhqT0jdGk8gT@paquier.xyz>
References: 
 <CACSdjfN7uYLS-+wyBbvZ6rmkSNuPm9H8Y+xdd_aSr-kNG_xu3w@mail.gmail.com>
 <ajfWZqpzgziKNzX5@alvherre.pgsql>
 <CACSdjfMt5obQc0QCX8E+v-pxROWFQxmeW6Ki9X16F60D3tSJgg@mail.gmail.com>
 <CAAAe_zD6oOibLjg6zBs5EvNk2C+y0H7wQVp86z_aGDXJg-sMyg@mail.gmail.com>
 <CACSdjfOFz=i2=YT_2kej-+OA0UTyUO1Hc5kEq-2wqzJcmdR6aw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="NVjeOz81ex3CWXfn"
Content-Disposition: inline
In-Reply-To: 
 <CACSdjfOFz=i2=YT_2kej-+OA0UTyUO1Hc5kEq-2wqzJcmdR6aw@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/akIHDhqT0jdGk8gT%40paquier.xyz>
Precedence: bulk


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

On Mon, Jun 29, 2026 at 01:45:37PM +0900, JoongHyuk Shin wrote:
> v7 attached, reworked along the lines discussed. The assign hooks
> no longer touch recoveryTarget; each just stores its own value,
> and recoveryTarget is derived once in validateRecoveryParameters()
> from the settled recovery_target* settings,
> which also rejects setting more than one target.
> The v6 review comments are folded in too.

Hmm.  One aspect of the patch that I have a hard time accepting is
this aspect:
ADD_TARGET_IF_SET("recovery_target", RECOVERY_TARGET_IMMEDIATE);

This part lacks future extensibility, IMO.  If we add more values to
the GUC recovery_target in the future, we push down more complication
to the resolution of the immediate target at the beginning of the
startup process.  Decoupling entirely RecoveryTargetType and the types
of values that can be assigned in the GUC recovery_target may lead to
a nicer result, I suspect..  It also feels wasteful to add an
hypothetical enum for the supported values if we don't have a use for=20
it yet.  Any opinions from others?

+       errdetail("Only one recovery target can be set.  Parameters set: %s=
=2E",
+                  buf.data),
+       errhint("See pg_settings for the parameter values and where each is=
 set."));

pg_settings is just one way to look at these values.  We have also
SHOW and other interfaces.  I would keep the errdetail() with your
first sentence, make the errhint the second sentence of the
errdetail().
--
Michael

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

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

iQIzBAEBCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmpCBw4ACgkQnvQgOdby
QH276w//XgsGk4rPXtEgBs7qvQ2pX67aZH+6Prey8XhWqqdxg4xQIk5FcNBnm/lA
Axsfkux1YTM5vPQrn85NVMlAD2fp99Gb+b07L4HF5qlAhcNk2dvEacINW5VRo7TM
y0UWzyP3Uiv0oCxhfngKrPpzTn9qQNE/BGjDU42XX2xMUD1GoZyI+AAEKfNPT2gw
KU41M7pfn9cWErKoK0zIKwAYdpl8qVH0eNYcO81KuggluvEOUSEXIpI6NsdQmq94
ERolFcocxQne3NYzCTOUm/2hfVe5UXB5CdHVfqAUEl88H4HGBeUCsBfe9aQAi+2Y
Jm2T69/EcaiJmI21ghUEoh5y1h6bAS9x07n4KM/dyM7z9YEc5A/FZ2SpEl1aWGTa
9XKi6v06BqF2vQxKt1CP6KPY/piFnoswm6MPQvVZut51GdDX6SFrlGGwdExLOiRd
5KRzU2ZnzREm6R7QPaGPXOgT5LJiidd1TPGYeydxokchUTWdFdiS3W5GKIZF92JK
CMGTMdy39uRv8dTPOmYyCraBuYukKTUilClDhsx3Iziyu57Q8CMu8k6BsY3K3Ais
ad4X1xQ5OTp0Weah08zStd/UUixE9skCG69/YkSzIZ4X1cPCGsFysO4OyjxIf5Eu
TIIeLKalZEqulmC2pOoRYjJ4gOSGACYmf8r5Bh4sr0L1BJ7BwRQ=
=LuDw
-----END PGP SIGNATURE-----

--NVjeOz81ex3CWXfn--