On Mon, Jun 13, 2016 at 5:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> One problem that I've realized that is related to this is that the way
>> that the consider_parallel flag is being set for upper rels is almost
>> totally bogus, which I believe accounts for your complaint at PGCon
>> that force_parallel_mode was not doing as much as it ought to do.
>
> Yeah, I just ran into a different reason to think that. I tried marking
> MinMaxAggPaths in the same way as other relation-scan paths, ie
>
> pathnode->path.parallel_safe = rel->consider_parallel;
>
> but that did nothing because the just-created UPPERREL_GROUP_AGG rel
> didn't have its consider_parallel flag set yet. Moreover, if I'd tried to
> fix that by invoking set_grouped_rel_consider_parallel() from planagg.c,
> it would still not work right because set_grouped_rel_consider_parallel()
> is hard-wired to consider only partial aggregation, which is not what's
> going on in a MinMaxAggPath. I'm not sure about the best rewrite here,
> but I would urge you to make sure that consider_parallel on an upper rel
> reflects only properties inherent to the rel (eg, safeness of quals that
> must be applied there) and not properties of specific implementation
> methods.
Yeah, I think that David and I goofed this up when adding parallel
aggregation. I just wasn't thinking clearly about the way upper rels
needed to work with consider_parallel. If you would be willing to do
any sort of review of the WIP patch I posted just upthread, that would
help.
> Also, please make sure that whatever logic is involved remains
> factored out where it can be called by an extension that might want to
> create an upperrel sooner than the core code would do.
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.
> BTW, do we need wholePlanParallelSafe at all, and if so why? Can't
> it be replaced by examining the parallel_safe flag on the selected
> topmost Path?
Oh, man. I think you're right. This is yet another piece of fallout
from upper planner pathification. That was absolutely necessary
before, but now if we do the other bits right we don't need it any
more.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company