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

Hi Shin,

Thanks for taking this one on. The XXX comment has been there for a
long time and fixing a known contract violation is worthwhile. The
mechanical part of the patch, moving the conflict check from the
assign hooks into validateRecoveryParameters(), looks right to me.

A few things worth discussing before this gets a committer's eye:

1. Behavioral change when recovery is not requested.

The patch's commit message notes that conflicting recovery target
settings are now silently accepted when ArchiveRecoveryRequested is
false, and argues this is "arguably more correct" because those GUCs
have no effect outside recovery. I am not sure I agree. Today a
misconfigured postgresql.conf (say, both recovery_target_time and
recovery_target_xid present) is caught immediately at postmaster
start. After the patch, that same misconfiguration boots
successfully, and the operator only finds out later when they add
recovery.signal and the startup process FATALs. That is a real
downgrade in error-detection timing.

Is it feasible to keep the early-detection behavior without violating
the assign-hook contract? One option: emit a WARNING (not ERROR)
from the assign hook, or defer the check to a post-config-file pass
(GUC_check_hook chain, or a one-shot check in PostmasterMain before
the startup process is forked). I do not have a strong opinion on
the mechanism, but I think the user-visible behavior (conflicting
recovery_target_* settings are caught at server start) is worth
preserving if possible.

If preserving that is not feasible, I think the commit message should
flag the timing change more prominently, since it is a user-visible
change to when postgres refuses to start.

2. Test coverage of the conflict-in-recovery path.

The existing 003_recovery_targets.pl has a test (not modified by
this patch, as far as I can tell) that exercises the multiple-targets
rejection. With v1, that rejection fires from
validateRecoveryParameters() rather than from an assign hook, so the
error message source changes even if the text is identical. CFBot
is green, so the existing regex must still match, but it would be
good to confirm which test covers this and to verify the test's
assertions actually validate the new FATAL path, not just "server
failed to start for some reason".

3. errdetail wording.

errdetail("At most one of \"recovery_target\", ..., "
"\"recovery_target_xid\" may be set.")

The error-message style guide (doc/src/sgml/error-style-guide.sgml)
says to avoid "may" because it reads as permission rather than
ability. Suggest "can be set" instead. Trivial, but since we are
already touching this message.

4. Test cleanup fragility.

The new test case does:

$node_primary->append_conf('postgresql.conf', "recovery_target_name = ...
recovery_target_time = ...");
$node_primary->start;
...
ALTER SYSTEM RESET recovery_target_name;
ALTER SYSTEM RESET recovery_target_time;
$node_primary->restart;

append_conf permanently extends postgresql.conf. The ALTER SYSTEM
RESET writes to postgresql.auto.conf which takes precedence, so the
stale postgresql.conf lines are effectively masked for subsequent
test steps, but they remain in the file, and any later test that
relies on a predictable postgresql.conf content (for example a test
that inspects ConfigFileVar or does its own append) could be
confused. Using ALTER SYSTEM SET for the setup would be cleaner,
or using a dedicated temporary cluster for this one case. Also,
the ok() predicate `defined $primary_pid && $primary_pid ne ''` is
redundant. safe_psql would have died earlier on start failure.

5. Nit: block comment.

The new block inside validateRecoveryParameters() could benefit from
noting that ArchiveRecoveryRequested is guaranteed true at this
point (because of the early return above it), so a reader does not
have to scroll up to confirm.

Nothing here is a blocker. I think the overall direction is right.
The behavioral change question in item 1 is the main design decision
I would want the author's thinking on.

Thanks,
Greg

On Mon, Apr 13, 2026 at 5:21 PM JoongHyuk Shin <sjh910805@gmail.com> wrote:

The "limited consequences" argument is true enough that this hasn't
caused visible failures in practice, but fixing a known contract
violation seems worthwhile.

+1

The fix is to remove the conflict check from all five assign hooks and
detect conflicts in validateRecoveryParameters() instead. That
function already runs after all GUCs have been loaded (called from
InitWalRecovery() in the startup process), so it can safely read each
GUC's current value via GetConfigOption() and count how many are
non-empty. If more than one is set, it reports FATAL, consistent with
the other validation errors already in that function.

In the master, when the following two recovery targets are specified,
the recovery target assign hook detects that multiple targets were given
and reports an error. With the patch, however, the same settings do not
raise an error, recoveryTarget is set to RECOVERY_TARGET_UNSET, and
recovery unexpectedly proceeds with no target. Could this be a bug
in the patch?

recovery_target_xid = '9999'
recovery_target_time = ''

Regards,

--
Fujii Masao

On Fri, Apr 24, 2026 at 10:08:04PM +0900, Fujii Masao wrote:

In the master, when the following two recovery targets are specified,
the recovery target assign hook detects that multiple targets were given
and reports an error. With the patch, however, the same settings do not
raise an error, recoveryTarget is set to RECOVERY_TARGET_UNSET, and
recovery unexpectedly proceeds with no target. Could this be a bug
in the patch?

recovery_target_xid = '9999'
recovery_target_time = ''

Don't think so. You are specifying for recovery_target_time the same
thing as the default, as in "I don't know and do nothing about the
time". Why would it matter to make the difference between a default
value set and what's stored by default if nothing is set in this case?

FWIW, I am wondering if we should seriously consider this stuff as
candidate for a backpatch because this is a design mistake: we should
never *ever* rely on the GUC hooks to do cross-checks of multiple
values, f2cbffc7a618 deciding that it was a right thing to do. It's
not. The risk of breaking something may not justify that a backpatch.

+1 for reworking that on HEAD, at least.
--
Michael

On Mon, Apr 27, 2026 at 10:52 AM Michael Paquier <michael@paquier.xyz> wrote:

On Fri, Apr 24, 2026 at 10:08:04PM +0900, Fujii Masao wrote:

In the master, when the following two recovery targets are specified,
the recovery target assign hook detects that multiple targets were given
and reports an error. With the patch, however, the same settings do not
raise an error, recoveryTarget is set to RECOVERY_TARGET_UNSET, and
recovery unexpectedly proceeds with no target. Could this be a bug
in the patch?

recovery_target_xid = '9999'
recovery_target_time = ''

Don't think so. You are specifying for recovery_target_time the same
thing as the default, as in "I don't know and do nothing about the
time". Why would it matter to make the difference between a default
value set and what's stored by default if nothing is set in this case?

With those settings, how should recovery behave? I would expect it to
behave as in master, i.e., detect that multiple targets were specified
and report an error. Alternatively, it might be OK for me to proceed
with recovery_target_xid = '9999' and ignore recovery_target_time = '',
since that matches the default.

With the proposed patch, however, both settings are ignored and
recovery starts with no target. That seems unexpected to me.

+1 for reworking that on HEAD, at least.

I was thinking the same. +1

Regards,

--
Fujii Masao

On Mon, Apr 27, 2026 at 02:36:11PM +0900, Fujii Masao wrote:

With the proposed patch, however, both settings are ignored and
recovery starts with no target. That seems unexpected to me.

If that's the case (not tested myself), agreed.
--
Michael

Thanks for the reviews.

v2 attached.

* Conflict check moved to a new static CheckRecoveryTargetConflicts(),
called from validateRecoveryParameters() before its early return.
It runs at every startup, so misconfiguration is caught as in master.
I kept it in startup process rather than PostmasterMain
(Greg'ssuggestion),
matching the existing recovery validation there.

* Removed each assign hook's `else recoveryTarget = UNSET` branch
(B in Fujii's framing). Fixes the empty-string clobber Fujii reported,
`recovery_target_xid='9999' + recovery_target_time=''` was silently
running with no target.
003_recovery_targets.pl now covers it (fails on v1, passes on v2).

* errdetail "may" -> "can" (Greg).

* TAP test that asserted the v1 regression is replaced with one
asserting conflict rejection at every startup.

Agreed: HEAD only, no backpatch.

--
JH Shin

On Mon, Apr 27, 2026 at 3:00 PM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Apr 29, 2026 at 6:30 PM JoongHyuk Shin <sjh910805@gmail.com> wrote:

Thanks for the reviews.

v2 attached.

Thanks for updating the patch!

When I started postgres with the following command, recovery_target_xid was
treated as unset in the master, but with the patch the recovery_target_xid=700
setting was used instead. This behavior seems unexpected to me. Thoughts?

postgres -D data -c "recovery_target_xid=700" -c "recovery_target_xid="

Regards,

--
Fujii Masao

Thanks for the reviews.

v3 attached.

Expected behavior matrix:

Same GUC, non-empty then empty -> the empty wins; target unset
Same GUC, empty then non-empty -> the non-empty wins; target set
Cross-GUC, both non-empty -> error (multiple recovery targets)
Cross-GUC, one empty -> the non-empty GUC's target stands
All empty -> no target, end-of-WAL recovery

* Restored same-GUC last-wins (row 1). v2 dropped each hook's
`else recoveryTarget = UNSET`; v3 narrows it to
`else if (recoveryTarget == MY_TYPE)`.

* Cross-GUC empty stays a no-op (row 4), as v2 introduced.
Strict reject via a source-aware variant is feasible
if reviewers prefer.

* 003_recovery_targets.pl gains seven CLI-path cases;
postgresql.conf dedup cannot exercise the same-GUC clear path.

--
JH Shin

On Fri, May 1, 2026 at 2:53 PM Fujii Masao <masao.fujii@gmail.com> wrote:

CFBot flagged a Windows MSVC failure on v3. Root cause: the new TAP
cases pass GUC values to pg_ctl via "--options" using single-quoted
shell tokens, which Windows cmd.exe does not strip the way POSIX
shells do, so postgres receives the quotes verbatim and rejects the
values.

v4 drops the single quotes and switches recovery_target_time from the
space-containing now() format to ISO 8601 with the T separator, so
each value is a single token without quoting. All other platforms
already passed; this only affected the new TAP cases on Windows MSVC.

No functional change.

--
JH Shin

On Mon, May 11, 2026 at 12:17 PM JoongHyuk Shin <sjh910805@gmail.com> 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=# 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

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

On Mon, Jun 1, 2026 at 6:11 AM 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 "=" 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

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

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

On 2026-Jun-07, JoongHyuk Shin wrote:

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.

Agreed on using =, although ...

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.

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/

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

On Mon, Jun 8, 2026 at 12:44 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:

Hello,

On 2026-Jun-21, JoongHyuk Shin wrote:

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.

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/

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

On Sun, Jun 21, 2026 at 9:32 PM Álvaro Herrera <alvherre@kurilemu.de> wrote: