From ce4f46fdc814eb1b704d81640f6d8f03625d0f53 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Tue, 12 Apr 2022 09:29:39 +0200 Subject: [PATCH] Change mechanism to set up source targetlist in MERGE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Author: Álvaro Herrera Reported-by: Joe Wildish Discussion: https://postgr.es/m/fab3b90a-914d-46a9-beb0-df011ee39ee5@www.fastmail.com --- src/backend/optimizer/plan/setrefs.c | 7 ++- src/backend/optimizer/prep/preptlist.c | 63 ++++++++++++++++++-------- src/backend/parser/parse_merge.c | 9 ++-- src/test/regress/expected/merge.out | 13 ++++++ src/test/regress/expected/with.out | 6 +-- src/test/regress/sql/merge.sql | 9 ++++ 6 files changed, 80 insertions(+), 27 deletions(-) diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c index 6ea35056465..d95fd898077 100644 --- 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 index 99ab3d75594..137b28323d6 100644 --- 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,6 +144,8 @@ 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, @@ -158,6 +153,36 @@ preprocess_targetlist(PlannerInfo *root) 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 index 5d0035a12b6..bb9d76306b7 100644 --- 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 index 5954f10b8ff..0fd037b45a8 100644 --- 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 index 7c6de7cc07c..57e145b0f5a 100644 --- 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 index 6d05a2f39ca..8815e0cc498 100644 --- 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