diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 89634b363f0..5e311f5956e 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -3592,6 +3592,71 @@ fetch from c; update bar set f2 = null where current of c; ERROR: WHERE CURRENT OF is not supported for this table type rollback; +-- Test that UPDATE/DELETE with inherited target works with row-level triggers +CREATE TRIGGER trig_row_before +BEFORE UPDATE OR DELETE ON bar2 +FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); +CREATE TRIGGER trig_row_after +AFTER UPDATE OR DELETE ON bar2 +FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); +explain (verbose, costs off) +update bar set f2 = f2 + 100; + QUERY PLAN +-------------------------------------------------------------------------------------- + Update on public.bar + Update on public.bar + Foreign Update on public.bar2 + Remote SQL: UPDATE public.loct2 SET f2 = $2 WHERE ctid = $1 RETURNING f1, f2, f3 + -> Seq Scan on public.bar + Output: bar.f1, (bar.f2 + 100), bar.ctid + -> Foreign Scan on public.bar2 + Output: bar2.f1, (bar2.f2 + 100), bar2.f3, bar2.ctid, bar2.* + Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR UPDATE +(9 rows) + +update bar set f2 = f2 + 100; +NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 +NOTICE: OLD: (3,233,33),NEW: (3,333,33) +NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 +NOTICE: OLD: (4,244,44),NEW: (4,344,44) +NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 +NOTICE: OLD: (7,177,77),NEW: (7,277,77) +NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 +NOTICE: OLD: (3,233,33),NEW: (3,333,33) +NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 +NOTICE: OLD: (4,244,44),NEW: (4,344,44) +NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 +NOTICE: OLD: (7,177,77),NEW: (7,277,77) +explain (verbose, costs off) +delete from bar where f2 < 400; + QUERY PLAN +--------------------------------------------------------------------------------------------- + Delete on public.bar + Delete on public.bar + Foreign Delete on public.bar2 + Remote SQL: DELETE FROM public.loct2 WHERE ctid = $1 RETURNING f1, f2, f3 + -> Seq Scan on public.bar + Output: bar.ctid + Filter: (bar.f2 < 400) + -> Foreign Scan on public.bar2 + Output: bar2.ctid, bar2.* + Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 WHERE ((f2 < 400)) FOR UPDATE +(10 rows) + +delete from bar where f2 < 400; +NOTICE: trig_row_before(23, skidoo) BEFORE ROW DELETE ON bar2 +NOTICE: OLD: (3,333,33) +NOTICE: trig_row_before(23, skidoo) BEFORE ROW DELETE ON bar2 +NOTICE: OLD: (4,344,44) +NOTICE: trig_row_before(23, skidoo) BEFORE ROW DELETE ON bar2 +NOTICE: OLD: (7,277,77) +NOTICE: trig_row_after(23, skidoo) AFTER ROW DELETE ON bar2 +NOTICE: OLD: (3,333,33) +NOTICE: trig_row_after(23, skidoo) AFTER ROW DELETE ON bar2 +NOTICE: OLD: (4,344,44) +NOTICE: trig_row_after(23, skidoo) AFTER ROW DELETE ON bar2 +NOTICE: OLD: (7,277,77) +-- cleanup drop table foo cascade; NOTICE: drop cascades to foreign table foo2 drop table bar cascade; diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 45ef1f29315..698be8e2917 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -825,6 +825,24 @@ fetch from c; update bar set f2 = null where current of c; rollback; +-- Test that UPDATE/DELETE with inherited target works with row-level triggers +CREATE TRIGGER trig_row_before +BEFORE UPDATE OR DELETE ON bar2 +FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + +CREATE TRIGGER trig_row_after +AFTER UPDATE OR DELETE ON bar2 +FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + +explain (verbose, costs off) +update bar set f2 = f2 + 100; +update bar set f2 = f2 + 100; + +explain (verbose, costs off) +delete from bar where f2 < 400; +delete from bar where f2 < 400; + +-- cleanup drop table foo cascade; drop table bar cascade; drop table loct1; diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml index 98fda71e106..6dae0bd79b3 100644 --- a/doc/src/sgml/fdwhandler.sgml +++ b/doc/src/sgml/fdwhandler.sgml @@ -382,11 +382,14 @@ AddForeignUpdateTargets (Query *parsetree, wholerow, or wholerowN, as the core system can generate junk columns of these names. + If the extra expressions are more complex than simple Vars, they + must be run through eval_const_expressions + before adding them to the targetlist. - This function is called in the rewriter, not the planner, so the - information available is a bit different from that available to the + Although this function is called during planning, the + information provided is a bit different from that available to other planning routines. parsetree is the parse tree for the UPDATE or DELETE command, while target_rte and diff --git a/doc/src/sgml/rules.sgml b/doc/src/sgml/rules.sgml index 1816ab59888..a55d28fe243 100644 --- a/doc/src/sgml/rules.sgml +++ b/doc/src/sgml/rules.sgml @@ -167,12 +167,12 @@ DELETE commands don't need a normal target list - because they don't produce any result. Instead, the rule system + because they don't produce any result. Instead, the planner adds a special CTID entry to the empty target list, to allow the executor to find the row to be deleted. (CTID is added when the result relation is an ordinary - table. If it is a view, a whole-row variable is added instead, - as described in .) + table. If it is a view, a whole-row variable is added instead, by + the rule system, as described in .) @@ -194,7 +194,7 @@ column = expression part of the command. The planner will handle missing columns by inserting expressions that copy the values from the old row into the new one. Just as for DELETE, - the rule system adds a CTID or whole-row variable so that + a CTID or whole-row variable is added so that the executor can identify the old row to be updated. diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 05b6e3337f8..ae2a147c62f 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -1310,7 +1310,7 @@ ExecModifyTable(ModifyTableState *node) JunkFilter *junkfilter; TupleTableSlot *slot; TupleTableSlot *planSlot; - ItemPointer tupleid = NULL; + ItemPointer tupleid; ItemPointerData tuple_ctid; HeapTupleData oldtupdata; HeapTuple oldtuple; @@ -1398,6 +1398,7 @@ ExecModifyTable(ModifyTableState *node) EvalPlanQualSetSlot(&node->mt_epqstate, planSlot); slot = planSlot; + tupleid = NULL; oldtuple = NULL; if (junkfilter != NULL) { diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index f25eb9b02da..716af49d465 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -1200,7 +1200,7 @@ inheritance_planner(PlannerInfo *root) /* although dummy, it must have a valid tlist for executor */ List *tlist; - tlist = preprocess_targetlist(root, parse->targetList); + tlist = preprocess_targetlist(root); return (Plan *) make_result(root, tlist, (Node *) list_make1(makeBoolConst(false, @@ -1262,7 +1262,7 @@ static Plan * grouping_planner(PlannerInfo *root, double tuple_fraction) { Query *parse = root->parse; - List *tlist = parse->targetList; + List *tlist; int64 offset_est = 0; int64 count_est = 0; double limit_tuples = -1.0; @@ -1327,7 +1327,7 @@ grouping_planner(PlannerInfo *root, double tuple_fraction) Assert(parse->commandType == CMD_SELECT); tlist = postprocess_setop_tlist(copyObject(result_plan->targetlist), - tlist); + parse->targetList); /* * Can't handle FOR [KEY] UPDATE/SHARE here (parser should have @@ -1364,7 +1364,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; @@ -1453,14 +1452,7 @@ grouping_planner(PlannerInfo *root, double tuple_fraction) numGroupCols = list_length(parse->groupClause); /* Preprocess targetlist */ - tlist = preprocess_targetlist(root, tlist); - - onconfl = parse->onConflict; - if (onconfl) - onconfl->onConflictSet = - preprocess_onconflict_targetlist(onconfl->onConflictSet, - parse->resultRelation, - parse->rtable); + tlist = preprocess_targetlist(root); /* * Expand any rangetable entries that have security barrier quals. diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c index 6b0c689e0c9..6b4c0a8d892 100644 --- a/src/backend/optimizer/prep/preptlist.c +++ b/src/backend/optimizer/prep/preptlist.c @@ -4,14 +4,29 @@ * Routines to preprocess the parse tree target list * * For INSERT and UPDATE queries, the targetlist must contain an entry for - * each attribute of the target relation in the correct order. For all query + * each attribute of the target relation in the correct order. For UPDATE and + * DELETE queries, it must also contain junk tlist entries needed to allow the + * executor to identify the rows to be updated or deleted. For all query * types, we may need to add junk tlist entries for Vars used in the RETURNING * list and row ID information needed for SELECT FOR UPDATE locking and/or * EvalPlanQual checking. * - * NOTE: the rewriter's rewriteTargetListIU and rewriteTargetListUD - * routines also do preprocessing of the targetlist. The division of labor - * between here and there is a bit arbitrary and historical. + * The query rewrite phase also does preprocessing of the targetlist (see + * rewriteTargetListIU). The division of labor between here and there is + * partially historical, but it's not entirely arbitrary. In particular, + * consider an UPDATE across an inheritance tree. What rewriteTargetListIU + * does need be done only once (because it depends only on the properties of + * the parent relation). What's done here has to be done over again for each + * child relation, because it depends on the properties of the child, which + * might be of a different relation type, or have more columns and/or a + * different column order than the parent. + * + * The fact that rewriteTargetListIU sorts non-resjunk tlist entries by column + * position, which expand_targetlist depends on, violates the above comment + * because the sorting is only valid for the parent relation. In inherited + * UPDATE cases, adjust_inherited_tlist runs in between to take care of fixing + * the tlists for child tables to keep expand_targetlist happy. We do it like + * that because it's faster in typical non-inherited cases. * * * Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group @@ -33,11 +48,12 @@ #include "optimizer/tlist.h" #include "parser/parsetree.h" #include "parser/parse_coerce.h" +#include "rewrite/rewriteHandler.h" #include "utils/rel.h" static List *expand_targetlist(List *tlist, int command_type, - Index result_relation, List *range_table); + Index result_relation, Relation rel); /* @@ -45,36 +61,61 @@ static List *expand_targetlist(List *tlist, int command_type, * Driver for preprocessing the parse tree targetlist. * * Returns the new targetlist. + * + * As a side effect, if there's an ON CONFLICT UPDATE clause, its targetlist + * is also preprocessed (and updated in-place). */ List * -preprocess_targetlist(PlannerInfo *root, List *tlist) +preprocess_targetlist(PlannerInfo *root) { Query *parse = root->parse; int result_relation = parse->resultRelation; List *range_table = parse->rtable; CmdType command_type = parse->commandType; + RangeTblEntry *target_rte = NULL; + Relation target_relation = NULL; + List *tlist; ListCell *lc; /* - * Sanity check: if there is a result relation, it'd better be a real - * relation not a subquery. Else parser or rewriter messed up. + * If there is a result relation, open it so we can look for missing + * columns and so on. We assume that previous code already acquired at + * least AccessShareLock on the relation, so we need no lock here. */ if (result_relation) { - RangeTblEntry *rte = rt_fetch(result_relation, range_table); + target_rte = rt_fetch(result_relation, range_table); - if (rte->subquery != NULL || rte->relid == InvalidOid) - elog(ERROR, "subquery cannot be result relation"); + /* + * Sanity check: it'd better be a real relation not, say, a subquery. + * Else parser or rewriter messed up. + */ + if (target_rte->rtekind != RTE_RELATION) + elog(ERROR, "result relation must be a regular relation"); + + target_relation = heap_open(target_rte->relid, NoLock); } + else + Assert(command_type == CMD_SELECT); + + /* + * For UPDATE/DELETE, add any junk column(s) needed to allow the executor + * to identify the rows to be updated or deleted. Note that this step + * scribbles on parse->targetList, which is not very desirable, but we + * keep it that way to avoid changing APIs used by FDWs. + */ + if (command_type == CMD_UPDATE || command_type == CMD_DELETE) + rewriteTargetListUD(parse, target_rte, target_relation); /* * for heap_form_tuple to work, the targetlist must match the exact order * of the attributes. We also need to fill in any missing attributes. -ay * 10/94 */ + tlist = parse->targetList; if (command_type == CMD_INSERT || command_type == CMD_UPDATE) tlist = expand_targetlist(tlist, command_type, - result_relation, range_table); + result_relation, target_relation); /* * Add necessary junk columns for rowmarked rels. These values are needed @@ -178,19 +219,21 @@ preprocess_targetlist(PlannerInfo *root, List *tlist) list_free(vars); } - return tlist; -} + /* + * If there's an ON CONFLICT UPDATE clause, preprocess its targetlist too + * while we have the relation open. + */ + if (parse->onConflict) + parse->onConflict->onConflictSet = + expand_targetlist(parse->onConflict->onConflictSet, + CMD_UPDATE, + result_relation, + target_relation); -/* - * preprocess_onconflict_targetlist - * Process ON CONFLICT SET targetlist. - * - * Returns the new targetlist. - */ -List * -preprocess_onconflict_targetlist(List *tlist, int result_relation, List *range_table) -{ - return expand_targetlist(tlist, CMD_UPDATE, result_relation, range_table); + if (target_relation) + heap_close(target_relation, NoLock); + + return tlist; } @@ -208,11 +251,10 @@ preprocess_onconflict_targetlist(List *tlist, int result_relation, List *range_t */ static List * expand_targetlist(List *tlist, int command_type, - Index result_relation, List *range_table) + Index result_relation, Relation rel) { List *new_tlist = NIL; ListCell *tlist_item; - Relation rel; int attrno, numattrs; @@ -223,12 +265,8 @@ expand_targetlist(List *tlist, int command_type, * order; but we have to insert TLEs for any missing attributes. * * Scan the tuple description in the relation's relcache entry to make - * sure we have all the user attributes in the right order. We assume - * that the rewriter already acquired at least AccessShareLock on the - * relation, so we need no lock here. + * sure we have all the user attributes in the right order. */ - rel = heap_open(getrelid(result_relation, range_table), NoLock); - numattrs = RelationGetNumberOfAttributes(rel); for (attrno = 1; attrno <= numattrs; attrno++) @@ -371,8 +409,6 @@ expand_targetlist(List *tlist, int command_type, tlist_item = lnext(tlist_item); } - heap_close(rel, NoLock); - return new_tlist; } diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index 6f10f7b2843..08899c1fcfc 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -63,8 +63,6 @@ static TargetEntry *process_matched_tle(TargetEntry *src_tle, static Node *get_assignment_input(Node *node); static void rewriteValuesRTE(RangeTblEntry *rte, Relation target_relation, List *attrnos); -static void rewriteTargetListUD(Query *parsetree, RangeTblEntry *target_rte, - Relation target_relation); static void markQueryForLocking(Query *qry, Node *jtnode, LockClauseStrength strength, LockWaitPolicy waitPolicy, bool pushedDown); @@ -685,6 +683,13 @@ adjustJoinTreeList(Query *parsetree, bool removert, int rt_index) * rewritten targetlist: an integer list of the assigned-to attnums, in * order of the original tlist's non-junk entries. This is needed for * processing VALUES RTEs. + * + * Note that for an inheritable UPDATE, this processing is only done once, + * using the parent relation as reference. It must not do anything that + * will not be correct when transposed to the child relation(s). (Step 4 + * is incorrect by this light, since child relations might have different + * colun ordering, but the planner will fix things by re-sorting the tlist + * for each child.) */ static List * rewriteTargetListIU(List *targetList, @@ -1230,14 +1235,15 @@ rewriteValuesRTE(RangeTblEntry *rte, Relation target_relation, List *attrnos) * This function adds a "junk" TLE that is needed to allow the executor to * find the original row for the update or delete. When the target relation * is a regular table, the junk TLE emits the ctid attribute of the original - * row. When the target relation is a view, there is no ctid, so we instead - * emit a whole-row Var that will contain the "old" values of the view row. - * If it's a foreign table, we let the FDW decide what to add. + * row. When the target relation is a foreign table, we let the FDW decide + * what to add. * - * For UPDATE queries, this is applied after rewriteTargetListIU. The - * ordering isn't actually critical at the moment. + * We used to do this during RewriteQuery(), but now that inheritance trees + * can contain a mix of regular and foreign tables, we must postpone it till + * planning, after the inheritance tree has been expanded. In that way we + * can do the right thing for each child table. */ -static void +void rewriteTargetListUD(Query *parsetree, RangeTblEntry *target_rte, Relation target_relation) { @@ -1294,19 +1300,6 @@ rewriteTargetListUD(Query *parsetree, RangeTblEntry *target_rte, attrname = "wholerow"; } } - else - { - /* - * Emit whole-row Var so that executor will have the "old" view row to - * pass to the INSTEAD OF trigger. - */ - var = makeWholeRowVar(target_rte, - parsetree->resultRelation, - 0, - false); - - attrname = "wholerow"; - } if (var != NULL) { @@ -1433,6 +1426,8 @@ ApplyRetrieveRule(Query *parsetree, parsetree->commandType == CMD_DELETE) { RangeTblEntry *newrte; + Var *var; + TargetEntry *tle; rte = rt_fetch(rt_index, parsetree->rtable); newrte = copyObject(rte); @@ -1463,6 +1458,20 @@ ApplyRetrieveRule(Query *parsetree, ChangeVarNodes((Node *) parsetree->returningList, rt_index, parsetree->resultRelation, 0); + /* + * To allow the executor to compute the original view row to pass + * to the INSTEAD OF trigger, we add a resjunk whole-row Var + * referencing the original RTE. This will later get expanded + * into a RowExpr computing all the OLD values of the view row. + */ + var = makeWholeRowVar(rte, rt_index, 0, false); + tle = makeTargetEntry((Expr *) var, + list_length(parsetree->targetList) + 1, + pstrdup("wholerow"), + true); + + parsetree->targetList = lappend(parsetree->targetList, tle); + /* Now, continue with expanding the original view RTE */ } else @@ -2901,26 +2910,6 @@ rewriteTargetView(Query *parsetree, Relation view) new_rte->securityQuals = view_rte->securityQuals; view_rte->securityQuals = NIL; - /* - * For UPDATE/DELETE, rewriteTargetListUD will have added a wholerow junk - * TLE for the view to the end of the targetlist, which we no longer need. - * Remove it to avoid unnecessary work when we process the targetlist. - * Note that when we recurse through rewriteQuery a new junk TLE will be - * added to allow the executor to find the proper row in the new target - * relation. (So, if we failed to do this, we might have multiple junk - * TLEs with the same name, which would be disastrous.) - */ - if (parsetree->commandType != CMD_INSERT) - { - TargetEntry *tle = (TargetEntry *) llast(parsetree->targetList); - - Assert(tle->resjunk); - Assert(IsA(tle->expr, Var) && - ((Var *) tle->expr)->varno == parsetree->resultRelation && - ((Var *) tle->expr)->varattno == 0); - parsetree->targetList = list_delete_ptr(parsetree->targetList, tle); - } - /* * Now update all Vars in the outer query that reference the view to * reference the appropriate column of the base relation instead. @@ -3271,11 +3260,10 @@ RewriteQuery(Query *parsetree, List *rewrite_events) rewriteTargetListIU(parsetree->targetList, parsetree->commandType, rt_entry_relation, parsetree->resultRelation, NULL); - rewriteTargetListUD(parsetree, rt_entry, rt_entry_relation); } else if (event == CMD_DELETE) { - rewriteTargetListUD(parsetree, rt_entry, rt_entry_relation); + /* Nothing to do here */ } else elog(ERROR, "unrecognized commandType: %d", (int) event); diff --git a/src/include/optimizer/prep.h b/src/include/optimizer/prep.h index 7b8c0a98f31..46fadff5360 100644 --- a/src/include/optimizer/prep.h +++ b/src/include/optimizer/prep.h @@ -43,10 +43,7 @@ extern void expand_security_quals(PlannerInfo *root, List *tlist); /* * prototypes for preptlist.c */ -extern List *preprocess_targetlist(PlannerInfo *root, List *tlist); - -extern List *preprocess_onconflict_targetlist(List *tlist, - int result_relation, List *range_table); +extern List *preprocess_targetlist(PlannerInfo *root); extern PlanRowMark *get_plan_rowmark(List *rowmarks, Index rtindex); diff --git a/src/include/rewrite/rewriteHandler.h b/src/include/rewrite/rewriteHandler.h index 1ee8fb04b43..c9a2d25f90e 100644 --- a/src/include/rewrite/rewriteHandler.h +++ b/src/include/rewrite/rewriteHandler.h @@ -23,6 +23,9 @@ extern void AcquireRewriteLocks(Query *parsetree, bool forUpdatePushedDown); extern Node *build_column_default(Relation rel, int attrno); +extern void rewriteTargetListUD(Query *parsetree, RangeTblEntry *target_rte, + Relation target_relation); + extern Query *get_view_query(Relation view); extern const char *view_query_is_auto_updatable(Query *viewquery, bool check_cols);