fixing consider_parallel for upper planner rels

On Sun, Jun 26, 2016 at 10:42 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Mon, Jun 13, 2016 at 05:27:13PM -0400, Robert Haas wrote:
>> 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.
>> When I originally wrote a lot of this logic, there were no upper rels,
>> which led to this code:
>>
>> if (force_parallel_mode == FORCE_PARALLEL_OFF || !glob->parallelModeOK)
>> {
>> glob->parallelModeNeeded = false;
>> glob->wholePlanParallelSafe = false; /* either
>> false or don't care */
>> }
>> else
>> {
>> glob->parallelModeNeeded = true;
>> glob->wholePlanParallelSafe =
>> !has_parallel_hazard((Node *) parse, false);
>> }
>>
>> The main way that wholePlanParallelSafe is supposed to be cleared is
>> in create_plan:
>>
>> /* Update parallel safety information if needed. */
>> if (!best_path->parallel_safe)
>> root->glob->wholePlanParallelSafe = false;
>>
>> However, at the time that code was written, it was impossible to rely
>> entirely on the latter mechanism. Since there were no upper rels and
>> no paths for upper plan nodes, you could have the case where every
>> path was parallel-safe but the whole plan was node. Therefore the
>> code shown above was needed to scan the whole darned plan for
>> parallel-unsafe things. Post-pathification, this whole thing is
>> pretty bogus: upper rels mostly don't get consider_parallel set at
>> all, which means plans involving upper rels get marked parallel-unsafe
>> even if they are not, which means the wholePlanParallelSafe flag gets
>> cleared in a bunch of cases where it shouldn't. And on the flip side
>> I think that the first chunk of code quoted above would be totally
>> unnecessary if we were actually setting consider_parallel correctly on
>> the upper rels.
>
> [Action required within 72 hours. This is a generic notification.]
>
> The above-described topic is currently a PostgreSQL 9.6 open item ("set
> consider_parallel correctly for upper rels"). Robert, since you committed the
> patch believed to have created it, you own this open item. If some other
> commit is more relevant or if this does not belong as a 9.6 open item, please
> let us know. Otherwise, please observe the policy on open item ownership[1]
> and send a status update within 72 hours of this message. Include a date for
> your subsequent status update. Testers may discover new open items at any
> time, and I want to plan to get them all fixed well in advance of shipping
> 9.6rc1. Consequently, I will appreciate your efforts toward speedy
> resolution. Thanks.

I'm not sure how to proceed here. I have asked Tom several times to
look at the WIP patch and offer comments, but he so far has not done
so. I am reluctant to commit more patches whacking the planner around
post-beta2 without some review from the guy who invented the upper
planner pathification stuff that broke this in the first place. What
I have here was more or less correct before that. It could be argued
that this problem should really be attributed to
3fc6e2d7f5b652b417fa6937c34de2438d60fa9f rather than any of the
parallel query commits -- though it's certainly the case that
e06a38965b3bcdaa881e7e06892d4d8ab6c2c980 was the result of massive
fuzzy thinking on my part. I am quite worried that if I whack this
around some more it's going to break more stuff, and I don't know that
I feel very comfortable doing that without some independent review.

Suggestions?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company