Horton
Menu

Re: Rename max_parallel_degree?

Amit Kapila <amit.kapila16@gmail.com>

View thread
From: Amit Kapila <amit.kapila16@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>, Julien Rouhaud <julien.rouhaud@dalibo.com>, Tom Lane <tgl@sss.pgh.pa.us>, Josh Berkus <josh@agliodbs.com>, "David G. Johnston" <david.g.johnston@gmail.com>, Peter Geoghegan <pg@heroku.com>, Noah Misch <noah@leadboat.com>, David Rowley <david.rowley@2ndquadrant.com>, Alvaro Herrera <alvherre@2ndquadrant.com>, Magnus Hagander <magnus@hagander.net>, Bruce Momjian <bruce@momjian.us>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2016-10-26T13:03:07Z
Lists: pgsql-hackers 
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