> On Feb 2, 2026, at 14:41, vignesh C <[email protected]> wrote: > > On Mon, 2 Feb 2026 at 11:55, Chao Li <[email protected]> wrote: >> >> Hi Hackers, >> >> 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(). >> >> 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(). >> >> The attached patch addresses both of these points. > > You have forgotten to attach the patch. > > Regards, > Vignesh
Oops, here comes it. Thanks for the reminder. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
v1-0001-walsender-Assert-MyReplicationSlot-is-set-before-.patch
Description: Binary data
