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.
[1]
https://postgr.es/m/CAOzEurSRii0tEYhu5cePmRcvS=zrxtlevxm3kj0d7_ukgdm...@mail.gmail.com
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
