On 27/06/2016 07:18, Amit Kapila wrote:
> On Mon, Jun 27, 2016 at 10:21 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> I think as the parallel_terminate_count is only modified by postmaster
>> and read by other processes, such an operation will be considered
>> atomic. We don't need to specifically make it atomic unless multiple
>> processes are allowed to modify such a counter. Although, I think we
>> need to use some barriers in code to make it safe.
>>
>> 1.
>> @@ -272,6 +279,8 @@ BackgroundWorkerStateChange(void)
>> pg_memory_barrier();
>>
>> slot->pid = 0;
>> slot->in_use = false;
>> + if (slot->parallel)
>> +
>> BackgroundWorkerData->parallel_terminate_count++;
>>
>> I think the check of slot->parallel and increment to
>> parallel_terminate_count should be done before marking slot->in_use to
>> false. Consider the case if slot->in_use is marked as false and then
>> before next line execution, one of the backends registers dynamic
>> worker (non-parallel worker), so that
>> backend can see this slot as free and use it. It will also mark the
>> parallel flag of slot as false. Now when postmaster will check the
>> slot->parallel flag, it will find it false and then skip the increment
>> to parallel_terminate_count.
>>
>
> If you agree with above theory, then you need to use write barrier as
> well after incrementing the parallel_terminate_count to avoid
> re-ordering with slot->in_use flag.
>
I totally agree, I didn't thought about this corner case.
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? I guess that a write
barrier will be needed in ForgetBacgroundWorker().
>> 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? (Sorry, I'm not
at all familiar with the pg_barrier mechanism).
>> 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.
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
