MIME-Version: 1.0
References: 
 <CACSdjfPUa4UvKjADgOERXoxNYmCg2mqqiqKkiJk6mX6E4qgVFw@mail.gmail.com>
 <CAHGQGwFNjrLfUSyT4McR4wH_2yFV3n7+63eyvDjzxQchyyQrPA@mail.gmail.com>
 <ae77WrLmbJ7t2niR@paquier.xyz>
 <CACSdjfPCPjP9tuzxgarcbkjahu-n+o+LJbUQ22aPNkOsOvrEZg@mail.gmail.com>
 <CAHGQGwF24XMh=35m+FdqLUw4QgK8HmAGwPZyBDKtsw7MWYp_hw@mail.gmail.com>
 <CACSdjfNY=aePEYGYV_5tamz-iTLQLitBuHc8youfsTzfDR1+_g@mail.gmail.com>
 <CACSdjfMohHudUh7Fq=pD=Zk+=J=4E65c6VGgScG3CAXvj_Sorg@mail.gmail.com>
 <M5a4v6-PJIZSNQlLhGy-GgBPCXNkrW3OnbESqTpaXh2TyZl4TsMyY_wPK-chiMOoZ4mr9Dm7vuqDSVmNeMKfZYNYlI5PxoQyC6iqWZ9MTAk=@scottray.io>
 <CACSdjfOxnez=Cv8CzvJcupnaSjaQFnXqk5t=C1ES0BXp52faFg@mail.gmail.com>
 <0WrsV1VojwurDot6hdS0norm0nK9QFDk0KSLqBcXK4Xz2b_sMuIdE0zyiXP0p1hTRD7Hwz3C8AQouGyV7yZMhf5joz3zUpQee3l_pWPs4dk=@scottray.io>
In-Reply-To: 
 <0WrsV1VojwurDot6hdS0norm0nK9QFDk0KSLqBcXK4Xz2b_sMuIdE0zyiXP0p1hTRD7Hwz3C8AQouGyV7yZMhf5joz3zUpQee3l_pWPs4dk=@scottray.io>
From: JoongHyuk Shin <sjh910805@gmail.com>
Date: Sun, 7 Jun 2026 19:30:02 +0900
Message-ID: 
 <CACSdjfNr8cm1-GPBv8Sqa7N9Uhi9-gR4wHzbTpm9rmW2D=4ftg@mail.gmail.com>
Subject: Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign
 hooks
To: Scott Ray <scott@scottray.io>
Cc: Fujii Masao <masao.fujii@gmail.com>,
 Michael Paquier <michael@paquier.xyz>,
	pgsql-hackers@lists.postgresql.org
Content-Type: multipart/alternative; boundary="0000000000001456770653a76120"
Archived-At: 
 <https://www.postgresql.org/message-id/CACSdjfNr8cm1-GPBv8Sqa7N9Uhi9-gR4wHzbTpm9rmW2D%3D4ftg%40mail.gmail.com>
Precedence: bulk

--0000000000001456770653a76120
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On "=3D" vs "set to": I'd stay with "=3D".
The list is assembled in appendStringInfo and ends up in the dynamic %s,
so prose like "set to" there never goes through gettext
and would print in English even under a translated lc_messages.
"=3D" is punctuation, so it sidesteps that.
PG already uses this form in the RI detail,
e.g. Key (%s)=3D(%s) in ri_triggers.c.

That said, I'm not sure the currently-set list belongs in the errdetail at
all,
since an operator can read the values back from the configuration.
I'd be interested in the committer's view on whether it is worth adding.

--=20
JH Shin

On Sun, Jun 7, 2026 at 5:10=E2=80=AFAM Scott Ray <scott@scottray.io> wrote:

> > * Function structure: the recovery_target_* set has been
> > historically stable, so array + loop abstraction adds limited
> > value; function size grows ~34% (32 -> 43 lines) for one line of
> > savings on a hypothetical sixth GUC, while the closest precedent
> > (archive_command / archive_library in pgarch.c) is a hard-coded literal=
.
> >
>
> > * errhint vs errdetail: errhint("At most one of %s can be set.")
> > reads more like a constraint than an action hint. The closest
> > precedent, archive_command / archive_library in pgarch.c
> > (ProcessPgArchInterrupts() / LoadArchiveLibrary()), keeps the
> > enumeration in errdetail and omits errhint entirely.
> >
>
> > * TAP regex: the added like() uses [^"]+ for the values, which
> > passes regardless of the actual value. Using quotemeta on the
> > expected values would verify the actual content, and anchoring
> > would also avoid accidentally matching the same tokens inside
> > errhint.
>
> Thanks for taking a look. I attached a v2 that applies your suggestions
> and uses "set to" instead of "=3D" to match convention. What do you think=
?
>
> Sample output:
>
>   FATAL:  multiple recovery targets specified
>   DETAIL:  At most one of "recovery_target", "recovery_target_lsn",
>   "recovery_target_name", "recovery_target_time",
>   "recovery_target_xid" can be set. Currently set:
>   "recovery_target_time" set to "2026-01-01 00:00:00",
>   "recovery_target_xid" set to "700".
>
> --
> Scott Ray
>

--0000000000001456770653a76120
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">On &quot;=3D&quot; vs &quot;set to&quot;: I&#39;d stay wit=
h &quot;=3D&quot;.<br>The list is assembled in appendStringInfo and ends up=
 in the dynamic %s,<br>so prose like &quot;set to&quot; there never goes th=
rough gettext<br>and would print in English even under a translated lc_mess=
ages.<br>&quot;=3D&quot; is punctuation, so it sidesteps that.<br>PG alread=
y uses this form in the RI detail,<br>e.g. Key (%s)=3D(%s) in ri_triggers.c=
.<br><br>That said, I&#39;m not sure the currently-set list belongs in the =
errdetail at all, <br>since an operator can read the values back from the c=
onfiguration.<br>I&#39;d be interested in the committer&#39;s view on wheth=
er it is worth adding.<br><br>-- <br>JH Shin</div><br><div class=3D"gmail_q=
uote gmail_quote_container"><div dir=3D"ltr" class=3D"gmail_attr">On Sun, J=
un 7, 2026 at 5:10=E2=80=AFAM Scott Ray &lt;<a href=3D"mailto:scott@scottra=
y.io">scott@scottray.io</a>&gt; wrote:<br></div><blockquote class=3D"gmail_=
quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,=
204);padding-left:1ex">&gt; * Function structure: the recovery_target_* set=
 has been<br>
&gt; historically stable, so array + loop abstraction adds limited<br>
&gt; value; function size grows ~34% (32 -&gt; 43 lines) for one line of<br=
>
&gt; savings on a hypothetical sixth GUC, while the closest precedent<br>
&gt; (archive_command / archive_library in pgarch.c) is a hard-coded litera=
l.<br>
&gt; <br>
<br>
&gt; * errhint vs errdetail: errhint(&quot;At most one of %s can be set.&qu=
ot;)<br>
&gt; reads more like a constraint than an action hint. The closest<br>
&gt; precedent, archive_command / archive_library in pgarch.c<br>
&gt; (ProcessPgArchInterrupts() / LoadArchiveLibrary()), keeps the<br>
&gt; enumeration in errdetail and omits errhint entirely.<br>
&gt; <br>
<br>
&gt; * TAP regex: the added like() uses [^&quot;]+ for the values, which<br=
>
&gt; passes regardless of the actual value. Using quotemeta on the<br>
&gt; expected values would verify the actual content, and anchoring<br>
&gt; would also avoid accidentally matching the same tokens inside<br>
&gt; errhint.<br>
<br>
Thanks for taking a look. I attached a v2 that applies your suggestions<br>
and uses &quot;set to&quot; instead of &quot;=3D&quot; to match convention.=
 What do you think?<br>
<br>
Sample output:<br>
<br>
=C2=A0 FATAL:=C2=A0 multiple recovery targets specified<br>
=C2=A0 DETAIL:=C2=A0 At most one of &quot;recovery_target&quot;, &quot;reco=
very_target_lsn&quot;,<br>
=C2=A0 &quot;recovery_target_name&quot;, &quot;recovery_target_time&quot;,<=
br>
=C2=A0 &quot;recovery_target_xid&quot; can be set. Currently set:<br>
=C2=A0 &quot;recovery_target_time&quot; set to &quot;2026-01-01 00:00:00&qu=
ot;,<br>
=C2=A0 &quot;recovery_target_xid&quot; set to &quot;700&quot;.<br>
<br>
--<br>
Scott Ray<br>
</blockquote></div>

--0000000000001456770653a76120--