Subject: Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks

From: Scott Ray <[email-masked]>
Date: Sun, 31 May 2026 21:11:07 +0000

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=# 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" = "2026-01-01 00:00:00",
    "recovery_target_xid" = "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

---

From: JoongHyuk Shin <[email-masked]>
Date: Thu, 4 Jun 2026 14:41:01 +0900

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.

-- 
JH Shin

---

From: Scott Ray <[email-masked]>
Date: Sat, 06 Jun 2026 20:10:10 +0000

> would also avoid accidentally matching the same tokens inside

Thanks for taking a look. I attached a v2 that applies your suggestions
and uses "set to" instead of "=" 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

---

From: JoongHyuk Shin <[email-masked]>
Date: Sun, 7 Jun 2026 19:30:02 +0900

On "=" vs "set to": I'd stay with "=".
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.
"=" is punctuation, so it sidesteps that.
PG already uses this form in the RI detail,
e.g. Key (%s)=(%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.

-- 
JH Shin

---

From: Álvaro Herrera <[email-masked]>
Date: Sun, 7 Jun 2026 17:44:54 +0200

> "=" is punctuation, so it sidesteps that.

Agreed on using =, although ...

> I'd be interested in the committer's view on whether it is worth adding.

It may be enough to list which ones are set, without listing their values.
Those can be obtained easily from pg_settings, which can be mentioned in
errhint -- useful also to figure out exactly _where_ they are set (e.g.,
in an include file, postgresql.auto.conf, and so on.)

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

---

From: JoongHyuk Shin <[email-masked]>
Date: Sun, 21 Jun 2026 18:27:42 +0900

Thanks for the reviews.

v5 attached.

The errdetail now lists which recovery_target_* parameters are actually set,
instead of the full candidate list.
This follows Scott's idea to surface the set targets;
following Álvaro, the values are dropped
and a new errhint points to pg_settings for them and their sources.

I kept the "which ones are set" list in errdetail rather than errhint,
it states the current configuration, which reads as detail,
while the errhint carries the actionable pg_settings pointer.

-- 
JH Shin

---

From: Álvaro Herrera <[email-masked]>
Date: Sun, 21 Jun 2026 14:31:55 +0200

Hello,

> while the errhint carries the actionable pg_settings pointer.

Please see https://postgr.es/c/3692a622d3fd for more on translatable
message construction.  You should end up with a translatable string in
a _() call like
  _(", \"%s\"")
and the GUC names in a separate string in each case.

Maybe you can make this a local macro to avoid repetitive coding,

#define considerAndComplainAboutGUC(gucname, buf) \
   do { \
       val = GetConfigOption(gucname, false, false); \
       if (val[0] != '\0')     \
       {                       \
            ntargets++;        \
            if (buf.len == 0)  \
                appendStringInfoString(&buf, _("\"%s\""), gucname); \
            else               \
                appendStringInfoString(&buf, _(", \"%s\""), gucname); \
       } \
   } while (0)

considerAndComplainAboutGUC("recovery_target", buf);
considerAndComplainAboutGUC("recovery_target_lsn", buf);
and so on.  (Of course, you should choose a less stupid macro name, but
you get my meaning.)

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"...  In accounting terms this makes perfect sense.  To rational humans, it
is insane.  Welcome to IBM."                           (Robert X. Cringely)
https://www.cringely.com/2015/06/03/autodesks-john-walker-explained-hp-and-ibm-in-1991/

---

From: JoongHyuk Shin <[email-masked]>
Date: Mon, 22 Jun 2026 13:37:27 +0900

Thanks for the review.

v6 attached.

The set-parameter list in the errdetail now wraps the separator and quotes
in _()
so the punctuation is translatable, following 3692a622d3fd.
The five GetConfigOption() checks are now a local macro, as suggested.

-- 
JH Shin