Re: Properly pathify the union planner

David Rowley <dgrowleyml(at)gmail(dot)com> writes:
> The problem is informing the UNION child query about what it is. I
> thought I could do root->parent_root->parse->setOperations for a UNION
> child to know what it is, but that breaks for a query such as:

Yeah, having grouping_planner poke into the parent level
doesn't seem like a great idea here. I continue to not like
the name "PlannerContext" but I agree passing down the setop
explicitly is the way to go.

>> Perhaps "SubqueryContext" or the like would be better? It
>> still has the conflict with memory contexts though.

> Maybe something with "Parameters" in the name?

SubqueryParameters might be OK. Or SubqueryPlannerExtra?
Since this is a bespoke struct that will probably only ever
be used with subquery_planner, naming it after that function
seems like a good idea. (And, given that fact and the fact
that it's not a Node, I'm not sure it belongs in pathnodes.h.
We could just declare it in planner.h.)

Some minor comments now that I've looked at 66c0185a3 a little:

* Near the head of grouping_planner is this bit:

if (parse->setOperations)
{
/*
* If there's a top-level ORDER BY, assume we have to fetch all the
* tuples. This might be too simplistic given all the hackery below
* to possibly avoid the sort; but the odds of accurate estimates here
* are pretty low anyway. XXX try to get rid of this in favor of
* letting plan_set_operations generate both fast-start and
* cheapest-total paths.
*/
if (parse->sortClause)
root->tuple_fraction = 0.0;

I'm pretty sure this comment is mine, but it's old enough that I don't
recall exactly what I had in mind. Still, it seems like your patch
has addressed precisely the issue of generating fast-start plans for
setops. Should we now remove this reset of tuple_fraction?

* generate_setop_child_grouplist does this:

/* assign a tleSortGroupRef, or reuse the existing one */
sgc->tleSortGroupRef = assignSortGroupRef(tle, targetlist);
tle->ressortgroupref = sgc->tleSortGroupRef;

That last line is redundant and confusing. It is not this code's
charter to change ressortgroupref.

regards, tom lane