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
