Subject: [PATCH 1/2] Fire per-statement triggers of partitioned tables

From: amit <[email-masked]>
Date: Mon, 24 Apr 2017 14:55:08 +0900

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

--------------D5B169309E6E9F67497E32A5
Content-Type: text/x-diff;
 name="0002-Fire-per-statement-triggers-of-inheritance-child-tab.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="0002-Fire-per-statement-triggers-of-inheritance-child-tab.pa";
 filename*1="tch"