On Thu, Nov 2, 2023 at 2:25 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Thu, Nov 2, 2023 at 1:58 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> >
> > Thanks you for the comments!
> >
> > At Wed, 1 Nov 2023 18:08:19 +1100, Peter Smith <smithpb2250@gmail.com> wrote in
> > > Hi, here are some minor review comments for the v3 patch.
> > >
> > > ======
> > > src/backend/access/transam/xlog.c
> >
> ...
> > > 2.
> >
> > > + GUC_check_errdetail("\"max_slot_wal_keep_size\" must be set to -1
> > > during binary upgrade mode.");
> >
> > > Some of the other GUC_check_errdetail()'s do not include the GUC name
> > > in the translatable message text. Isn't that a preferred style?
> >
> > > SUGGESTION
> > > GUC_check_errdetail("\"%s\" must be set to -1 during binary upgrade mode.",
> > > "max_slot_wal_keep_size");
> >
> > I believe that that style was adopted to minimize translatable
> > messages by consolidting identical ones that only differ in variable
> > names. I see both versions in the tree. I didn't find necessity to
> > adopt this approach for this specific message, especially since I'm
> > skeptical about adding new messages that end with "must be set to -1
> > during binary upgrade mode". (pg_upgrade sets synchronous_commit,
> > fsync and full_page_writes to "off".)
> >
> > However, some unique messages are in this style, so I'm fine with
> > using that style. Revised accordingly.
> >
>
> Checking this patch yesterday prompted me to create a new thread
> questioning the inconsistencies of the "GUC names in messages". In
> that thread, Tom Lane replied and gave some background information [1]
> about the GUC name embedding versus substitution. In hindsight, I
> think your original message was fine as-is, but there seem to be
> examples of every kind of style, so whatever you do would have some
> precedent.
>
> The patch v4 LGTM.
>
To clarify, all the current code LGTM, but the patch is still missing
a guc_hook test case, right?
======
Kind Regards,
Peter Smith.
Fujitsu Australia