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