Date: Sat, 18 Apr 2020 19:22:06 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Amit Langote <amitlangote09@gmail.com>,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables
Message-ID: <20200419002206.GM26953@telsasoft.com>
References: <20200408180110.GA17093@alvherre.pgsql>
 <5954.1586369358@sss.pgh.pa.us>
 <CA+HiwqEwjmZqyCHahSU+vRM62w03U0t7fxPZ_MhYuj+r6-d0wA@mail.gmail.com>
 <27752.1586439998@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="sm4nu43k4a2Rpi4c"
Content-Disposition: inline
In-Reply-To: <27752.1586439998@sss.pgh.pa.us>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk


--sm4nu43k4a2Rpi4c
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Apr 09, 2020 at 09:46:38AM -0400, Tom Lane wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > On Thu, Apr 9, 2020 at 3:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> My point is that so long as you only allow the case of exactly one parent,
> >> you can just delete the child trigger, because it must belong to that
> >> parent.  As soon as there's any flexibility, you are going to end up
> >> reinventing all the stuff we had to invent to manage
> >> maybe-or-maybe-not-inherited columns.  So I think the "detach" idea
> >> is the first step on that road, and I counsel not taking that step.
> 
> > As mentioned upthread, we have behavior #1 for indexes (attach
> > existing / detach & keep), without any of the *islocal, *inhcount
> > infrastructure. It is a bit complex, because we need logic to check
> > the equivalence of an existing index on the partition being attached,
> > so implementing the same behavior for trigger is going to have to be
> > almost as complex.  Considering that #2 will be much simpler to
> > implement, but would be asymmetric with everything else.
> 
> I think there is justification for jumping through some hoops for
> indexes, because they can be extremely expensive to recreate.
> The same argument doesn't hold even a little bit for child
> triggers, though.
> 
> Also it can be expected that an index will still behave sensibly after
> its table is standalone, whereas that's far from obvious for a trigger
> that was meant to work on partition member tables.

I haven't heard a compelling argument for or against either way.

Maybe the worst behavior might be if, during ATTACH, we searched for a matching
trigger, and "merged" it (marked it inherited) if it matched.  That could be
bad if someone *wanted* two triggers, which seems unlikely, but to each their
own.

I implemented the simple way (and, as an experiment, 75% of the hard way).

It occured to me that we don't currently distinguish between a trigger on a
child table, and a trigger on a parent table which was recursively created on a
child.  That makes sense for indexes and constraints, since the objects persist
if the table is detached, so it doesn't matter how it was defined.

But if we remove trigger during DETACH, then it's *not* the same as a trigger
that was defined on the child, and I suggest that in v13 we should make that
visible.

-- 
Justin

--sm4nu43k4a2Rpi4c
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v2-0001-fix-detaching-tables-with-inherited-after-row-tri.patch"

From 19fe0c70e0b4f2c05c538d1a9042f9303c927ae2 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Fri, 3 Apr 2020 22:43:26 -0500
Subject: [PATCH v2] fix detaching tables with inherited after-row triggers

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.

See also:
86f575948c773b0ec5b0f27066e37dd93a7f0a96

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       | 37 ++++++++++++++++++++++
 src/bin/psql/describe.c                | 12 +++++--
 src/test/regress/expected/triggers.out | 44 ++++++++++++++++++++++++++
 src/test/regress/sql/triggers.sql      | 18 +++++++++++
 5 files changed, 110 insertions(+), 2 deletions(-)

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..78236d152f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16797,6 +16797,43 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
 	}
 	table_close(classRel, RowExclusiveLock);
 
+	/*
+	 * Drop any ROW triggers inherited from partitioned table.
+	 * This undoes what CloneRowTriggersToPartition did.
+	 */
+	{
+		ScanKeyData skey;
+		SysScanDesc	scan;
+		HeapTuple	trigtup;
+		Relation	tgrel;
+
+		ScanKeyInit(&skey, Anum_pg_trigger_tgrelid, BTEqualStrategyNumber,
+				F_OIDEQ, ObjectIdGetDatum(RelationGetRelid(partRel)));
+		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);
+
+			if (OidIsValid(pg_trigger->tgparentid) &&
+					pg_trigger->tgisinternal &&
+					TRIGGER_FOR_ROW(pg_trigger->tgtype))
+				RemoveTriggerById(pg_trigger->oid);
+
+			deleteDependencyRecordsFor(TriggerRelationId,
+					pg_trigger->oid,
+					false);
+			deleteDependencyRecordsFor(RelationRelationId,
+					pg_trigger->oid,
+					false);
+		}
+
+		systable_endscan(scan);
+		table_close(tgrel, RowExclusiveLock);
+	}
+
 	/*
 	 * Detach any foreign keys that are inherited.  This includes creating
 	 * additional action triggers.
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index f05e914b4d..4cbc741c97 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2939,14 +2939,17 @@ describeOneTableDetails(const char *schemaname,
 		printfPQExpBuffer(&buf,
 						  "SELECT t.tgname, "
 						  "pg_catalog.pg_get_triggerdef(t.oid%s), "
-						  "t.tgenabled, %s\n"
+						  "t.tgenabled, %s, %s\n"
 						  "FROM pg_catalog.pg_trigger t\n"
 						  "WHERE t.tgrelid = '%s' AND ",
 						  (pset.sversion >= 90000 ? ", true" : ""),
 						  (pset.sversion >= 90000 ? "t.tgisinternal" :
 						   pset.sversion >= 80300 ?
 						   "t.tgconstraint <> 0 AS tgisinternal" :
-						   "false AS tgisinternal"), oid);
+						   "false AS tgisinternal"),
+						  (pset.sversion >= 13000 ? "t.tgparentid" :
+						   "0 AS tgparentid"),
+						  oid);
 		if (pset.sversion >= 110000)
 			appendPQExpBufferStr(&buf, "(NOT t.tgisinternal OR (t.tgisinternal AND t.tgenabled = 'D') \n"
 								 "    OR EXISTS (SELECT 1 FROM pg_catalog.pg_depend WHERE objid = t.oid \n"
@@ -3062,6 +3065,11 @@ describeOneTableDetails(const char *schemaname,
 						tgdef = usingpos + 9;
 
 					printfPQExpBuffer(&buf, "    %s", tgdef);
+
+					/* Visually distinguish inherited triggers XXX: ROW only? */
+					if (*PQgetvalue(result, i, 4) != '0')
+						appendPQExpBufferStr(&buf, ", PARTITION");
+
 					printTableAddFooter(&cont, buf.data);
 				}
 			}
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index e9da4ef983..9b544148bf 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2023,6 +2023,50 @@ 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(), PARTITION
+
+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 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
+ trigpart4  | trg1   | trigger_nothing | O         | t
+ trigpart41 | trg1   | trigger_nothing | O         | t
+ trigpart42 | trg1   | trigger_nothing | O         | t
+(5 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..5ab7cc62d2 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1380,6 +1380,24 @@ 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 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.17.0


--sm4nu43k4a2Rpi4c--