Re: A recent message added to pg_upgade

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

> I asked ChatGPT to suggest alternative wording for that comment, and
> it came up with something that I felt was a slight improvement.
> 
> SUGGESTION
> ...
> If WALs needed by logical replication slots are deleted, these slots
> become inoperable. During a binary upgrade, pg_upgrade sets this
> variable to -1 via the command line in an attempt to prevent such
> deletions, but users have ways to override it. To ensure the
> successful completion of the upgrade, it's essential to keep this
> variable unaltered.
> ...
> 
> ~~~

ChatGPT seems to tend to generate sentences in a slightly different
from our usual writing. While I tried to retain the original phrasing
in the patch, I don't mind using the suggested version.  Used as is.

> 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.

> ======
> src/backend/replication/slot.c
> 
> 3. InvalidatePossiblyObsoleteSlot

> + Assert (!*invalidated || !SlotIsLogical(s) || !IsBinaryUpgrade);
> 
> IMO new Assert became trickier to understand than the original condition. YMMV.
> 
> SUGGESTION
> Assert(!(*invalidated && SlotIsLogical(s) && IsBinaryUpgrade));

Yeah, I also liked that style and considered using it, but I didn't
feel it was too hard to read in this particular case, so I ended up
using the current way.  Just like with the point of other comments,
I'm not particularly attached to this style. Thus if someone find it
difficult to read, I have no issue with changing it. Revised as
suggested.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center