On Thu, Nov 2, 2023 at 1:58 PM Kyotaro Horiguchi <horikyota....@gmail.com> wrote: > > Thanks you for the comments! > > At Wed, 1 Nov 2023 18:08:19 +1100, Peter Smith <smithpb2...@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