On Sun, Nov 9, 2025 at 8:51 PM Peter Smith <[email protected]> wrote: > > Hi Sawada-San. > > Some review comments for v24-0001. > > ====== > src/backend/replication/slot.c > > ReplicationSlotsDropDBSlots: > > 1. > LWLockAcquire(ReplicationSlotControlLock, LW_SHARED); > for (i = 0; i < max_replication_slots; i++) > { > ReplicationSlot *s; > + bool invalidated; > char *slotname; > int active_pid; > > @@ -1462,11 +1494,22 @@ restart: > if (!SlotIsLogical(s)) > continue; > > + 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++; > + > > IMO it's misleading/backwards to have bool invalidated == true when > the invalidated member is RS_INVAL_NONE. > > It's fine to rename the bool var as 'invalidated', but don't you also > need to reverse the assignment to (s->data.invalidated != > RS_INVAL_NONE) instead of (s->data.invalidated == RS_INVAL_NONE)?
Thank you for the comment! I'll fix it in the next version patch. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
