Re: Rename max_parallel_degree?

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.

> 1.
> + if (parallel && (BackgroundWorkerData->parallel_register_count -
> +
> BackgroundWorkerData->parallel_terminate_count) >=
> +
> max_parallel_workers)
> + return false;
>
> I think this check should be done after acquiring
> BackgroundWorkerLock, otherwise some other backend can simultaneously
> increment parallel_register_count.
>

You're right, fixed.

> 2.
>
> +/*
> + * This flag is used internally for parallel queries, to keep track of the
> + * number of active
> parallel workers and make sure we never launch more than
> + * max_parallel_workers parallel workers at
> the same time. Third part
> + * background workers should not use this flag.
> + */
> +#define
> BGWORKER_IS_PARALLEL_WORKER 0x0004
> +
>
> "Third part", do yo want to say Third party?
>

Yes, sorry. Fixed

> 3.
> static bool
> SanityCheckBackgroundWorker(BackgroundWorker *worker, int elevel)
> {
> ..
> }
>
> Isn't it better to have a check in above function such that if
> bgw_flags is BGWORKER_IS_PARALLEL_WORKER and max_parallel_workers is
> zero, then ereport? Also, consider if it is better to have some other
> checks related to BGWORKER_IS_PARALLEL_WORKER, like if caller sets
> BGWORKER_IS_PARALLEL_WORKER, then it must have database connection and
> shared memory access.
>

I added these checks. I don't see another check to add right now.

> 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?

> 5.
> <primary><varname>max_parallel_workers_per_gather</> configuration
> parameter</primary>
> </indexterm>
> </term>
> <listitem>
> <para>
> Sets the maximum number of workers that can be started by a single
> <literal>Gather</literal> node. Parallel workers are taken from the
> pool of processes established by
> <xref linkend="guc-max-worker-processes">.
>
> I think it is better to change above in documentation to indicate that
> "pool of processes established by guc-max-parallel-workers".
>

The real limit is the minimum of these two values, I think it's
important to be explicit here, since this pool is shared for parallelism
and custom bgworkers.

What about "pool of processes established by guc-max-worker-processes,
limited by guc-max-parallel-workers" (used in attached v5 patch)

--
Julien Rouhaud
http://dalibo.com - http://dalibo.org