Re: fixing consider_parallel for upper planner rels

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Here is a new patch addressing (I hope) the above comments and a few
> other things.

Some more comments:

* It seems like the initialization of root->parallelModeNeeded is still
overcomplicated and/or underexplained. Why do you not merely set it false
to start with, and allow it to become true when/if a Gather is added to
the plan? AFAICS its only point is to tell the executor that there's a
Gather someplace.

* typo in new comment in grouping_planner: "final_rel is can be marked"

+ * If the input relation is not parallel-safe, then the window relation
+ * can't be parallel-safe, either. Otherwise, we need to examine the
+ * target list and windows for non-parallel-safe constructs.
+ * (XXX: Do we also need to check wflists?)

No, we don't, because the wflists just contain windowfuncs extracted from
the target list. But I'd say "target list and active windows for..." for
more clarity (the point being it's okay to ignore any unused window
clauses).

* root->parallelModeOK is probably not being consulted in as many places
as it would be useful; could we use it to short-circuit any more
has_parallel_hazard tests?

* I'm still not real happy with the has_parallel_hazard test added to
apply_projection_to_path, and here is why not: if the target path is
not projection-capable, we'll never get to that test. We just pass
the problem off to create_projection_path, which looks no further
than rel->consider_parallel. It seems like we have to add a
has_parallel_hazard test there as well, which is a tad annoying,
not least because all the direct callers of create_projection_path
seem to have made the test already.

This gets back to the question of whether rel->consider_parallel is even
useful at the level of upperrels. As far as I can tell, that flag is
really little more than a crutch to let scan/join paths be marked parallel
safe or not with minimum ado. I would rather like to get to a point where
consider_parallel is not used for upperrels, because that avoids the
problem that you're not setting it early enough to be useful for
GetForeignUpperPaths(). (After that I'd like to get to a point where we
don't have it at all, but that may not happen for 9.6.)

> Unfortunately, I can't think of a clean way to make the "right" thing
> happen here. In general, it's right to prefer a parallel-safe plan
> over an ever-so-slightly more expensive plan that isn't, because that
> might let us join it to a partial path later on. However, if we're at
> the very top of the plan tree, that argument holds no force, so we
> could try to install some kind of exception for that case. That seems
> like it would be fairly invasive and fairly ugly, though. It seems
> like a better option would be to try to kludge the test case somehow
> so that the backward index scan looks at least 1% cheaper than the
> forward index scan, but I can't see how to do that. I don't see, for
> example, that changing any of the usual costing factors would help
> much. We could do something very specialized here, like adding a GUC
> that makes MinMaxAggPath always win, but that seems ugly, too.

If we made add_path not consider parallel safety as a preference item
when glob->parallelModeOK is false, then we could set
max_parallel_workers_per_gather to zero for this test case and the
problem would go away. In general, that seems like it would be a good
thing for end users too: they could avoid plan changes that are made
on the basis of completely irrelevant parallel-safety. It might be
that we don't actually need an explicit check for that in add_path,
as long as we make sure that no path ever gets marked parallel_safe
when parallelModeOK is false.

regards, tom lane