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.
======
[1] https://www.postgresql.org/message-id/2758485.1698848717%40sss.pgh.pa.us
Kind Regards,
Peter Smith.
Fujitsu Australia
