On Wed, Oct 26, 2016 at 5:54 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Oct 24, 2016 at 3:48 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> On Thu, Oct 13, 2016 at 5:28 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> On Wed, Oct 12, 2016 at 8:13 AM, Peter Eisentraut
>>> <peter.eisentraut@2ndquadrant.com> wrote:
>>>> On 10/4/16 10:16 AM, Julien Rouhaud wrote:
>>>>> Please find attached v9 of the patch, adding the parallel worker class
>>>>> and changing max_worker_processes default to 16 and max_parallel_workers
>>>>> to 8. I also added Amit's explanation for the need of a write barrier
>>>>> in ForgetBackgroundWorker().
>>>>
>>>> This approach totally messes up the decoupling between the background
>>>> worker facilities and consumers of those facilities. Having dozens of
>>>> lines of code in bgworker.c that does the accounting and resource
>>>> checking on behalf of parallel.c looks very suspicious. Once we add
>>>> logical replication workers or whatever, we'll be tempted to add even
>>>> more stuff in there and it will become a mess.
>>>
>>> I attach a new version of the patch that I've been hacking on in my
>>> "spare time", which reduces the footprint in bgworker.c quite a bit.
>>>
>>
>> Couple of comments -
>>
>> @@ -370,6 +388,9 @@ ForgetBackgroundWorker(slist_mutable_iter *cur)
>>
>> Assert(rw->rw_shmem_slot <
>> max_worker_processes);
>> slot = &BackgroundWorkerData->slot[rw->rw_shmem_slot];
>> + if ((rw-
>>>rw_worker.bgw_flags & BGWORKER_CLASS_PARALLEL) != 0)
>> + BackgroundWorkerData-
>>>parallel_terminate_count++;
>> +
>> slot->in_use = false;
>>
>> It seems upthread [1], we agreed to have a write barrier before the
>> setting slot->in_use, but I don't see the same in patch.
>
> That's because I removed it. The reason given for the barrier was
> that otherwise it might be reordered before the check of
> is_parallel_worker, but that's now done by checking the postmaster's
> backend-private copy of the flags, not the copy in shared memory. So
> the reordering can't affect the result.
>
You are right. I missed to notice that.
The patch looks good to me.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
