Re: walsender: Assert MyReplicationSlot is set before use

> 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:
>>
>> While reviewing patch [1], I noticed that in walsender.c, the function
>> NeedToWaitForStandbys() dereferences MyReplicationSlot->data.failover
>> without first checking whether MyReplicationSlot is NULL.
>>
>> Looking at the call chain:
>> ```
>> logical_read_xlog_page() // XLogReaderRoutine->page_read callback
>> -> WalSndWaitForWal()
>> -> NeedToWaitForWal()
>> -> NeedToWaitForStandbys()
>> ```
>>
>> None of these callers explicitly checks whether MyReplicationSlot is NULL.
>> Since they also don’t dereference MyReplicationSlot themselves, it wouldn’t
>> be fair to push that responsibility up to the callers.
>>
>> Looking elsewhere in the codebase, other places that dereference
>> MyReplicationSlot (for example CreateReplicationSlot()) either do an explicit if
>> (MyReplicationSlot != NULL) check or assert MyReplicationSlot != NULL. So it
>> seems reasonable to make the assumption explicit by adding an
>> Assert(MyReplicationSlot) in NeedToWaitForStandbys().
>
> I think it makes sense to add an Assert as you suggested.

Thanks for confirming.

>
>>
>> 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?

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