On 28/06/2016 04:44, Amit Kapila wrote:
> On Mon, Jun 27, 2016 at 10:35 PM, Julien Rouhaud
>>
>> There's already a pg_memory_barrier() call in
>> BackgroundWorkerStateChange(), to avoid reordering the notify_pid load.
>> Couldn't we use it to also make sure the parallel_terminate_count
>> increment happens before the slot->in_use store?
>>
>
> Yes, that is enough, as memory barrier ensures that both loads and
> stores are completed before any loads and stores that are after
> barrier.
>
>> I guess that a write
>> barrier will be needed in ForgetBacgroundWorker().
>>
>
> Yes.
>
>>>> 2.
>>>> + if (parallel && (BackgroundWorkerData->parallel_register_count -
>>>> +
>>>> BackgroundWorkerData->parallel_terminate_count) >=
>>>> +
>>>> max_parallel_workers)
>>>> + {
>>>> + LWLockRelease(BackgroundWorkerLock);
>>>> + return
>>>> false;
>>>> + }
>>>> +
>>>>
>>>> I think we need a read barrier here, so that this check doesn't get
>>>> reordered with the for loop below it.
>>
>> You mean between the end of this block and the for loop?
>>
>
> Yes.
>
>>>> Also, see if you find the code
>>>> more readable by moving the after && part of check to next line.
>>
>> I think I'll just pgindent the file.
>>
>
> make sense.
>
>
Thanks a lot for the help!
PFA v6 which should fix all the issues mentioned. Also, after second
thought I didn't add the extra hint about max_worker_processes in the
max_parallel_worker paragraph, since this line was a duplicate of the
precedent paragraph, it seemed better to leave the text as is.
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
