Re: Rename max_parallel_degree?

On Mon, Oct 24, 2016 at 3:48 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Thu, Oct 13, 2016 at 5:28 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Wed, Oct 12, 2016 at 8:13 AM, Peter Eisentraut
>> <peter(dot)eisentraut(at)2ndquadrant(dot)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.

> Isn't it better to keep planner related checks from Julien's patch,
> especially below one:
>
> --- a/src/backend/optimizer/plan/planner.c
> +++ b/src/backend/optimizer/plan/planner.c
> @@ -249,6 +249,7 @@ standard_planner(Query *parse, int cursorOptions,
> ParamListInfo boundParams)
> parse->utilityStmt == NULL &&
> !parse->hasModifyingCTE &&
> max_parallel_workers_per_gather > 0 &&
> + max_parallel_workers > 0 &&
> !IsParallelWorker() &&
> !IsolationIsSerializable())

I don't really think we need to do that. If you set
max_parallel_workers_per_gather > max_parallel_workers, you might get
some plans that will never succeed in obtaining the number of workers
for which they planned. If that bothers you, don't do it. It's
actually quite useful to do that for testing purposes, though.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company