Re: Properly pathify the union planner - Mailing list pgsql-hackers

From Tom Lane
Subject Re: Properly pathify the union planner
Date
Msg-id 3703023.1711654574@sss.pgh.pa.us
Whole thread Raw
In response to Re: Properly pathify the union planner  (David Rowley <dgrowleyml@gmail.com>)
Responses Re: Properly pathify the union planner
List pgsql-hackers
David Rowley <dgrowleyml@gmail.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



pgsql-hackers by date:

Previous
From: Bruce Momjian
Date:
Subject: Re: Possibility to disable `ALTER SYSTEM`
Next
From: Melih Mutlu
Date:
Subject: Re: Flushing large data immediately in pqcomm