Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 25 Jun 2026 17:00:33 +0900
From: Michael Paquier <michael@paquier.xyz>
To: JoongHyuk Shin <sjh910805@gmail.com>
Cc: =?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: <ajzgIRrl8378bnlt@paquier.xyz>
References: 
 <CACSdjfN7uYLS-+wyBbvZ6rmkSNuPm9H8Y+xdd_aSr-kNG_xu3w@mail.gmail.com>
 <ajfWZqpzgziKNzX5@alvherre.pgsql>
 <CACSdjfMt5obQc0QCX8E+v-pxROWFQxmeW6Ki9X16F60D3tSJgg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="YGBSPxgisI4v+V6L"
Content-Disposition: inline
In-Reply-To: 
 <CACSdjfMt5obQc0QCX8E+v-pxROWFQxmeW6Ki9X16F60D3tSJgg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ajzgIRrl8378bnlt%40paquier.xyz>
Precedence: bulk


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

On Mon, Jun 22, 2026 at 01:37:27PM +0900, JoongHyuk Shin wrote:
> Thanks for the review.

Please avoid top-posting when replying on the lists.  This breaks the
logical flow of the thread.

> The five GetConfigOption() checks are now a local macro, as suggested.

+                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                 errmsg("multiple recovery targets specified"),
+                 errdetail("Recovery targets %s are set, but only one can =
be set.",
+                           buf.data),

Is this errdetail() the optimal choice, though?  It feels a bit odd to
have that mid-sentence.  Perhaps we should use a colon to separate the
list of parameters, say simply:
"Parameters set are: %s."

The trick of Alvaro to make the list of parameters translatable is
interesting.  Didn't know that.

The tests feel bloated to me, bumping the time it takes to run 003 by
30%~40% or so.  The "Three conflicting targets" has for example little
value.  I think that we should also get rid of most of the tests where
we do the patterns for "-c recovery_target_foo=3Dbar -c
recovery_target_foo=3D", repeated for each target type.  One should be
enough.  Keeping only one failure case should be enough for two
recovery targets (or all of them set, why not).

With the first tests in place, I am not convinced that the
anti-pattern "-c recovery_target_foo=3D -c recovery_target_foo=3Dbar" is=20
needed.  Let's just remove it.

This code is clearly AI-generated.  Comments are equally bloated with
descriptions that can be understood just by reading the code.  Let's
simplify all that.

In CheckRecoveryTargetConflicts(), initStringInfo() does an
allocation.  Perhaps we should free it after making sure we don't
FATAL.  Just a good practice.
--
Michael

--YGBSPxgisI4v+V6L
Content-Type: application/pgp-signature; name=signature.asc

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

iQIzBAEBCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmo84CEACgkQnvQgOdby
QH0ERg/5AfdnHYuAoJ6O6ZRsLXQeMzDbLyFVolsM/qHxEFasjX/AKcEI83REbH8x
VIxppwll3ny73LzqZNecrMfucBcRkA3TMqFGvwVeF1NVp10/V1MVF+tTQYPB2iDp
QHo8/iMOqF6P66yHz8+ebSF+jKfDXSIBbyF3lD0c/bSSReTHxtnTCNKR0/MiOXq9
Ogm0+au86XmB0AHuNdGvGgjDP7HDzMggI1h3ioGntOElsbr3yoLG9KkN1g/H+dBb
zwp1Yo1iQcBqLF8IDyWmsOW/4GzbkMehi7W2PI32DgUsrjD4IzyOT2VkdlnVkxUL
igcoDoWgU6kJ+HP5ok5S1f9zDhTqq2+LFKMnShFx/ShrtmZYwj8UGux7qs+vzOyx
amzelzudacbRpEY09kMo+XoVz8QHdKyIZxrzMtT2zQi+nqSlCdaF0T00KXx91/Cx
dy3vnnZmEcdm2R706Y2HQeBrI6Z2pBpkdCVAG1Okrp3j77rDKpJfztYj7XOCWjwu
I/ycKCZXZcurdAu18etYtHRncZleqwe7OYt2NzuuCO65TtHBrRR3ClHqHlWxLwaX
BHMKKn9SGlO2GxpD0rmgQQ2A+OugExOToF9CvFpfk0rSfzgz9fiUPokGbsOed3it
sbOUZfYkHDFnzUVq7D/Oo05myOIJ2rbLsfqBOj72D4uyhFOp6hk=
=ygBN
-----END PGP SIGNATURE-----

--YGBSPxgisI4v+V6L--