Fix nested PlaceHolderVar expressions that appear only in targetlists.

tglsfdc · tglsfdc · commit 77ba23256404 · 2011-08-09T00:50:07.000-04:00
A PlaceHolderVar's expression might contain another, lower-level
PlaceHolderVar.  If the outer PlaceHolderVar is used, the inner one
certainly will be also, and so we have to make sure that both of them get
into the placeholder_list with correct ph_may_need values during the
initial pre-scan of the query (before deconstruct_jointree starts).
We did this correctly for PlaceHolderVars appearing in the query quals,
but overlooked the issue for those appearing in the top-level targetlist;
with the result that nested placeholders referenced only in the targetlist
did not work correctly, as illustrated in bug #6154.

While at it, add some error checking to find_placeholder_info to ensure
that we don't try to create new placeholders after it's too late to do so;
they have to all be created before deconstruct_jointree starts.

Back-patch to 8.4 where the PlaceHolderVar mechanism was introduced.
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
@@ -3491,7 +3491,7 @@ set_rel_width(PlannerInfo *root, RelOptInfo *rel)
 		else if (IsA(node, PlaceHolderVar))
 		{
 			PlaceHolderVar *phv = (PlaceHolderVar *) node;
-			PlaceHolderInfo *phinfo = find_placeholder_info(root, phv);
+			PlaceHolderInfo *phinfo = find_placeholder_info(root, phv, false);
 
 			tuple_width += phinfo->ph_width;
 		}
diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
@@ -850,7 +850,7 @@ generate_base_implied_equalities_no_const(PlannerInfo *root,
 										   PVC_RECURSE_AGGREGATES,
 										   PVC_INCLUDE_PLACEHOLDERS);
 
-		add_vars_to_targetlist(root, vars, ec->ec_relids);
+		add_vars_to_targetlist(root, vars, ec->ec_relids, false);
 		list_free(vars);
 	}
 }
diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c
@@ -137,7 +137,7 @@ build_base_rel_tlists(PlannerInfo *root, List *final_tlist)
 
 	if (tlist_vars != NIL)
 	{
-		add_vars_to_targetlist(root, tlist_vars, bms_make_singleton(0));
+		add_vars_to_targetlist(root, tlist_vars, bms_make_singleton(0), true);
 		list_free(tlist_vars);
 	}
 }
@@ -151,10 +151,15 @@ build_base_rel_tlists(PlannerInfo *root, List *final_tlist)
  *
  *	  The list may also contain PlaceHolderVars.  These don't necessarily
  *	  have a single owning relation; we keep their attr_needed info in
- *	  root->placeholder_list instead.
+ *	  root->placeholder_list instead.  If create_new_ph is true, it's OK
+ *	  to create new PlaceHolderInfos, and we also have to update ph_may_need;
+ *	  otherwise, the PlaceHolderInfos must already exist, and we should only
+ *	  update their ph_needed.  (It should be true before deconstruct_jointree
+ *	  begins, and false after that.)
  */
 void
-add_vars_to_targetlist(PlannerInfo *root, List *vars, Relids where_needed)
+add_vars_to_targetlist(PlannerInfo *root, List *vars,
+					   Relids where_needed, bool create_new_ph)
 {
 	ListCell   *temp;
 
@@ -185,18 +190,20 @@ add_vars_to_targetlist(PlannerInfo *root, List *vars, Relids where_needed)
 		else if (IsA(node, PlaceHolderVar))
 		{
 			PlaceHolderVar *phv = (PlaceHolderVar *) node;
-			PlaceHolderInfo *phinfo = find_placeholder_info(root, phv);
+			PlaceHolderInfo *phinfo = find_placeholder_info(root, phv,
+															create_new_ph);
 
+			/* Always adjust ph_needed */
 			phinfo->ph_needed = bms_add_members(phinfo->ph_needed,
 												where_needed);
 
 			/*
-			 * Update ph_may_need too.	This is currently only necessary when
-			 * being called from build_base_rel_tlists, but we may as well do
-			 * it always.
+			 * If we are creating PlaceHolderInfos, mark them with the
+			 * correct maybe-needed locations.  Otherwise, it's too late to
+			 * change that.
 			 */
-			phinfo->ph_may_need = bms_add_members(phinfo->ph_may_need,
-												  where_needed);
+			if (create_new_ph)
+				mark_placeholder_maybe_needed(root, phinfo, where_needed);
 		}
 		else
 			elog(ERROR, "unrecognized node type: %d", (int) nodeTag(node));
@@ -1035,7 +1042,7 @@ distribute_qual_to_rels(PlannerInfo *root, Node *clause,
 										   PVC_RECURSE_AGGREGATES,
 										   PVC_INCLUDE_PLACEHOLDERS);
 
-		add_vars_to_targetlist(root, vars, relids);
+		add_vars_to_targetlist(root, vars, relids, false);
 		list_free(vars);
 	}
 
diff --git a/src/backend/optimizer/util/placeholder.c b/src/backend/optimizer/util/placeholder.c
@@ -24,7 +24,7 @@
 
 /* Local functions */
 static Relids find_placeholders_recurse(PlannerInfo *root, Node *jtnode);
-static void find_placeholders_in_qual(PlannerInfo *root, Node *qual,
+static void mark_placeholders_in_expr(PlannerInfo *root, Node *expr,
 						  Relids relids);
 
 
@@ -50,18 +50,24 @@ make_placeholder_expr(PlannerInfo *root, Expr *expr, Relids phrels)
 
 /*
  * find_placeholder_info
- *		Fetch the PlaceHolderInfo for the given PHV; create it if not found
+ *		Fetch the PlaceHolderInfo for the given PHV
+ *
+ * If the PlaceHolderInfo doesn't exist yet, create it if create_new_ph is
+ * true, else throw an error.
  *
  * This is separate from make_placeholder_expr because subquery pullup has
  * to make PlaceHolderVars for expressions that might not be used at all in
  * the upper query, or might not remain after const-expression simplification.
  * We build PlaceHolderInfos only for PHVs that are still present in the
  * simplified query passed to query_planner().
  *
- * Note: this should only be called after query_planner() has started.
+ * Note: this should only be called after query_planner() has started.  Also,
+ * create_new_ph must not be TRUE after deconstruct_jointree begins, because
+ * make_outerjoininfo assumes that we already know about all placeholders.
  */
 PlaceHolderInfo *
-find_placeholder_info(PlannerInfo *root, PlaceHolderVar *phv)
+find_placeholder_info(PlannerInfo *root, PlaceHolderVar *phv,
+					  bool create_new_ph)
 {
 	PlaceHolderInfo *phinfo;
 	ListCell   *lc;
@@ -77,6 +83,9 @@ find_placeholder_info(PlannerInfo *root, PlaceHolderVar *phv)
 	}
 
 	/* Not found, so create it */
+	if (!create_new_ph)
+		elog(ERROR, "too late to create a new PlaceHolderInfo");
+
 	phinfo = makeNode(PlaceHolderInfo);
 
 	phinfo->phid = phv->phid;
@@ -157,7 +166,7 @@ find_placeholders_recurse(PlannerInfo *root, Node *jtnode)
 		/*
 		 * Now process the top-level quals.
 		 */
-		find_placeholders_in_qual(root, f->quals, jtrelids);
+		mark_placeholders_in_expr(root, f->quals, jtrelids);
 	}
 	else if (IsA(jtnode, JoinExpr))
 	{
@@ -173,7 +182,7 @@ find_placeholders_recurse(PlannerInfo *root, Node *jtnode)
 		jtrelids = bms_join(leftids, rightids);
 
 		/* Process the qual clauses */
-		find_placeholders_in_qual(root, j->quals, jtrelids);
+		mark_placeholders_in_expr(root, j->quals, jtrelids);
 	}
 	else
 	{
@@ -185,14 +194,15 @@ find_placeholders_recurse(PlannerInfo *root, Node *jtnode)
 }
 
 /*
- * find_placeholders_in_qual
- *		Process a qual clause for find_placeholders_in_jointree.
+ * mark_placeholders_in_expr
+ *		Find all PlaceHolderVars in the given expression, and mark them
+ *		as possibly needed at the specified join level.
  *
  * relids is the syntactic join level to mark as the "maybe needed" level
- * for each PlaceHolderVar found in the qual clause.
+ * for each PlaceHolderVar found in the expression.
  */
 static void
-find_placeholders_in_qual(PlannerInfo *root, Node *qual, Relids relids)
+mark_placeholders_in_expr(PlannerInfo *root, Node *expr, Relids relids)
 {
 	List	   *vars;
 	ListCell   *vl;
@@ -201,7 +211,7 @@ find_placeholders_in_qual(PlannerInfo *root, Node *qual, Relids relids)
 	 * pull_var_clause does more than we need here, but it'll do and it's
 	 * convenient to use.
 	 */
-	vars = pull_var_clause(qual,
+	vars = pull_var_clause(expr,
 						   PVC_RECURSE_AGGREGATES,
 						   PVC_INCLUDE_PLACEHOLDERS);
 	foreach(vl, vars)
@@ -214,25 +224,47 @@ find_placeholders_in_qual(PlannerInfo *root, Node *qual, Relids relids)
 			continue;
 
 		/* Create a PlaceHolderInfo entry if there's not one already */
-		phinfo = find_placeholder_info(root, phv);
-
-		/* Mark the PHV as possibly needed at the qual's syntactic level */
-		phinfo->ph_may_need = bms_add_members(phinfo->ph_may_need, relids);
+		phinfo = find_placeholder_info(root, phv, true);
 
-		/*
-		 * This is a bit tricky: the PHV's contained expression may contain
-		 * other, lower-level PHVs.  We need to get those into the
-		 * PlaceHolderInfo list, but they aren't going to be needed where the
-		 * outer PHV is referenced.  Rather, they'll be needed where the outer
-		 * PHV is evaluated.  We can estimate that (conservatively) as the
-		 * syntactic location of the PHV's expression.  Recurse to take care
-		 * of any such PHVs.
-		 */
-		find_placeholders_in_qual(root, (Node *) phv->phexpr, phv->phrels);
+		/* Mark it, and recursively process any contained placeholders */
+		mark_placeholder_maybe_needed(root, phinfo, relids);
 	}
 	list_free(vars);
 }
 
+/*
+ * mark_placeholder_maybe_needed
+ *		Mark a placeholder as possibly needed at the specified join level.
+ *
+ * relids is the syntactic join level to mark as the "maybe needed" level
+ * for the placeholder.
+ *
+ * This is called during an initial scan of the query's targetlist and quals
+ * before we begin deconstruct_jointree.  Once we begin deconstruct_jointree,
+ * all active placeholders must be present in root->placeholder_list with
+ * their correct ph_may_need values, because make_outerjoininfo and
+ * update_placeholder_eval_levels require this info to be available while
+ * we crawl up the join tree.
+ */
+void
+mark_placeholder_maybe_needed(PlannerInfo *root, PlaceHolderInfo *phinfo,
+							  Relids relids)
+{
+	/* Mark the PHV as possibly needed at the given syntactic level */
+	phinfo->ph_may_need = bms_add_members(phinfo->ph_may_need, relids);
+
+	/*
+	 * This is a bit tricky: the PHV's contained expression may contain other,
+	 * lower-level PHVs.  We need to get those into the PlaceHolderInfo list,
+	 * but they aren't going to be needed where the outer PHV is referenced.
+	 * Rather, they'll be needed where the outer PHV is evaluated.  We can
+	 * estimate that (conservatively) as the syntactic location of the PHV's
+	 * expression.  Recurse to take care of any such PHVs.
+	 */
+	mark_placeholders_in_expr(root, (Node *) phinfo->ph_var->phexpr,
+							  phinfo->ph_var->phrels);
+}
+
 /*
  * update_placeholder_eval_levels
  *		Adjust the target evaluation levels for placeholders
@@ -353,7 +385,7 @@ fix_placeholder_input_needed_levels(PlannerInfo *root)
 											   PVC_RECURSE_AGGREGATES,
 											   PVC_INCLUDE_PLACEHOLDERS);
 
-			add_vars_to_targetlist(root, vars, eval_at);
+			add_vars_to_targetlist(root, vars, eval_at, false);
 			list_free(vars);
 		}
 	}
diff --git a/src/include/optimizer/placeholder.h b/src/include/optimizer/placeholder.h
@@ -20,8 +20,10 @@
 extern PlaceHolderVar *make_placeholder_expr(PlannerInfo *root, Expr *expr,
 					  Relids phrels);
 extern PlaceHolderInfo *find_placeholder_info(PlannerInfo *root,
-					  PlaceHolderVar *phv);
+					  PlaceHolderVar *phv, bool create_new_ph);
 extern void find_placeholders_in_jointree(PlannerInfo *root);
+extern void mark_placeholder_maybe_needed(PlannerInfo *root,
+							  PlaceHolderInfo *phinfo, Relids relids);
 extern void update_placeholder_eval_levels(PlannerInfo *root,
 							   SpecialJoinInfo *new_sjinfo);
 extern void fix_placeholder_input_needed_levels(PlannerInfo *root);
diff --git a/src/include/optimizer/planmain.h b/src/include/optimizer/planmain.h
@@ -92,7 +92,7 @@ extern int	join_collapse_limit;
 extern void add_base_rels_to_query(PlannerInfo *root, Node *jtnode);
 extern void build_base_rel_tlists(PlannerInfo *root, List *final_tlist);
 extern void add_vars_to_targetlist(PlannerInfo *root, List *vars,
-					   Relids where_needed);
+					   Relids where_needed, bool create_new_ph);
 extern List *deconstruct_jointree(PlannerInfo *root);
 extern void distribute_restrictinfo_to_rels(PlannerInfo *root,
 								RestrictInfo *restrictinfo);
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
@@ -2488,6 +2488,51 @@ order by c.name;
 (3 rows)
 
 rollback;
+--
+-- test incorrect handling of placeholders that only appear in targetlists,
+-- per bug #6154
+--
+SELECT * FROM
+( SELECT 1 as key1 ) sub1
+LEFT JOIN
+( SELECT sub3.key3, sub4.value2, COALESCE(sub4.value2, 66) as value3 FROM
+    ( SELECT 1 as key3 ) sub3
+    LEFT JOIN
+    ( SELECT sub5.key5, COALESCE(sub6.value1, 1) as value2 FROM
+        ( SELECT 1 as key5 ) sub5
+        LEFT JOIN
+        ( SELECT 2 as key6, 42 as value1 ) sub6
+        ON sub5.key5 = sub6.key6
+    ) sub4
+    ON sub4.key5 = sub3.key3
+) sub2
+ON sub1.key1 = sub2.key3;
+ key1 | key3 | value2 | value3 
+------+------+--------+--------
+    1 |    1 |      1 |      1
+(1 row)
+
+-- test the path using join aliases, too
+SELECT * FROM
+( SELECT 1 as key1 ) sub1
+LEFT JOIN
+( SELECT sub3.key3, value2, COALESCE(value2, 66) as value3 FROM
+    ( SELECT 1 as key3 ) sub3
+    LEFT JOIN
+    ( SELECT sub5.key5, COALESCE(sub6.value1, 1) as value2 FROM
+        ( SELECT 1 as key5 ) sub5
+        LEFT JOIN
+        ( SELECT 2 as key6, 42 as value1 ) sub6
+        ON sub5.key5 = sub6.key6
+    ) sub4
+    ON sub4.key5 = sub3.key3
+) sub2
+ON sub1.key1 = sub2.key3;
+ key1 | key3 | value2 | value3 
+------+------+--------+--------
+    1 |    1 |      1 |      1
+(1 row)
+
 --
 -- test the corner cases FULL JOIN ON TRUE and FULL JOIN ON FALSE
 --
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
@@ -602,6 +602,43 @@ order by c.name;
 
 rollback;
 
+--
+-- test incorrect handling of placeholders that only appear in targetlists,
+-- per bug #6154
+--
+SELECT * FROM
+( SELECT 1 as key1 ) sub1
+LEFT JOIN
+( SELECT sub3.key3, sub4.value2, COALESCE(sub4.value2, 66) as value3 FROM
+    ( SELECT 1 as key3 ) sub3
+    LEFT JOIN
+    ( SELECT sub5.key5, COALESCE(sub6.value1, 1) as value2 FROM
+        ( SELECT 1 as key5 ) sub5
+        LEFT JOIN
+        ( SELECT 2 as key6, 42 as value1 ) sub6
+        ON sub5.key5 = sub6.key6
+    ) sub4
+    ON sub4.key5 = sub3.key3
+) sub2
+ON sub1.key1 = sub2.key3;
+
+-- test the path using join aliases, too
+SELECT * FROM
+( SELECT 1 as key1 ) sub1
+LEFT JOIN
+( SELECT sub3.key3, value2, COALESCE(value2, 66) as value3 FROM
+    ( SELECT 1 as key3 ) sub3
+    LEFT JOIN
+    ( SELECT sub5.key5, COALESCE(sub6.value1, 1) as value2 FROM
+        ( SELECT 1 as key5 ) sub5
+        LEFT JOIN
+        ( SELECT 2 as key6, 42 as value1 ) sub6
+        ON sub5.key5 = sub6.key6
+    ) sub4
+    ON sub4.key5 = sub3.key3
+) sub2
+ON sub1.key1 = sub2.key3;
+
 --
 -- test the corner cases FULL JOIN ON TRUE and FULL JOIN ON FALSE
 --

Original file line number	Diff line number	Diff line change
`@@ -3491,7 +3491,7 @@ set_rel_width(PlannerInfo root, RelOptInfo rel)`
`3491`	`3491`	`else if (IsA(node, PlaceHolderVar))`
`3492`	`3492`	`{`
`3493`	`3493`	`PlaceHolderVar phv = (PlaceHolderVar ) node;`
`3494`		`- PlaceHolderInfo *phinfo = find_placeholder_info(root, phv);`
	`3494`	`+ PlaceHolderInfo *phinfo = find_placeholder_info(root, phv, false);`
`3495`	`3495`
`3496`	`3496`	`tuple_width += phinfo->ph_width;`
`3497`	`3497`	`}`
Original file line number	Diff line number	Diff line change
`@@ -850,7 +850,7 @@ generate_base_implied_equalities_no_const(PlannerInfo *root,`
`850`	`850`	`PVC_RECURSE_AGGREGATES,`
`851`	`851`	`PVC_INCLUDE_PLACEHOLDERS);`
`852`	`852`
`853`		`- add_vars_to_targetlist(root, vars, ec->ec_relids);`
	`853`	`+ add_vars_to_targetlist(root, vars, ec->ec_relids, false);`
`854`	`854`	`list_free(vars);`
`855`	`855`	`}`
`856`	`856`	`}`