Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 7 Jul 2026 15:58:18 +0900
From: Michael Paquier <michael@paquier.xyz>
To: JoongHyuk Shin <sjh910805@gmail.com>
Cc: Zsolt Parragi <zsolt.parragi@percona.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC
 assign hooks
Message-ID: <akyjimBBiJt20ecB@paquier.xyz>
References: <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>
 <akIHDhqT0jdGk8gT@paquier.xyz>
 <CAN4CZFMT0gcN5n86U4HBhcq5ppsJP-tjGd2wZGwSLhnXoFODsg@mail.gmail.com>
 <akMBSHLlmeeuzK1a@paquier.xyz>
 <CACSdjfM9JMFfr0H+fYor5Y7Hkxh8_Ap6_5y=8B9ZoVaOh1CFKQ@mail.gmail.com>
 <akwsiJJ_f0u60j-v@paquier.xyz>
 <CACSdjfPm+_EE2BTmu7oyW_Gt_7o2eZyNdvaT3fQKbaoKCwGOMQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="zVLD23aPq3tu7mfO"
Content-Disposition: inline
In-Reply-To: 
 <CACSdjfPm+_EE2BTmu7oyW_Gt_7o2eZyNdvaT3fQKbaoKCwGOMQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/akyjimBBiJt20ecB%40paquier.xyz>
Precedence: bulk


--zVLD23aPq3tu7mfO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Jul 07, 2026 at 12:01:59PM +0900, JoongHyuk Shin wrote:
> Attached v8 with the errhint removed.  No other changes from v7.

+               errmsg("multiple recovery targets specified"),
+               errdetail("Only one recovery target can be set. Parameters set: %s.",
+                         buf.data));

FWIW, I think that this is redundant, as the errmsg and the errdetail
are basically saying the same thing.  I'd suggest a simpler:
errmsg: cannot specify more than one recovery target
errdetail: Parameters set are: %s.

+# that does not clear another GUC's target.  Conflicts are detected at every
+# server start by DetermineRecoveryTargetType().

We don't really care about the function name here, just that multiple
targets are blocked.

+# LSN after the final 6000-row insert and WAL switch.  The set-then-clear case
+# below has no recovery target and replays all WAL, so it polls on this instead
+# of $lsn5, which would race the 5001-6000 rows.

I smell of an AI set of comments.  We could just remove the whole and
not lose value in understanding the meaning of the test.  A bunch of
the comments added to the TAP script could also be trimmed down quite
a bit, made simpler..

With recovery_target assign hook being removed for the case of
immediate, a test case that checks for a conflict between immediate
and a secondary target may be in order.

Please note that Fujii-san is registered as a committer of this patch,
so I am not planning to go beyond a review here.
--
Michael

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

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

iQIzBAEBCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmpMo4kACgkQnvQgOdby
QH3r4hAApx/ZbC3BMVT5mjGmElS1QGRfwnmiZCd2gv3n9O6TlXqc3alud+MSYP1i
Fx06Jv4srB4OaETRO1SGfL9/+ZvZZYpgghiNWLBwwj7wBBEmhKc9w/+TENWPYbAw
XawoqXXD5DcbiE4bLIhA3vglgTYSl8N8N02fUD2scwT2jtxJC0f2mnigRa8GsCE2
QBQkjIa2YMFaIpXXlT+SJW39TWmuZ4/zcsGrX/dhoLKeWPWMSWOOivVd8suIgVRJ
nTkuyYn4+L13BlDJFPXEjQh3pVXWebmU6GQJtbHMJ+iy94XGpP/n/K2YjUzxzjyZ
5WhUzOIaMrGOdQNxnbGrzNag/skq/ZXmDgpECCC53+Bp2vqYNagvnf2JYZ0O6DNy
7lZuh4Oto6dYhnpxOl8i2vS4k8HPdwlLq0JtKC/pq3TUbnF5e1R8ExEGQFhAdyKS
XcsBxaCEBUSmcp2b6P9EM176rFqHb7D2MmOnZrMg1FuLiiTvN3J961a/GyZuFz04
9FtPsSanbBWavt/Kd9zgrFLA4xSOoovkhXfiUfKErP8MnwEDgNE/OLJNlPSz85+Q
uOaL31jP+0epdXBJtO7BfBH9pJB+cqkEDeTW51UvcZhr/Xu5wOz+umJlTUJt7zNW
2TeULbPdkq9GRBJa4k3vjePpbWaC1Ik5IXPWkIuKzOVpqGACgLI=
=O6Aa
-----END PGP SIGNATURE-----

--zVLD23aPq3tu7mfO--