Re: walsender: Assert MyReplicationSlot is set before use

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

Thanks for the information. I will verify that.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/