On 2017/04/27 1:52, Robert Haas wrote: > On Tue, Apr 25, 2017 at 10:34 PM, Amit Langote > <langote_amit...@lab.ntt.co.jp> wrote: >> FWIW, I too prefer the latter, that is, fire only the parent's triggers. >> In that case, applying only the patch 0001 will do. > > Do we need to update the documentation?
Yes, I think we should. How about as in the attached? By the way, code changes I made in the attached are such that a subsequent patch could implement firing statement-level triggers of all the tables in a partition hierarchy, which it seems we don't want to do. Should then the code be changed to not create ResultRelInfos of all the tables but only the root table (the one mentioned in the command)? You will see that the patch adds fields named es_nonleaf_result_relations and es_num_nonleaf_result_relations, whereas just es_root_result_relation would perhaps do, for example. Thanks, Amit
>From c8b785a77a2a193906967762066e03e09de80442 Mon Sep 17 00:00:00 2001 From: amit <amitlangot...@gmail.com> Date: Mon, 24 Apr 2017 14:55:08 +0900 Subject: [PATCH 1/2] Fire per-statement triggers of partitioned tables Current implementation completely misses them, because it assumes that no ResultRelInfos need to be created for partitioned tables, whereas they *are* needed to fire the per-statment triggers, which we *do* allow to be defined on the partitioned tables. Reported By: Rajkumar Raghuwanshi Patch by: Amit Langote Report: https://www.postgresql.org/message-id/CAKcux6%3DwYospCRY2J4XEFuVy0L41S%3Dfic7rmkbsU-GXhhSbmBg%40mail.gmail.com --- doc/src/sgml/trigger.sgml | 26 ++++++++----- src/backend/executor/execMain.c | 33 ++++++++++++++++- src/backend/executor/nodeModifyTable.c | 66 ++++++++++++++++++++++++++++----- src/backend/nodes/copyfuncs.c | 1 + src/backend/nodes/outfuncs.c | 1 + src/backend/nodes/readfuncs.c | 1 + src/backend/optimizer/plan/createplan.c | 1 + src/backend/optimizer/plan/setrefs.c | 4 ++ src/include/nodes/execnodes.h | 11 ++++++ src/include/nodes/plannodes.h | 2 + src/test/regress/expected/triggers.out | 54 +++++++++++++++++++++++++++ src/test/regress/sql/triggers.sql | 48 ++++++++++++++++++++++++ 12 files changed, 227 insertions(+), 21 deletions(-) diff --git a/doc/src/sgml/trigger.sgml b/doc/src/sgml/trigger.sgml index 2a718d7f47..5771bd5fdc 100644 --- a/doc/src/sgml/trigger.sgml +++ b/doc/src/sgml/trigger.sgml @@ -33,7 +33,8 @@ <para> A trigger is a specification that the database should automatically execute a particular function whenever a certain type of operation is - performed. Triggers can be attached to tables, views, and foreign tables. + performed. Triggers can be attached to tables (partitioned or not), + views, and foreign tables. </para> <para> @@ -111,14 +112,21 @@ Statement-level <literal>BEFORE</> triggers naturally fire before the statement starts to do anything, while statement-level <literal>AFTER</> triggers fire at the very end of the statement. These types of - triggers may be defined on tables or views. Row-level <literal>BEFORE</> - triggers fire immediately before a particular row is operated on, - while row-level <literal>AFTER</> triggers fire at the end of the - statement (but before any statement-level <literal>AFTER</> triggers). - These types of triggers may only be defined on tables and foreign tables. - Row-level <literal>INSTEAD OF</> triggers may only be defined on views, - and fire immediately as each row in the view is identified as needing to - be operated on. + triggers may be defined on tables, views, or foreign tables. Row-level + <literal>BEFORE</> triggers fire immediately before a particular row is + operated on, while row-level <literal>AFTER</> triggers fire at the end of + the statement (but before any statement-level <literal>AFTER</> triggers). + These types of triggers may only be defined on non-partitioned tables and + foreign tables. Row-level <literal>INSTEAD OF</> triggers may only be + defined on views, and fire immediately as each row in the view is + identified as needing to be operated on. + </para> + + <para> + A statement-level trigger defined on partitioned tables is fired only + once for the table itself, not once for every table in the partitioning + hierarchy. However, row-level triggers of any affected leaf partitions + will be fired. </para> <para> diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 5c12fb457d..586b396b3e 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -861,18 +861,35 @@ InitPlan(QueryDesc *queryDesc, int eflags) /* * In the partitioned result relation case, lock the non-leaf result - * relations too. We don't however need ResultRelInfos for them. + * relations too. We also need ResultRelInfos so that per-statement + * triggers defined on these relations can be fired. */ if (plannedstmt->nonleafResultRelations) { + numResultRelations = + list_length(plannedstmt->nonleafResultRelations); + + resultRelInfos = (ResultRelInfo *) + palloc(numResultRelations * sizeof(ResultRelInfo)); + resultRelInfo = resultRelInfos; foreach(l, plannedstmt->nonleafResultRelations) { Index resultRelationIndex = lfirst_int(l); Oid resultRelationOid; + Relation resultRelation; resultRelationOid = getrelid(resultRelationIndex, rangeTable); - LockRelationOid(resultRelationOid, RowExclusiveLock); + resultRelation = heap_open(resultRelationOid, RowExclusiveLock); + InitResultRelInfo(resultRelInfo, + resultRelation, + resultRelationIndex, + NULL, + estate->es_instrument); + resultRelInfo++; } + + estate->es_nonleaf_result_relations = resultRelInfos; + estate->es_num_nonleaf_result_relations = numResultRelations; } } else @@ -883,6 +900,8 @@ InitPlan(QueryDesc *queryDesc, int eflags) estate->es_result_relations = NULL; estate->es_num_result_relations = 0; estate->es_result_relation_info = NULL; + estate->es_nonleaf_result_relations = NULL; + estate->es_num_nonleaf_result_relations = 0; } /* @@ -1566,6 +1585,16 @@ ExecEndPlan(PlanState *planstate, EState *estate) } /* + * close any non-leaf target relations + */ + resultRelInfo = estate->es_nonleaf_result_relations; + for (i = estate->es_num_nonleaf_result_relations; i > 0; i--) + { + heap_close(resultRelInfo->ri_RelationDesc, NoLock); + resultRelInfo++; + } + + /* * likewise close any trigger target relations */ foreach(l, estate->es_trig_target_relations) diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 71e3b8ec2d..6cab15646f 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -62,6 +62,8 @@ static bool ExecOnConflictUpdate(ModifyTableState *mtstate, EState *estate, bool canSetTag, TupleTableSlot **returning); +static void fireBSTriggersFor(ModifyTableState *node, ResultRelInfo *resultRelInfo); +static void fireASTriggersFor(ModifyTableState *node, ResultRelInfo *resultRelInfo); /* * Verify that the tuples to be produced by INSERT or UPDATE match the @@ -1328,19 +1330,39 @@ ExecOnConflictUpdate(ModifyTableState *mtstate, static void fireBSTriggers(ModifyTableState *node) { + /* Fire for the root partitioned table, if any, and return. */ + if (node->nonleafResultRelInfo) + { + fireBSTriggersFor(node, node->nonleafResultRelInfo); + return; + } + + /* + * Fire for the main result relation. In the partitioned table case, + * the following happens to be the first leaf partition, which we don't + * need to fire triggers for. + */ + fireBSTriggersFor(node, node->resultRelInfo); +} + +/* + * Process BEFORE EACH STATEMENT triggers for one result relation + */ +static void +fireBSTriggersFor(ModifyTableState *node, ResultRelInfo *resultRelInfo) +{ switch (node->operation) { case CMD_INSERT: - ExecBSInsertTriggers(node->ps.state, node->resultRelInfo); + ExecBSInsertTriggers(node->ps.state, resultRelInfo); if (node->mt_onconflict == ONCONFLICT_UPDATE) - ExecBSUpdateTriggers(node->ps.state, - node->resultRelInfo); + ExecBSUpdateTriggers(node->ps.state, resultRelInfo); break; case CMD_UPDATE: - ExecBSUpdateTriggers(node->ps.state, node->resultRelInfo); + ExecBSUpdateTriggers(node->ps.state, resultRelInfo); break; case CMD_DELETE: - ExecBSDeleteTriggers(node->ps.state, node->resultRelInfo); + ExecBSDeleteTriggers(node->ps.state, resultRelInfo); break; default: elog(ERROR, "unknown operation"); @@ -1354,19 +1376,39 @@ fireBSTriggers(ModifyTableState *node) static void fireASTriggers(ModifyTableState *node) { + /* Fire for the root partitioned table, if any, and return. */ + if (node->nonleafResultRelInfo) + { + fireASTriggersFor(node, node->nonleafResultRelInfo); + return; + } + + /* + * Fire for the main result relation. In the partitioned table case, + * the following happens to be the first leaf partition, which we don't + * need to fire triggers for. + */ + fireASTriggersFor(node, node->resultRelInfo); +} + +/* + * Process AFTER EACH STATEMENT triggers for one result relation + */ +static void +fireASTriggersFor(ModifyTableState *node, ResultRelInfo *resultRelInfo) +{ switch (node->operation) { case CMD_INSERT: if (node->mt_onconflict == ONCONFLICT_UPDATE) - ExecASUpdateTriggers(node->ps.state, - node->resultRelInfo); - ExecASInsertTriggers(node->ps.state, node->resultRelInfo); + ExecASUpdateTriggers(node->ps.state, resultRelInfo); + ExecASInsertTriggers(node->ps.state, resultRelInfo); break; case CMD_UPDATE: - ExecASUpdateTriggers(node->ps.state, node->resultRelInfo); + ExecASUpdateTriggers(node->ps.state, resultRelInfo); break; case CMD_DELETE: - ExecASDeleteTriggers(node->ps.state, node->resultRelInfo); + ExecASDeleteTriggers(node->ps.state, resultRelInfo); break; default: elog(ERROR, "unknown operation"); @@ -1628,6 +1670,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) ModifyTableState *mtstate; CmdType operation = node->operation; int nplans = list_length(node->plans); + int num_nonleafrels = list_length(node->partitioned_rels); ResultRelInfo *saved_resultRelInfo; ResultRelInfo *resultRelInfo; TupleDesc tupDesc; @@ -1652,6 +1695,9 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) mtstate->mt_plans = (PlanState **) palloc0(sizeof(PlanState *) * nplans); mtstate->resultRelInfo = estate->es_result_relations + node->resultRelIndex; + mtstate->mt_numnonleafrels = num_nonleafrels; + mtstate->nonleafResultRelInfo = estate->es_nonleaf_result_relations + + node->nonleafResultRelIndex; mtstate->mt_arowmarks = (List **) palloc0(sizeof(List *) * nplans); mtstate->mt_nplans = nplans; mtstate->mt_onconflict = node->onConflictAction; diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 00a0fed23d..e425a57feb 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -205,6 +205,7 @@ _copyModifyTable(const ModifyTable *from) COPY_NODE_FIELD(partitioned_rels); COPY_NODE_FIELD(resultRelations); COPY_SCALAR_FIELD(resultRelIndex); + COPY_SCALAR_FIELD(nonleafResultRelIndex); COPY_NODE_FIELD(plans); COPY_NODE_FIELD(withCheckOptionLists); COPY_NODE_FIELD(returningLists); diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index 28cef85579..a501f250ba 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -350,6 +350,7 @@ _outModifyTable(StringInfo str, const ModifyTable *node) WRITE_NODE_FIELD(partitioned_rels); WRITE_NODE_FIELD(resultRelations); WRITE_INT_FIELD(resultRelIndex); + WRITE_INT_FIELD(nonleafResultRelIndex); WRITE_NODE_FIELD(plans); WRITE_NODE_FIELD(withCheckOptionLists); WRITE_NODE_FIELD(returningLists); diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c index a883220a49..aa6f54870c 100644 --- a/src/backend/nodes/readfuncs.c +++ b/src/backend/nodes/readfuncs.c @@ -1548,6 +1548,7 @@ _readModifyTable(void) READ_NODE_FIELD(partitioned_rels); READ_NODE_FIELD(resultRelations); READ_INT_FIELD(resultRelIndex); + READ_INT_FIELD(nonleafResultRelIndex); READ_NODE_FIELD(plans); READ_NODE_FIELD(withCheckOptionLists); READ_NODE_FIELD(returningLists); diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index 95e6eb7d28..de2c77a22c 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -6437,6 +6437,7 @@ make_modifytable(PlannerInfo *root, node->partitioned_rels = partitioned_rels; node->resultRelations = resultRelations; node->resultRelIndex = -1; /* will be set correctly in setrefs.c */ + node->nonleafResultRelIndex = -1; /* will be set correctly in setrefs.c */ node->plans = subplans; if (!onconflict) { diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c index 1278371b65..27a145187e 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -883,7 +883,11 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset) * If the main target relation is a partitioned table, the * following list contains the RT indexes of partitioned child * relations, which are not included in the above list. + * Set nonleafResultRelIndex to reflect the starting position + * in the global list. */ + splan->nonleafResultRelIndex = + list_length(root->glob->nonleafResultRelations); root->glob->nonleafResultRelations = list_concat(root->glob->nonleafResultRelations, list_copy(splan->partitioned_rels)); diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index 4330a851c3..3f801b9b0c 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -422,6 +422,14 @@ typedef struct EState int es_num_result_relations; /* length of array */ ResultRelInfo *es_result_relation_info; /* currently active array elt */ + /* + * Info about non-leaf target tables during update/delete on partitioned + * tables. They are required only to fire the per-statement triggers + * defined on the non-leaf tables in a partitioned table hierarchy. + */ + ResultRelInfo *es_nonleaf_result_relations; /* array of ResultRelInfos */ + int es_num_nonleaf_result_relations; /* length of array */ + /* Stuff used for firing triggers: */ List *es_trig_target_relations; /* trigger-only ResultRelInfos */ TupleTableSlot *es_trig_tuple_slot; /* for trigger output tuples */ @@ -914,6 +922,9 @@ typedef struct ModifyTableState int mt_nplans; /* number of plans in the array */ int mt_whichplan; /* which one is being executed (0..n-1) */ ResultRelInfo *resultRelInfo; /* per-subplan target relations */ + int mt_numnonleafrels; /* number of non-leaf result rels in the + * below array */ + ResultRelInfo *nonleafResultRelInfo; /* non-leaf target relations */ List **mt_arowmarks; /* per-subplan ExecAuxRowMark lists */ EPQState mt_epqstate; /* for evaluating EvalPlanQual rechecks */ bool fireBSTriggers; /* do we need to fire stmt triggers? */ diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h index cba915572e..474343d340 100644 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -211,6 +211,8 @@ typedef struct ModifyTable List *partitioned_rels; List *resultRelations; /* integer list of RT indexes */ int resultRelIndex; /* index of first resultRel in plan's list */ + int nonleafResultRelIndex; /* index of first nonleaf resultRel in + * plan's list */ List *plans; /* plan(s) producing source data */ List *withCheckOptionLists; /* per-target-table WCO lists */ List *returningLists; /* per-target-table RETURNING tlists */ diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out index 4b0b3b7c42..ec0e0c2782 100644 --- a/src/test/regress/expected/triggers.out +++ b/src/test/regress/expected/triggers.out @@ -1787,3 +1787,57 @@ create trigger my_trigger after update on my_table_42 referencing old table as o drop trigger my_trigger on my_table_42; drop table my_table_42; drop table my_table; +-- +-- Verify that per-statement triggers are fired for partitioned tables +-- +create table parted_stmt_trig (a int) partition by list (a); +create table parted_stmt_trig1 partition of parted_stmt_trig for values in (1); +create table parted_stmt_trig2 partition of parted_stmt_trig for values in (2); +create or replace function trigger_notice() returns trigger as $$ + begin raise notice 'trigger on % % %', TG_TABLE_NAME, TG_WHEN, TG_OP; return null; end; + $$ language plpgsql; +-- insert/update/delete triggers on the parent +create trigger trig_ins_before before insert on parted_stmt_trig + for each statement execute procedure trigger_notice(); +create trigger trig_ins_after after insert on parted_stmt_trig + for each statement execute procedure trigger_notice(); +create trigger trig_upd_before before update on parted_stmt_trig + for each statement execute procedure trigger_notice(); +create trigger trig_upd_after after update on parted_stmt_trig + for each statement execute procedure trigger_notice(); +create trigger trig_del_before before delete on parted_stmt_trig + for each statement execute procedure trigger_notice(); +create trigger trig_del_after after delete on parted_stmt_trig + for each statement execute procedure trigger_notice(); +-- insert/update/delete triggers on the first partition +create trigger trig_ins_before before insert on parted_stmt_trig1 + for each statement execute procedure trigger_notice(); +create trigger trig_ins_after after insert on parted_stmt_trig1 + for each statement execute procedure trigger_notice(); +create trigger trig_upd_before before update on parted_stmt_trig1 + for each statement execute procedure trigger_notice(); +create trigger trig_upd_after after update on parted_stmt_trig1 + for each statement execute procedure trigger_notice(); +create trigger trig_del_before before delete on parted_stmt_trig1 + for each statement execute procedure trigger_notice(); +create trigger trig_del_after after delete on parted_stmt_trig1 + for each statement execute procedure trigger_notice(); +with ins (partname, a) as ( + insert into parted_stmt_trig values (1) returning tableoid::regclass, a +) insert into parted_stmt_trig select a+1 from ins returning tableoid::regclass, a; +NOTICE: trigger on parted_stmt_trig BEFORE INSERT +NOTICE: trigger on parted_stmt_trig BEFORE INSERT +NOTICE: trigger on parted_stmt_trig AFTER INSERT +NOTICE: trigger on parted_stmt_trig AFTER INSERT + tableoid | a +-------------------+--- + parted_stmt_trig2 | 2 +(1 row) + +update parted_stmt_trig set a = a; +NOTICE: trigger on parted_stmt_trig BEFORE UPDATE +NOTICE: trigger on parted_stmt_trig AFTER UPDATE +delete from parted_stmt_trig; +NOTICE: trigger on parted_stmt_trig BEFORE DELETE +NOTICE: trigger on parted_stmt_trig AFTER DELETE +drop table parted_stmt_trig; diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql index 4473ce0518..a767098465 100644 --- a/src/test/regress/sql/triggers.sql +++ b/src/test/regress/sql/triggers.sql @@ -1263,3 +1263,51 @@ create trigger my_trigger after update on my_table_42 referencing old table as o drop trigger my_trigger on my_table_42; drop table my_table_42; drop table my_table; + +-- +-- Verify that per-statement triggers are fired for partitioned tables +-- +create table parted_stmt_trig (a int) partition by list (a); +create table parted_stmt_trig1 partition of parted_stmt_trig for values in (1); +create table parted_stmt_trig2 partition of parted_stmt_trig for values in (2); + +create or replace function trigger_notice() returns trigger as $$ + begin raise notice 'trigger on % % %', TG_TABLE_NAME, TG_WHEN, TG_OP; return null; end; + $$ language plpgsql; + +-- insert/update/delete triggers on the parent +create trigger trig_ins_before before insert on parted_stmt_trig + for each statement execute procedure trigger_notice(); +create trigger trig_ins_after after insert on parted_stmt_trig + for each statement execute procedure trigger_notice(); +create trigger trig_upd_before before update on parted_stmt_trig + for each statement execute procedure trigger_notice(); +create trigger trig_upd_after after update on parted_stmt_trig + for each statement execute procedure trigger_notice(); +create trigger trig_del_before before delete on parted_stmt_trig + for each statement execute procedure trigger_notice(); +create trigger trig_del_after after delete on parted_stmt_trig + for each statement execute procedure trigger_notice(); + +-- insert/update/delete triggers on the first partition +create trigger trig_ins_before before insert on parted_stmt_trig1 + for each statement execute procedure trigger_notice(); +create trigger trig_ins_after after insert on parted_stmt_trig1 + for each statement execute procedure trigger_notice(); +create trigger trig_upd_before before update on parted_stmt_trig1 + for each statement execute procedure trigger_notice(); +create trigger trig_upd_after after update on parted_stmt_trig1 + for each statement execute procedure trigger_notice(); +create trigger trig_del_before before delete on parted_stmt_trig1 + for each statement execute procedure trigger_notice(); +create trigger trig_del_after after delete on parted_stmt_trig1 + for each statement execute procedure trigger_notice(); + +with ins (partname, a) as ( + insert into parted_stmt_trig values (1) returning tableoid::regclass, a +) insert into parted_stmt_trig select a+1 from ins returning tableoid::regclass, a; + +update parted_stmt_trig set a = a; +delete from parted_stmt_trig; + +drop table parted_stmt_trig; -- 2.11.0
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
