Re: Choosing parallel_degree

On 04/04/2016 08:55, Amit Kapila wrote:

Thanks for the review!

> Few comments:
> 1.
> +      limited according to the <xref linkend="gux-max-parallel-degree">
>
> A. typo.
>    /gux-max-parallel-degree/guc-max-parallel-degree
>    /worker/workers

Oops, fixed.

> B. +     <para>
> +      Number of workers wanted for this table. The number of worker will be
> +      limited according to
> the <xref linkend="gux-max-parallel-degree">
> +      parameter.
> +     </para>
>
> How about writing the above as:
> Sets the degree of parallelism for an individual relation.  The
> requested number of workers will be limited by <xref
> linkend="guc-max-parallel-degree">
>

That's clearly better, changed.

> 2.
> +{
> +{
> +"parallel_degree",
> +"Number of parallel processes
> per executor node wanted for this relation.",
> +RELOPT_KIND_HEAP,
> +
> AccessExclusiveLock
> +},
> +-1, 1, INT_MAX
> +},
>
> I think here min and max values should be same as for
> max_parallel_degree (I have verified that for some of the other
> reloption parameters, min and max are same as their guc values); Is
> there a reason to keep them different?
>

No reason. I put 0 and MAX_BACKENDS, as the GUC value.

> 3.
> @@ -1291,7 +1300,9 @@ default_reloptions(Datum reloptions, bool
> validate, relopt_kind kind)
>
> Comment on top of this function says:
> /*
>  * Option parser for anything that uses StdRdOptions (i.e. fillfactor and
>  * autovacuum)
>  */
>
> I think it is better to include parallel_degree in above comment along
> with fillfactor and autovacuum.
>

Agreed. BTW the user_catalog_table option isn't listed either.

>
> 4.
> /*
> + * RelationGetMaxParallelDegree
> + *Returns the relation's parallel_degree.  Note multiple eval of
> argument!
> + */
> +#define RelationGetParallelDegree(relation, defaultmpd) \
> +((relation)->rd_options ? \
> +
> ((StdRdOptions *) (relation)->rd_options)->parallel_degree : (defaultmpd))
> +
>
> There are minor in-consistencies in the above macro definition.
>
> a. RelationGetMaxParallelDegree - This should be RelationGetParallelDegree.
> b. defaultmpd - it is better to name it as defaultpd
>

Yes, I forgot to update it when I renamed the option, fixed.

>
>>
>>
>> The feature freeze is now very close.  If this GUC is still wanted,
>> should I add this patch to the next commitfest?
>>
>
> I am hoping that this will be committed to 9.6, but I think it is good
> to register it in next CF.
>

So attached v6 fixes all the problems above.

I'll add it to the next commitfest.

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

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