On Mon, Jun 13, 2016 at 5:46 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: > Robert Haas <robertmh...@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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers