On Mon, Jun 27, 2016 at 10:21 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Sun, Jun 26, 2016 at 3:57 PM, Julien Rouhaud
> <julien.rouhaud@dalibo.com> wrote:
>> On 26/06/2016 08:37, Amit Kapila wrote:
>>>
>>> @@ -370,6 +379,8 @@ ForgetBackgroundWorker(slist_mutable_iter *cur)
>>> Assert(rw->rw_shmem_slot <
>>> max_worker_processes);
>>> slot = &BackgroundWorkerData->slot[rw->rw_shmem_slot];
>>> slot->in_use =
>>> false;
>>> + if (slot->parallel)
>>> + BackgroundWorkerData->parallel_terminate_count++;
>>>
>>> I think operations on parallel_terminate_count are not safe.
>>> ForgetBackgroundWorker() and RegisterDynamicBackgroundWorker() can try
>>> to read write at same time. It seems you need to use atomic
>>> operations to ensure safety.
>>>
>>>
>>
>> But operations on parallel_terminate_count are done by the postmaster,
>> and per Robert's previous email postmaster can't use atomics:
>>
>
> 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.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com