public inbox for [email protected]
help / color / mirror / Atom feedFrom: JoongHyuk Shin <[email protected]>
To: [email protected]
Cc: Fujii Masao <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
Date: Thu, 4 Jun 2026 14:41:01 +0900
Message-ID: <CACSdjfOxnez=Cv8CzvJcupnaSjaQFnXqk5t=C1ES0BXp52faFg@mail.gmail.com> (raw)
In-Reply-To: <M5a4v6-PJIZSNQlLhGy-GgBPCXNkrW3OnbESqTpaXh2TyZl4TsMyY_wPK-chiMOoZ4mr9Dm7vuqDSVmNeMKfZYNYlI5PxoQyC6iqWZ9MTAk=@scottray.io>
References: <CACSdjfPUa4UvKjADgOERXoxNYmCg2mqqiqKkiJk6mX6E4qgVFw@mail.gmail.com>
<CAHGQGwExsY9XvxN=u4ip7pA+_gO-ms8EwgcRZF3Nj84Vi8yeBQ@mail.gmail.com>
<[email protected]>
<CAHGQGwFNjrLfUSyT4McR4wH_2yFV3n7+63eyvDjzxQchyyQrPA@mail.gmail.com>
<[email protected]>
<CACSdjfPCPjP9tuzxgarcbkjahu-n+o+LJbUQ22aPNkOsOvrEZg@mail.gmail.com>
<CAHGQGwF24XMh=35m+FdqLUw4QgK8HmAGwPZyBDKtsw7MWYp_hw@mail.gmail.com>
<CACSdjfNY=aePEYGYV_5tamz-iTLQLitBuHc8youfsTzfDR1+_g@mail.gmail.com>
<CACSdjfMohHudUh7Fq=pD=Zk+=J=4E65c6VGgScG3CAXvj_Sorg@mail.gmail.com>
<M5a4v6-PJIZSNQlLhGy-GgBPCXNkrW3OnbESqTpaXh2TyZl4TsMyY_wPK-chiMOoZ4mr9Dm7vuqDSVmNeMKfZYNYlI5PxoQyC6iqWZ9MTAk=@scottray.io>
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 <[email protected]> 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
>
view thread (8+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
In-Reply-To: <CACSdjfOxnez=Cv8CzvJcupnaSjaQFnXqk5t=C1ES0BXp52faFg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox