Re: Pushdown target list below gather node (WAS Re: WIP: Upper planner pathification) - Mailing list pgsql-hackers

On Wed, Mar 16, 2016 at 12:36 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.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