From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Yeb Havinga <yebhavinga(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: EquivalenceClasses and subqueries and PlaceHolderVars, oh my |
Date: | 2012-03-16 15:16:14 |
Message-ID: | 11635.1331910974@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
I wrote:
> So I now propose reverting the earlier two patches (but not their
> regression test cases of course) and instead hacking MergeAppend plan
> building as per (2).
Attached is a draft patch for that. There are several things going on
here:
* Revert the preceding two patches (except for their regression test
cases).
* Install defenses to ensure that child EC members are ignored except in
very specific contexts, and improve documentation around that. The most
significant change is that get_eclass_for_sort_expr now takes a Relids
argument, and won't consider child members unless they match the Relids.
It turns out that the places that were trying to match indexes to ECs
already acted that way; which I think I had done just as a speed
optimization, but it turns out to be important for correctness too.
* Rewrite prepare_sort_from_pathkeys() to allow required sort column
numbers to be passed in, thus fixing Teodor's original problem more
directly. As part of that, I removed createplan.c's add_sort_column()
code, which was meant as a last-ditch check that we don't build sorts
with useless duplicate sort columns. As far as I can tell, that's dead
code and has been for a long time, because we already eliminate
duplicate pathkeys or SortGroupClause list entries far upstream of this.
Even if there are some corner cases where it does something useful,
that's rare enough to not really justify expending the cycles. I had to
remove it because if it did fire and remove a sort column, the outer
loop in prepare_sort_from_pathkeys() wouldn't be in sync with the input
column-number array.
* While testing this I noticed that planagg.c failed to optimize min/max
aggregates on appendrels that are made from UNION ALL subqueries rather
than inherited relations. So this patch also tweaks planagg.c to handle
such cases.
Viewing the problem this way, the only known symptom is the originally
reported "MergeAppend child's targetlist doesn't match MergeAppend",
which of course is not an issue before 9.1, so I'm not going to try
to back-patch further than 9.1. It is possible that we need some of the
child EC member defenses further back, but I'll await some evidence of
user-visible bugs first.
regards, tom lane
Attachment | Content-Type | Size |
---|---|---|
placeholdervar-fix-this-time-for-sure.patch | text/x-patch | 52.2 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Haas | 2012-03-16 15:27:57 | Re: Command Triggers, v16 |
Previous Message | Dimitri Fontaine | 2012-03-16 15:06:35 | Re: Command Triggers, v16 |