Re: [COMMITTERS] pgsql: Try again to fix the way the scanjoin_target is used with partia

On Tue, Jun 21, 2016 at 1:28 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
>> If we keep it like this, we definitely ought to refactor things so that
>> fewer places are aware of the possibility of the Result getting omitted.
>> Maybe push that logic into create_projection_path? If we did, we might
>> not need a separate apply_projection_to_path function at all? Too tired
>> to decide about it right now.
>
> After some contemplation, it seems to me that the right thing is to make
> ProjectionPath explicitly aware of the possibility that it's just a
> placeholder used for the purpose of not modifying the input Path node,
> and have create_projection_path calculate the correct costing either way.
> In this design, the principal difference between create_projection_path
> and apply_projection_to_path is that the latter assumes it can scribble
> directly on the given Path while the former doesn't modify that Path.

+1.

> There is one other difference: I did not do anything about making
> create_projection_path push the target below a Gather. In principle,
> it could do that by cloning the given GatherPath, but right now I think
> that'd be unreachable code so I did not bother.

Works for me.

> I had hoped that this would result in simplifying create_projection_plan
> so that it just makes a Result or not according to what
> create_projection_path decided, but there's one regression test case that
> fails (in the sense of showing a Result in the plan that isn't really
> needed). That happens because create_merge_append_plan adds sort columns
> to the tlist and so a tlist match is possible after that happens when it
> didn't match before. For the moment I kluged create_projection_plan so
> that that keeps working, but I wonder if it'd be better to just accept an
> extra Result in that case.

Not sure if it's better to accept an extra Result in that case, but I
agree that's ugly.

> Also, I got rid of the target_parallel argument to
> apply_projection_to_path, as I thought that that was just way too much
> interconnection between apply_projection_to_path and its callers than
> is justified for what it saves (namely one call of has_parallel_hazard
> when planning a Gather). In general, having that argument could *add*
> extra calls of has_parallel_hazard, since callers might have to do
> that computation whether or not a Gather is present.

I had a feeling you weren't going to like that, but it also didn't
seem great to redo that computation for every path. Right now, we
only need it for Gather paths, but if we start marking subquery RTEs
as parallel-safe and fix upper rels to correctly set
consider_parallel, I have a feeling this is going to be needed more.
But feel free to ignore that for now since I don't have a completely
well-thought-out theory on this.

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