On Fri, Oct 27, 2023 at 9:37 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Fri, Oct 27, 2023 at 1:58 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> >
> > Hello.
> >
> > Some messages recently introduced by commit 29d0a77fa6 seem to deviate
> > slightly from our standards.
> >
> > + if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade)
> > + {
> > + 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"));
> >
> > The message for errhint is not a complete sentence. And errmsg is not
> > in telegraph style. The first attached makes minimum changes.
> >
> > However, if allowed, I'd like to propose an alternative set of
> > messages as follows:
> >
> > + errmsg("replication slot is invalidated during upgrade"),
> > + errhint("Set \"max_slot_wal_keep_size\" to -1 to avoid invalidation."));
> >
> > The second attached does this.
> >
> > What do you think about those?
> >
>
> IIUC the only possible way to reach this error (according to the
> comment preceding it) is by the user overriding the GUC value (which
> was defaulted -1) on the command line.
>
Yeah, this is my understanding as well.
> + /*
> + * The logical replication slots shouldn't be invalidated as
> + * max_slot_wal_keep_size GUC is set to -1 during the upgrade.
> + *
> + * The following is just a sanity check.
> + */
>
> Given that, I felt a more relevant msg/hint might be like:
>
> errmsg("\"max_slot_wal_keep_size\" must be set to -1 during the upgrade"),
> errhint("Do not override \"max_slot_wal_keep_size\" using command line
> options."));
>
But OTOH, we don't have a value of user-passed options to ensure that.
So, how about a slightly different message: "This can be caused by
overriding \"max_slot_wal_keep_size\" using command line options." or
something along those lines? I see a somewhat similar message in the
existing code (errhint("This can be caused ...")).
--
With Regards,
Amit Kapila.