fixing consider_parallel for upper planner rels - Mailing list pgsql-hackers

On Sun, Jun 26, 2016 at 10:42 PM, Noah Misch <noah@leadboat.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