From: Tom Lane <tgl@sss.pgh.pa.us>
To: David Rowley <dgrowleyml@gmail.com>
cc: Richard Guo <guofenglinux@gmail.com>,
        Alexander Lakhin <exclusion@gmail.com>,
        PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Properly pathify the union planner
In-reply-to: 
 <CAApHDvqO-YREBHju31bF-_b77Zg4zwNe44UhzRGn1c+GVTTibQ@mail.gmail.com>
References: 
 <CAApHDvpb_63XQodmxKUF8vb9M7CxyUyT4sWvEgqeQU-GB7QFoQ@mail.gmail.com>
 <CAApHDvoV4QgNsXsegaCJQFHK4v8-gE7_DzXmSMgm_Hg+C+4oYg@mail.gmail.com>
 <CAMbWs49J2jv61DsQ_D=JgnuPMjbd1W3WoMw=-DB=zRpGYRh+yA@mail.gmail.com>
 <CAApHDvq05PiyYzqeOwH36_PqDEkz21d6zqPSTdzgx+HGzv5pkA@mail.gmail.com>
 <CAApHDvovdy=tqHD+13CXVNkXjbB0+9F=GcKi9ndkgUhu9cUkSw@mail.gmail.com>
 <CAMbWs48R7Lp0CwcioeyenXHkrWdMfpOkaZ2OStjr7YeftRHe0Q@mail.gmail.com>
 <CAApHDvrMc2tFQFx86T-mrz6g--WNSmhgSDJWgA1xJe+-KnDDcQ@mail.gmail.com>
 <CAMbWs49H5oKvsUM_SrPbrAPkNqabuxHacjWC+Qd77dmVyMyXEQ@mail.gmail.com>
 <CAApHDvqo1rV8O4pMU2-22iTASBXgnm4kbHF6A8_VMqiDR3hG8A@mail.gmail.com>
 <CAApHDvqaJJ0Q-yqTybpqdyMLeGviA3zF2Rghs-zG=dcQh3yP6g@mail.gmail.com>
 <242fc7c6-a8aa-2daf-ac4c-0a231e2619c1@gmail.com>
 <CAApHDvqYa_1HcUM4TjzKqXxuZCgFuwMXqQWwXvwR7ZoDiAknJA@mail.gmail.com>
 <CAMbWs49gWbhSVdX0kT2x5XGKmP5OZerp6VhEA-hM4BvLHxKaqQ@mail.gmail.com>
 <CAApHDvq1mJWSwQwDTcpRpqipsv4grSR+NQ5FWgB6hfK_ZQOJAg@mail.gmail.com>
 <CAMbWs4_efcU0OxWtmJm8uECLoC=cr!
 T+Dwj98FiSXEw8tK10E4Q@mail.gmail.com> <3485967.1711594595@sss.pgh.pa.us>
 <CAApHDvqO-YREBHju31bF-_b77Zg4zwNe44UhzRGn1c+GVTTibQ@mail.gmail.com>
Comments: In-reply-to David Rowley <dgrowleyml@gmail.com>
	message dated "Thu, 28 Mar 2024 16:06:46 +1300"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3703022.1711654574.1@sss.pgh.pa.us>
Date: Thu, 28 Mar 2024 15:36:14 -0400
Message-ID: <3703023.1711654574@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3703023.1711654574%40sss.pgh.pa.us>
Precedence: bulk

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