Fix incorrect handling of subquery pullup in the presence of grouping sets.

If we flatten a subquery whose target list contains constants or
expressions, when those output columns are used in GROUPING SET columns,
the planner was capable of doing the wrong thing by merging a pulled-up
expression into the surrounding expression during const-simplification.
Then the late processing that attempts to match subexpressions to grouping
sets would fail to match those subexpressions to grouping sets, with the
effect that they'd not go to null when expected.

To fix, wrap such subquery outputs in PlaceHolderVars, ensuring that
they preserve their separate identity throughout the planner's expression
processing.  This is a bit of a band-aid, because the wrapper defeats
const-simplification even in places where it would be safe to allow.
But a nicer fix would likely be too invasive to back-patch, and the
consequences of the missed optimizations probably aren't large in most
cases.

Back-patch to 9.5 where grouping sets were introduced.

Heikki Linnakangas, with small mods and better test cases by me;
additional review by Andrew Gierth

Discussion: https://postgr.es/m/7dbdcf5c-b5a6-ef89-4958-da212fe10176@iki.fi

diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index 0e2a220ad083d9921e066367398189351a142f27..45d82da4591a2132f55dcb428613d9d43fa598c2 100644 (file)
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -1003,11 +1003,8 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
 
        /*
         * The subquery's targetlist items are now in the appropriate form to
-        * insert into the top query, but if we are under an outer join then
-        * non-nullable items and lateral references may have to be turned into
-        * PlaceHolderVars.  If we are dealing with an appendrel member then
-        * anything that's not a simple Var has to be turned into a
-        * PlaceHolderVar.  Set up required context data for pullup_replace_vars.
+        * insert into the top query, except that we may need to wrap them in
+        * PlaceHolderVars.  Set up required context data for pullup_replace_vars.
         */
        rvcontext.root = root;
        rvcontext.targetlist = subquery->targetList;
@@ -1019,13 +1016,48 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
                rvcontext.relids = NULL;
        rvcontext.outer_hasSubLinks = &parse->hasSubLinks;
        rvcontext.varno = varno;
-       rvcontext.need_phvs = (lowest_nulling_outer_join != NULL ||
-                                                  containing_appendrel != NULL);
-       rvcontext.wrap_non_vars = (containing_appendrel != NULL);
+       /* these flags will be set below, if needed */
+       rvcontext.need_phvs = false;
+       rvcontext.wrap_non_vars = false;
        /* initialize cache array with indexes 0 .. length(tlist) */
        rvcontext.rv_cache = palloc0((list_length(subquery->targetList) + 1) *
                                                                 sizeof(Node *));
 
+       /*
+        * If we are under an outer join then non-nullable items and lateral
+        * references may have to be turned into PlaceHolderVars.
+        */
+       if (lowest_nulling_outer_join != NULL)
+               rvcontext.need_phvs = true;
+
+       /*
+        * If we are dealing with an appendrel member then anything that's not a
+        * simple Var has to be turned into a PlaceHolderVar.  We force this to
+        * ensure that what we pull up doesn't get merged into a surrounding
+        * expression during later processing and then fail to match the
+        * expression actually available from the appendrel.
+        */
+       if (containing_appendrel != NULL)
+       {
+               rvcontext.need_phvs = true;
+               rvcontext.wrap_non_vars = true;
+       }
+
+       /*
+        * If the parent query uses grouping sets, we need a PlaceHolderVar for
+        * anything that's not a simple Var.  Again, this ensures that expressions
+        * retain their separate identity so that they will match grouping set
+        * columns when appropriate.  (It'd be sufficient to wrap values used in
+        * grouping set columns, and do so only in non-aggregated portions of the
+        * tlist and havingQual, but that would require a lot of infrastructure
+        * that pullup_replace_vars hasn't currently got.)
+        */
+       if (parse->groupingSets)
+       {
+               rvcontext.need_phvs = true;
+               rvcontext.wrap_non_vars = true;
+       }
+
        /*
         * Replace all of the top query's references to the subquery's outputs
         * with copies of the adjusted subtlist items, being careful not to

diff --git a/src/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out
index 833d5151743b17409bcda853547dbfce9e6a559f..cbfdbfd8563f8704acf96755c0755446509bbfa3 100644 (file)
--- a/src/test/regress/expected/groupingsets.out
+++ b/src/test/regress/expected/groupingsets.out
@@ -389,6 +389,51 @@ select g as alias1, g as alias2
       3 |       
 (6 rows)
 
+-- check that pulled-up subquery outputs still go to null when appropriate
+select four, x
+  from (select four, ten, 'foo'::text as x from tenk1) as t
+  group by grouping sets (four, x)
+  having x = 'foo';
+ four |  x  
+------+-----
+      | foo
+(1 row)
+
+select four, x || 'x'
+  from (select four, ten, 'foo'::text as x from tenk1) as t
+  group by grouping sets (four, x)
+  order by four;
+ four | ?column? 
+------+----------
+    0 | 
+    1 | 
+    2 | 
+    3 | 
+      | foox
+(5 rows)
+
+select (x+y)*1, sum(z)
+ from (select 1 as x, 2 as y, 3 as z) s
+ group by grouping sets (x+y, x);
+ ?column? | sum 
+----------+-----
+        3 |   3
+          |   3
+(2 rows)
+
+select x, not x as not_x, q2 from
+  (select *, q1 = 1 as x from int8_tbl i1) as t
+  group by grouping sets(x, q2)
+  order by x, q2;
+ x | not_x |        q2         
+---+-------+-------------------
+ f | t     |                  
+   |       | -4567890123456789
+   |       |               123
+   |       |               456
+   |       |  4567890123456789
+(5 rows)
+
 -- simple rescan tests
 select a, b, sum(v.x)
   from (values (1),(2)) v(x), gstest_data(v.x)

diff --git a/src/test/regress/sql/groupingsets.sql b/src/test/regress/sql/groupingsets.sql
index 2b4ab692c4f976404a8bd1ed4098852f1caf50a8..b28d8217c127a20c9574ee93171d228254a07277 100644 (file)
--- a/src/test/regress/sql/groupingsets.sql
+++ b/src/test/regress/sql/groupingsets.sql
@@ -152,6 +152,26 @@ select g as alias1, g as alias2
   from generate_series(1,3) g
  group by alias1, rollup(alias2);
 
+-- check that pulled-up subquery outputs still go to null when appropriate
+select four, x
+  from (select four, ten, 'foo'::text as x from tenk1) as t
+  group by grouping sets (four, x)
+  having x = 'foo';
+
+select four, x || 'x'
+  from (select four, ten, 'foo'::text as x from tenk1) as t
+  group by grouping sets (four, x)
+  order by four;
+
+select (x+y)*1, sum(z)
+ from (select 1 as x, 2 as y, 3 as z) s
+ group by grouping sets (x+y, x);
+
+select x, not x as not_x, q2 from
+  (select *, q1 = 1 as x from int8_tbl i1) as t
+  group by grouping sets(x, q2)
+  order by x, q2;
+
 -- simple rescan tests
 
 select a, b, sum(v.x)