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

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