On Mon, Nov 10, 2025 at 8:05 PM shveta malik <[email protected]> wrote: > > On Thu, Nov 6, 2025 at 4:32 AM Masahiko Sawada <[email protected]> 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.
Agreed with all the above points. Will fix and update the updated version. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
