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

On Mon, Jun 22, 2026 at 01:37:27PM +0900, JoongHyuk Shin wrote:
> Thanks for the review.

Please avoid top-posting when replying on the lists. This breaks the
logical flow of the thread.

> The five GetConfigOption() checks are now a local macro, as suggested.

+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("multiple recovery targets specified"),
+ errdetail("Recovery targets %s are set, but only one can be set.",
+ buf.data),

Is this errdetail() the optimal choice, though? It feels a bit odd to
have that mid-sentence. Perhaps we should use a colon to separate the
list of parameters, say simply:
"Parameters set are: %s."

The trick of Alvaro to make the list of parameters translatable is
interesting. Didn't know that.

The tests feel bloated to me, bumping the time it takes to run 003 by
30%~40% or so. The "Three conflicting targets" has for example little
value. I think that we should also get rid of most of the tests where
we do the patterns for "-c recovery_target_foo=bar -c
recovery_target_foo=", repeated for each target type. One should be
enough. Keeping only one failure case should be enough for two
recovery targets (or all of them set, why not).

With the first tests in place, I am not convinced that the
anti-pattern "-c recovery_target_foo= -c recovery_target_foo=bar" is
needed. Let's just remove it.

This code is clearly AI-generated. Comments are equally bloated with
descriptions that can be understood just by reading the code. Let's
simplify all that.

In CheckRecoveryTargetConflicts(), initStringInfo() does an
allocation. Perhaps we should free it after making sure we don't
FATAL. Just a good practice.
--
Michael