On Fri, Oct 27, 2023 at 9:37 AM Peter Smith <smithpb2...@gmail.com> wrote: > > 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. >
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.