From: | Julien Rouhaud <julien(dot)rouhaud(at)dalibo(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | James Sewell <james(dot)sewell(at)lisasoft(dot)com>, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Paul Ramsey <pramsey(at)cleverelephant(dot)ca> |
Subject: | Re: Choosing parallel_degree |
Date: | 2016-04-04 15:03:58 |
Message-ID: | 5702825E.2010108@dalibo.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
rel_parallel_degree_v6.diff | text/plain | 7.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Langote | 2016-04-04 15:05:20 | Re: postgres_fdw : altering foreign table not invalidating prepare statement execution plan. |
Previous Message | Craig Ringer | 2016-04-04 14:59:41 | Re: Timeline following for logical slots |