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
