Thread

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

   JoongHyuk Shin <sjh910805@gmail.com> — 2026-04-13T08:21:39Z

   The five recovery target GUC assign hooks -- assign_recovery_target,
   assign_recovery_target_lsn, assign_recovery_target_name,
   assign_recovery_target_time, and assign_recovery_target_xid -- all call
   error_multiple_recovery_targets() when a second conflicting target is
   detected, which invokes ereport(ERROR).  The GUC README
   (src/backend/utils/misc/README) is explicit: "There is no provision for
   a failure result code. assign_hooks should never fail."  Raising an
   error from an assign hook leaves guc.c's internal state inconsistent
   before the abort, because the abort handling path was not designed to
   run mid-assign.
   
   The code acknowledges this with an XXX comment that has been there for
   years:
   
       XXX this code is broken by design.  Throwing an error from a GUC
       assign hook breaks fundamental assumptions of guc.c.  So long as
       all the variables for which this can happen are PGC_POSTMASTER, the
       consequences are limited, since we'd just abort postmaster startup
       anyway.  Nonetheless it's likely that we have odd behaviors such as
       unexpected GUC ordering dependencies.
   
   The "limited consequences" argument is true enough that this hasn't
   caused visible failures in practice, but fixing a known contract
   violation seems worthwhile.
   
   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.
   
   This changes when the error fires: it now happens in the startup process
   rather than in the postmaster's ProcessConfigFile.  The outcome is the
   same (server does not start), but guc.c's state is no longer disturbed.
   
   There is one secondary behavioral change: when recovery is not actually
   requested (ArchiveRecoveryRequested is false), validateRecoveryParameters
   returns early and never checks for conflicts, so conflicting recovery
   target settings are silently ignored.  The old code would reject them
   even then, since assign hooks fire unconditionally during ProcessConfigFile.
   I think the new behavior is arguably more correct -- those GUCs have no
   effect when recovery is not requested, so there is no reason to treat
   their values as an error.  The existing TAP test in 003_recovery_targets.pl
   already covers the conflict-in-recovery case; this patch adds a test for
   the new behavior (conflicting targets accepted outside recovery).
   
   Patch attached.
   

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

   Greg Lamberson <greg@lamco.io> — 2026-04-24T01:10:44Z

   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
   

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

   Fujii Masao <masao.fujii@gmail.com> — 2026-04-24T13:08:04Z

   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
   
   
   
   

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

   Michael Paquier <michael@paquier.xyz> — 2026-04-27T01:52:03Z

   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
   

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

   Fujii Masao <masao.fujii@gmail.com> — 2026-04-27T05:36:11Z

   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
   
   
   
   

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

   Michael Paquier <michael@paquier.xyz> — 2026-04-27T05:59:54Z

   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
   

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

   JoongHyuk Shin <sjh910805@gmail.com> — 2026-04-29T09:29:59Z

   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 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
   >
   

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

   Fujii Masao <masao.fujii@gmail.com> — 2026-05-01T05:53:18Z

   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
   
   
   
   

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

   JoongHyuk Shin <sjh910805@gmail.com> — 2026-05-11T03:17:36Z

   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:
   
   > 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
   >
   

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

    JoongHyuk Shin <sjh910805@gmail.com> — 2026-05-11T06:01:56Z

    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 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:
    >
    >> 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
    >>
    >
    

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

    Scott Ray <scott@scottray.io> — 2026-05-31T21:11:07Z

    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
    

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

    JoongHyuk Shin <sjh910805@gmail.com> — 2026-06-04T05:41:01Z

    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:
    
    > 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
    >
    

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

    Scott Ray <scott@scottray.io> — 2026-06-06T20:10:10Z

    > * 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
    

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

    JoongHyuk Shin <sjh910805@gmail.com> — 2026-06-07T10:30:02Z

    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:
    
    > > * 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
    >
    

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

    Álvaro Herrera <alvherre@kurilemu.de> — 2026-06-07T15:44:54Z

    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/
    
    
    
    

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

    JoongHyuk Shin <sjh910805@gmail.com> — 2026-06-21T09:27:42Z

    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:
    
    > 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/
    >
    

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

    Álvaro Herrera <alvherre@kurilemu.de> — 2026-06-21T12:31:55Z

    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/
    
    
    
    

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

    JoongHyuk Shin <sjh910805@gmail.com> — 2026-06-22T04:37:27Z

    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:
    
    > 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/
    >
    

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

    Michael Paquier <michael@paquier.xyz> — 2026-06-25T08:00:33Z

    On Mon, Jun 22, 2026 at 01:37:27PM +0900, JoongHyuk Shin wrote:
    > 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
    

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

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2026-06-26T07:15:53Z

    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
    
    
    
    

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

    Michael Paquier <michael@paquier.xyz> — 2026-06-26T08:12:15Z

    On Fri, Jun 26, 2026 at 04:15:53PM +0900, Kyotaro Horiguchi wrote:
    > 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.
    
    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
    

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

    Henson Choi <assam258@gmail.com> — 2026-06-26T11:12:15Z

    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
    

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

    JoongHyuk Shin <sjh910805@gmail.com> — 2026-06-29T04:45:37Z

    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
    

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

    Michael Paquier <michael@paquier.xyz> — 2026-06-29T05:47:58Z

    On Mon, Jun 29, 2026 at 01:45:37PM +0900, JoongHyuk Shin wrote:
    > 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.
    
    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
    

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

    JoongHyuk Shin <sjh910805@gmail.com> — 2026-06-29T07:59:40Z

    On Mon, Jun 29, 2026 at 2:48 PM Michael Paquier <michael@paquier.xyz> wrote:
    > 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().
    
    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
    

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

    Zsolt Parragi <zsolt.parragi@percona.com> — 2026-06-29T21:16:59Z

    On Mon, 29 Jun 2026, Michael Paquier <michael@paquier.xyz> wrote:
    > +       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().
    
    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.
    
    
    
    

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

    Michael Paquier <michael@paquier.xyz> — 2026-06-29T23:35:36Z

    On Mon, Jun 29, 2026 at 04:16:59PM -0500, Zsolt Parragi wrote:
    > 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.
    
    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
    

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

    JoongHyuk Shin <sjh910805@gmail.com> — 2026-07-06T07:53:44Z

    On Mon, Jun 29, 2026 at 04:16:59PM -0500, Zsolt Parragi wrote:
    > 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.
    
    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
    

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

    Michael Paquier <michael@paquier.xyz> — 2026-07-06T22:30:32Z

    On Mon, Jul 06, 2026 at 04:53:44PM +0900, JoongHyuk Shin wrote:
    > I would rather drop the errhint from this patch entirely than fold it into
    > the errdetail.
    > Any objections?
    
    Sounds good here to remove the errhint().  No hint is better than a
    potentially wrong hint.  Thanks.
    --
    Michael
    

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

    JoongHyuk Shin <sjh910805@gmail.com> — 2026-07-07T03:01:59Z

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

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

    Michael Paquier <michael@paquier.xyz> — 2026-07-07T06:58:18Z

    On Tue, Jul 07, 2026 at 12:01:59PM +0900, JoongHyuk Shin wrote:
    > Attached v8 with the errhint removed.  No other changes from v7.
    
    +               errmsg("multiple recovery targets specified"),
    +               errdetail("Only one recovery target can be set. Parameters set: %s.",
    +                         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
    

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

    Michael Paquier <michael@paquier.xyz> — 2026-07-07T06:59:26Z

    On Tue, Jul 07, 2026 at 12:01:59PM +0900, JoongHyuk Shin wrote:
    > Attached v8 with the errhint removed.  No other changes from v7.
    
    +               errmsg("multiple recovery targets specified"),
    +               errdetail("Only one recovery target can be set. Parameters set: %s.",
    +                         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
    

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

    Fujii Masao <masao.fujii@gmail.com> — 2026-07-08T01:38:54Z

    On Tue, Jul 7, 2026 at 3:59 PM Michael Paquier <michael@paquier.xyz> wrote:
    >
    > On Tue, Jul 07, 2026 at 12:01:59PM +0900, JoongHyuk Shin wrote:
    > > 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?
    
    
    > Please note that Fujii-san is registered as a committer of this patch,
    > so I am not planning to go beyond a review here.
    
    Thanks for the review! Yes, I will handle this patch.
    
    Regards,
    
    -- 
    Fujii Masao
    
    
    
    

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

    Michael Paquier <michael@paquier.xyz> — 2026-07-08T01:43:53Z

    On Wed, Jul 08, 2026 at 10:38:54AM +0900, Fujii Masao wrote:
    > 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?
    
    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