Re: EquivalenceClasses and subqueries and PlaceHolderVars, oh my

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

In response to

Responses

Browse pgsql-hackers by date

  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