| From: | JoongHyuk Shin <sjh910805(at)gmail(dot)com> |
|---|---|
| To: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks |
| Date: | 2026-04-13 08:21:39 |
| Message-ID: | CACSdjfPUa4UvKjADgOERXoxNYmCg2mqqiqKkiJk6mX6E4qgVFw@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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.
| Attachment | Content-Type | Size |
|---|---|---|
| 0001-Don-t-call-ereport-ERROR-from-recovery-target-GUC-as.patch | application/octet-stream | 8.9 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Lakshmi N | 2026-04-13 08:22:07 | Show VIRTUAL keyword for virtual generated columns in pg_dump and psql |
| Previous Message | Richard Guo | 2026-04-13 08:20:35 | Re: Bug: Rule actions see wrong values for generated columns (NEW.gen reads OLD value) |