From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
---|---|
To: | Peter Smith <smithpb2250(at)gmail(dot)com> |
Cc: | Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: A recent message added to pg_upgade |
Date: | 2023-10-27 04:21:43 |
Message-ID: | CAA4eK1JWaTh7CgFbAPZzq1xdaMpUwyxCbrg4jCDAs8evCn3ZZw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Oct 27, 2023 at 9:37 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Fri, Oct 27, 2023 at 1:58 PM Kyotaro Horiguchi
> <horikyota(dot)ntt(at)gmail(dot)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.
From | Date | Subject | |
---|---|---|---|
Next Message | Philip Warner | 2023-10-27 04:25:30 | pg_dump not dumping the run_as_owner setting from version 16? |
Previous Message | Peter Smith | 2023-10-27 04:06:50 | Re: A recent message added to pg_upgade |