Re: [BUGS] Improper const-evaluation of HAVING with grouping sets and subquery pullup

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk>
Cc: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Pg Bugs <pgsql-bugs(at)postgresql(dot)org>
Subject: Re: [BUGS] Improper const-evaluation of HAVING with grouping sets and subquery pullup
Date: 2018-01-11 22:46:54
Message-ID: 22850.1515710814@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-bugs

I wrote:
> FWIW, I'm not really comfortable that the proposed patch is correct
> or complete. It may just need more study to get there.

I've done another round of study on this patch. The attached updated
version is the same code as Heikki proposed (minus the incorrect
restriction to queries with HAVING quals), but I reworked the comments
and expanded the regression test cases.

One thing that wasn't clear to me before was whether we need wrap_non_vars
for this case or not; we don't for outer joins, so I was unconvinced about
it here. It turns out we do: the point of the wrapper is to prevent
constant folding or other expression preprocessing from merging a
pulled-up expression with the surrounding expression, resulting in
something that won't match the grouping set expression when it comes time
to do that matching. For instance if we have a boolean subquery output
expression, say "x = y as cond", and that gets hoisted into an upper
expression "not cond", then without the PHV wrapper we will happily
simplify that to "x <> y" which will not match the grouping set
expression. There's a regression test below that misbehaves if you take
out the "wrap_non_vars = true" line.

I spent some time thinking about Andrew's observation that we don't really
need the wrappers everyplace. It's true, but pullup_replace_vars is far
from being able to do the right thing there, and I'm not sure that trying
to teach it to do so is reasonable. (I'm inclined to think that the idea
I threw out upthread about converting grouping expressions into Vars
belonging to a new RTE kind might be the way to go.) In any case I don't
think we'd possibly come out with a patch simple enough to back-patch.
So let's leave that optimization for future work.

I think the attached is probably ready to go, though I've not checked yet
whether it will work pre-9.6. Anyone want to do more review?

regards, tom lane

Attachment Content-Type Size
fix-grouping-sets-pullup-v2.patch text/x-diff 6.3 KB

In response to

Responses

Browse pgsql-bugs by date

  From Date Subject
Next Message Thomas Munro 2018-01-11 23:01:55 Re: [HACKERS] [BUGS] BUG #14825: enum type: unsafe use?
Previous Message Bruce Momjian 2018-01-11 21:59:54 Re: [BUGS] BUG #14875: pg_createcluster fails to load --createclusterconf