On 2020/11/05 23:32, Fujii Masao wrote:
On 2020/11/05 6:02, Fujii Masao wrote:
On 2020/11/05 5:36, Heikki Linnakangas wrote:
On 04/11/2020 15:17, Heikki Linnakangas wrote:
On 04/11/2020 14:03, Fujii Masao wrote:
Or ISTM that WakeupRecovery() should set the latch only when the latch
has not been reset to NULL yet.
Got to be careful with race conditions, if the latch is set to NULL at
the same time that WakeupRecovery() is called.
I don't think commit 113d3591b8 got this quite right:
void
WakeupRecovery(void)
{
if (XLogCtl->recoveryWakeupLatch)
SetLatch(XLogCtl->recoveryWakeupLatch);
}
If XLogCtl->recoveryWakeupLatch is set to NULL between the if and the SetLatch,
you'll still get a segfault. That's highly unlikely to happen in practice because
the compiler will optimize that into a single load instruction, but could happen
with -O0. I think you'd need to do the access only once, using a volatile pointer,
to make that safe.
On second thought, since fetching the latch pointer might not be atomic,
maybe we need to use spinlock like WalRcvForceReply() does. But using
spinlock in every calls of WakeupRecovery() might cause performance
overhead. So I'm thinking to use spinlock only when it's necessary, i.e.,
when the latch may be reset to NULL concurrently. Attached is the POC
patch implementing that. Thought?
Previously I added this patch to next CommitFest. But I reverted the commit
ac22929a26 and 113d3591b8 because those changes have other issue. So this
patch is no longer necessary, and I dropped it from next CommitFest.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
