> 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:
>>
>> 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/
