diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 4339bbf9df..1063d92825 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -7022,6 +7022,63 @@ update bar set f2 = f2 + 100 returning *; 7 | 277 (6 rows) +-- 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,333,33),NEW: (3,433,33) +NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 +NOTICE: OLD: (4,344,44),NEW: (4,444,44) +NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 +NOTICE: OLD: (7,277,77),NEW: (7,377,77) +NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 +NOTICE: OLD: (3,333,33),NEW: (3,433,33) +NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 +NOTICE: OLD: (4,344,44),NEW: (4,444,44) +NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 +NOTICE: OLD: (7,277,77),NEW: (7,377,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: (7,377,77) +NOTICE: trig_row_after(23, skidoo) AFTER ROW DELETE ON bar2 +NOTICE: OLD: (7,377,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 ddfec7930d..09869578da 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -1656,6 +1656,24 @@ explain (verbose, costs off) update bar set f2 = f2 + 100 returning *; update bar set f2 = f2 + 100 returning *; +-- 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 a2f8137713..0ed3a47233 100644 --- a/doc/src/sgml/fdwhandler.sgml +++ b/doc/src/sgml/fdwhandler.sgml @@ -429,11 +429,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 2074fcca8e..3372b1ac2b 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 503b89f606..201c607002 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -1576,7 +1576,7 @@ ExecModifyTable(PlanState *pstate) JunkFilter *junkfilter; TupleTableSlot *slot; TupleTableSlot *planSlot; - ItemPointer tupleid = NULL; + ItemPointer tupleid; ItemPointerData tuple_ctid; HeapTupleData oldtupdata; HeapTuple oldtuple; @@ -1699,6 +1699,7 @@ ExecModifyTable(PlanState *pstate) 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 f6b8bbf5fa..ef2eaeac0a 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -1555,7 +1555,7 @@ grouping_planner(PlannerInfo *root, bool inheritance_update, 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; @@ -1685,13 +1685,7 @@ grouping_planner(PlannerInfo *root, bool inheritance_update, } /* Preprocess targetlist */ - tlist = preprocess_targetlist(root, tlist); - - if (parse->onConflict) - parse->onConflict->onConflictSet = - preprocess_onconflict_targetlist(parse->onConflict->onConflictSet, - parse->resultRelation, - parse->rtable); + tlist = preprocess_targetlist(root); /* * We are now done hacking up the query's targetlist. Most of the diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c index d7db32ebf5..3abea92335 100644 --- a/src/backend/optimizer/prep/preptlist.c +++ b/src/backend/optimizer/prep/preptlist.c @@ -4,20 +4,22 @@ * 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. * - * The rewriter's rewriteTargetListIU and rewriteTargetListUD routines - * also do preprocessing of the targetlist. 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 the - * rewriter 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 column list of - * the child, which might have more columns and/or a different column order - * than the parent. + * 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 @@ -47,11 +49,12 @@ #include "optimizer/var.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); /* @@ -59,36 +62,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 @@ -193,19 +221,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; } @@ -223,11 +253,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; @@ -238,12 +267,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++) @@ -386,8 +411,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 c2bc3ad4c5..e93552a8f3 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -72,8 +72,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); @@ -707,6 +705,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, @@ -1293,14 +1298,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) { @@ -1358,19 +1364,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) { @@ -1497,6 +1490,8 @@ ApplyRetrieveRule(Query *parsetree, parsetree->commandType == CMD_DELETE) { RangeTblEntry *newrte; + Var *var; + TargetEntry *tle; rte = rt_fetch(rt_index, parsetree->rtable); newrte = copyObject(rte); @@ -1527,6 +1522,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 @@ -2966,26 +2975,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. @@ -3347,11 +3336,10 @@ RewriteQuery(Query *parsetree, List *rewrite_events) parsetree->override, 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 80fbfd6ea9..804c9e3b8b 100644 --- a/src/include/optimizer/prep.h +++ b/src/include/optimizer/prep.h @@ -38,10 +38,7 @@ extern Expr *canonicalize_qual(Expr *qual); /* * 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 494fa29f10..86ae571eb1 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);