Re: fixing consider_parallel for upper planner rels

On Mon, Jun 27, 2016 at 4:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Mon, Jun 27, 2016 at 3:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Oh, I thought you were still actively working on it.  What patch do
>>> you want me to review?
>
>> I'm looking for an opinion on the WIP patch attached to:
>> https://www.postgresql.org/message-id/CA+TgmoZwJB9EaiBj-MEeAQ913WkKOz4aQ7VQnCfrDLs9WYhZdQ@mail.gmail.com
>
> OK, I took a quick look.  Some of the details are obsolete due to my
> over-the-weekend hacking on parallel aggregation, but I think generally
> you have the right idea that we should set upperrel consider_parallel
> flags on the basis of "input rel has consider_parallel = true AND there
> are no parallel hazards in operations to be performed at this level".

OK, great.  Thanks.

> A few specific comments:
>
> * Can't we remove wholePlanParallelSafe altogether, in favor of just
> examining best_path->parallel_safe in standard_planner?

Yes, I think we can.  Before the upper planner used paths, we needed a
way to get wholePlanParallelSafe = false even if every generated path
was parallel-safe, but now that all planning stages have paths, we can
get rid of that.

> * In grouping_planner, I'm not exactly convinced that
> final_rel->consider_parallel can just be copied up without any further
> restrictions.  Easiest counterexample is a parallel restricted function in
> LIMIT.

OK, will look.

> * Why have the try_parallel_path flag at all in create_grouping_paths,
> rather than just setting or not setting grouped_rel->consider_parallel?
> If your thinking is that this is dealing with implementation restrictions
> that might not apply to paths added by an extension, then OK, but the
> variable needs to have a less generic name.  Maybe try_parallel_aggregation.

Suppose all of the relevant functions are parallel-safe, but the
aggregates don't have combine functions.  In that case,
consider_parallel ought to be true, because parallel strategies are OK
as a general matter, but the one and only parallel strategy we have
today - two-phase aggregation - will not work.

> * Copy/paste error in comment in create_distinct_paths: s/window/distinct/

OK, will fix.

> * Not following what you did to apply_projection_to_path, and the comment
> therein isn't helping.

Gee, I wonder why not?  :-)

The basic problem here is that applying a projection to a path can
render a formerly parallel-safe path no longer parallel-safe.  If we
jam a parallel-restricted target list into a formerly parallel-safe
path, we'd better also clear path->parallel_safe.  Currently,
apply_projection_to_path only needs to call has_parallel_hazard for an
input which is a GatherPath, which isn't too expensive because most
paths are not GatherPaths and if we get one that is, well, we can hope
parallel query will win enough during execution to make up for the
extra planning cost.  But if we want the consider_parallel and
parallel_safe flags to be set correctly for all upper rels and paths,
it seems that we need to do it always - hence the dismayed comment.
Thoughts?

>> It may not be correct in detail, but I'd like to know whether you
>> think it's going in the generally correct direction and what major
>> concerns you might have before spending more time on it.  Also, I'd
>> like to know whether you think it's something we should try to put
>> into 9.6 or whether you think we should leave it for next cycle.
>
> I think we should try to get this right in 9.6.  For one thing, the
> more stuff we can easily exercise in parallel mode, the more likely
> we are to find bugs before they reach the field.

OK.

(Official status update: I will post an updated version of this patch
by Thursday.)

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