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

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