From 8925d4eda2a86b730faa7e1abca125ac52d566fe Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <hlinnakangas@pivotal.io>
Date: Thu, 2 Feb 2017 08:04:52 +0200
Subject: [PATCH] Fix SubPlan handling in adjust_appendrel_attrs().

The old coding failed to apply the mutator to the children of a SubPlan.

While at it, try to explain in the comment *why* SubPlans need special
treatment in GPDB, while they don't in PostgreSQL. I'm not 100% I got the
reasoning correct, since I'm basically just reverse-engineering the
original thinking here. But seems plausible..

Fixes github issue #368.
---
 src/backend/optimizer/prep/prepunion.c    | 38 ++++++++++++++---------
 src/test/regress/expected/bfv_planner.out | 30 ++++++++++++++++++
 src/test/regress/sql/bfv_planner.sql      | 17 ++++++++++
 3 files changed, 71 insertions(+), 14 deletions(-)

diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c
index b75dcb019d..bb23e745a3 100644
--- a/src/backend/optimizer/prep/prepunion.c
+++ b/src/backend/optimizer/prep/prepunion.c
@@ -1319,13 +1319,27 @@ adjust_appendrel_attrs_mutator(Node *node, AppendRelInfoContext *ctx)
 		return (Node *) newinfo;
 	}
 
-	/**
-	 * We may have to create a copy of the subplan
+	/*
+	 * NOTE: we do not need to recurse into sublinks, because they should
+	 * already have been converted to subplans before we see them.
+	 */
+	Assert(!IsA(node, SubLink));
+	Assert(!IsA(node, Query));
+
+	node = expression_tree_mutator(node, adjust_appendrel_attrs_mutator,
+								   (void *) ctx);
+
+	/*
+	 * In GPDB, if you have two SubPlans referring to the same initplan, we
+	 * require two separate copies of the subplan, one for each SubPlan
+	 * reference. That's because even if a plan is otherwise the same, we
+	 * may want to later apply different flow to different SubPlans
+	 * referring it.
 	 */
 	if (IsA(node, SubPlan))
 	{
 		SubPlan *sp = (SubPlan *) node;
-		SubPlan *newsp = copyObject(sp);
+
 		if (!sp->is_initplan)
 		{
 			PlannerInfo *root = (PlannerInfo *) ctx->base.node;
@@ -1337,20 +1351,16 @@ adjust_appendrel_attrs_mutator(Node *node, AppendRelInfoContext *ctx)
 			 */
 			root->glob->subplans = lappend(root->glob->subplans, newsubplan);
 			root->glob->subrtables = lappend(root->glob->subrtables, newrtable);
-			newsp->plan_id = list_length(root->glob->subplans);
+
+			/*
+			 * expression_tree_mutator made a copy of the SubPlan already, so
+			 * we can modify it directly.
+			 */
+			sp->plan_id = list_length(root->glob->subplans);
 		}
-		return (Node *) newsp;
 	}
 
-	/*
-	 * NOTE: we do not need to recurse into sublinks, because they should
-	 * already have been converted to subplans before we see them.
-	 */
-	Assert(!IsA(node, SubLink));
-	Assert(!IsA(node, Query));
-
-	return expression_tree_mutator(node, adjust_appendrel_attrs_mutator,
-								   (void *) ctx);
+	return node;
 }
 
 /*
diff --git a/src/test/regress/expected/bfv_planner.out b/src/test/regress/expected/bfv_planner.out
index fd1f9ef52a..461c7d49f9 100644
--- a/src/test/regress/expected/bfv_planner.out
+++ b/src/test/regress/expected/bfv_planner.out
@@ -192,6 +192,36 @@ order by b,c;
      25 |     1
 (1 row)
 
+-- Test inheritance planning, when a SubPlan is duplicated for different
+-- child tables.
+create table r (a int) distributed by (a);
+create table p (a int, b int) distributed by (a);
+create table p2 (a int, b int) inherits (p) distributed by (b);
+NOTICE:  merging column "a" with inherited definition
+NOTICE:  merging column "b" with inherited definition
+insert into r values (3);
+insert into p select a, b from generate_series(1,3) a, generate_series(1,3) b;
+delete from p where b = 1 or (b=2 and a in (select r.a from r));
+select * from p;
+ a | b 
+---+---
+ 3 | 3
+ 1 | 2
+ 1 | 3
+ 2 | 2
+ 2 | 3
+(5 rows)
+
+delete from p where b = 1 or (b=2 and a in (select b from r));
+select * from p;
+ a | b 
+---+---
+ 1 | 2
+ 1 | 3
+ 2 | 3
+ 3 | 3
+(4 rows)
+
 -- start_ignore
 drop table if exists bfv_planner_x;
 drop table if exists testbadsql;
diff --git a/src/test/regress/sql/bfv_planner.sql b/src/test/regress/sql/bfv_planner.sql
index 64d78d6a3c..549edfa05b 100644
--- a/src/test/regress/sql/bfv_planner.sql
+++ b/src/test/regress/sql/bfv_planner.sql
@@ -131,6 +131,23 @@ where c ='25'
 group by b, c, value2
 order by b,c;
 
+
+-- Test inheritance planning, when a SubPlan is duplicated for different
+-- child tables.
+
+create table r (a int) distributed by (a);
+create table p (a int, b int) distributed by (a);
+create table p2 (a int, b int) inherits (p) distributed by (b);
+
+insert into r values (3);
+insert into p select a, b from generate_series(1,3) a, generate_series(1,3) b;
+
+delete from p where b = 1 or (b=2 and a in (select r.a from r));
+select * from p;
+
+delete from p where b = 1 or (b=2 and a in (select b from r));
+select * from p;
+
 -- start_ignore
 drop table if exists bfv_planner_x;
 drop table if exists testbadsql;
-- 
GitLab