Re: A recent message added to pg_upgade

On Mon, Oct 30, 2023 at 7:58 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Fri, 27 Oct 2023 14:57:10 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in
> > On Fri, Oct 27, 2023 at 2:02 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > >
> > > On 2023-Oct-27, Kyotaro Horiguchi wrote:
> > >
> > > > @@ -1433,8 +1433,8 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
> > > >               {
> > > >                       ereport(ERROR,
> > > >                                       errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > > > -                                     errmsg("replication slots must not be invalidated during the upgrade"),
> > > > -                                     errhint("\"max_slot_wal_keep_size\" must be set to -1 during the
upgrade"));
> > >
> > > Hmm, if I read this code right, this error is going to be thrown by the
> > > checkpointer while finishing a checkpoint.  Fortunately, the checkpoint
> > > record has already been written, but I don't know what would happen if
> > > this is thrown while trying to write the shutdown checkpoint.  Probably
> > > nothing terribly good.
> > >
> > > I don't think this is useful.  If the setting is invalid during binary
> > > upgrade, let's prevent it from being set at all right from the start of
> > > the upgrade process.
> >
> > We are already forcing the required setting
> > "max_slot_wal_keep_size=-1" during the upgrade similar to some of the
> > other settings like "full_page_writes". However, the user can provide
> > an option for "max_slot_wal_keep_size" in which case it will be
> > overridden. Now, I think (a) we can ensure that our setting always
> > takes precedence in this case. The other idea is (b) to parse the
> > user-provided options and check if "max_slot_wal_keep_size" has a
> > value different than expected and raise an error accordingly. Or we
> > can simply (c) document the usage of max_slot_wal_keep_size in the
> > upgrade. I am not sure whether it is worth complicating the code for
> > this as the user shouldn't be using such an option during the upgrade.
> > So, I think doing (a) and (c) could be simpler.
> > >
> > >  In InvalidatePossiblyObsoleteSlot() we could have
> > > just an Assert() or elog(PANIC).
> > >
> >
> > Yeah, we can change to either of those.
>
> This discussion seems like a bit off from my point. I suggested adding
> a check for that setting when IsBinaryUpgraded is true at the GUC
> level as shown in the attached patch. I believe Álvaro made a similar
> suggestion.  While the error message is somewhat succinct, I think it
> is sufficient given the low possilibility of the scenario and the fact
> that it cannot occur inadvertently.
>

I think we can simply change that error message to assert if we want
to go with the check hook idea of yours. BTW, can we add
GUC_check_errdetail() with a better message as some of the other check
function uses? Also, I guess we can add some comments or at least
refer to the existing comments to explain the reason of such a check.

--
With Regards,
Amit Kapila.