Re: POC: enable logical decoding when wal_level = 'replica' without a server restart

On Thu, Nov 6, 2025 at 4:32 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
>
> I've updated and rebased the patch.
>

Thanks for the patch. Please find a few comments:


1)
ReplicationSlotsDropDBSlots:

+ SpinLockAcquire(&s->mutex);
+ invalidated = s->data.invalidated == RS_INVAL_NONE;
+ SpinLockRelease(&s->mutex);
+
+ /*
+ * Count slots on other databases too so we can disable logical
+ * decoding only if no slots in the cluster.
+ */
+ if (invalidated)
+ n_valid_logicalslots++;


This seems confusing to me. Can we instead do:

SpinLockAcquire(&s->mutex);
if (s->data.invalidated == RS_INVAL_NONE)
n_valid_logicalslots++;
SpinLockRelease(&s->mutex);

2)
InvalidateObsoleteReplicationSlots:

+ bool islogical = SlotIsLogical(s);

  /* Prevent invalidation of logical slots during binary upgrade */
  if (SlotIsLogical(s) && IsBinaryUpgrade)
+ {
+ SpinLockAcquire(&s->mutex);
+ if (s->data.invalidated == RS_INVAL_NONE)
+ n_valid_logicalslots++;
+ SpinLockRelease(&s->mutex);
+
  continue;
+ }

We should use 'islogical' instead of SlotIsLogical here.

3)
InvalidateObsoleteReplicationSlots() is more robust now as we are
using both 'invalidated' and 'released_lock' flags but still nowhere
we guarantee that invalidated=true implies released_lock=true. Since
we jump to 'restart' label only if released_lock is true, it becomes
important to have an ASSERT which says invalidated=true implicitly
means released_lock=true or vice versa. Because at the end we go by
'invalidated_logical' rather than 'released_lock' to decide about
logical-decoding disabling.

In this logic:

+ if (InvalidatePossiblyObsoleteSlot(possible_causes, s, oldestLSN,
+    dboid, snapshotConflictHorizon,
+    &released_lock))
  {
- /* if the lock was released, start from scratch */
- goto restart;
+ /* Remember we have invalidated a physical or logical slot */
+ invalidated = true;
+
+ /*
+ * Additionally, remember we have invalidated a logical slot too
+ * as we can request disabling logical decoding later.
+ */
+ if (islogical)
+ invalidated_logical = true;
  }

Shall we have an Assert(released_lock) if
InvalidatePossiblyObsoleteSlot returns true. Or any better way?

4)
+ SpinLockAcquire(&s->mutex);
+ if (s->data.invalidated == RS_INVAL_NONE)
+ n_valid_logicalslots++;

In the same function, isn't the above code problematic: Don't we need
'islogical' check before incrementing 'n_valid_logicalslots',
otherwise it may wrongly count valid physical slots as well.

thanks
Shveta