From 39c85acd7b9c11d6b7c6612459bb4fb8f36ef45b Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Mon, 25 May 2015 01:08:30 -0700 Subject: [PATCH 1/2] Fix bug with whole row Vars in excluded targetlist Follow the approach taken with RETURNING clauses containing references to multiple relations; pull up Vars referencing non-target relations (limited here to the ON CONFLICT DO UPDATE excluded pseudo-relation) that appear in ON CONFLICT DO UPDATE, and add the Vars to the excluded relation targetlist as resjunk. Have setrefs.c infrastructure avoid changing the varattno of whole row Var resjunk attributes from ON CONFLICT related fix_join_expr() calls. Failing to do so would now cause the executor to misidentify the Var as a scalar Var rather than a whole row Var (it specifically recognizes "varattno == InvalidAttrNumber" as representing the whole row case). setrefs.c is also changed to call build_tlist_index_other_vars() rather than build_tlist_index(); this isn't important for correctness, but it seems preferable to make the code more consistent with the well established set_returning_clause_references() case. In passing, make a few minor stylistic tweaks, and reject Vars referencing the excluded.* pseudo relation in an invalid manner. These system column Vars are never useful or meaningful, since, of course, excluded is not a real table. --- src/backend/executor/execMain.c | 2 + src/backend/executor/nodeModifyTable.c | 9 +++- src/backend/optimizer/plan/planner.c | 14 +++-- src/backend/optimizer/plan/setrefs.c | 38 +++++++++++--- src/backend/optimizer/prep/preptlist.c | 74 +++++++++++++++++++++++++-- src/include/optimizer/prep.h | 4 +- src/test/regress/expected/insert_conflict.out | 39 ++++++++++++++ src/test/regress/sql/insert_conflict.sql | 22 ++++++++ 8 files changed, 179 insertions(+), 23 deletions(-) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index a1561ce..f2c0c64 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -1246,6 +1246,8 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo, resultRelInfo->ri_ConstraintExprs = NULL; resultRelInfo->ri_junkFilter = NULL; resultRelInfo->ri_projectReturning = NULL; + resultRelInfo->ri_onConflictSetProj = NULL; + resultRelInfo->ri_onConflictSetWhere = NIL; } /* diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 874ca6a..30a092b 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -1505,13 +1505,18 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) mtstate->resultRelInfo = estate->es_result_relations + node->resultRelIndex; mtstate->mt_arowmarks = (List **) palloc0(sizeof(List *) * nplans); mtstate->mt_nplans = nplans; - mtstate->mt_onconflict = node->onConflictAction; - mtstate->mt_arbiterindexes = node->arbiterIndexes; /* set up epqstate with dummy subplan data for the moment */ EvalPlanQualInit(&mtstate->mt_epqstate, estate, NULL, NIL, node->epqParam); mtstate->fireBSTriggers = true; + /* initialize ON CONFLICT data */ + mtstate->mt_onconflict = node->onConflictAction; + mtstate->mt_arbiterindexes = node->arbiterIndexes; + mtstate->mt_existing = NULL; + mtstate->mt_excludedtlist = NIL; + mtstate->mt_conflproj = NULL; + /* * call ExecInitNode on each of the plans to be executed and save the * results into the array "mt_plans". This is also a convenient place to diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 8afde2b..4ce6ee85 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -488,7 +488,7 @@ subquery_planner(PlannerGlobal *glob, Query *parse, EXPRKIND_TARGET); parse->onConflict->onConflictWhere = - preprocess_expression(root, (Node *) parse->onConflict->onConflictWhere, + preprocess_expression(root, parse->onConflict->onConflictWhere, EXPRKIND_QUAL); } @@ -1273,7 +1273,6 @@ grouping_planner(PlannerInfo *root, double tuple_fraction) bool use_hashed_grouping = false; WindowFuncLists *wflists = NULL; List *activeWindows = NIL; - OnConflictExpr *onconfl; int maxref = 0; int *tleref_to_colnum_map; List *rollup_lists = NIL; @@ -1364,12 +1363,11 @@ grouping_planner(PlannerInfo *root, double tuple_fraction) /* Preprocess targetlist */ tlist = preprocess_targetlist(root, tlist); - onconfl = parse->onConflict; - if (onconfl) - onconfl->onConflictSet = - preprocess_onconflict_targetlist(onconfl->onConflictSet, - parse->resultRelation, - parse->rtable); + /* Preprocess targetlist for ON CONFLICT DO UPDATE */ + if (parse->onConflict) + preprocess_onconflict_targetlist(parse->onConflict, + parse->resultRelation, + parse->rtable); /* * Expand any rangetable entries that have security barrier quals. diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c index a7f65dd..fa683ab 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -33,6 +33,7 @@ typedef struct Index varno; /* RT index of Var */ AttrNumber varattno; /* attr number of Var */ AttrNumber resno; /* TLE position of Var */ + bool resjunk; /* TLE is resjunk? */ } tlist_vinfo; typedef struct @@ -41,6 +42,7 @@ typedef struct int num_vars; /* number of plain Var tlist entries */ bool has_ph_vars; /* are there PlaceHolderVar entries? */ bool has_non_vars; /* are there other entries? */ + bool change_resjunk; /* change varattno of resjunk entries? */ tlist_vinfo vars[FLEXIBLE_ARRAY_MEMBER]; /* has num_vars entries */ } indexed_tlist; @@ -106,6 +108,8 @@ static void set_join_references(PlannerInfo *root, Join *join, int rtoffset); static void set_upper_references(PlannerInfo *root, Plan *plan, int rtoffset); static void set_dummy_tlist_references(Plan *plan, int rtoffset); static indexed_tlist *build_tlist_index(List *tlist); +static indexed_tlist *build_tlist_index_other_vars(List *tlist, + Index ignore_rel); static Var *search_indexed_tlist_for_var(Var *var, indexed_tlist *itlist, Index newvarno, @@ -762,7 +766,15 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset) { indexed_tlist *itlist; - itlist = build_tlist_index(splan->exclRelTlist); + itlist = build_tlist_index_other_vars(splan->exclRelTlist, + linitial_int(splan->resultRelations)); + + /* + * Don't allow fix_join_expr to change the varattno of + * resjunk TLE's Vars. This can occur due to whole row Var + * excluded.* references, but for no other reason. + */ + itlist->change_resjunk = false; splan->onConflictSet = fix_join_expr(root, splan->onConflictSet, @@ -775,6 +787,7 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset) NULL, itlist, linitial_int(splan->resultRelations), rtoffset); + pfree(itlist); splan->exclRelTlist = fix_scan_list(root, splan->exclRelTlist, rtoffset); @@ -1722,6 +1735,7 @@ build_tlist_index(List *tlist) itlist->tlist = tlist; itlist->has_ph_vars = false; itlist->has_non_vars = false; + itlist->change_resjunk = true; /* Find the Vars and fill in the index array */ vinfo = itlist->vars; @@ -1736,6 +1750,7 @@ build_tlist_index(List *tlist) vinfo->varno = var->varno; vinfo->varattno = var->varattno; vinfo->resno = tle->resno; + vinfo->resjunk = tle->resjunk; vinfo++; } else if (tle->expr && IsA(tle->expr, PlaceHolderVar)) @@ -1772,6 +1787,7 @@ build_tlist_index_other_vars(List *tlist, Index ignore_rel) itlist->tlist = tlist; itlist->has_ph_vars = false; itlist->has_non_vars = false; + itlist->change_resjunk = true; /* Find the desired Vars and fill in the index array */ vinfo = itlist->vars; @@ -1788,6 +1804,7 @@ build_tlist_index_other_vars(List *tlist, Index ignore_rel) vinfo->varno = var->varno; vinfo->varattno = var->varattno; vinfo->resno = tle->resno; + vinfo->resjunk = tle->resjunk; vinfo++; } } @@ -1827,7 +1844,11 @@ search_indexed_tlist_for_var(Var *var, indexed_tlist *itlist, Var *newvar = copyVar(var); newvar->varno = newvarno; - newvar->varattno = vinfo->resno; + if (itlist->change_resjunk || !vinfo->resjunk) + newvar->varattno = vinfo->resno; + else + Assert(newvar->varattno == InvalidAttrNumber); + if (newvar->varnoold > 0) newvar->varnoold += rtoffset; return newvar; @@ -1917,10 +1938,13 @@ search_indexed_tlist_for_sortgroupref(Node *node, * * This is used in two different scenarios: a normal join clause, where all * the Vars in the clause *must* be replaced by OUTER_VAR or INNER_VAR - * references; and a RETURNING clause, which may contain both Vars of the - * target relation and Vars of other relations. In the latter case we want - * to replace the other-relation Vars by OUTER_VAR references, while leaving - * target Vars alone. + * references; and a RETURNING/ON CONFLICT DO UPDATE SET/ON CONFLICT DO UPDATE + * WHERE clause, which may contain both Vars of the target relation and Vars of + * other relations. In the latter case we want to replace the other-relation + * Vars by OUTER_VAR references (or INNER_VAR references for ON CONFLICT), + * while leaving target Vars alone. Also, for ON CONFLICT, caller has us avoid + * incrementing resjunk TLE Vars' varattno -- this is for the benefit of + * excluded.* whole row Vars. * * For a normal join, acceptable_rel should be zero so that any failure to * match a Var will be reported as an error. For the RETURNING case, pass @@ -1978,7 +2002,7 @@ fix_join_expr_mutator(Node *node, fix_join_expr_context *context) return (Node *) newvar; } - /* Then in the outer */ + /* Then in the inner */ if (context->inner_itlist) { newvar = search_indexed_tlist_for_var(var, diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c index 6b0c689..e6a5933 100644 --- a/src/backend/optimizer/prep/preptlist.c +++ b/src/backend/optimizer/prep/preptlist.c @@ -38,6 +38,8 @@ static List *expand_targetlist(List *tlist, int command_type, Index result_relation, List *range_table); +static void pull_var_targetlist_clause(OnConflictExpr *onconfl, List* clause, + int result_relation); /* @@ -185,12 +187,28 @@ preprocess_targetlist(PlannerInfo *root, List *tlist) * preprocess_onconflict_targetlist * Process ON CONFLICT SET targetlist. * - * Returns the new targetlist. + * Modifies passed ON CONFLICT expression in-place. */ -List * -preprocess_onconflict_targetlist(List *tlist, int result_relation, List *range_table) +void +preprocess_onconflict_targetlist(OnConflictExpr *onconfl, int result_relation, + List *range_table) { - return expand_targetlist(tlist, CMD_UPDATE, result_relation, range_table); + List *tlist; + + tlist = expand_targetlist(onconfl->onConflictSet, CMD_UPDATE, + result_relation, range_table); + + /* + * Perform similar pull up process to that perfomred by + * preprocess_targetlist() for multi-relation RETURNING lists. This is + * only necessary for the benefit of excluded.* whole row Vars, and to + * enforce that excluded.* system column references are disallowed. + */ + pull_var_targetlist_clause(onconfl, tlist, result_relation); + pull_var_targetlist_clause(onconfl, (List *) onconfl->onConflictWhere, + result_relation); + + onconfl->onConflictSet = tlist; } @@ -376,6 +394,54 @@ expand_targetlist(List *tlist, int command_type, return new_tlist; } +/* + * pull_var_targetlist_clause + * Given an ON CONFLICT clause as generated by the parser and a result + * relation, add resjunk entries for any Vars used that are not + * associated with the target and are not excluded.* non-resjunk + * TLE Vars. + */ +static void +pull_var_targetlist_clause(OnConflictExpr *onconfl, List* clause, + int result_relation) +{ + List *vars; + ListCell *l; + + vars = pull_var_clause((Node *) clause, + PVC_RECURSE_AGGREGATES, + PVC_INCLUDE_PLACEHOLDERS); + foreach(l, vars) + { + Var *var = (Var *) lfirst(l); + TargetEntry *tle; + + if (IsA(var, Var) && var->varno == result_relation) + continue; /* don't need it */ + + if (IsA(var, Var) && var->varno == onconfl->exclRelIndex) + { + if (var->varattno < 0) + ereport(ERROR, + (errcode(ERRCODE_INVALID_COLUMN_REFERENCE), + errmsg("system columns from excluded table cannot be referenced"))); + else if (var->varattno > 0) /* don't need it */ + continue; + } + + if (tlist_member((Node *) var, onconfl->exclRelTlist)) + continue; /* already got it */ + + tle = makeTargetEntry((Expr *) var, + list_length(clause) + 1, + NULL, + true); + + onconfl->exclRelTlist = lappend(onconfl->exclRelTlist, tle); + } + + list_free(vars); +} /* * Locate PlanRowMark for given RT index, or return NULL if none diff --git a/src/include/optimizer/prep.h b/src/include/optimizer/prep.h index 7b8c0a9..8d61c7c 100644 --- a/src/include/optimizer/prep.h +++ b/src/include/optimizer/prep.h @@ -45,8 +45,8 @@ extern void expand_security_quals(PlannerInfo *root, List *tlist); */ extern List *preprocess_targetlist(PlannerInfo *root, List *tlist); -extern List *preprocess_onconflict_targetlist(List *tlist, - int result_relation, List *range_table); +extern void preprocess_onconflict_targetlist(OnConflictExpr *onconfl, + int result_relation, List *range_table); extern PlanRowMark *get_plan_rowmark(List *rowmarks, Index rtindex); diff --git a/src/test/regress/expected/insert_conflict.out b/src/test/regress/expected/insert_conflict.out index eca9690..442d9fc 100644 --- a/src/test/regress/expected/insert_conflict.out +++ b/src/test/regress/expected/insert_conflict.out @@ -363,6 +363,45 @@ ERROR: there is no unique or exclusion constraint matching the ON CONFLICT spec insert into insertconflicttest values (23, 'Blackberry') on conflict (fruit) where fruit like '%berry' do update set fruit = excluded.fruit; ERROR: there is no unique or exclusion constraint matching the ON CONFLICT specification drop index partial_key_index; +-- +-- Test resjunk in excluded.* pseudo-relation +-- +create unique index plain on insertconflicttest(key); +-- Succeeds, updates existing row: +insert into insertconflicttest as i values (23, 'Jackfruit') on conflict (key) do update set fruit = excluded.fruit + where i.* != excluded.* returning *; + key | fruit +-----+----------- + 23 | Jackfruit +(1 row) + +-- No update this time, though: +insert into insertconflicttest as i values (23, 'Jackfruit') on conflict (key) do update set fruit = excluded.fruit + where i.* != excluded.* returning *; + key | fruit +-----+------- +(0 rows) + +-- Predicate changed to require match rather than non-match, so updates once more: +insert into insertconflicttest as i values (23, 'Jackfruit') on conflict (key) do update set fruit = excluded.fruit + where i.* = excluded.* returning *; + key | fruit +-----+----------- + 23 | Jackfruit +(1 row) + +-- Assign: +insert into insertconflicttest as i values (23, 'Avocado') on conflict (key) do update set fruit = excluded.*::text + returning *; + key | fruit +-----+-------------- + 23 | (23,Avocado) +(1 row) + +-- Forbidden, fails: +insert into insertconflicttest values (23, 'Blackberry') on conflict (key) do update set fruit = excluded.ctid; +ERROR: system columns from excluded table cannot be referenced +drop index plain; -- Cleanup drop table insertconflicttest; -- ****************************************************************** diff --git a/src/test/regress/sql/insert_conflict.sql b/src/test/regress/sql/insert_conflict.sql index a0bdd7f..a2b6aba 100644 --- a/src/test/regress/sql/insert_conflict.sql +++ b/src/test/regress/sql/insert_conflict.sql @@ -212,6 +212,28 @@ insert into insertconflicttest values (23, 'Blackberry') on conflict (fruit) whe drop index partial_key_index; +-- +-- Test resjunk in excluded.* pseudo-relation +-- +create unique index plain on insertconflicttest(key); + +-- Succeeds, updates existing row: +insert into insertconflicttest as i values (23, 'Jackfruit') on conflict (key) do update set fruit = excluded.fruit + where i.* != excluded.* returning *; +-- No update this time, though: +insert into insertconflicttest as i values (23, 'Jackfruit') on conflict (key) do update set fruit = excluded.fruit + where i.* != excluded.* returning *; +-- Predicate changed to require match rather than non-match, so updates once more: +insert into insertconflicttest as i values (23, 'Jackfruit') on conflict (key) do update set fruit = excluded.fruit + where i.* = excluded.* returning *; +-- Assign: +insert into insertconflicttest as i values (23, 'Avocado') on conflict (key) do update set fruit = excluded.*::text + returning *; +-- Forbidden, fails: +insert into insertconflicttest values (23, 'Blackberry') on conflict (key) do update set fruit = excluded.ctid; + +drop index plain; + -- Cleanup drop table insertconflicttest; -- 1.9.1