From 9c7543615ccb05c004591a9176f818413df7ea9d Mon Sep 17 00:00:00 2001 From: amit 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 --- 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 ++++++++++++++++++++++++ 11 files changed, 210 insertions(+), 12 deletions(-) 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