Robert Haas <robertmhaas@gmail.com> writes:
> Again, please have a look at the patch and see if it seems unsuitable
> to you for some reason. I'm not confident of my ability to get all of
> this path stuff right without a bit of help from the guy who designed
> it.
I'm kind of in a bind right now because Tomas has produced an
FK-selectivity patch rewrite on schedule, and I already committed to
review that before beta2, so I better go do that first. If you can wait
awhile I will try to help out more with parallel query.
Having said that, the main thing I noticed in your draft patch is that
you'd removed set_grouped_rel_consider_parallel() in favor of hard-wiring
this in create_grouping_paths():
+ if (input_rel->consider_parallel &&
+ !has_parallel_hazard((Node *) target->exprs, false) &&
+ !has_parallel_hazard((Node *) parse->havingQual, false))
+ grouped_rel->consider_parallel = true;
I think this is bad because it forces any external creators of
UPPERREL_GROUP_AGG to duplicate that code --- and heaven help everybody
if anyone is out of sync on whether to set the flag. So I'd rather keep
set_grouped_rel_consider_parallel(), even if all it does is the above.
And make it global not static. Ditto for the other upperrels.
Also, I wonder whether we could reduce that test to just the
has_parallel_hazard tests, so as to avoid the ordering dependency of
needing to create the top scan/join rel (and set its consider_parallel
flag) before we can create the UPPERREL_GROUP_AGG rel. This would mean
putting more dependency on per-path parallel_safe flags to detect whether
you can't parallelize a given step for lack of parallel-safe input, but
I'm not sure that that's a bad thing.
regards, tom lane
