Re: A recent message added to pg_upgade

On Mon, Oct 30, 2023 at 7:58 AM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
>
> At Fri, 27 Oct 2023 14:57:10 +0530, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote in
> > On Fri, Oct 27, 2023 at 2:02 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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.