Re: walsender: Assert MyReplicationSlot is set before use

On Mon, 2 Feb 2026 at 11:55, Chao Li <li.evan.chao@gmail.com> wrote:
>
> Hi Hackers,
>
> While reviewing patch [1], I noticed that in walsender.c, the function NeedToWaitForStandbys() dereferences
MyReplicationSlot->data.failoverwithout 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
MyReplicationSlotthemselves, 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.
Soit seems reasonable to make the assumption explicit by adding an Assert(MyReplicationSlot) in
NeedToWaitForStandbys().
>
> Another related issue is in StartLogicalReplication(): the function initially asserts MyReplicationSlot == NULL, then
callsReplicationSlotAcquire(), which is expected to set up MyReplicationSlot. Since MyReplicationSlot is dereferenced
laterin this function, I think it would be clearer and safer to add an Assert(MyReplicationSlot != NULL) immediately
afterthe call to ReplicationSlotAcquire(). 
>
> The attached patch addresses both of these points.

You have forgotten to attach the patch.

Regards,
Vignesh