Wrap appendrel member outputs in PlaceHolderVars in additional cases.

Add PlaceHolderVar wrappers as needed to make UNION ALL sub-select output
expressions appear non-constant and distinct from each other.  This makes
the world safe for add_child_rel_equivalences to do what it does.  Before,
it was possible for that function to add identical expressions to different
EquivalenceClasses, which logically should imply merging such ECs, which
would be wrong; or to improperly add a constant to an EquivalenceClass,
drastically changing its behavior.  Per report from Teodor Sigaev.

The only currently known consequence of this bug is "MergeAppend child's
targetlist doesn't match MergeAppend" planner failures in 9.1 and later.
I am suspicious that there may be other failure modes that could affect
older release branches; but in the absence of any hard evidence, I'll
refrain from back-patching further than 9.1.

Author: tglsfdc

src/backend/optimizer/plan/planmain.c

@@ -176,7 +176,7 @@ query_planner(PlannerInfo *root, List *tlist,
 	 */
 	build_base_rel_tlists(root, tlist);
 
-	find_placeholders_in_jointree(root);
+	find_placeholders_in_query(root);
 
 	joinlist = deconstruct_jointree(root);
 

src/backend/optimizer/prep/prepjointree.c

@@ -784,22 +784,15 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
 	parse->havingQual = pullup_replace_vars(parse->havingQual, &rvcontext);
 
 	/*
-	 * Replace references in the translated_vars lists of appendrels. When
-	 * pulling up an appendrel member, we do not need PHVs in the list of the
-	 * parent appendrel --- there isn't any outer join between. Elsewhere, use
-	 * PHVs for safety.  (This analysis could be made tighter but it seems
-	 * unlikely to be worth much trouble.)
+	 * Replace references in the translated_vars lists of appendrels, too.
+	 * We do it this way because we must preserve the AppendRelInfo structs.
 	 */
 	foreach(lc, root->append_rel_list)
 	{
 		AppendRelInfo *appinfo = (AppendRelInfo *) lfirst(lc);
-		bool		save_need_phvs = rvcontext.need_phvs;
 
-		if (appinfo == containing_appendrel)
-			rvcontext.need_phvs = false;
 		appinfo->translated_vars = (List *)
 			pullup_replace_vars((Node *) appinfo->translated_vars, &rvcontext);
-		rvcontext.need_phvs = save_need_phvs;
 	}
 
 	/*
@@ -1407,14 +1400,31 @@ pullup_replace_vars_callback(Var *var,
 			if (newnode && IsA(newnode, Var) &&
 				((Var *) newnode)->varlevelsup == 0)
 			{
-				/* Simple Vars always escape being wrapped */
-				wrap = false;
+				/*
+				 * Simple Vars normally escape being wrapped.  However, in
+				 * wrap_non_vars mode (ie, we are dealing with an appendrel
+				 * member), we must ensure that each tlist entry expands to a
+				 * distinct expression, else we may have problems with
+				 * improperly placing identical entries into different
+				 * EquivalenceClasses.  Therefore, we wrap a Var in a
+				 * PlaceHolderVar if it duplicates any earlier entry in the
+				 * tlist (ie, we've got "SELECT x, x, ...").  Since each PHV
+				 * is distinct, this fixes the ambiguity.  We can use
+				 * tlist_member to detect whether there's an earlier
+				 * duplicate.
+				 */
+				wrap = (rcon->wrap_non_vars &&
+						tlist_member(newnode, rcon->targetlist) != tle);
 			}
 			else if (newnode && IsA(newnode, PlaceHolderVar) &&
 					 ((PlaceHolderVar *) newnode)->phlevelsup == 0)
 			{
-				/* No need to wrap a PlaceHolderVar with another one, either */
-				wrap = false;
+				/*
+				 * No need to directly wrap a PlaceHolderVar with another one,
+				 * either, unless we need to prevent duplication.
+				 */
+				wrap = (rcon->wrap_non_vars &&
+						tlist_member(newnode, rcon->targetlist) != tle);
 			}
 			else if (rcon->wrap_non_vars)
 			{

src/backend/optimizer/util/placeholder.c

@@ -104,28 +104,41 @@ find_placeholder_info(PlannerInfo *root, PlaceHolderVar *phv,
 }
 
 /*
- * find_placeholders_in_jointree
- *		Search the jointree for PlaceHolderVars, and build PlaceHolderInfos
+ * find_placeholders_in_query
+ *		Search the query for PlaceHolderVars, and build PlaceHolderInfos
  *
- * We don't need to look at the targetlist because build_base_rel_tlists()
- * will already have made entries for any PHVs in the tlist.
+ * We need to examine the jointree, but not the targetlist, because
+ * build_base_rel_tlists() will already have made entries for any PHVs
+ * in the targetlist.
+ *
+ * We also need to search for PHVs in AppendRelInfo translated_vars
+ * lists.  In most cases, translated_vars entries aren't directly referenced
+ * elsewhere, but we need to create PlaceHolderInfo entries for them to
+ * support set_rel_width() calculations for the appendrel child relations.
  */
 void
-find_placeholders_in_jointree(PlannerInfo *root)
+find_placeholders_in_query(PlannerInfo *root)
 {
 	/* We need do nothing if the query contains no PlaceHolderVars */
 	if (root->glob->lastPHId != 0)
 	{
-		/* Start recursion at top of jointree */
+		/* Recursively search the jointree */
 		Assert(root->parse->jointree != NULL &&
 			   IsA(root->parse->jointree, FromExpr));
 		(void) find_placeholders_recurse(root, (Node *) root->parse->jointree);
+
+		/*
+		 * Also search the append_rel_list for translated vars that are PHVs.
+		 * Barring finding them elsewhere in the query, they do not need any
+		 * ph_may_need bits, only to be present in the PlaceHolderInfo list.
+		 */
+		mark_placeholders_in_expr(root, (Node *) root->append_rel_list, NULL);
 	}
 }
 
 /*
  * find_placeholders_recurse
- *	  One recursion level of find_placeholders_in_jointree.
+ *	  One recursion level of jointree search for find_placeholders_in_query.
  *
  * jtnode is the current jointree node to examine.
  *

src/include/optimizer/placeholder.h

@@ -21,7 +21,7 @@ extern PlaceHolderVar *make_placeholder_expr(PlannerInfo *root, Expr *expr,
 					  Relids phrels);
 extern PlaceHolderInfo *find_placeholder_info(PlannerInfo *root,
 					  PlaceHolderVar *phv, bool create_new_ph);
-extern void find_placeholders_in_jointree(PlannerInfo *root);
+extern void find_placeholders_in_query(PlannerInfo *root);
 extern void mark_placeholder_maybe_needed(PlannerInfo *root,
 							  PlaceHolderInfo *phinfo, Relids relids);
 extern void update_placeholder_eval_levels(PlannerInfo *root,

src/test/regress/expected/inherit.out

@@ -1242,3 +1242,61 @@ NOTICE:  drop cascades to 3 other objects
 DETAIL:  drop cascades to table matest1
 drop cascades to table matest2
 drop cascades to table matest3
+--
+-- Test merge-append for UNION ALL append relations
+-- Check handling of duplicated, constant, or volatile targetlist items
+--
+set enable_seqscan = off;
+set enable_indexscan = on;
+set enable_bitmapscan = off;
+explain (costs off)
+SELECT thousand, tenthous FROM tenk1
+UNION ALL
+SELECT thousand, thousand FROM tenk1
+ORDER BY thousand, tenthous;
+                              QUERY PLAN                               
+-----------------------------------------------------------------------
+ Result
+   ->  Merge Append
+         Sort Key: public.tenk1.thousand, public.tenk1.tenthous
+         ->  Index Only Scan using tenk1_thous_tenthous on tenk1
+         ->  Sort
+               Sort Key: public.tenk1.thousand, public.tenk1.thousand
+               ->  Index Only Scan using tenk1_thous_tenthous on tenk1
+(7 rows)
+
+explain (costs off)
+SELECT thousand, tenthous FROM tenk1
+UNION ALL
+SELECT 42, 42 FROM tenk1
+ORDER BY thousand, tenthous;
+                              QUERY PLAN                               
+-----------------------------------------------------------------------
+ Result
+   ->  Merge Append
+         Sort Key: public.tenk1.thousand, public.tenk1.tenthous
+         ->  Index Only Scan using tenk1_thous_tenthous on tenk1
+         ->  Sort
+               Sort Key: (42), (42)
+               ->  Index Only Scan using tenk1_thous_tenthous on tenk1
+(7 rows)
+
+explain (costs off)
+SELECT thousand, tenthous FROM tenk1
+UNION ALL
+SELECT thousand, random()::integer FROM tenk1
+ORDER BY thousand, tenthous;
+                              QUERY PLAN                               
+-----------------------------------------------------------------------
+ Result
+   ->  Merge Append
+         Sort Key: public.tenk1.thousand, public.tenk1.tenthous
+         ->  Index Only Scan using tenk1_thous_tenthous on tenk1
+         ->  Sort
+               Sort Key: public.tenk1.thousand, ((random())::integer)
+               ->  Index Only Scan using tenk1_thous_tenthous on tenk1
+(7 rows)
+
+reset enable_seqscan;
+reset enable_indexscan;
+reset enable_bitmapscan;

src/test/regress/sql/inherit.sql

@@ -406,3 +406,34 @@ select * from matest0 order by 1-id;
 reset enable_seqscan;
 
 drop table matest0 cascade;
+
+--
+-- Test merge-append for UNION ALL append relations
+-- Check handling of duplicated, constant, or volatile targetlist items
+--
+
+set enable_seqscan = off;
+set enable_indexscan = on;
+set enable_bitmapscan = off;
+
+explain (costs off)
+SELECT thousand, tenthous FROM tenk1
+UNION ALL
+SELECT thousand, thousand FROM tenk1
+ORDER BY thousand, tenthous;
+
+explain (costs off)
+SELECT thousand, tenthous FROM tenk1
+UNION ALL
+SELECT 42, 42 FROM tenk1
+ORDER BY thousand, tenthous;
+
+explain (costs off)
+SELECT thousand, tenthous FROM tenk1
+UNION ALL
+SELECT thousand, random()::integer FROM tenk1
+ORDER BY thousand, tenthous;
+
+reset enable_seqscan;
+reset enable_indexscan;
+reset enable_bitmapscan;