From 4af6e61a363246cf7fff3368a76603b0ce9945dd Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Wed, 13 May 2015 00:13:22 +0200 Subject: [PATCH] Fix ON CONFLICT bugs that manifest when used in rules. Specifically the tlist and rti of the pseudo "excluded" relation weren't properly treated by expression_tree_walker, which lead to errors when excluded was referenced inside a rule because the varnos where not properly adjusted. Similar omissions in OffsetVarNodes and expression_tree_mutator had less impact, but should obviously be fixed nonetheless. A couple tests of for ON CONFLICT UPDATE into INSERT rule bearing relations have been added. In passing I updated a couple comments. --- src/backend/executor/nodeModifyTable.c | 1 + src/backend/nodes/nodeFuncs.c | 3 + src/backend/optimizer/plan/setrefs.c | 9 ++- src/backend/rewrite/rewriteManip.c | 15 +++- src/test/regress/expected/rules.out | 105 ++++++++++++++++++++++--- src/test/regress/sql/rules.sql | 35 ++++++++- 6 files changed, 151 insertions(+), 17 deletions(-) diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index aec4151094..89f1f57ae3 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -1675,6 +1675,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) ExecSetSlotDescriptor(mtstate->mt_existing, resultRelInfo->ri_RelationDesc->rd_att); + /* carried forward solely for the benefit of explain */ mtstate->mt_excludedtlist = node->exclRelTlist; /* create target slot for UPDATE SET projection */ diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c index 4135f9c3cf..eac0215923 100644 --- a/src/backend/nodes/nodeFuncs.c +++ b/src/backend/nodes/nodeFuncs.c @@ -1922,6 +1922,8 @@ expression_tree_walker(Node *node, return true; if (walker(onconflict->onConflictWhere, context)) return true; + if (walker(onconflict->exclRelTlist, context)) + return true; } break; case T_JoinExpr: @@ -2642,6 +2644,7 @@ expression_tree_mutator(Node *node, MUTATE(newnode->arbiterWhere, oc->arbiterWhere, Node *); MUTATE(newnode->onConflictSet, oc->onConflictSet, List *); MUTATE(newnode->onConflictWhere, oc->onConflictWhere, Node *); + MUTATE(newnode->exclRelTlist, oc->exclRelTlist, List *); return (Node *) newnode; } diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c index fac51c9147..517409d28a 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -740,9 +740,9 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset) /* * We treat ModifyTable with ON CONFLICT as a form of 'pseudo - * join', where the inner side is the EXLUDED tuple. Therefore - * use fix_join_expr to setup the relevant variables to - * INNER_VAR. We explicitly don't create any OUTER_VARs as + * join', where the inner side is the EXCLUDED tuple. + * Therefore use fix_join_expr to setup the relevant variables + * to INNER_VAR. We explicitly don't create any OUTER_VARs as * those are already used by RETURNING and it seems better to * be non-conflicting. */ @@ -763,6 +763,9 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset) NULL, itlist, linitial_int(splan->resultRelations), rtoffset); + + splan->exclRelTlist = + fix_scan_list(root, splan->exclRelTlist, rtoffset); } splan->nominalRelation += rtoffset; diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c index df457080fe..a9c6e626ba 100644 --- a/src/backend/rewrite/rewriteManip.c +++ b/src/backend/rewrite/rewriteManip.c @@ -426,9 +426,9 @@ OffsetVarNodes(Node *node, int offset, int sublevels_up) /* * If we are starting at a Query, and sublevels_up is zero, then we * must also fix rangetable indexes in the Query itself --- namely - * resultRelation and rowMarks entries. sublevels_up cannot be zero - * when recursing into a subquery, so there's no need to have the same - * logic inside OffsetVarNodes_walker. + * resultRelation, exclRelIndex and rowMarks entries. sublevels_up + * cannot be zero when recursing into a subquery, so there's no need + * to have the same logic inside OffsetVarNodes_walker. */ if (sublevels_up == 0) { @@ -436,6 +436,10 @@ OffsetVarNodes(Node *node, int offset, int sublevels_up) if (qry->resultRelation) qry->resultRelation += offset; + + if (qry->onConflict && qry->onConflict->exclRelIndex) + qry->onConflict->exclRelIndex += offset; + foreach(l, qry->rowMarks) { RowMarkClause *rc = (RowMarkClause *) lfirst(l); @@ -617,6 +621,11 @@ ChangeVarNodes(Node *node, int rt_index, int new_index, int sublevels_up) if (qry->resultRelation == rt_index) qry->resultRelation = new_index; + + /* this is unlikely to ever be used, but ... */ + if (qry->onConflict && qry->onConflict->exclRelIndex == rt_index) + qry->onConflict->exclRelIndex = new_index; + foreach(l, qry->rowMarks) { RowMarkClause *rc = (RowMarkClause *) lfirst(l); diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index a379a7279c..cb18bb931a 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -2817,25 +2817,112 @@ CREATE RULE hat_upsert AS ON INSERT TO hats INSERT INTO hat_data VALUES ( NEW.hat_name, NEW.hat_color) - ON CONFLICT (hat_name) DO UPDATE SET hat_color = 'Orange' RETURNING *; + ON CONFLICT (hat_name) + DO UPDATE + SET hat_name = hat_data.hat_name, hat_color = excluded.hat_color + WHERE excluded.hat_color <> 'forbidden' + RETURNING *; -- Works (does upsert) -INSERT INTO hats VALUES ('h7', 'black') RETURNING *; +INSERT INTO hats VALUES ('h8', 'black') RETURNING *; + hat_name | hat_color +------------+------------ + h8 | black +(1 row) + +SELECT * FROM hat_data WHERE hat_name = 'h8'; + hat_name | hat_color +------------+------------ + h8 | black +(1 row) + +INSERT INTO hats VALUES ('h8', 'white') RETURNING *; + hat_name | hat_color +------------+------------ + h8 | white +(1 row) + +SELECT * FROM hat_data WHERE hat_name = 'h8'; + hat_name | hat_color +------------+------------ + h8 | white +(1 row) + +INSERT INTO hats VALUES ('h8', 'forbidden') RETURNING *; + hat_name | hat_color +----------+----------- +(0 rows) + +SELECT * FROM hat_data WHERE hat_name = 'h8'; hat_name | hat_color ------------+------------ - h7 | Orange + h8 | white (1 row) SELECT tablename, rulename, definition FROM pg_rules WHERE tablename = 'hats'; - tablename | rulename | definition ------------+------------+----------------------------------------------------------------------------------------------- - hats | hat_upsert | CREATE RULE hat_upsert AS + - | | ON INSERT TO hats DO INSTEAD INSERT INTO hat_data (hat_name, hat_color) + - | | VALUES (new.hat_name, new.hat_color) ON CONFLICT DO UPDATE SET hat_color = 'Orange'::bpchar+ - | | RETURNING hat_data.hat_name, + + tablename | rulename | definition +-----------+------------+------------------------------------------------------------------------------------------------------------------------------- + hats | hat_upsert | CREATE RULE hat_upsert AS + + | | ON INSERT TO hats DO INSTEAD INSERT INTO hat_data (hat_name, hat_color) + + | | VALUES (new.hat_name, new.hat_color) ON CONFLICT DO UPDATE SET hat_name = hat_data.hat_name, hat_color = excluded.hat_color+ + | | WHERE (excluded.hat_color <> 'forbidden'::bpchar) + + | | RETURNING hat_data.hat_name, + | | hat_data.hat_color; (1 row) +-- ensure explain works for on insert conflict rules +explain (costs off) INSERT INTO hats VALUES ('h8', 'forbidden') RETURNING *; + QUERY PLAN +---------------------------------------------------------------- + Insert on hat_data + Conflict Resolution: UPDATE + Conflict Arbiter Indexes: hat_data_pkey + Conflict Filter: (excluded.hat_color <> 'forbidden'::bpchar) + -> Result +(5 rows) + +-- ensure upserting into a rule, with a CTE (different offsets!) works +WITH data(hat_name, hat_color) AS ( + VALUES ('h8', 'green'), + ('h9', 'blue'), + ('h7', 'forbidden') +) +INSERT INTO hats + SELECT * FROM data +RETURNING *; + hat_name | hat_color +------------+------------ + h8 | green + h9 | blue +(2 rows) + +EXPLAIN (costs off) WITH data(hat_name, hat_color) AS ( + VALUES ('h8', 'green'), + ('h9', 'blue'), + ('h7', 'forbidden') +) +INSERT INTO hats + SELECT * FROM data +RETURNING *; + QUERY PLAN +---------------------------------------------------------------- + Insert on hat_data + Conflict Resolution: UPDATE + Conflict Arbiter Indexes: hat_data_pkey + Conflict Filter: (excluded.hat_color <> 'forbidden'::bpchar) + CTE data + -> Values Scan on "*VALUES*" + -> CTE Scan on data +(7 rows) + +SELECT * FROM hat_data WHERE hat_name IN ('h8', 'h9', 'h7') ORDER BY hat_name; + hat_name | hat_color +------------+------------ + h7 | black + h8 | green + h9 | blue +(3 rows) + DROP RULE hat_upsert ON hats; drop table hats; drop table hat_data; diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql index 6f1a1b84e7..1a81155bf1 100644 --- a/src/test/regress/sql/rules.sql +++ b/src/test/regress/sql/rules.sql @@ -1074,12 +1074,43 @@ CREATE RULE hat_upsert AS ON INSERT TO hats INSERT INTO hat_data VALUES ( NEW.hat_name, NEW.hat_color) - ON CONFLICT (hat_name) DO UPDATE SET hat_color = 'Orange' RETURNING *; + ON CONFLICT (hat_name) + DO UPDATE + SET hat_name = hat_data.hat_name, hat_color = excluded.hat_color + WHERE excluded.hat_color <> 'forbidden' + RETURNING *; -- Works (does upsert) -INSERT INTO hats VALUES ('h7', 'black') RETURNING *; +INSERT INTO hats VALUES ('h8', 'black') RETURNING *; +SELECT * FROM hat_data WHERE hat_name = 'h8'; +INSERT INTO hats VALUES ('h8', 'white') RETURNING *; +SELECT * FROM hat_data WHERE hat_name = 'h8'; +INSERT INTO hats VALUES ('h8', 'forbidden') RETURNING *; +SELECT * FROM hat_data WHERE hat_name = 'h8'; SELECT tablename, rulename, definition FROM pg_rules WHERE tablename = 'hats'; +-- ensure explain works for on insert conflict rules +explain (costs off) INSERT INTO hats VALUES ('h8', 'forbidden') RETURNING *; + +-- ensure upserting into a rule, with a CTE (different offsets!) works +WITH data(hat_name, hat_color) AS ( + VALUES ('h8', 'green'), + ('h9', 'blue'), + ('h7', 'forbidden') +) +INSERT INTO hats + SELECT * FROM data +RETURNING *; +EXPLAIN (costs off) WITH data(hat_name, hat_color) AS ( + VALUES ('h8', 'green'), + ('h9', 'blue'), + ('h7', 'forbidden') +) +INSERT INTO hats + SELECT * FROM data +RETURNING *; +SELECT * FROM hat_data WHERE hat_name IN ('h8', 'h9', 'h7') ORDER BY hat_name; + DROP RULE hat_upsert ON hats; drop table hats; -- 2.40.0