Re: Rename max_parallel_degree?

On Sat, Jun 25, 2016 at 2:27 PM, Julien Rouhaud
<julien(dot)rouhaud(at)dalibo(dot)com> wrote:
> On 25/06/2016 09:33, Amit Kapila wrote:
>> On Wed, Jun 15, 2016 at 11:43 PM, Julien Rouhaud
>> <julien(dot)rouhaud(at)dalibo(dot)com> wrote:
>>>
>>> Attached v4 implements the design you suggested, I hope everything's ok.
>>>
>>
>> Few review comments:
>>
>
> Thanks for the review.
>
>
>
>> 4.
>> + <varlistentry id="guc-max-parallel-workers"
>> xreflabel="max_parallel_workers">
>> + <term><varname>max_parallel_workers</varname> (<type>integer</type>)
>> + <indexterm>
>> + <primary><varname>max_parallel_workers</> configuration
>> parameter</primary>
>> + </indexterm>
>> + </term>
>> + <listitem>
>> + <para>
>> + Sets the maximum number of workers that can be launched at the same
>> + time for the whole server. This parameter allows the administrator to
>> + reserve background worker slots for for third part dynamic background
>> + workers. The default value is 4. Setting this value to 0 disables
>> + parallel query execution.
>> + </para>
>> + </listitem>
>> + </varlistentry>
>>
>> How about phrasing it as:
>> Sets the maximum number of workers that the system can support for
>> parallel queries. The default value is 4. Setting this value to 0
>> disables parallel query execution.
>>
>
> It's better thanks. Should we document somewhere the link between this
> parameter and custom dynamic background workers or is it pretty
> self-explanatory?
>

How about if add an additiona line like:
Parallel workers are taken from the pool of processes established by
guc-max-worker-processes.

I think one might feel some duplication of text between this and what
we have for max_parallel_workers_per_gather, but it seems genuine to
me.

@@ -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.

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