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

---

From: Michael Paquier <[email-masked]>
Date: Thu, 25 Jun 2026 17:00:33 +0900

> 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=bar -c
recovery_target_foo=", 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= -c recovery_target_foo=bar" is 
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

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Fri, 26 Jun 2026 16:15:53 +0900 (JST)

Hello,

I spent some time thinking about this issue and the proposed patch,
and I wonder if we can simplify it a bit further.

If we don't want to rely on GUC hooks for cross-checking multiple
recovery target settings, perhaps the assign hooks could stop updating
the shared recoveryTarget state altogether and only maintain their own
underlying variables.

As far as I can tell, the existing recovery target variables already
seem sufficient to determine whether each target is configured. The
only exception appears to be recovery_target = 'immediate', which
would probably need an additional boolean flag (or similar state) to
represent whether it has been specified.

Then validateRecoveryParameters() could determine which recovery
target is configured by inspecting those variables directly, report an
error if more than one target is set, and reconstruct the same
recoveryTarget state that the current code expects.

Does that make sense?

Regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: Michael Paquier <[email-masked]>
Date: Fri, 26 Jun 2026 17:12:15 +0900

> recoveryTarget state that the current code expects.

And delay setting recoveryTarget at all until we enter the validation
step in the startup process?  We don't use it in any of the early
steps before entering validateRecoveryParameters() (if we do so, that
would be an incorrect thing to do anyway).

I don't see on top of my mind why that would not work, skipping all
the RECOVERY_TARGET_UNSET manipulations in the assign hooks.  One
source of the confusion, to me, is that we have taken the decision to
link directly recoveryTarget with the result of the GUC, while
recoveryTarget is also used to track if the other target GUCs are set.
It looks to me that we shouldn't use recoveryTarget to track both if a
target is set and if the immediate state is set, and split both
things.  This is what I guess you are getting at with your extra
boolean.  Let's call it a new recoveryTargetImmediate.

At the end recoveryTarget should never be touched in any of the GUC
hooks, just once we enter validateRecoveryParameters().
--
Michael

---

From: Henson Choi <[email-masked]>
Date: Fri, 26 Jun 2026 20:12:15 +0900

Hi hackers,

Thanks for v6.  I think there is still one reachable case on the
command-line (-c) path where the conflict detection is bypassed and a
recovery target is silently dropped, so recovery runs to the end of WAL
instead of stopping at the requested target.  On master the same input
is rejected at startup, so for the -c path this is a loud-reject ->
silent behavior change.

Reproduction
------------

Using --check so it is side-effect-free; the conflict is detected during
GUC assignment, so an actual startup behaves the same:

    postgres --check -D data \
        -c "recovery_target_xid=700" \
        -c "recovery_target_name=foo" \
        -c "recovery_target_name="

  master rejects it at the second assignment:

      FATAL:  multiple recovery targets specified
      DETAIL:  At most one of "recovery_target", "recovery_target_lsn",
      "recovery_target_name", "recovery_target_time", "recovery_target_xid"
      may be set.

  v6 passes the check with no error (exit 0); recoveryTarget ends up
  RECOVERY_TARGET_UNSET.  In an actual PITR this means
  recovery_target_xid=700 is silently ignored and the server recovers to
  the end of WAL (target overshoot).

postgresql.conf does not reproduce this: duplicate keys are collapsed
there, so only the final value reaches the assign hook.  The
set-then-clear of a *second*, different target only happens via -c,
where each option is applied in order and fires the assign hook every
time.

Why it happens
--------------

recoveryTarget is a single enum that the per-GUC assign hooks maintain
incrementally, and it alone drives recovery.  CheckRecoveryTargetConflicts()
independently re-reads the committed GUC strings via GetConfigOption().
With the three -c options above the two sides diverge:

    step        assign hooks -> recoveryTarget      final strings
    xid=700     XID                                 xid="700"
    name=foo    NAME (overwrites XID)               xid="700", name="foo"
    name=""     UNSET (NAME was the last type)      xid="700", name=""

At "name=foo" two targets are momentarily set, but the single enum
cannot represent that, so it just overwrites; "name=" then clears it.
By the time CheckRecoveryTargetConflicts() runs, the strings have
settled to a single non-empty target (xid), so ntargets == 1 and the
check passes -- while recovery actually runs with recoveryTarget ==
UNSET.

In other words, the v6 matrix row "cross-GUC, both non-empty -> error"
no longer holds once the second GUC is subsequently cleared: the
set-time guard was removed, and the startup check only inspects the
final strings, which no longer show the transient conflict.  This is the
same class of unexpected behavior reported earlier in this thread; the
three-step -c form is the variant that still slips through.

One option would be to derive recoveryTarget from the settled GUC strings
in CheckRecoveryTargetConflicts() instead of maintaining it incrementally
in the assign hooks, but any other approach is perfectly fine.

Minor nit
---------

While here: the new ereport() in CheckRecoveryTargetConflicts() still
wraps its arguments in the legacy extra parentheses,

    ereport(FATAL,
            (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
             errmsg("multiple recovery targets specified"),
             ...));

Since the parenthesis-free ereport() form is available, new code can drop
them:

    ereport(FATAL,
            errcode(ERRCODE_INVALID_PARAMETER_VALUE),
            errmsg("multiple recovery targets specified"),
            ...);

Regards,
Henson

---

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

Thanks for the reviews.

v7 attached, reworked along the lines discussed. The assign hooks
no longer touch recoveryTarget; each just stores its own value,
and recoveryTarget is derived once in validateRecoveryParameters()
from the settled recovery_target* settings,
which also rejects setting more than one target.
The v6 review comments are folded in too.

> recovery_target_xid=700 is silently ignored ... target overshoot
> derive recoveryTarget from the settled GUC strings

Done that way. A new test, standby_clobber_clear, sets a competing target
and clears it again; on master that is rejected outright,
while here recovery stops at the xid.

I left out the extra boolean. recovery_target's own string is "immediate"
or empty, so immediate is detected like the other four.
Deriving it from the typed value variables would need the flag,
but those aren't reliable "is set" signals anyway,
since the time value is parsed late and an LSN of 0/0 is valid.
Happy to add it back if you prefer it explicit.

> the legacy extra parentheses in ereport()

Dropped.

-- 
JH Shin

---

From: Michael Paquier <[email-masked]>
Date: Mon, 29 Jun 2026 14:47:58 +0900

> The v6 review comments are folded in too.

Hmm.  One aspect of the patch that I have a hard time accepting is
this aspect:
ADD_TARGET_IF_SET("recovery_target", RECOVERY_TARGET_IMMEDIATE);

This part lacks future extensibility, IMO.  If we add more values to
the GUC recovery_target in the future, we push down more complication
to the resolution of the immediate target at the beginning of the
startup process.  Decoupling entirely RecoveryTargetType and the types
of values that can be assigned in the GUC recovery_target may lead to
a nicer result, I suspect..  It also feels wasteful to add an
hypothetical enum for the supported values if we don't have a use for 
it yet.  Any opinions from others?

+       errdetail("Only one recovery target can be set.  Parameters set: %s.",
+                  buf.data),
+       errhint("See pg_settings for the parameter values and where each is set."));

pg_settings is just one way to look at these values.  We have also
SHOW and other interfaces.  I would keep the errdetail() with your
first sentence, make the errhint the second sentence of the
errdetail().
--
Michael

---

From: JoongHyuk Shin <[email-masked]>
Date: Mon, 29 Jun 2026 16:59:40 +0900

On Mon, Jun 29, 2026 at 2:48 PM Michael Paquier <[email-masked]> wrote:
> first sentence, make the errhint the second sentence of the errdetail().

I split it along the message style guide's line, factual information
in the detail and suggestions in the hint. The detail states the facts,
which targets are set, and the "see pg_settings" pointer is guidance
for tracking them down rather than a fact about the conflict,
so it went into the hint.
I'd keep it there rather than fold it into the detail.

SHOW does return the values, but I named pg_settings
specifically because it also reports where each parameter is set,
through its sourcefile and sourceline columns.
With several recovery_target* values possibly spread
across configuration files, finding where each one is set
is usually what it takes to resolve the conflict,
which is the part SHOW leaves out.

-- 
JH Shin

---

From: Zsolt Parragi <[email-masked]>
Date: Mon, 29 Jun 2026 16:16:59 -0500

> first sentence, make the errhint the second sentence of the

Isn't mentioning pg_settings confusing instead of helpful during a
server restart? With a reload it can help, but when the server can't
start, hinting that the user should query pg_settings doesn't seem
that useful.

---

From: Michael Paquier <[email-masked]>
Date: Tue, 30 Jun 2026 08:35:36 +0900

> start, hinting that the user should query pg_settings doesn't seem

Yeah, it is.  I don't see a reason why we should be specific about the
location where these parameters are set.  One has been setting up
recovery parameters in the GUC machine, so they most likely know where
these strings are.
--
Michael

---

From: JoongHyuk Shin <[email-masked]>
Date: Mon, 6 Jul 2026 16:53:44 +0900

> start, hinting that the user should query pg_settings doesn't seem

You're right, and I had missed this.  Thanks.

I would rather drop the errhint from this patch entirely than fold it into
the errdetail.
Any objections?

-- 
JH Shin

---

From: Michael Paquier <[email-masked]>
Date: Tue, 7 Jul 2026 07:30:32 +0900

> I would rather drop the errhint from this patch entirely than fold it into

Sounds good here to remove the errhint().  No hint is better than a
potentially wrong hint.  Thanks.
--
Michael

---

From: JoongHyuk Shin <[email-masked]>
Date: Tue, 7 Jul 2026 12:01:59 +0900

Attached v8 with the errhint removed.  No other changes from v7.

-- 
JH Shin

---

From: Michael Paquier <[email-masked]>
Date: Tue, 7 Jul 2026 15:58:18 +0900

+                         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

---

From: Michael Paquier <[email-masked]>
Date: Tue, 7 Jul 2026 15:59:26 +0900

---

From: Fujii Masao <[email-masked]>
Date: Wed, 8 Jul 2026 10:38:54 +0900

> > Attached v8 with the errhint removed.  No other changes from v7.

We can remove assign_recovery_target_name()? With the v8 patch, it only
assigns the GUC string pointer to recoveryTargetName. It seems we could
instead have recovery_target_name store its value directly in
recoveryTargetName, remove recovery_target_name_string, and drop
the assign hook altogether. Thoughts?

> so I am not planning to go beyond a review here.

Thanks for the review! Yes, I will handle this patch.

Regards,

-- 
Fujii Masao

---

From: Michael Paquier <[email-masked]>
Date: Wed, 8 Jul 2026 10:43:53 +0900

> the assign hook altogether. Thoughts?

Yeah, perhaps we should just do that.  I've also found the consistency
with all these _string variables interesting to keep, but that's
minor compared to less code.
--
Michael