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

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