On Fri, Oct 27, 2023 at 1:58 PM Kyotaro Horiguchi
<horikyota....@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
