Hi, here are some minor review comments for the v3 patch. ====== src/backend/access/transam/xlog.c
1. check_max_slot_wal_keep_size +/* + * GUC check_hook for max_slot_wal_keep_size + * + * If WALs required by logical replication slots are removed, the slots are + * unusable. While pg_upgrade sets this variable to -1 via the command line to + * attempt to prevent such removal during binary upgrade, there are ways for + * users to override it. For the sake of completing the objective, ensure that + * this variable remains unchanged. See InvalidatePossiblyObsoleteSlot() and + * start_postmaster() in pg_upgrade for more details. + */ 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. ... ~~~ 2. +bool +check_max_slot_wal_keep_size(int *newval, void **extra, GucSource source) +{ + if (IsBinaryUpgrade && *newval != -1) + { + GUC_check_errdetail("\"max_slot_wal_keep_size\" must be set to -1 during binary upgrade mode."); + return false; + } + return true; +} 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"); ====== src/backend/replication/slot.c 3. InvalidatePossiblyObsoleteSlot - if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade) - { - ereport(ERROR, - errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("replication slots must not be invalidated during the upgrade"), - errhint("\"max_slot_wal_keep_size\" must be set to -1 during the upgrade")); - } + Assert (!*invalidated || !SlotIsLogical(s) || !IsBinaryUpgrade); IMO new Assert became trickier to understand than the original condition. YMMV. SUGGESTION Assert(!(*invalidated && SlotIsLogical(s) && IsBinaryUpgrade)); ====== Kind Regards, Peter Smith. Fujitsu Australia