Re: [BUGS] Improper const-evaluation of HAVING with grouping sets and subquery pullup
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andrew Gierth <andrew@tao11.riddles.org.uk>
Cc: Michael Paquier <michael.paquier@gmail.com>,
Heikki Linnakangas <hlinnaka@iki.fi>,
Pg Bugs <pgsql-bugs@postgresql.org>
Date: 2018-01-11T22:46:54Z
Lists: pgsql-bugs
Attachments
- fix-grouping-sets-pullup-v2.patch (text/x-diff) patch v2
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
Commits
-
Fix incorrect handling of subquery pullup in the presence of grouping sets.
- ff99d7761aa4 9.5.11 landed
- d3ca1a6c3721 10.2 landed
- 90947674fc98 11.0 landed
- 6520d4a96928 9.6.7 landed
-
Make setrefs.c match by ressortgroupref even for plain Vars.
- 6a81ba1d4d26 10.1 landed
- 37b4e0fe9964 9.6.6 landed
- 08f1e1f0a47b 11.0 landed
-
Make the upper part of the planner work by generating and comparing Paths.
- 3fc6e2d7f5b6 9.6.0 cited