On Monday, February 2, 2026 2:25 PM Chao Li <li.evan.chao@gmail.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.
>
> 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.
Best Regards,
Hou zj
