Re: pgsql: Get rid of the dedicated latch for signaling the startup process

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