Re: Pushdown target list below gather node (WAS Re: WIP: Upper planner pathification)

On Wed, Mar 16, 2016 at 12:36 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > That doesn't update the cost of the subpath, which it probably needs to
> > do. I wonder if this shouldn't be implemented by recursing.
>
> > if (IsA(path, GatherPath) && !has_parallel_hazard((Node *) target->exprs,
> > false))
> > apply_projection_to_path(root, something, ((GatherPath *)
> > path)->subpath, target);
>
> > Tom, any comments? I think it would be smart to push this into 9.6.
>
> I agree with the idea that this problem should be solved, but there's
> nothing much to like about the particular details here. If we're going
> to push down stuff into the workers, I think adding a Result if needed
> to do that is something we'd definitely want it to do. So more
> along the lines of
>
> if (IsA(path, GatherPath) &&
> !has_parallel_hazard((Node *) target->exprs, false))
> {
> GatherPath *gpath = (GatherPath *) path;
>
> gpath->subpath =
> apply_projection_to_path(root,
> gpath->subpath->parent,
> gpath->subpath,
> target);
> }

I agree. That's a cleaned-up version of the formula I posted.

> However, I continue to doubt that apply_projection_to_path really ought to
> know about parallel safety.
>
> And there is a larger problem with this: I'm not sure that it's
> appropriate for apply_projection_to_path to assume that the subpath is not
> shared with any other purposes. If it is shared, and we update the
> subpath's target in-place, we just broke the other path chains.

That's true. I don't see an obvious hazard here, because the Gather's
child came from the rel's partial_pathlist, and the only way it gets
used from there is to stick the Gather on top of it. So it really
can't show up anywhere else. I think. But I agree it's a little
scary. (To some lesser extent, apply_projection_to_path is always
scary like that.)

One idea is to skip generate_gather_paths() for the final rel and then
make up the difference later: apply the final rel's target list once
we've computed it, and then generate a gather path based on that. But
I don't see an obvious way of doing that.

> Now the existing uses of apply_projection_to_path don't have to worry
> about that because they're always messing with paths whose parent rel
> isn't going to be used in any other way. Maybe this one doesn't either
> because the subpath would always be a partial path that won't have any
> other potential application besides being a child of *this specific*
> Gather path. But I'd like to see that addressed in a comment, if so.
>
> If it's not so, maybe the answer is to always interpose a Result node,
> in which case just use create_projection_path() in the above fragment.

Mmmph. That seems like a 2-bit solution, but I guess it would work.
What if we taught create_projection_plan() to elide the Result node in
that case? That is, instead of this:

if (tlist_same_exprs(tlist, subplan->targetlist))

We could do this:

if (is_projection_capable_path(subplan) || tlist_same_exprs(tlist,
subplan->targetlist))

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