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
