Date: Mon, 20 Apr 2020 19:04:06 -0400
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Amit Langote <amitlangote09@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables
Message-ID: <20200420230406.GA27302@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="FCuugMFkClbJLl1L"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200420195740.GE3890@telsasoft.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk


--FCuugMFkClbJLl1L
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

> +			deleteDependencyRecordsFor(TriggerRelationId,
> +					pg_trigger->oid,
> +					false);
> +			deleteDependencyRecordsFor(RelationRelationId,
> +					pg_trigger->oid,
> +					false);
> +
> +			CommandCounterIncrement();
> +			ObjectAddressSet(object, TriggerRelationId, pg_trigger->oid);
> +			performDeletion(&object, DROP_RESTRICT, PERFORM_DELETION_INTERNAL);
> +		}
> +
> +		systable_endscan(scan);
> +		table_close(tgrel, RowExclusiveLock);
> +	}

Two small issues here.  First, your second call to
deleteDependencyRecordsFor did nothing, because your first call deleted
all the dependency records.  I changed that to two
deleteDependencyRecordsForClass() calls, that actually do what you
intended.

The other is that instead of deleting each trigger, we can accumulate
them to delete with a single performMultipleDeletions call; this also
means we get to do CommandCounterIncrement just once.

v6 fixes those things and AFAICS is ready to push.

I haven't reviewed your 0002 carefully, but (as inventor of the "TABLE
t" marker for FK constraints) I agree with Amit that we should imitate
that instead of coming up with a new way to show it.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--FCuugMFkClbJLl1L
Content-Type: text/x-diff; charset=iso-8859-1
Content-Disposition: attachment;
 filename="v6-0001-Fix-detaching-tables-with-inherited-row-triggers.patch"
Content-Transfer-Encoding: 8bit

From b8aeae162e03ccd0346212e19ae2d75ec6495288 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Mon, 20 Apr 2020 18:39:59 -0400
Subject: [PATCH v6] Fix detaching tables with inherited row triggers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The old behavior is buggy, and the intended behavior was not previously
documented.  So define the behavior that the trigger is removed if the table is
detached.  It is an error if a table being attached already has a trigger of
the same.  This differs from the behavior for indexes and constraints.

Should backpatch to v11 with s/OidIsValid(tgparentid)/isPartitionTrigger/.

See also: 86f575948c77

Author: Justin Pryzby <pryzby@telsasoft.com>
Reviewed-by: Amit Langote <amitlangote09@gmail.com>
Reviewed-by: Álvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://www.postgresql.org/message-id/flat/20200408152412.GZ2228%40telsasoft.com
---
 doc/src/sgml/ref/create_trigger.sgml   |  1 +
 src/backend/commands/tablecmds.c       | 66 ++++++++++++++++++++++++++
 src/test/regress/expected/triggers.out | 45 ++++++++++++++++++
 src/test/regress/sql/triggers.sql      | 21 ++++++++
 4 files changed, 133 insertions(+)

diff --git a/doc/src/sgml/ref/create_trigger.sgml b/doc/src/sgml/ref/create_trigger.sgml
index bde3a63f90..303881fcfb 100644
--- a/doc/src/sgml/ref/create_trigger.sgml
+++ b/doc/src/sgml/ref/create_trigger.sgml
@@ -526,6 +526,7 @@ UPDATE OF <replaceable>column_name1</replaceable> [, <replaceable>column_name2</
    Creating a row-level trigger on a partitioned table will cause identical
    triggers to be created in all its existing partitions; and any partitions
    created or attached later will contain an identical trigger, too.
+   If the partition is detached from its parent, the trigger is removed.
    Triggers on partitioned tables may not be <literal>INSTEAD OF</literal>.
   </para>
 
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 037d457c3d..3ebbd5d013 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -547,6 +547,7 @@ static void QueuePartitionConstraintValidation(List **wqueue, Relation scanrel,
 											   List *partConstraint,
 											   bool validate_default);
 static void CloneRowTriggersToPartition(Relation parent, Relation partition);
+static void DropClonedTriggersFromPartition(Oid partitionId);
 static ObjectAddress ATExecDetachPartition(Relation rel, RangeVar *name);
 static ObjectAddress ATExecAttachPartitionIdx(List **wqueue, Relation rel,
 											  RangeVar *name);
@@ -16797,6 +16798,9 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
 	}
 	table_close(classRel, RowExclusiveLock);
 
+	/* Drop any triggers that were cloned on creation/attach. */
+	DropClonedTriggersFromPartition(RelationGetRelid(partRel));
+
 	/*
 	 * Detach any foreign keys that are inherited.  This includes creating
 	 * additional action triggers.
@@ -16881,6 +16885,68 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
 	return address;
 }
 
+/*
+ * DropClonedTriggersFromPartition
+ *		subroutine for ATExecDetachPartition to remove any triggers that were
+ *		cloned to the partition when it was created-as-partition or attached.
+ *		This undoes what CloneRowTriggersToPartition did.
+ */
+static void
+DropClonedTriggersFromPartition(Oid partitionId)
+{
+	ScanKeyData skey;
+	SysScanDesc	scan;
+	HeapTuple	trigtup;
+	Relation	tgrel;
+	ObjectAddresses *objects;
+
+	objects = new_object_addresses();
+
+	/*
+	 * Scan pg_trigger to search for all triggers on this rel.
+	 */
+	ScanKeyInit(&skey, Anum_pg_trigger_tgrelid, BTEqualStrategyNumber,
+				F_OIDEQ, ObjectIdGetDatum(partitionId));
+	tgrel = table_open(TriggerRelationId, RowExclusiveLock);
+	scan = systable_beginscan(tgrel, TriggerRelidNameIndexId,
+							  true, NULL, 1, &skey);
+	while (HeapTupleIsValid(trigtup = systable_getnext(scan)))
+	{
+		Form_pg_trigger pg_trigger = (Form_pg_trigger) GETSTRUCT(trigtup);
+		ObjectAddress trig;
+
+		/* Ignore triggers that weren't cloned */
+		if (!OidIsValid(pg_trigger->tgparentid) ||
+			!pg_trigger->tgisinternal ||
+			!TRIGGER_FOR_ROW(pg_trigger->tgtype))
+			continue;
+
+		/*
+		 * This is ugly, but necessary: remove the dependency markings on the
+		 * trigger so that it can be removed.
+		 */
+		deleteDependencyRecordsForClass(TriggerRelationId, pg_trigger->oid,
+										TriggerRelationId,
+										DEPENDENCY_PARTITION_PRI);
+		deleteDependencyRecordsForClass(TriggerRelationId, pg_trigger->oid,
+										RelationRelationId,
+										DEPENDENCY_PARTITION_SEC);
+
+		/* remember this trigger to remove it below */
+		ObjectAddressSet(trig, TriggerRelationId, pg_trigger->oid);
+		add_exact_object_address(&trig, objects);
+	}
+
+	/* make the dependency removal visible to the deletion below */
+	CommandCounterIncrement();
+	performMultipleDeletions(objects, DROP_RESTRICT, PERFORM_DELETION_INTERNAL);
+
+	/* done */
+	free_object_addresses(objects);
+	systable_endscan(scan);
+	table_close(tgrel, RowExclusiveLock);
+}
+
 /*
  * Before acquiring lock on an index, acquire the same lock on the owning
  * table.
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index e9da4ef983..c1482e185b 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2023,6 +2023,51 @@ select tgrelid::regclass, tgname, tgfoid::regproc from pg_trigger
 ---------+--------+--------
 (0 rows)
 
+-- check detach behavior
+create trigger trg1 after insert on trigpart for each row execute procedure trigger_nothing();
+\d trigpart3
+             Table "public.trigpart3"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+Partition of: trigpart FOR VALUES FROM (2000) TO (3000)
+Triggers:
+    trg1 AFTER INSERT ON trigpart3 FOR EACH ROW EXECUTE FUNCTION trigger_nothing()
+
+alter table trigpart detach partition trigpart3;
+drop trigger trg1 on trigpart3; -- fail due to "does not exist"
+ERROR:  trigger "trg1" for table "trigpart3" does not exist
+alter table trigpart detach partition trigpart4;
+drop trigger trg1 on trigpart41; -- fail due to "does not exist"
+ERROR:  trigger "trg1" for table "trigpart41" does not exist
+drop table trigpart4;
+alter table trigpart attach partition trigpart3 for values from (2000) to (3000);
+alter table trigpart detach partition trigpart3;
+alter table trigpart attach partition trigpart3 for values from (2000) to (3000);
+drop table trigpart3;
+select tgrelid::regclass::text, tgname, tgfoid::regproc, tgenabled, tgisinternal from pg_trigger
+  where tgname ~ '^trg1' order by 1;
+  tgrelid  | tgname |     tgfoid      | tgenabled | tgisinternal 
+-----------+--------+-----------------+-----------+--------------
+ trigpart  | trg1   | trigger_nothing | O         | f
+ trigpart1 | trg1   | trigger_nothing | O         | t
+(2 rows)
+
+create table trigpart3 (like trigpart);
+create trigger trg1 after insert on trigpart3 for each row execute procedure trigger_nothing();
+\d trigpart3
+             Table "public.trigpart3"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+Triggers:
+    trg1 AFTER INSERT ON trigpart3 FOR EACH ROW EXECUTE FUNCTION trigger_nothing()
+
+alter table trigpart attach partition trigpart3 FOR VALUES FROM (2000) to (3000); -- fail
+ERROR:  trigger "trg1" for relation "trigpart3" already exists
+drop table trigpart3;
 drop table trigpart;
 drop function trigger_nothing();
 --
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 80ffbb4b02..e228d0a8a5 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1380,6 +1380,27 @@ drop trigger trg1 on trigpart;		-- ok, all gone
 select tgrelid::regclass, tgname, tgfoid::regproc from pg_trigger
   where tgrelid::regclass::text like 'trigpart%' order by tgrelid::regclass::text;
 
+-- check detach behavior
+create trigger trg1 after insert on trigpart for each row execute procedure trigger_nothing();
+\d trigpart3
+alter table trigpart detach partition trigpart3;
+drop trigger trg1 on trigpart3; -- fail due to "does not exist"
+alter table trigpart detach partition trigpart4;
+drop trigger trg1 on trigpart41; -- fail due to "does not exist"
+drop table trigpart4;
+alter table trigpart attach partition trigpart3 for values from (2000) to (3000);
+alter table trigpart detach partition trigpart3;
+alter table trigpart attach partition trigpart3 for values from (2000) to (3000);
+drop table trigpart3;
+
+select tgrelid::regclass::text, tgname, tgfoid::regproc, tgenabled, tgisinternal from pg_trigger
+  where tgname ~ '^trg1' order by 1;
+create table trigpart3 (like trigpart);
+create trigger trg1 after insert on trigpart3 for each row execute procedure trigger_nothing();
+\d trigpart3
+alter table trigpart attach partition trigpart3 FOR VALUES FROM (2000) to (3000); -- fail
+drop table trigpart3;
+
 drop table trigpart;
 drop function trigger_nothing();
 
-- 
2.20.1


--FCuugMFkClbJLl1L--