On 2025-Jul-09, Dilip Kumar wrote:
> On Wed, Jul 9, 2025 at 9:07 AM Dilip Kumar <dilipbal...@gmail.com> wrote:
> > After further consideration, I believe your proposed method is
> > superior to forcing the max_slot_wal_keep_size to -1 via a check hook.
> > The ultimate goal is to prevent WAL removal during a binary upgrade,
> > and your approach directly addresses this issue rather than
> > controlling it by forcing the GUC value. I am planning to send a
> > patch using this approach for both max_slot_wal_keep_size as well as
> > for idle_replication_slot_timeout.
>
> PFA, with this approach.
This indeed makes the whole thing a lot simpler and more direct than the
original code, and solves this subthread's complaint. It's a bit weird
that slot.c and xlog.c now have to worry about IsBinaryUpgrade, but I
don't feel any guilt about that.
I propose a few comment updates on top of your patch.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Oh, great altar of passive entertainment, bestow upon me thy discordant images
at such speed as to render linear thought impossible" (Calvin a la TV)
diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index 91568024aed..304b60933c9 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8131,25 +8131,22 @@ KeepLogSeg(XLogRecPtr recptr, XLogSegNo *logSegNo)
XLByteToSeg(recptr, currSegNo, wal_segment_size);
segno = currSegNo;
- /*
- * Calculate how many segments are kept by slots first, adjusting for
- * max_slot_wal_keep_size.
- */
+ /* Calculate how many segments are kept by slots. */
keep = XLogGetReplicationSlotMinimumLSN();
if (keep != InvalidXLogRecPtr && keep < recptr)
{
XLByteToSeg(keep, segno, wal_segment_size);
/*
- * Avoid WAL removal by the checkpointer process during Binary
- * upgrade. If WALs required by logical replication slots are
removed,
- * the slots are unusable.
+ * Account for max_slot_wal_keep_size to avoid keeping more than
+ * configured. However, don't do that during a binary upgrade:
if
+ * slots were to be invalidated because of this, it would not be
+ * possible to preserve logical ones during the upgrade.
*/
if (max_slot_wal_keep_size_mb >= 0 && !IsBinaryUpgrade)
{
uint64 slot_keep_segs;
- /* Cap by max_slot_wal_keep_size ... */
slot_keep_segs =
ConvertToXSegs(max_slot_wal_keep_size_mb,
wal_segment_size);
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 82c7d263f94..0e1e85d52cf 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -2036,7 +2036,7 @@ restart:
if (!s->in_use)
continue;
- /* Prevent logical slot invalidation during binary upgrade. */
+ /* Prevent invalidation of logical slots during binary upgrade.
*/
if (SlotIsLogical(s) && IsBinaryUpgrade)
continue;
