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