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.

1 files changed, 23 insertions, 13 deletions

diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index aeaae8c8d87..8bb011b7116 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/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)
 			{