Re: A recent message added to pg_upgade

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.

+ /*
+ * 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."));

======
Kind Regards,
Peter Smith.
Fujitsu Australia