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

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