Re: Rename max_parallel_degree?

On Sun, Jun 26, 2016 at 3:57 PM, Julien Rouhaud
<julien(dot)rouhaud(at)dalibo(dot)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.

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. Also, see if you find the code
more readable by moving the after && part of check to next line.

3.
typedef struct BackgroundWorkerArray
{
int total_slots;
+ uint32
parallel_register_count;
+ uint32 parallel_terminate_count;
BackgroundWorkerSlot
slot[FLEXIBLE_ARRAY_MEMBER];
} BackgroundWorkerArray;

See, if you can add comments on top of this structure to explain the
usage/meaning of newly added parallel_* counters?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com