public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: JoongHyuk Shin <[email protected]>
Cc: Álvaro Herrera <[email protected]>
Cc: Scott Ray <[email protected]>
Cc: Fujii Masao <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
Date: Thu, 25 Jun 2026 17:00:33 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CACSdjfMt5obQc0QCX8E+v-pxROWFQxmeW6Ki9X16F60D3tSJgg@mail.gmail.com>
References: <CACSdjfN7uYLS-+wyBbvZ6rmkSNuPm9H8Y+xdd_aSr-kNG_xu3w@mail.gmail.com>
<[email protected]>
<CACSdjfMt5obQc0QCX8E+v-pxROWFQxmeW6Ki9X16F60D3tSJgg@mail.gmail.com>
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
Attachments:
[application/pgp-signature] signature.asc (833B, ../[email protected]/2-signature.asc)
download
view thread (25+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox