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

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(at)scottray(dot)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=# 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
>