Horton

Thread

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

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

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

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