Change mechanism to set up source targetlist in MERGE

alvherre · alvherre · commit ce4f46fdc814 · 2022-04-12T09:29:39.000+02:00
We were setting MERGE source subplan's targetlist by expanding the individual attributes of the source relation completely, early in the parse analysis phase. This failed to work when the condition of an action included a whole-row reference, causing setrefs.c to error out with ERROR: variable not found in subplan target lists because at that point there is nothing to resolve the whole-row reference with. We can fix this by having preprocess_targetlist expand the source targetlist for Vars required from the source rel by all actions. Moreover, by using this expansion mechanism we can do away with the targetlist expansion in transformMergeStmt, which is good because then we no longer pull in columns that aren't needed for anything. Add a test case for the problem. While at it, remove some redundant code in preprocess_targetlist(): MERGE was doing separately what is already being done for UPDATE/DELETE, so we can just rely on the latter and remove the former. (The handling of inherited rels was different for MERGE, but that was a no-longer- necessary hack.) Fix outdated, related comments for fix_join_expr also. Author: Richard Guo <guofenglinux@gmail.com> Author: Álvaro Herrera <alvherre@alvh.no-ip.org> Reported-by: Joe Wildish <joe@lateraljoin.com> Discussion: https://postgr.es/m/fab3b90a-914d-46a9-beb0-df011ee39ee5@www.fastmail.com
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
@@ -2748,7 +2748,7 @@ search_indexed_tlist_for_sortgroupref(Expr *node,
  *	   relation target lists.  Also perform opcode lookup and add
  *	   regclass OIDs to root->glob->relationOids.
  *
- * This is used in three different scenarios:
+ * This is used in four different scenarios:
  * 1) a normal join clause, where all the Vars in the clause *must* be
  *	  replaced by OUTER_VAR or INNER_VAR references.  In this case
  *	  acceptable_rel should be zero so that any failure to match a Var will be
@@ -2763,6 +2763,11 @@ search_indexed_tlist_for_sortgroupref(Expr *node,
  *	  to-be-updated relation) alone. Correspondingly inner_itlist is to be
  *	  EXCLUDED elements, outer_itlist = NULL and acceptable_rel the target
  *	  relation.
+ * 4) MERGE.  In this case, references to the source relation are to be
+ *    replaced with INNER_VAR references, leaving Vars of the target
+ *    relation (the to-be-modified relation) alone.  So inner_itlist is to be
+ *    the source relation elements, outer_itlist = NULL and acceptable_rel
+ *    the target relation.
  *
  * 'clauses' is the targetlist or list of join clauses
  * 'outer_itlist' is the indexed target list of the outer join relation,
diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c
@@ -107,14 +107,15 @@ preprocess_targetlist(PlannerInfo *root)
 		root->update_colnos = extract_update_targetlist_colnos(tlist);
 
 	/*
-	 * For non-inherited UPDATE/DELETE, register any junk column(s) needed to
-	 * allow the executor to identify the rows to be updated or deleted.  In
-	 * the inheritance case, we do nothing now, leaving this to be dealt with
-	 * when expand_inherited_rtentry() makes the leaf target relations.  (But
-	 * there might not be any leaf target relations, in which case we must do
-	 * this in distribute_row_identity_vars().)
+	 * For non-inherited UPDATE/DELETE/MERGE, register any junk column(s)
+	 * needed to allow the executor to identify the rows to be updated or
+	 * deleted.  In the inheritance case, we do nothing now, leaving this to
+	 * be dealt with when expand_inherited_rtentry() makes the leaf target
+	 * relations.  (But there might not be any leaf target relations, in which
+	 * case we must do this in distribute_row_identity_vars().)
 	 */
-	if ((command_type == CMD_UPDATE || command_type == CMD_DELETE) &&
+	if ((command_type == CMD_UPDATE || command_type == CMD_DELETE ||
+		 command_type == CMD_MERGE) &&
 		!target_rte->inh)
 	{
 		/* row-identity logic expects to add stuff to processed_tlist */
@@ -125,23 +126,15 @@ preprocess_targetlist(PlannerInfo *root)
 	}
 
 	/*
-	 * For MERGE we need to handle the target list for the target relation,
-	 * and also target list for each action (only INSERT/UPDATE matter).
+	 * For MERGE we also need to handle the target list for each INSERT and
+	 * UPDATE action separately.  In addition, we examine the qual of each
+	 * action and add any Vars there (other than those of the target rel) to
+	 * the subplan targetlist.
 	 */
 	if (command_type == CMD_MERGE)
 	{
 		ListCell   *l;
 
-		/*
-		 * For MERGE, add any junk column(s) needed to allow the executor to
-		 * identify the rows to be inserted or updated.
-		 */
-		root->processed_tlist = tlist;
-		add_row_identity_columns(root, result_relation,
-								 target_rte, target_relation);
-
-		tlist = root->processed_tlist;
-
 		/*
 		 * For MERGE, handle targetlist of each MergeAction separately. Give
 		 * the same treatment to MergeAction->targetList as we would have
@@ -151,13 +144,45 @@ preprocess_targetlist(PlannerInfo *root)
 		foreach(l, parse->mergeActionList)
 		{
 			MergeAction *action = (MergeAction *) lfirst(l);
+			List	   *vars;
+			ListCell   *l2;
 
 			if (action->commandType == CMD_INSERT)
 				action->targetList = expand_insert_targetlist(action->targetList,
 															  target_relation);
 			else if (action->commandType == CMD_UPDATE)
 				action->updateColnos =
 					extract_update_targetlist_colnos(action->targetList);
+
+			/*
+			 * Add resjunk entries for any Vars used in each action's
+			 * targetlist and WHEN condition that belong to relations other
+			 * than target.  Note that aggregates, window functions and
+			 * placeholder vars are not possible anywhere in MERGE's WHEN
+			 * clauses.  (PHVs may be added later, but they don't concern us
+			 * here.)
+			 */
+			vars = pull_var_clause((Node *)
+								   list_concat_copy((List *) action->qual,
+													action->targetList),
+								   0);
+			foreach(l2, vars)
+			{
+				Var		   *var = (Var *) lfirst(l2);
+				TargetEntry *tle;
+
+				if (IsA(var, Var) && var->varno == result_relation)
+					continue;	/* don't need it */
+
+				if (tlist_member((Expr *) var, tlist))
+					continue;	/* already got it */
+
+				tle = makeTargetEntry((Expr *) var,
+									  list_length(tlist) + 1,
+									  NULL, true);
+				tlist = lappend(tlist, tle);
+			}
+			list_free(vars);
 		}
 	}
 
diff --git a/src/backend/parser/parse_merge.c b/src/backend/parser/parse_merge.c
@@ -18,7 +18,6 @@
 #include "access/sysattr.h"
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
-#include "nodes/nodeFuncs.h"
 #include "parser/analyze.h"
 #include "parser/parse_collate.h"
 #include "parser/parsetree.h"
@@ -205,9 +204,11 @@ transformMergeStmt(ParseState *pstate, MergeStmt *stmt)
 					   pstate->p_target_nsitem->p_names->aliasname),
 				errdetail("The name is used both as MERGE target table and data source."));
 
-	qry->targetList = expandNSItemAttrs(pstate, nsitem, 0, false,
-										exprLocation(stmt->sourceRelation));
-
+	/*
+	 * There's no need for a targetlist here; it'll be set up by
+	 * preprocess_targetlist later.
+	 */
+	qry->targetList = NIL;
 	qry->rtable = pstate->p_rtable;
 
 	/*
diff --git a/src/test/regress/expected/merge.out b/src/test/regress/expected/merge.out
@@ -791,6 +791,19 @@ SELECT * FROM wq_target;
    1 |     299
 (1 row)
 
+-- check source-side whole-row references
+BEGIN;
+MERGE INTO wq_target t
+USING wq_source s ON (t.tid = s.sid)
+WHEN matched and t = s or t.tid = s.sid THEN
+	UPDATE SET balance = t.balance + s.balance;
+SELECT * FROM wq_target;
+ tid | balance 
+-----+---------
+   1 |     399
+(1 row)
+
+ROLLBACK;
 -- check if subqueries work in the conditions?
 MERGE INTO wq_target t
 USING wq_source s ON t.tid = s.sid
diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.out
@@ -2800,7 +2800,7 @@ WHEN NOT MATCHED THEN INSERT VALUES(o.k, o.v);
      ->  Result
            Output: 1, 'cte_basic val'::text
    ->  Hash Right Join
-         Output: (0), ('merge source SubPlan'::text), m.ctid
+         Output: m.ctid, (0), ('merge source SubPlan'::text)
          Hash Cond: (m.k = (0))
          ->  Seq Scan on public.m
                Output: m.ctid, m.k
@@ -2847,7 +2847,7 @@ WHEN NOT MATCHED THEN INSERT VALUES(o.k, o.v);
                  Output: (cte_init.b || ' merge update'::text)
                  Filter: (cte_init.a = 1)
    ->  Hash Right Join
-         Output: (1), ('merge source InitPlan'::text), m.ctid
+         Output: m.ctid, (1), ('merge source InitPlan'::text)
          Hash Cond: (m.k = (1))
          ->  Seq Scan on public.m
                Output: m.ctid, m.k
@@ -2889,7 +2889,7 @@ WHEN NOT MATCHED THEN INSERT VALUES(o.a, o.b || (SELECT merge_source_cte.*::text
      ->  CTE Scan on merge_source_cte merge_source_cte_2
            Output: ((merge_source_cte_2.*)::text || ' merge insert'::text)
    ->  Hash Right Join
-         Output: merge_source_cte.a, merge_source_cte.b, m.ctid
+         Output: m.ctid, merge_source_cte.a, merge_source_cte.b
          Hash Cond: (m.k = merge_source_cte.a)
          ->  Seq Scan on public.m
                Output: m.ctid, m.k
diff --git a/src/test/regress/sql/merge.sql b/src/test/regress/sql/merge.sql
@@ -527,6 +527,15 @@ WHEN MATCHED AND t.balance = 199 OR s.balance > 100 THEN
 	UPDATE SET balance = t.balance + s.balance;
 SELECT * FROM wq_target;
 
+-- check source-side whole-row references
+BEGIN;
+MERGE INTO wq_target t
+USING wq_source s ON (t.tid = s.sid)
+WHEN matched and t = s or t.tid = s.sid THEN
+	UPDATE SET balance = t.balance + s.balance;
+SELECT * FROM wq_target;
+ROLLBACK;
+
 -- check if subqueries work in the conditions?
 MERGE INTO wq_target t
 USING wq_source s ON t.tid = s.sid
