From b0129b7e1c24d67b0609139f5416c31e03545b25 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Thu, 12 Oct 2017 14:56:21 +0300
Subject: [PATCH 1/1] Fix subquery pullup into a query containing GROUPING SETS
 and HAVING.

---
 src/backend/optimizer/prep/prepjointree.c | 44 +++++++++++++++++++++++++------
 src/test/regress/expected/subselect.out   | 14 ++++++++++
 src/test/regress/sql/subselect.sql        | 10 +++++++
 3 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index f3bb73a664..7fa65696c3 100644
--- 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. Set up required context data for
+	 * pullup_replace_vars.
 	 */
 	rvcontext.root = root;
 	rvcontext.targetlist = subquery->targetList;
@@ -1019,14 +1016,45 @@ 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.
+	 */
+	if (containing_appendrel != NULL)
+	{
+		rvcontext.need_phvs = true;
+		rvcontext.wrap_non_vars = true;
+	}
+
+	/*
+	 * If the parent query uses GROUPING SETS and HAVING, we need
+	 * PlaceHolderVars for anything that's not a simple Var. Because
+	 * for the "rolled up" columns, the HAVING should consider them as
+	 * NULL.  XXX: We would really only need to use PHVs in the havingQual
+	 * itself, but grouping sets planning gets confused if there are
+	 * references to both the underlying expression, and placeholders.
+	 */
+	if (parse->groupingSets && parse->havingQual)
+	{
+		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
 	 * replace any of the jointree structure. (This'd be a lot cleaner if we
diff --git a/src/test/regress/expected/subselect.out b/src/test/regress/expected/subselect.out
index f3ebad5857..c8be7854be 100644
--- a/src/test/regress/expected/subselect.out
+++ b/src/test/regress/expected/subselect.out
@@ -1093,3 +1093,17 @@ select * from (select pk,c2 from sq_limit order by c1,pk) as x limit 3;
 
 drop function explain_sq_limit();
 drop table sq_limit;
+--
+-- Check that when a subquery is pulled up into a query with grouping sets,
+-- HAVING refers to the after-grouping, possibly NULLed value. I.e. the
+-- below HAVING x = 'foo' condition is not always true, even though x is
+-- a constant 'foo' in the subquery.
+--
+select four, x
+  from (select four, ten, 'foo' as x from tenk1 ) as t
+  group by grouping sets(four, x) having x = 'foo' order by four;
+ four |  x  
+------+-----
+      | foo
+(1 row)
+
diff --git a/src/test/regress/sql/subselect.sql b/src/test/regress/sql/subselect.sql
index 5ac8badabe..aac3286ea1 100644
--- a/src/test/regress/sql/subselect.sql
+++ b/src/test/regress/sql/subselect.sql
@@ -581,3 +581,13 @@ select * from (select pk,c2 from sq_limit order by c1,pk) as x limit 3;
 drop function explain_sq_limit();
 
 drop table sq_limit;
+
+--
+-- Check that when a subquery is pulled up into a query with grouping sets,
+-- HAVING refers to the after-grouping, possibly NULLed value. I.e. the
+-- below HAVING x = 'foo' condition is not always true, even though x is
+-- a constant 'foo' in the subquery.
+--
+select four, x
+  from (select four, ten, 'foo' as x from tenk1 ) as t
+  group by grouping sets(four, x) having x = 'foo' order by four;
-- 
2.11.0

