On Monday, February 2, 2026 3:16 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
> > On Feb 2, 2026, at 14:43, Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
> wrote:
> >
> > On Monday, February 2, 2026 2:25 PM Chao Li <li.evan.chao@gmail.com> 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
