MIME-Version: 1.0
References: 
 <CACSdjfPUa4UvKjADgOERXoxNYmCg2mqqiqKkiJk6mX6E4qgVFw@mail.gmail.com>
 <CAHGQGwExsY9XvxN=u4ip7pA+_gO-ms8EwgcRZF3Nj84Vi8yeBQ@mail.gmail.com>
 <ae7BQx5D6Y-E2Dqu@paquier.xyz>
 <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>
In-Reply-To: 
 <M5a4v6-PJIZSNQlLhGy-GgBPCXNkrW3OnbESqTpaXh2TyZl4TsMyY_wPK-chiMOoZ4mr9Dm7vuqDSVmNeMKfZYNYlI5PxoQyC6iqWZ9MTAk=@scottray.io>
From: JoongHyuk Shin <sjh910805@gmail.com>
Date: Thu, 4 Jun 2026 14:41:01 +0900
Message-ID: 
 <CACSdjfOxnez=Cv8CzvJcupnaSjaQFnXqk5t=C1ES0BXp52faFg@mail.gmail.com>
Subject: Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign
 hooks
To: 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="000000000000ebf59d065366fd40"
Archived-At: 
 <https://www.postgresql.org/message-id/CACSdjfOxnez%3DCv8CzvJcupnaSjaQFnXqk5t%3DC1ES0BXp52faFg%40mail.gmail.com>
Precedence: bulk

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

Thanks for the patch.
I went through 'v1-0001-Report-....patch'
and have a few observations to share.

* 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.

On the reload trap:
I reproduced this on master and confirmed it's there exactly as you noted.
ALTER SYSTEM doesn't trigger the assign hook;
it just writes to postgresql.auto.conf,
so the trap window is intrinsic to PGC_POSTMASTER + ALTER SYSTEM.
A separate follow-up patch in the reload path feels natural.

--=20
JH Shin

On Mon, Jun 1, 2026 at 6:11=E2=80=AFAM Scott Ray <scott@scottray.io> wrote:

> Thanks for the patch.  I've attached v1-0001 (atop v4) addressing the
> UX and test-coverage items below.  Happy to rework or fold in however
> you prefer.
>
> 1. There's a configuration trap in master and in this branch that
> could be prevented using something very similar to
> CheckRecoveryTargetConflicts to check pending GUCs:
>
>     psql -c "ALTER SYSTEM SET recovery_target_xid TO '700'"
>     psql -c "ALTER SYSTEM SET recovery_target_time TO '2026-01-01
> 00:00:00'"
>     pg_ctl reload
>
> The log shows:
>
>     LOG:  received SIGHUP, reloading configuration files
>     LOG:  parameter "recovery_target_xid" cannot be changed without
> restarting the server
>     LOG:  parameter "recovery_target_time" cannot be changed without
> restarting the server
>     LOG:  configuration file "postgresql.auto.conf" contains errors;
> unaffected changes were applied
>
> pg_settings shows:
>
>     postgres=3D# SELECT name, setting, pending_restart FROM pg_settings
>                 WHERE name LIKE 'recovery_target%' AND pending_restart;
>             name         | setting | pending_restart
>     ---------------------+---------+-----------------
>      recovery_target_time |         | t
>      recovery_target_xid  |         | t
>
> The db runs fine until the next restart, maybe hours later:
>
>     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.
>
> Is it worth a follow-up to report the conflict early and loud?
>
> 2. There's an opportunity to provide a better UX by reporting which
> flags were set and what the values were, so that the user doesn't have
> to search config files or other logs to find this info.  For instance,
> in the postgresql.auto.conf scenario above, instead of:
>
>     DETAIL:  At most one of "recovery_target", "recovery_target_lsn",
>     "recovery_target_name", "recovery_target_time",
>     "recovery_target_xid" can be set.
>
> The operator could see:
>
>     DETAIL:  The following recovery target parameters are set:
>     "recovery_target_time" =3D "2026-01-01 00:00:00",
>     "recovery_target_xid" =3D "700".
>     HINT:  At most one of "recovery_target", "recovery_target_lsn",
>     "recovery_target_name", "recovery_target_time",
>     "recovery_target_xid" can be set.
>
> 3. 003_recovery_targets.pl:339 currently tests recovery_target_xid's
> cleared-then-set behavior.  The patch adds the same coverage for the
> other four recovery_target_* GUCs.
>
>
> --
> Scott Ray
>

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

<div dir=3D"ltr">Thanks for the patch. =C2=A0<br>I went through &#39;v1-000=
1-Report-....patch&#39; <br>and have a few observations to share.<br><br>* =
Function structure: the recovery_target_* set has been<br>=C2=A0 historical=
ly stable, so array + loop abstraction adds limited<br>=C2=A0 value; functi=
on size grows ~34% (32 -&gt; 43 lines) for one line of<br>=C2=A0 savings on=
 a hypothetical sixth GUC, while the closest precedent<br>=C2=A0 (archive_c=
ommand / archive_library in pgarch.c) is a hard-coded literal.<br><br>* err=
hint vs errdetail: errhint(&quot;At most one of %s can be set.&quot;)<br>=
=C2=A0 reads more like a constraint than an action hint.=C2=A0 The closest<=
br>=C2=A0 precedent, archive_command / archive_library in pgarch.c<br>=C2=
=A0 (ProcessPgArchInterrupts() / LoadArchiveLibrary()), keeps the<br>=C2=A0=
 enumeration in errdetail and omits errhint entirely.<br><br>* TAP regex: t=
he added like() uses [^&quot;]+ for the values, which<br>=C2=A0 passes rega=
rdless of the actual value.=C2=A0 Using quotemeta on the<br>=C2=A0 expected=
 values would verify the actual content, and anchoring<br>=C2=A0 would also=
 avoid accidentally matching the same tokens inside<br>=C2=A0 errhint.<br><=
br>On the reload trap: <br>I reproduced this on master and confirmed it&#39=
;s there exactly as you noted. =C2=A0<br>ALTER SYSTEM doesn&#39;t trigger t=
he assign hook; <br>it just writes to postgresql.auto.conf, <br>so the trap=
 window is intrinsic to PGC_POSTMASTER + ALTER SYSTEM. =C2=A0<br>A separate=
 follow-up patch in the reload path feels natural.<br><br>-- <br>JH Shin</d=
iv><br><div class=3D"gmail_quote gmail_quote_container"><div dir=3D"ltr" cl=
ass=3D"gmail_attr">On Mon, Jun 1, 2026 at 6:11=E2=80=AFAM Scott Ray &lt;<a =
href=3D"mailto:scott@scottray.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">Thanks for the patch.=C2=
=A0 I&#39;ve attached v1-0001 (atop v4) addressing the<br>
UX and test-coverage items below.=C2=A0 Happy to rework or fold in however<=
br>
you prefer.<br>
<br>
1. There&#39;s a configuration trap in master and in this branch that<br>
could be prevented using something very similar to<br>
CheckRecoveryTargetConflicts to check pending GUCs:<br>
<br>
=C2=A0 =C2=A0 psql -c &quot;ALTER SYSTEM SET recovery_target_xid TO &#39;70=
0&#39;&quot;<br>
=C2=A0 =C2=A0 psql -c &quot;ALTER SYSTEM SET recovery_target_time TO &#39;2=
026-01-01 00:00:00&#39;&quot;<br>
=C2=A0 =C2=A0 pg_ctl reload<br>
<br>
The log shows:<br>
<br>
=C2=A0 =C2=A0 LOG:=C2=A0 received SIGHUP, reloading configuration files<br>
=C2=A0 =C2=A0 LOG:=C2=A0 parameter &quot;recovery_target_xid&quot; cannot b=
e changed without restarting the server<br>
=C2=A0 =C2=A0 LOG:=C2=A0 parameter &quot;recovery_target_time&quot; cannot =
be changed without restarting the server<br>
=C2=A0 =C2=A0 LOG:=C2=A0 configuration file &quot;postgresql.auto.conf&quot=
; contains errors; unaffected changes were applied<br>
<br>
pg_settings shows:<br>
<br>
=C2=A0 =C2=A0 postgres=3D# SELECT name, setting, pending_restart FROM pg_se=
ttings<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 WHERE name LIKE =
9;recovery_target%&#39; AND pending_restart;<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 name=C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0| setting | pending_restart<br>
=C2=A0 =C2=A0 ---------------------+---------+-----------------<br>
=C2=A0 =C2=A0 =C2=A0recovery_target_time |=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0| t<br>
=C2=A0 =C2=A0 =C2=A0recovery_target_xid=C2=A0 |=C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0| t<br>
<br>
The db runs fine until the next restart, maybe hours later:<br>
<br>
=C2=A0 =C2=A0 FATAL:=C2=A0 multiple recovery targets specified<br>
=C2=A0 =C2=A0 DETAIL:=C2=A0 At most one of &quot;recovery_target&quot;, &qu=
ot;recovery_target_lsn&quot;,<br>
=C2=A0 =C2=A0 &quot;recovery_target_name&quot;, &quot;recovery_target_time&=
quot;,<br>
=C2=A0 =C2=A0 &quot;recovery_target_xid&quot; can be set.<br>
<br>
Is it worth a follow-up to report the conflict early and loud?<br>
<br>
2. There&#39;s an opportunity to provide a better UX by reporting which<br>
flags were set and what the values were, so that the user doesn&#39;t have<=
br>
to search config files or other logs to find this info.=C2=A0 For instance,=
<br>
in the postgresql.auto.conf scenario above, instead of:<br>
<br>
=C2=A0 =C2=A0 DETAIL:=C2=A0 At most one of &quot;recovery_target&quot;, &qu=
ot;recovery_target_lsn&quot;,<br>
=C2=A0 =C2=A0 &quot;recovery_target_name&quot;, &quot;recovery_target_time&=
quot;,<br>
=C2=A0 =C2=A0 &quot;recovery_target_xid&quot; can be set.<br>
<br>
The operator could see:<br>
<br>
=C2=A0 =C2=A0 DETAIL:=C2=A0 The following recovery target parameters are se=
t:<br>
=C2=A0 =C2=A0 &quot;recovery_target_time&quot; =3D &quot;2026-01-01 00:00:0=
0&quot;,<br>
=C2=A0 =C2=A0 &quot;recovery_target_xid&quot; =3D &quot;700&quot;.<br>
=C2=A0 =C2=A0 HINT:=C2=A0 At most one of &quot;recovery_target&quot;, &quot=
;recovery_target_lsn&quot;,<br>
=C2=A0 =C2=A0 &quot;recovery_target_name&quot;, &quot;recovery_target_time&=
quot;,<br>
=C2=A0 =C2=A0 &quot;recovery_target_xid&quot; can be set.<br>
<br>
3. <a href=3D"http://003_recovery_targets.pl:339" rel=3D"noreferrer" target=
=3D"_blank">003_recovery_targets.pl:339</a> currently tests recovery_target=
_xid&#39;s<br>
cleared-then-set behavior.=C2=A0 The patch adds the same coverage for the<b=
r>
other four recovery_target_* GUCs.<br>
<br>
<br>
--<br>
Scott Ray<br>
</blockquote></div>

--000000000000ebf59d065366fd40--