Re: fixing consider_parallel for upper planner rels

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Jun 27, 2016 at 4:49 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> * 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?

Seems to me that it should generally be the case that consider_parallel
would already be clear on the parent rel if the tlist isn't parallel safe,
and if it isn't we probably have a bug elsewhere. If it makes you feel
better, maybe you could add Assert(!has_parallel_hazard(...)) here?

I guess this means that apply_projection_to_path would need to clear
parallel_safe if rel->consider_parallel isn't true. This would correspond
to situations where we are taking a parallel-safe path for a lower-level
relation that has consider_parallel true, and repurposing it for a new
upperrel that has consider_parallel false. Maybe it'd be better to
not do that but just force use of a separate ProjectionPath if the
parallel_safe flag needs to change.

(I think 8b9d323cb may have made this a little less messy than it
was when you did your draft patch, btw.)

regards, tom lane