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