public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: JoongHyuk Shin <[email protected]>
Cc: [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: Mon, 29 Jun 2026 14:47:58 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CACSdjfOFz=i2=YT_2kej-+OA0UTyUO1Hc5kEq-2wqzJcmdR6aw@mail.gmail.com>
References: <CACSdjfN7uYLS-+wyBbvZ6rmkSNuPm9H8Y+xdd_aSr-kNG_xu3w@mail.gmail.com>
<[email protected]>
<CACSdjfMt5obQc0QCX8E+v-pxROWFQxmeW6Ki9X16F60D3tSJgg@mail.gmail.com>
<CAAAe_zD6oOibLjg6zBs5EvNk2C+y0H7wQVp86z_aGDXJg-sMyg@mail.gmail.com>
<CACSdjfOFz=i2=YT_2kej-+OA0UTyUO1Hc5kEq-2wqzJcmdR6aw@mail.gmail.com>
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
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], [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