On Monday, February 2, 2026 3:16 PM Chao Li <[email protected]> wrote: > > > On Feb 2, 2026, at 14:43, Zhijie Hou (Fujitsu) <[email protected]> > wrote: > > > > On Monday, February 2, 2026 2:25 PM Chao Li <[email protected]> wrote: > > > >> > >> Another related issue is in StartLogicalReplication(): the function > >> initially asserts MyReplicationSlot == NULL, then calls > >> ReplicationSlotAcquire(), which is expected to set up > >> MyReplicationSlot. Since MyReplicationSlot is dereferenced later in > >> this function, I think it would be clearer and safer to add an > >> Assert(MyReplicationSlot != NULL) immediately after the call to > ReplicationSlotAcquire(). > > > > I don't think the suggested Assert is unnecessary, as the primary > > function of > > ReplicationSlotAcquire() is to get MyReplicationSlot assigned a valid > > value. Any issues that arise should be handled within the function > > itself. I think an Assert placed after this function does not add additional > safety. > > > > Yes, ReplicationSlotAcquire() has a retry logic to ensure MyReplicationSlot to > be set. So adding a NULL check here is a bit redundant. How about > Assert(SlotIsLogical(MyReplicationSlot))? Does it make more sense?
I think we cannot assume the slot type here. A suitable checking might be: If a physical slot was acquired during logical replication, report an error, just like we do in StartReplication(). I haven't verified whether this error will occur in practice, but I think we could reproduce this by dropping the original logical slot and then creating a new physical slot with the same name concurrently. Best Regards, Hou zj
