Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Justin Pryzby]

On Tue, Apr 21, 2020 at 07:03:30PM -0400, Alvaro Herrera wrote:
> On 2020-Apr-20, Justin Pryzby wrote:
> 
> > On Mon, Apr 20, 2020 at 06:35:44PM +0900, Amit Langote wrote:
> 
> > > Also, how about, for consistency, making the parent table labeling of
> > > the trigger look similar to that for the foreign constraint, so
> > > Triggers:
> > > TABLE "f1" TRIGGER "trig" BEFORE INSERT ON f11 FOR EACH ROW EXECUTE 
> > > FUNCTION trigfunc()
> > 
> > I'll leave that for committer to decide.
> 
> Pushed.  Many thanks for this!

Thanks for polishing it.

I was just about to convince myself of the merits of doing it Amit's way :)

I noticed a few issues:

 - should put \n's around Amit's subquery to make psql -E look pretty;
 - maybe should quote the TABLE, like \"%s\" ?

#3 is that *if* we did it Amit's way, I *think* maybe we should show the
parent's triggerdef, not the childs.
It seems strange to me to say "TABLE trigpart .* INSERT ON trigpart3"

-TABLE "trigpart" TRIGGER trg1 AFTER INSERT ON trigpart3 FOR EACH ROW 
EXECUTE FUNCTION trigger_nothing()
+TABLE "trigpart" TRIGGER trg1 AFTER INSERT ON trigpart FOR EACH ROW 
EXECUTE FUNCTION trigger_nothing()


-- 
Justin
>From 86ff4e592afe56d2611e22b63fa3f1268b583e58 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Fri, 3 Apr 2020 22:43:26 -0500
Subject: [PATCH v6 1/2] fixups: c33869cc3bfc42bce822251f2fa1a2a346f86cc5

---
 src/bin/psql/describe.c| 14 +++---
 src/test/regress/expected/triggers.out |  2 +-
 src/test/regress/sql/triggers.sql  |  2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 8dca6d8bb4..5ef56f7a9d 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2947,12 +2947,12 @@ describeOneTableDetails(const char *schemaname,
 		   pset.sversion >= 80300 ?
 		   "t.tgconstraint <> 0 AS tgisinternal" :
 		   "false AS tgisinternal"),
-		  (pset.sversion >= 13 ?
-		   "(SELECT (NULLIF(a.relid, t.tgrelid))::pg_catalog.regclass"
-		   " FROM pg_catalog.pg_trigger AS u, "
-		   "  pg_catalog.pg_partition_ancestors(t.tgrelid) AS a"
-		   " WHERE u.tgname = t.tgname AND u.tgrelid = a.relid"
-		   "   AND u.tgparentid = 0) AS parent" :
+		  (pset.sversion >= 13 ? "\n"
+		   "  (SELECT (NULLIF(a.relid, t.tgrelid))::pg_catalog.regclass\n"
+		   "   FROM pg_catalog.pg_trigger AS u,\n"
+		   "   pg_catalog.pg_partition_ancestors(t.tgrelid) AS a\n"
+		   "   WHERE u.tgname = t.tgname AND u.tgrelid = a.relid\n"
+		   "AND u.tgparentid = 0) AS parent" :
 		   "NULL AS parent"),
 		  oid);
 		if (pset.sversion >= 11)
@@ -3073,7 +3073,7 @@ describeOneTableDetails(const char *schemaname,
 
 	/* Visually distinguish inherited triggers */
 	if (!PQgetisnull(result, i, 4))
-		appendPQExpBuffer(, ", ON TABLE %s",
+		appendPQExpBuffer(, ", ON TABLE \"%s\"",
 PQgetvalue(result, i, 4));
 
 	printTableAddFooter(, buf.data);
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 5e76b3a47e..4104711c29 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2065,7 +2065,7 @@ create trigger trg1 after insert on trigpart3 for each row execute procedure tri
 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
+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;
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index e228d0a8a5..c359c6d3fa 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1398,7 +1398,7 @@ select tgrelid::regclass::text, tgname, tgfoid::regproc, tgenabled, tgisinternal
 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
+alter table trigpart attach partition trigpart3 for values from (2000) to (3000); -- fail
 drop table trigpart3;
 
 drop table trigpart;
-- 
2.17.0

>From 4712253f66e03fd57a7a7a971a9f00c492c6 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Mon, 20 Apr 2020 13:46:06 -0500
Subject: [PATCH v6 2/2] show inherited triggers Amit's way..

..this also updates the query to avoid saying things like:
TABLE trigpart .* INSERT ON trigpart3
---
 src/bin/psql/describe.c| 42 +-
 src/test/regress/expected/triggers.out |  2 +-
 2 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 

Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

On 2020-Apr-20, Justin Pryzby wrote:

> On Mon, Apr 20, 2020 at 06:35:44PM +0900, Amit Langote wrote:

> > Also, how about, for consistency, making the parent table labeling of
> > the trigger look similar to that for the foreign constraint, so
> > Triggers:
> > TABLE "f1" TRIGGER "trig" BEFORE INSERT ON f11 FOR EACH ROW EXECUTE 
> > FUNCTION trigfunc()
> 
> I'll leave that for committer to decide.

Pushed.  Many thanks for this!

Changes: I thought that printing the "ON TABLE" bit when it's defined in
the same table is pointless and ugly, so I added a NULLIF to prevent it
in that case (it's not every day that you can put NULLIF to work).  I
also changed the empty string to NULL for the case with older servers,
so that it doesn't print a lame "ON TABLE " clause for them.  Lastly,
added pg_catalog qualifications everywhere needed.

Contrary to what I had said, I decided to leave the output as submitted;
the constraint lines are not really precedent against it:

55432 13devel 24286=# \d lev3
  Partitioned table "public.lev3"
 Column │  Type   │ Collation │ Nullable │ Default 
┼─┼───┼──┼─
 a  │ integer │   │ not null │ 
Partition of: lev2 FOR VALUES IN (3)
Partition key: LIST (a)
Indexes:
"lev3_pkey" PRIMARY KEY, btree (a)
Foreign-key constraints:
TABLE "lev1" CONSTRAINT "lev1_a_fkey" FOREIGN KEY (a) REFERENCES lev1(a)
Referenced by:
TABLE "lev1" CONSTRAINT "lev1_a_fkey" FOREIGN KEY (a) REFERENCES lev1(a)
Triggers:
tt AFTER UPDATE ON lev3 FOR EACH ROW EXECUTE FUNCTION trigger_nothing(), ON 
TABLE lev2
Number of partitions: 1 (Use \d+ to list them.)

In the "FK constraints" and "referenced by" entries, it looks natural
since the constraint refers to a table.  Not so in the trigger case.

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

Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Justin Pryzby]

On Tue, Apr 21, 2020 at 12:20:38PM -0400, Alvaro Herrera wrote:
> diff --git a/doc/src/sgml/ref/alter_table.sgml 
> b/doc/src/sgml/ref/alter_table.sgml
> index 7595e609b5..233905552c 100644
> --- a/doc/src/sgml/ref/alter_table.sgml
> +++ b/doc/src/sgml/ref/alter_table.sgml
> @@ -941,13 +943,14 @@ WITH ( MODULUS  class="parameter">numeric_literal, REM
>  DETACH PARTITION  class="parameter">partition_name
>  
>   
>This form detaches specified partition of the target table.  The 
> detached
>partition continues to exist as a standalone table, but no longer has 
> any
>ties to the table from which it was detached.  Any indexes that were
> -  attached to the target table's indexes are detached.
> +  attached to the target table's indexes are detached.  Any triggers that
> +  were created to mirror those in the target table are removed.

Can you say "cloned" here instead of mirror ?
> +  attached to the target table's indexes are detached.  Any triggers that
> +  were created as clones of triggers in the target table are removed.

Also, I see in the surrounding context a missing word? 
This form detaches THE specified partition of the target table.

-- 
Justin

Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

I think I also owe the attached doc updates.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 7595e609b5..233905552c 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -869,13 +869,15 @@ WITH ( MODULUS numeric_literal, REM
   one will be created in the attached table; or, if an equivalent
   index already exists, it will be attached to the target table's index,
   as if ALTER INDEX ATTACH PARTITION had been executed.
   Note that if the existing table is a foreign table, it is currently not
   allowed to attach the table as a partition of the target table if there
   are UNIQUE indexes on the target table.  (See also
-  .)
+  .)  For each user-defined
+  row-level trigger that exists in the target table, a corresponding one
+  is created in the attached table.
  
 
  
   A partition using FOR VALUES uses same syntax for
   partition_bound_spec as
   .  The partition bound specification
@@ -941,13 +943,14 @@ WITH ( MODULUS numeric_literal, REM
 DETACH PARTITION partition_name
 
  
   This form detaches specified partition of the target table.  The detached
   partition continues to exist as a standalone table, but no longer has any
   ties to the table from which it was detached.  Any indexes that were
-  attached to the target table's indexes are detached.
+  attached to the target table's indexes are detached.  Any triggers that
+  were created to mirror those in the target table are removed.
  
 

 
   
   
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 155866c7c8..c49c770dd0 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -396,13 +396,15 @@ WITH ( MODULUS numeric_literal, REM
 PARTITION OF parent_table { FOR VALUES partition_bound_spec | DEFAULT }
 
  
   Creates the table as a partition of the specified
   parent table. The table can be created either as a partition for specific
   values using FOR VALUES or as a default partition
-  using DEFAULT.
+  using DEFAULT.  Any indexes, constraints and
+  user-defined row-level triggers that exist in the parent table are cloned
+  on the new partition.
  
 
  
   The partition_bound_spec
   must correspond to the partitioning method and partition key of the
   parent table, and must not overlap with any existing partition of that

Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

On 2020-Apr-20, Alvaro Herrera wrote:

> + 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;

Actually, shouldn't we be checking just "!OidIsValid(pg_trigger->tgparentid)"
here?  Surely the other two conditions should already not matter either
way if tgparentid is set.  I can't see us starting to clone
for-statement triggers, but I'm not sure I trust the internal marking to
remain one way or the other.

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

Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

> + deleteDependencyRecordsFor(TriggerRelationId,
> + pg_trigger->oid,
> + false);
> + deleteDependencyRecordsFor(RelationRelationId,
> + pg_trigger->oid,
> + false);
> +
> + CommandCounterIncrement();
> + ObjectAddressSet(object, TriggerRelationId, 
> pg_trigger->oid);
> + performDeletion(, 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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From b8aeae162e03ccd0346212e19ae2d75ec6495288 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
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 
Reviewed-by: Amit Langote 
Reviewed-by: Álvaro Herrera 
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 column_name1 [, column_name2INSTEAD OF.
   
 
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(, Anum_pg_trigger_tgrelid, BTEqualStrategyNumber,
+F_OIDEQ, ObjectIdGetDatum(partitionId));
+	tgrel = table_open(TriggerRelationId, RowExclusiveLock);
+	scan = systable_beginscan(tgrel, TriggerRelidNameIndexId,
+			  true, NULL, 1, );
+	while (HeapTupleIsValid(trigtup = systable_getnext(scan)))
+	{
+		

Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Justin Pryzby]

On Mon, Apr 20, 2020 at 06:35:44PM +0900, Amit Langote wrote:
> On Mon, Apr 20, 2020 at 5:49 AM Justin Pryzby  wrote:
> > On Sun, Apr 19, 2020 at 03:13:29PM -0400, Alvaro Herrera wrote:
> > > What happens if you detach the parent?  I mean, should the trigger
> > > removal recurse to children?
> >
> > It think it should probably exactly undo what CloneRowTriggersToPartition 
> > does.
> > ..and I guess you're trying to politely say that it didn't.  I tried to fix 
> > in
> > v4 - please check if that's right.
> 
> Looks correct to me.  Maybe have a test cover that?

I included such a test with the v4 patch.

> > > > 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.
> > >
> > > Hmm, interesting point -- whether the trigger is partition or not is
> > > important because it affects what happens on detach.  I agree that we
> > > should make it visible.  Is the proposed single bit "PARTITION" good
> > > enough, or should we indicate what's the ancestor table that defines the
> > > partition?
> >
> > Yea, it's an obvious thing to do.
> 
> This:
> 
> +  "false AS tgisinternal"),
> + (pset.sversion >= 13000 ?
> +  "pg_partition_root(t.tgrelid) AS parent" :
> +  "'' AS parent"),
> + oid);
> 
> 
> looks wrong, because the actual partition root may not also be the
> trigger parent root, for example:

Sigh, right.

> The following gets the correct parent for me:
> 
> - (pset.sversion >= 13000 ?
> -  "pg_partition_root(t.tgrelid) AS parent" :
> -  "'' AS parent"),
> + (pset.sversion >= 13 ?
> +  "(SELECT relid"
> +  " FROM pg_trigger, 
> pg_partition_ancestors(t.tgrelid)"
> +  " WHERE tgname = t.tgname AND tgrelid = relid"
> +  " AND tgparentid = 0) AS parent" :
> +  " null AS parent"),

I'm happy to see that this doesn't require a recursive cte, at least.
I was trying to think how to break it by returning multiple results or results
out of order, but I think that can't happen.

> Also, how about, for consistency, making the parent table labeling of
> the trigger look similar to that for the foreign constraint, so
> Triggers:
> TABLE "f1" TRIGGER "trig" BEFORE INSERT ON f11 FOR EACH ROW EXECUTE 
> FUNCTION trigfunc()

I'll leave that for committer to decide.

I split into separate patches since only 0001 should be backpatched (with
s/OidIsValid(tgparentid)/isPartitionTrigger/).

-- 
Justin
>From dc24d8fdbe4435feaea9a99fcf3e1e1121cc8950 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Fri, 3 Apr 2020 22:43:26 -0500
Subject: [PATCH v5 1/2] fix detaching tables with inherited 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.

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

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   | 42 
 src/test/regress/expected/triggers.out | 45 ++
 src/test/regress/sql/triggers.sql  | 21 
 4 files changed, 109 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 column_name1 [, column_name2INSTEAD OF.
   
 
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 037d457c3d..514c5ff844 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16797,6 +16797,48 @@ 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(, Anum_pg_trigger_tgrelid, BTEqualStrategyNumber,
+F_OIDEQ, ObjectIdGetDatum(RelationGetRelid(partRel)));
+		tgrel = table_open(TriggerRelationId, RowExclusiveLock);
+		scan = systable_beginscan(tgrel, TriggerRelidNameIndexId,
+true, NULL, 1, );
+
+		while (HeapTupleIsValid(trigtup = systable_getnext(scan)))
+		{
+			

Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Amit Langote]

On Mon, Apr 20, 2020 at 5:49 AM Justin Pryzby  wrote:
> On Sun, Apr 19, 2020 at 03:13:29PM -0400, Alvaro Herrera wrote:
> > On 2020-Apr-18, Justin Pryzby wrote:
> > > 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 think the simplicity argument trumps the other ones, so I agree to go
> > with your v3 patch proposed downthread.
> >
> > What happens if you detach the parent?  I mean, should the trigger
> > removal recurse to children?
>
> It think it should probably exactly undo what CloneRowTriggersToPartition 
> does.
> ..and I guess you're trying to politely say that it didn't.  I tried to fix in
> v4 - please check if that's right.

Looks correct to me.  Maybe have a test cover that?

> > > 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.
> >
> > Hmm, interesting point -- whether the trigger is partition or not is
> > important because it affects what happens on detach.  I agree that we
> > should make it visible.  Is the proposed single bit "PARTITION" good
> > enough, or should we indicate what's the ancestor table that defines the
> > partition?
>
> Yea, it's an obvious thing to do.

This:

+  "false AS tgisinternal"),
+ (pset.sversion >= 13000 ?
+  "pg_partition_root(t.tgrelid) AS parent" :
+  "'' AS parent"),
+ oid);


looks wrong, because the actual partition root may not also be the
trigger parent root, for example:

create table f (a int references p) partition by list (a);
create table f1 partition of f for values in (1) partition by list (a);
create table f11 partition of f for values in (1);
create function trigfunc() returns trigger language plpgsql as $$
begin raise notice '%', new; return new; end; $$;
create trigger trig before insert on f1 for each row execute function
trigfunc();
\d f11
 Table "public.f11"
 Column |  Type   | Collation | Nullable | Default
+-+---+--+-
 a  | integer |   |  |
Partition of: f1 FOR VALUES IN (1)
Triggers:
trig BEFORE INSERT ON f11 FOR EACH ROW EXECUTE FUNCTION
trigfunc(), ON TABLE f

Here, ON TABLE should say "f1".

The following gets the correct parent for me:

- (pset.sversion >= 13000 ?
-  "pg_partition_root(t.tgrelid) AS parent" :
-  "'' AS parent"),
+ (pset.sversion >= 13 ?
+  "(SELECT relid"
+  " FROM pg_trigger, pg_partition_ancestors(t.tgrelid)"
+  " WHERE tgname = t.tgname AND tgrelid = relid"
+  " AND tgparentid = 0) AS parent" :
+  " null AS parent"),

The server version number being compared against was missing a zero in
your patch.

Also, how about, for consistency, making the parent table labeling of
the trigger look similar to that for the foreign constraint, so
instead of:

Triggers:
trig BEFORE INSERT ON f11 FOR EACH ROW EXECUTE FUNCTION
trigfunc(), ON TABLE f1

how about:

Triggers:
TABLE "f1" TRIGGER "trig" BEFORE INSERT ON f11 FOR EACH ROW
EXECUTE FUNCTION trigfunc()

--
Amit Langote
EnterpriseDB: http://www.enterprisedb.com

Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Justin Pryzby]

On Sun, Apr 19, 2020 at 03:13:29PM -0400, Alvaro Herrera wrote:
> On 2020-Apr-18, Justin Pryzby wrote:
> > 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 think the simplicity argument trumps the other ones, so I agree to go
> with your v3 patch proposed downthread.
> 
> What happens if you detach the parent?  I mean, should the trigger
> removal recurse to children?

It think it should probably exactly undo what CloneRowTriggersToPartition does.
..and I guess you're trying to politely say that it didn't.  I tried to fix in
v4 - please check if that's right.

> > 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.
> 
> Hmm, interesting point -- whether the trigger is partition or not is
> important because it affects what happens on detach.  I agree that we
> should make it visible.  Is the proposed single bit "PARTITION" good
> enough, or should we indicate what's the ancestor table that defines the
> partition?

Yea, it's an obvious thing to do.

One issue is that tgparentid is new, so showing the partition status of the
trigger when connected to an pre-13 server would be nontrivial: b9b408c48.

-- 
Justin
>From b45a047f37d215cac2e72661b92e0ade4730b1e1 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Fri, 3 Apr 2020 22:43:26 -0500
Subject: [PATCH v4] fix detaching tables with inherited 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   | 42 
 src/bin/psql/describe.c| 14 ++--
 src/test/regress/expected/triggers.out | 45 ++
 src/test/regress/sql/triggers.sql  | 21 
 5 files changed, 121 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 column_name1 [, column_name2INSTEAD OF.
   
 
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 037d457c3d..514c5ff844 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16797,6 +16797,48 @@ 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(, Anum_pg_trigger_tgrelid, BTEqualStrategyNumber,
+F_OIDEQ, ObjectIdGetDatum(RelationGetRelid(partRel)));
+		tgrel = table_open(TriggerRelationId, RowExclusiveLock);
+		scan = systable_beginscan(tgrel, TriggerRelidNameIndexId,
+true, NULL, 1, );
+
+		while (HeapTupleIsValid(trigtup = systable_getnext(scan)))
+		{
+			Form_pg_trigger pg_trigger = (Form_pg_trigger) GETSTRUCT(trigtup);
+			ObjectAddress object;
+
+			if (!OidIsValid(pg_trigger->tgparentid) ||
+	!pg_trigger->tgisinternal ||
+	!TRIGGER_FOR_ROW(pg_trigger->tgtype))
+continue;
+
+			deleteDependencyRecordsFor(TriggerRelationId,
+	pg_trigger->oid,
+	false);
+			deleteDependencyRecordsFor(RelationRelationId,
+	pg_trigger->oid,
+	false);
+
+			CommandCounterIncrement();
+			ObjectAddressSet(object, TriggerRelationId, pg_trigger->oid);
+			performDeletion(, DROP_RESTRICT, PERFORM_DELETION_INTERNAL);
+		}
+
+		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..1de33ac772 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2939,14 +2939,18 

Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

On 2020-Apr-19, Justin Pryzby wrote:

> It's probably rare that we'd be inserting into a table old enough to be
> detached, and normally that would be ok, but if a trigger were missing, it
> would misbehave.  In our use-case, we're creating trigger on the parent as a
> convenient way to maintain them on the partitions, which doesn't work if a
> table exists but detached..
> 
> So we'd actually prefer the behavior of indexes/constraints, where the trigger
> is preserved if the child is detached.  I'm not requesting to do that just for
> our use case, which may be atypical or not a good model, but adding our one
> data point.

I think the easiest way to implement this is to have two triggers -- the
one that's direct in the partition checks whether the table is a
partition and does nothing in that case.

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

Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Justin Pryzby]

On Wed, Apr 08, 2020 at 11:44:33AM -0500, Justin Pryzby wrote:
> On Wed, Apr 08, 2020 at 12:02:39PM -0400, Alvaro Herrera wrote:
> > On 2020-Apr-08, Justin Pryzby wrote:
> > 
> > > This seems to be a bug in master, v12, and (probably) v11, where "FOR 
> > > EACH FOR"
> > > was first allowed on partition tables (86f575948).
> > > 
> > > I thought this would work like partitioned indexes (8b08f7d48), where 
> > > detaching
> > > a partition makes its index non-inherited, and attaching a partition 
> > > marks a
> > > pre-existing, matching partition as inherited rather than creating a new 
> > > one.
> > 
> > Hmm.  Let's agree to what behavior we want, and then we implement that.
> > It seems to me there are two choices:
> > 
> > 1. on detach, keep the trigger but make it independent of the trigger on
> > parent.  (This requires that the trigger is made dependent on the
> > trigger on parent, if the table is attached as partition again;
> > otherwise you'd end up with multiple copies of the trigger if you
> > detach/attach multiple times).
> > 
> > 2. on detach, remove the trigger from the partition.
> > 
> > I think (2) is easier to implement, but (1) is the more convenient
> > behavior.
> 
> At telsasoft, we don't care (we uninherit tables before ALTERing parents to
> avoid disruptive locking and to avoid worst-case disk use).

I realized that I was wrong about what would be most desirable for us, for an
uncommon case:

Our loader INSERTs into the child table, not the parent (I think I did that to
try to implement UPSERT before partitioned indexes in v11).

All but the newest partitions are DETACHed when we need to promote a column.

It's probably rare that we'd be inserting into a table old enough to be
detached, and normally that would be ok, but if a trigger were missing, it
would misbehave.  In our use-case, we're creating trigger on the parent as a
convenient way to maintain them on the partitions, which doesn't work if a
table exists but detached..

So we'd actually prefer the behavior of indexes/constraints, where the trigger
is preserved if the child is detached.  I'm not requesting to do that just for
our use case, which may be atypical or not a good model, but adding our one
data point.

-- 
Justin

Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

On 2020-Apr-18, Justin Pryzby wrote:

> 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 think the simplicity argument trumps the other ones, so I agree to go
with your v3 patch proposed downthread.

What happens if you detach the parent?  I mean, should the trigger
removal recurse to children?

> 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.

Hmm, interesting point -- whether the trigger is partition or not is
important because it affects what happens on detach.  I agree that we
should make it visible.  Is the proposed single bit "PARTITION" good
enough, or should we indicate what's the ancestor table that defines the
partition?

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

Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Justin Pryzby]

v3 fixes a brown-paper-bag logic error.

-- 
Justin
>From b5de1fc71f805bb8c7ec7e77bfce9a604ccd4bc8 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Fri, 3 Apr 2020 22:43:26 -0500
Subject: [PATCH v3] fix detaching tables with inherited 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   | 38 ++
 src/bin/psql/describe.c| 12 +--
 src/test/regress/expected/triggers.out | 44 ++
 src/test/regress/sql/triggers.sql  | 18 +++
 5 files changed, 111 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 column_name1 [, column_name2INSTEAD OF.
   
 
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 037d457c3d..480ea777ac 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16797,6 +16797,44 @@ 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(, Anum_pg_trigger_tgrelid, BTEqualStrategyNumber,
+F_OIDEQ, ObjectIdGetDatum(RelationGetRelid(partRel)));
+		tgrel = table_open(TriggerRelationId, RowExclusiveLock);
+		scan = systable_beginscan(tgrel, TriggerRelidNameIndexId,
+true, NULL, 1, );
+
+		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))
+continue;
+
+			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(,
 		  "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 >= 9 ? ", true" : ""),
 		  (pset.sversion >= 9 ? "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 >= 11)
 			appendPQExpBufferStr(, "(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(, "%s", tgdef);
+
+	/* Visually distinguish inherited triggers XXX: ROW only? */
+	if (*PQgetvalue(result, i, 4) != '0')
+		appendPQExpBufferStr(, ", PARTITION");
+
 	printTableAddFooter(, 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 

Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Justin Pryzby]

On Thu, Apr 09, 2020 at 09:46:38AM -0400, Tom Lane wrote:
> Amit Langote  writes:
> > On Thu, Apr 9, 2020 at 3:09 AM Tom Lane  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
>From 19fe0c70e0b4f2c05c538d1a9042f9303c927ae2 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
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 column_name1 [, column_name2INSTEAD OF.
   
 
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(, Anum_pg_trigger_tgrelid, BTEqualStrategyNumber,
+F_OIDEQ, ObjectIdGetDatum(RelationGetRelid(partRel)));
+		tgrel = table_open(TriggerRelationId, RowExclusiveLock);
+		scan = systable_beginscan(tgrel, TriggerRelidNameIndexId,
+true, NULL, 1, );
+
+		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);
+	}

Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Tom Lane]

Amit Langote  writes:
> On Thu, Apr 9, 2020 at 3:09 AM Tom Lane  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.

regards, tom lane

Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Amit Langote]

On Thu, Apr 9, 2020 at 3:09 AM Tom Lane  wrote:
> Alvaro Herrera  writes:
> > On 2020-Apr-08, Tom Lane wrote:
> >> I think that #1 would soon lead to needing all the same infrastructure
> >> as we have for inherited columns and constraints, ie triggers would need
> >> equivalents of attislocal and attinhcount.  I don't really want to go
> >> there, so I'd vote for #2.
>
> > Hmm.  Those things are used for the legacy inheritance case supporting
> > multiple inheritance, where we need to figure out which parent the table
> > is being detached (disinherited) from.  But for partitioning we know
> > which parent it is, since there can only be one.  So I don't think that
> > argument applies.
>
> 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.

-- 

Amit Langote
EnterpriseDB: http://www.enterprisedb.com

Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Tom Lane]

Alvaro Herrera  writes:
> On 2020-Apr-08, Tom Lane wrote:
>> I think that #1 would soon lead to needing all the same infrastructure
>> as we have for inherited columns and constraints, ie triggers would need
>> equivalents of attislocal and attinhcount.  I don't really want to go
>> there, so I'd vote for #2.

> Hmm.  Those things are used for the legacy inheritance case supporting
> multiple inheritance, where we need to figure out which parent the table
> is being detached (disinherited) from.  But for partitioning we know
> which parent it is, since there can only be one.  So I don't think that
> argument applies.

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.

(This implies that when creating a child trigger, we should error out,
*not* allow the case, if there's already a trigger by that name.  Not
sure if that's what happens today, but again I'd say that's what we
should do to avoid complicated cases.)

regards, tom lane

Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

On 2020-Apr-08, Tom Lane wrote:

> Alvaro Herrera  writes:
> > Hmm.  Let's agree to what behavior we want, and then we implement that.
> > It seems to me there are two choices:
> 
> > 1. on detach, keep the trigger but make it independent of the trigger on
> > parent.  (This requires that the trigger is made dependent on the
> > trigger on parent, if the table is attached as partition again;
> > otherwise you'd end up with multiple copies of the trigger if you
> > detach/attach multiple times).
> 
> > 2. on detach, remove the trigger from the partition.
> 
> > I think (2) is easier to implement, but (1) is the more convenient
> > behavior.
> 
> I think that #1 would soon lead to needing all the same infrastructure
> as we have for inherited columns and constraints, ie triggers would need
> equivalents of attislocal and attinhcount.  I don't really want to go
> there, so I'd vote for #2.

Hmm.  Those things are used for the legacy inheritance case supporting
multiple inheritance, where we need to figure out which parent the table
is being detached (disinherited) from.  But for partitioning we know
which parent it is, since there can only be one.  So I don't think that
argument applies.

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

Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Tom Lane]

Alvaro Herrera  writes:
> Hmm.  Let's agree to what behavior we want, and then we implement that.
> It seems to me there are two choices:

> 1. on detach, keep the trigger but make it independent of the trigger on
> parent.  (This requires that the trigger is made dependent on the
> trigger on parent, if the table is attached as partition again;
> otherwise you'd end up with multiple copies of the trigger if you
> detach/attach multiple times).

> 2. on detach, remove the trigger from the partition.

> I think (2) is easier to implement, but (1) is the more convenient
> behavior.

I think that #1 would soon lead to needing all the same infrastructure
as we have for inherited columns and constraints, ie triggers would need
equivalents of attislocal and attinhcount.  I don't really want to go
there, so I'd vote for #2.

regards, tom lane

Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Justin Pryzby]

On Wed, Apr 08, 2020 at 12:02:39PM -0400, Alvaro Herrera wrote:
> On 2020-Apr-08, Justin Pryzby wrote:
> 
> > This seems to be a bug in master, v12, and (probably) v11, where "FOR EACH 
> > FOR"
> > was first allowed on partition tables (86f575948).
> > 
> > I thought this would work like partitioned indexes (8b08f7d48), where 
> > detaching
> > a partition makes its index non-inherited, and attaching a partition marks a
> > pre-existing, matching partition as inherited rather than creating a new 
> > one.
> 
> Hmm.  Let's agree to what behavior we want, and then we implement that.
> It seems to me there are two choices:
> 
> 1. on detach, keep the trigger but make it independent of the trigger on
> parent.  (This requires that the trigger is made dependent on the
> trigger on parent, if the table is attached as partition again;
> otherwise you'd end up with multiple copies of the trigger if you
> detach/attach multiple times).
> 
> 2. on detach, remove the trigger from the partition.
> 
> I think (2) is easier to implement, but (1) is the more convenient
> behavior.

At telsasoft, we don't care (we uninherit tables before ALTERing parents to
avoid disruptive locking and to avoid worst-case disk use).

(1) is consistent with the behavior for indexes, which is a slight advantage
for users' ability to understand and keep track of the behavior.  But adding
triggers is pretty different so I'm not sure it's a totally compelling
parallel.

-- 
Justin

Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

On 2020-Apr-08, Justin Pryzby wrote:

> This seems to be a bug in master, v12, and (probably) v11, where "FOR EACH 
> FOR"
> was first allowed on partition tables (86f575948).
> 
> I thought this would work like partitioned indexes (8b08f7d48), where 
> detaching
> a partition makes its index non-inherited, and attaching a partition marks a
> pre-existing, matching partition as inherited rather than creating a new one.

Hmm.  Let's agree to what behavior we want, and then we implement that.
It seems to me there are two choices:

1. on detach, keep the trigger but make it independent of the trigger on
parent.  (This requires that the trigger is made dependent on the
trigger on parent, if the table is attached as partition again;
otherwise you'd end up with multiple copies of the trigger if you
detach/attach multiple times).

2. on detach, remove the trigger from the partition.

I think (2) is easier to implement, but (1) is the more convenient
behavior.

(The current behavior is obviously a bug.)

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

DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

[Justin Pryzby]

This seems to be a bug in master, v12, and (probably) v11, where "FOR EACH FOR"
was first allowed on partition tables (86f575948).

I thought this would work like partitioned indexes (8b08f7d48), where detaching
a partition makes its index non-inherited, and attaching a partition marks a
pre-existing, matching partition as inherited rather than creating a new one.

DROP TABLE t, t1;
CREATE TABLE t(i int)PARTITION BY RANGE(i);
CREATE TABLE t1 PARTITION OF t FOR VALUES FROM(1)TO(2);
CREATE OR REPLACE FUNCTION trigf() RETURNS trigger LANGUAGE plpgsql AS $$ BEGIN 
END $$;
CREATE TRIGGER trig AFTER INSERT ON t FOR EACH ROW EXECUTE FUNCTION trigf();
SELECT tgrelid::regclass, * FROM pg_trigger WHERE tgrelid='t1'::regclass;
ALTER TABLE t DETACH PARTITION t1;
ALTER TABLE t ATTACH PARTITION t1 FOR VALUES FROM (1)TO(2);
ERROR:  trigger "trig" for relation "t1" already exists

DROP TRIGGER trig ON t1;
ERROR:  cannot drop trigger trig on table t1 because trigger trig on table t 
requires it
HINT:  You can drop trigger trig on table t instead.

I remember these, but they don't seem to be relevant to this issue, which seems
to be independant.

1fa846f1c9 Fix cloning of row triggers to sub-partitions
b9b408c487 Record parents of triggers

The commit for partitioned indexes talks about using an pre-existing index on
the child as a "convenience gadget", puts indexes into pg_inherit, and
introduces "ALTER INDEX..ATTACH PARTITION" and "CREATE INDEX..ON ONLY".

It's probably rare for a duplicate index to be useful (unless rebuilding to be
more optimal, which is probably not reasonably interspersed with altering
inheritence).  But I don't know if that's equally true for triggers.  So I'm
not sure what the intended behavior is, so I've stopped after implementing
a partial fix.
>From 2c31cac22178d904ee108b77f316886d1e2f6288 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Fri, 3 Apr 2020 22:43:26 -0500
Subject: [PATCH] WIP: fix detaching tables with inherited triggers

---
 src/backend/commands/tablecmds.c | 33 
 1 file changed, 33 insertions(+)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 037d457c3d..10a60e158f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16797,6 +16797,39 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
 	}
 	table_close(classRel, RowExclusiveLock);
 
+	/* detach triggers too */
+	{
+		/* XXX: relcache.c */
+		ScanKeyData skey;
+		SysScanDesc	scan;
+		HeapTuple	trigtup;
+		Relation	tgrel = table_open(TriggerRelationId, RowExclusiveLock);
+
+		ScanKeyInit(, Anum_pg_trigger_tgrelid, BTEqualStrategyNumber,
+F_OIDEQ, ObjectIdGetDatum(RelationGetRelid(partRel)));
+
+		scan = systable_beginscan(tgrel, TriggerRelidNameIndexId,
+true, NULL, 1, );
+
+		while (HeapTupleIsValid(trigtup = systable_getnext(scan)))
+		{
+			Form_pg_trigger pg_trigger;
+			trigtup = heap_copytuple(trigtup);  /* need a modifiable copy */
+			pg_trigger = (Form_pg_trigger) GETSTRUCT(trigtup);
+			/* Set the trigger's parent to Invalid */
+			if (!OidIsValid(pg_trigger->tgparentid))
+continue;
+			if (!pg_trigger->tgisinternal)
+continue;
+			pg_trigger->tgparentid = InvalidOid;
+			pg_trigger->tgisinternal = false;
+			CatalogTupleUpdate(tgrel, >t_self, trigtup);
+			heap_freetuple(trigtup);
+		}
+		systable_endscan(scan);
+		table_close(tgrel, RowExclusiveLock);
+	}
+
 	/*
 	 * Detach any foreign keys that are inherited.  This includes creating
 	 * additional action triggers.
-- 
2.17.0

Re: FOR EACH ROW triggers on partitioned tables

[Amit Langote]

On 2018/04/30 18:38, Ashutosh Bapat wrote:
> On Fri, Mar 23, 2018 at 7:19 PM, Alvaro Herrera  
> wrote:
>> Pushed.  Thanks for all the review.
>>
> 
> https://www.postgresql.org/docs/devel/static/ddl-partitioning.html
> 
> section 5.10.2.3 still says
> "Row triggers, if necessary, must be defined on individual partitions,
> not the partitioned table."
> 
> Should that change?
> 
> Per commit 86f575948c773b0ec5b0f27066e37dd93a7f0a96, we are supporting
> only AFTER row triggers. May be we should change the above line to
> "Before row triggers, if necessary, must ".

A patch to fix that has been posted.

https://www.postgresql.org/message-id/9386c128-1131-d115-cda5-63ac88d15db1%40lab.ntt.co.jp

Thanks,
Amit

Re: FOR EACH ROW triggers on partitioned tables

[Ashutosh Bapat]

On Fri, Mar 23, 2018 at 7:19 PM, Alvaro Herrera  wrote:
> Pushed.  Thanks for all the review.
>

https://www.postgresql.org/docs/devel/static/ddl-partitioning.html

section 5.10.2.3 still says
"Row triggers, if necessary, must be defined on individual partitions,
not the partitioned table."

Should that change?

Per commit 86f575948c773b0ec5b0f27066e37dd93a7f0a96, we are supporting
only AFTER row triggers. May be we should change the above line to
"Before row triggers, if necessary, must ".

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

Pushed.  Thanks for all the review.

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

Re: FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

Peter Eisentraut wrote:
> On 3/21/18 19:18, Alvaro Herrera wrote:
> > Here's v8, which addresses all your comments except the doc updates.  I
> > added a few more tests, too.  Thanks for the review!  I intend to commit
> > this shortly, probably not before Friday to give some more time for
> > others to review/comment.
> 
> Looks good, does what it needs to do.
> 
> A small fixup attached.  In particular, I renamed one trigger from "f",
> which created confusing output, looking like a boolean column.

Thanks!

> This comment in the tests I don't understand:
> 
> -- verify that the FOR UPDATE OF (columns) is propagated correctly
> 
> I don't see how this applies to the tests that follow.  Does this have
> something to do with the subsequent foreign keys patch perhaps?

Not at all ... I meant "AFTER UPDATE OF columns" (used as a firing
event).  Not sure how I typo'ed it that badly.

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

Re: FOR EACH ROW triggers on partitioned tables

[Peter Eisentraut]

On 3/21/18 19:18, Alvaro Herrera wrote:
> Here's v8, which addresses all your comments except the doc updates.  I
> added a few more tests, too.  Thanks for the review!  I intend to commit
> this shortly, probably not before Friday to give some more time for
> others to review/comment.

Looks good, does what it needs to do.

A small fixup attached.  In particular, I renamed one trigger from "f",
which created confusing output, looking like a boolean column.

This comment in the tests I don't understand:

-- verify that the FOR UPDATE OF (columns) is propagated correctly

I don't see how this applies to the tests that follow.  Does this have
something to do with the subsequent foreign keys patch perhaps?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 37c41e1be7fbc1a02c7d543a471c84aee7b75a9f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Thu, 22 Mar 2018 12:07:54 -0400
Subject: [PATCH] fixup! Allow FOR EACH ROW triggers on partitioned tables

---
 src/include/catalog/pg_constraint.h|  2 +-
 src/include/commands/trigger.h |  2 +-
 src/test/regress/expected/triggers.out | 36 +-
 src/test/regress/sql/triggers.sql  | 10 +-
 4 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/src/include/catalog/pg_constraint.h 
b/src/include/catalog/pg_constraint.h
index 45b26cdfa8..3957e07235 100644
--- a/src/include/catalog/pg_constraint.h
+++ b/src/include/catalog/pg_constraint.h
@@ -73,7 +73,7 @@ CATALOG(pg_constraint,2606)
Oid conindid;   /* index supporting 
this constraint */
 
/*
-* if this constraint is on a partition inherited from a partitioned 
table,
+* If this constraint is on a partition inherited from a partitioned 
table,
 * this is the OID of the corresponding constraint in the parent.
 */
Oid conparentid;
diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h
index 2a6f2cd934..a5b8610fa2 100644
--- a/src/include/commands/trigger.h
+++ b/src/include/commands/trigger.h
@@ -159,7 +159,7 @@ extern PGDLLIMPORT int SessionReplicationRole;
 
 extern ObjectAddress CreateTrigger(CreateTrigStmt *stmt, const char 
*queryString,
  Oid relOid, Oid refRelOid, Oid constraintOid, Oid 
indexOid,
- Oid funcid, Oid parentTriggerOid, Node *whenClause,
+ Oid funcoid, Oid parentTriggerOid, Node *whenClause,
  bool isInternal, bool in_partition);
 
 extern void RemoveTriggerById(Oid trigOid);
diff --git a/src/test/regress/expected/triggers.out 
b/src/test/regress/expected/triggers.out
index 2cb56efdf9..a4e9ea03f3 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -1871,7 +1871,7 @@ drop table parted_trig;
 --
 create table trigpart (a int, b int) partition by range (a);
 create table trigpart1 partition of trigpart for values from (0) to (1000);
-create trigger f after insert on trigpart for each row execute procedure 
trigger_nothing();
+create trigger trg1 after insert on trigpart for each row execute procedure 
trigger_nothing();
 create table trigpart2 partition of trigpart for values from (1000) to (2000);
 create table trigpart3 (like trigpart);
 alter table trigpart attach partition trigpart3 for values from (2000) to 
(3000);
@@ -1879,32 +1879,32 @@ select tgrelid::regclass, tgname, tgfoid::regproc from 
pg_trigger
   where tgrelid::regclass::text like 'trigpart%' order by 
tgrelid::regclass::text;
   tgrelid  | tgname | tgfoid  
 ---++-
- trigpart  | f  | trigger_nothing
- trigpart1 | f  | trigger_nothing
- trigpart2 | f  | trigger_nothing
- trigpart3 | f  | trigger_nothing
+ trigpart  | trg1   | trigger_nothing
+ trigpart1 | trg1   | trigger_nothing
+ trigpart2 | trg1   | trigger_nothing
+ trigpart3 | trg1   | trigger_nothing
 (4 rows)
 
-drop trigger f on trigpart1;   -- fail
-ERROR:  cannot drop trigger f on table trigpart1 because trigger f on table 
trigpart requires it
-HINT:  You can drop trigger f on table trigpart instead.
-drop trigger f on trigpart2;   -- fail
-ERROR:  cannot drop trigger f on table trigpart2 because trigger f on table 
trigpart requires it
-HINT:  You can drop trigger f on table trigpart instead.
-drop trigger f on trigpart3;   -- fail
-ERROR:  cannot drop trigger f on table trigpart3 because trigger f on table 
trigpart requires it
-HINT:  You can drop trigger f on table trigpart instead.
+drop trigger trg1 on trigpart1;-- fail
+ERROR:  cannot drop trigger trg1 on table trigpart1 because trigger trg1 on 
table trigpart requires it
+HINT:  You can drop trigger trg1 on table trigpart instead.
+drop trigger trg1 on trigpart2;-- fai

Re: FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

Here's v8, which addresses all your comments except the doc updates.  I
added a few more tests, too.  Thanks for the review!  I intend to commit
this shortly, probably not before Friday to give some more time for
others to review/comment.

Some notes:

Peter Eisentraut wrote:

> I'm not sure why you have the CommandCounterIncrement() changes in
> separate patches.

Clearly it was wise to have it separately, because it was not entirely
trivial to fix the unexpected fallout :-)

> I'm wondering about deferrable unique constraint triggers.  In index.c,
> the CreateTrigger() call doesn't pass any parent trigger OID.  How is
> this meant to work?  I mean, it does work, it seems.  Some comments maybe.

Yeah, it seems pretty complicated ... it already worked this way: if you
don't pass a constraint OID, the constraint is created internally.  We
make use of that here.

> What is the story with transition tables?  Why are they not supported?
> I don't understand this comment in CreateTrigger():
> 
> +   /*
> +* Disallow use of transition tables.  If this partitioned table
> +* has any partitions, the error would occur below; but if it
> +* doesn't then we would only hit that code when the first CREATE
> +* TABLE ... PARTITION OF is executed, which is too late.  Check
> +* early to avoid the problem.
> +*/
> 
> Earlier in the thread, others have indicated that transition tables
> should work.

Yeah, this is a pre-existing restriction actually -- it was purposefully
introduced by commit 501ed02cf6f4.  Maybe it can be lifted, but I don't
think it's this patch's job to do so.  I reworded this comment.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 5d55e3f752fc90a7a64e426c491493ce548d016e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Tue, 21 Nov 2017 15:53:11 -0300
Subject: [PATCH v8] Allow FOR EACH ROW triggers on partitioned tables

---
 src/backend/catalog/heap.c |   1 +
 src/backend/catalog/index.c|   4 +-
 src/backend/catalog/pg_constraint.c|   3 +
 src/backend/commands/tablecmds.c   | 150 -
 src/backend/commands/trigger.c | 291 ++--
 src/backend/commands/typecmds.c|   1 +
 src/backend/tcop/utility.c |   3 +-
 src/include/catalog/indexing.h |   2 +
 src/include/catalog/pg_constraint.h|  39 ++--
 src/include/catalog/pg_constraint_fn.h |   1 +
 src/include/commands/trigger.h |   5 +-
 src/test/regress/expected/triggers.out | 344 +++--
 src/test/regress/input/constraints.source  |  16 ++
 src/test/regress/output/constraints.source |  26 +++
 src/test/regress/sql/triggers.sql  | 234 +++-
 15 files changed, 1039 insertions(+), 81 deletions(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 5ed4654875..b69bb1e2a4 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2121,6 +2121,7 @@ StoreRelCheck(Relation rel, const char *ccname, Node 
*expr,
  false,/* Is 
Deferrable */
  false,/* Is 
Deferred */
  is_validated,
+ InvalidOid,   /* no 
parent constraint */
  
RelationGetRelid(rel),/* relation */
  attNos,   /* 
attrs in the constraint */
  keycount, /* # attrs 
in the constraint */
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 9e2dd0e729..ec8661228a 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1282,6 +1282,7 @@ index_constraint_create(Relation heapRelation,
   deferrable,
   initdeferred,
   true,
+  
parentConstraintId,
   
RelationGetRelid(heapRelation),
   
indexInfo->ii_KeyAttrNumbers,
   
indexInfo->ii_NumIndexAttrs,
@@ -1360,7 +1361,8 @@ index_constraint_create(Relation heapRelation,
trigger->constrrel = NULL;
 
(void) CreateTrigger(trigger, NULL, 
RelationGetRelid(heapRelation),

Re: FOR EACH ROW triggers on partitioned tables

[Peter Eisentraut]

On 3/9/18 15:41, Alvaro Herrera wrote:
>> One thing I'd like to add before claiming this committable (backend-
>> side) is enabling constraint triggers.  AFAICT that requires a bit of
>> additional logic, but it shouldn't be too terrible.  This would allow
>> for deferrable unique constraints, for example.
> 
> v7 supports constraint triggers.  I added an example using a UNIQUE
> DEFERRABLE constraint, and another one using plain CREATE CONSTRAINT TRIGGER.
> It's neat to see that the WHEN clause is executed at the time of the
> operation, and the trigger action is deferred (or not) till COMMIT time.

I'm not sure why you have the CommandCounterIncrement() changes in
separate patches.

It looks like there are some test cases that are essentially duplicates,
e.g.,

+create trigger failed before insert or update or delete on parted_trig
+  for each row execute procedure trigger_nothing();

+create trigger trig_ins_before_1 before insert on parted_stmt_trig
+  for each row execute procedure trigger_notice();

Perhaps the latter is supposed to be testing statement triggers instead?

Some documentation updates are needed, at least in catalogs.sgml and
CREATE TRIGGER reference page.

The argument names of CreateTrigger() are slightly different in the .h
and .c files.

I'm wondering about deferrable unique constraint triggers.  In index.c,
the CreateTrigger() call doesn't pass any parent trigger OID.  How is
this meant to work?  I mean, it does work, it seems.  Some comments maybe.

In CloneRowTriggersToPartition(), for this piece

+   /*
+* We only clone a) FOR EACH ROW triggers b) timed AFTER
events, c)
+* that are not constraint triggers.
+*/
+   if (!TRIGGER_FOR_ROW(trigForm->tgtype) ||
+   !TRIGGER_FOR_AFTER(trigForm->tgtype) ||
+   OidIsValid(trigForm->tgconstraint))
+   continue;

I would rather have some elog(ERROR)'s if it finds triggers it can't
support instead of silently skipping them.

What is the story with transition tables?  Why are they not supported?
I don't understand this comment in CreateTrigger():

+   /*
+* Disallow use of transition tables.  If this partitioned table
+* has any partitions, the error would occur below; but if it
+* doesn't then we would only hit that code when the first CREATE
+* TABLE ... PARTITION OF is executed, which is too late.  Check
+* early to avoid the problem.
+*/

Earlier in the thread, others have indicated that transition tables
should work.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: FOR EACH ROW triggers on partitioned tables

[Peter Eisentraut]

On 3/9/18 16:05, Alvaro Herrera wrote:
> ... and this little addendum makes pg_dump work correctly.

The header file says "recursing", but the .c file calls the argument
"in_partition".

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: FOR EACH ROW triggers on partitioned tables

[Thomas Munro]

On Fri, Mar 9, 2018 at 7:06 AM, Alvaro Herrera  wrote:
> Thomas Munro wrote:
>> +create trigger failed after update on parted_trig
>> +  referencing old table as old_table
>> +  for each statement execute procedure trigger_nothing();
>>
>> It doesn't fail as you apparently expected.  Perhaps it was supposed
>> to be "for each row" so you could hit your new error with
>> errdetail("Triggers on partitioned tables cannot have transition
>> tables.")?
>
> You're absolutely right.  Fixed in the attached version.

+create trigger failed after update on parted_trig
+  referencing old table as old_table
+  for each row execute procedure trigger_nothing();
+ERROR:  "parted_trig" is a partitioned table
+DETAIL:  Triggers on partitioned tables cannot have transition tables.

I think this should probably say "row-level".  Statement-level
triggers on partitioned tables can have transition tables.

-- 
Thomas Munro
http://www.enterprisedb.com

Re: FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

Peter Eisentraut wrote:
> On 3/7/18 20:57, Alvaro Herrera wrote:
> > So, unless someone has a brilliant idea on how to construct a column
> > mapping from partitioned table to partition, I'm going back to the
> > design I was proposing earlier, ie., creating individual pg_trigger rows
> > for each partition that are essentially adjusted copies of the ones for
> > the partitioned table.
> 
> Yes, that seems easiest.
> 
> The idea of having only one pg_trigger entry was derived from the
> assumption that we wouldn't need the other ones for anything.  But if
> that doesn't apply, then it's better to just go with the straightforward
> way instead of bending the single-pg_trigger way to our will.

I think you changed the commitfest status to "waiting on author" after
posting this comment, but I had already posted an updated version which
addressed this problem.  I have changed it back to needs-review.

thanks

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

Re: FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

... and this little addendum makes pg_dump work correctly.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 867bbe8f1e..ca0a66753e 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1363,7 +1363,7 @@ index_constraint_create(Relation heapRelation,
 
(void) CreateTrigger(trigger, NULL, 
RelationGetRelid(heapRelation),
 InvalidOid, conOid, 
indexRelationId, InvalidOid,
-InvalidOid, true);
+InvalidOid, true, 
false);
}
 
/*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 4303c5a131..f5fc0938a6 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8467,7 +8467,7 @@ CreateFKCheckTrigger(Oid myRelOid, Oid refRelOid, 
Constraint *fkconstraint,
fk_trigger->args = NIL;
 
(void) CreateTrigger(fk_trigger, NULL, myRelOid, refRelOid, 
constraintOid,
-indexOid, InvalidOid, 
InvalidOid, true);
+indexOid, InvalidOid, 
InvalidOid, true, false);
 
/* Make changes-so-far visible */
CommandCounterIncrement();
@@ -8541,7 +8541,7 @@ createForeignKeyTriggers(Relation rel, Oid refRelOid, 
Constraint *fkconstraint,
fk_trigger->args = NIL;
 
(void) CreateTrigger(fk_trigger, NULL, refRelOid, myRelOid, 
constraintOid,
-indexOid, InvalidOid, 
InvalidOid, true);
+indexOid, InvalidOid, 
InvalidOid, true, false);
 
/* Make changes-so-far visible */
CommandCounterIncrement();
@@ -8596,7 +8596,7 @@ createForeignKeyTriggers(Relation rel, Oid refRelOid, 
Constraint *fkconstraint,
fk_trigger->args = NIL;
 
(void) CreateTrigger(fk_trigger, NULL, refRelOid, myRelOid, 
constraintOid,
-indexOid, InvalidOid, 
InvalidOid, true);
+indexOid, InvalidOid, 
InvalidOid, true, false);
 
/* Make changes-so-far visible */
CommandCounterIncrement();
@@ -14324,7 +14324,7 @@ CloneRowTriggersToPartition(Oid parentId, Oid 
partitionId)
 
CreateTrigger(trigStmt, NULL, partitionId,
  InvalidOid, InvalidOid, InvalidOid,
- trigForm->tgfoid, 
HeapTupleGetOid(tuple), false);
+ trigForm->tgfoid, 
HeapTupleGetOid(tuple), false, true);
pfree(trigStmt);
}
 
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index c4f63c8b90..6a857df566 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -151,7 +151,8 @@ static bool before_stmt_triggers_fired(Oid relid, CmdType 
cmdType);
 ObjectAddress
 CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
  Oid relOid, Oid refRelOid, Oid constraintOid, Oid 
indexOid,
- Oid funcoid, Oid parentTriggerOid, bool isInternal)
+ Oid funcoid, Oid parentTriggerOid, bool isInternal,
+ bool in_partition)
 {
int16   tgtype;
int ncolumns;
@@ -780,6 +781,11 @@ CreateTrigger(CreateTrigStmt *stmt, const char 
*queryString,
 
/*
 * Build the new pg_trigger tuple.
+*
+* When we're creating a trigger in a partition, we mark it as internal,
+* even though we don't do the isInternal magic in this function.  This
+* makes the triggers in partitions identical to the ones in the
+* partitioned tables, except that they are marked internal.
 */
memset(nulls, false, sizeof(nulls));
 
@@ -789,7 +795,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
values[Anum_pg_trigger_tgfoid - 1] = ObjectIdGetDatum(funcoid);
values[Anum_pg_trigger_tgtype - 1] = Int16GetDatum(tgtype);
values[Anum_pg_trigger_tgenabled - 1] = 
CharGetDatum(TRIGGER_FIRES_ON_ORIGIN);
-   values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal);
+   values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal || 
in_partition);
values[Anum_pg_trigger_tgconstrrelid - 1] = 
ObjectIdGetDatum(constrrelid);
values[Anum_pg_trigger_tgconstrindid - 1] = ObjectIdGetDatum(indexOid);
values[Anum_pg_trigger_tgconstraint - 1] = 
ObjectIdGetDatum(constraintOid);
@@ -1089,7 +1095,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char 
*queryString,

Re: FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

Alvaro Herrera wrote:

> One thing I'd like to add before claiming this committable (backend-
> side) is enabling constraint triggers.  AFAICT that requires a bit of
> additional logic, but it shouldn't be too terrible.  This would allow
> for deferrable unique constraints, for example.

v7 supports constraint triggers.  I added an example using a UNIQUE
DEFERRABLE constraint, and another one using plain CREATE CONSTRAINT TRIGGER.
It's neat to see that the WHEN clause is executed at the time of the
operation, and the trigger action is deferred (or not) till COMMIT time.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From a34e786924b54d94dbf28c182aed27d0e92dba06 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Thu, 8 Mar 2018 14:01:39 -0300
Subject: [PATCH v7 1/3] add missing CommandCounterIncrement in
 StorePartitionBound

---
 src/backend/catalog/heap.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index cf36ce4add..2b5377bdf2 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -3299,6 +3299,9 @@ StorePartitionBound(Relation rel, Relation parent, 
PartitionBoundSpec *bound)
heap_freetuple(newtuple);
heap_close(classRel, RowExclusiveLock);
 
+   /* Make update visible */
+   CommandCounterIncrement();
+
/*
 * The partition constraint for the default partition depends on the
 * partition bounds of every other partition, so we must invalidate the
-- 
2.11.0

>From 1165c0438c627ea214de9ee4cffa83d89b0aa485 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Thu, 8 Mar 2018 14:04:13 -0300
Subject: [PATCH v7 2/3] Add missing CommandCounterIncrement() in partitioned
 index code

---
 src/backend/catalog/pg_constraint.c | 4 
 src/backend/commands/indexcmds.c| 6 ++
 src/backend/commands/tablecmds.c| 2 --
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/backend/catalog/pg_constraint.c 
b/src/backend/catalog/pg_constraint.c
index 731c5e4317..38fdf72877 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -18,6 +18,7 @@
 #include "access/heapam.h"
 #include "access/htup_details.h"
 #include "access/sysattr.h"
+#include "access/xact.h"
 #include "catalog/dependency.h"
 #include "catalog/indexing.h"
 #include "catalog/objectaccess.h"
@@ -781,6 +782,9 @@ ConstraintSetParentConstraint(Oid childConstrId, Oid 
parentConstrId)
recordDependencyOn(, , DEPENDENCY_INTERNAL_AUTO);
 
heap_close(constrRel, RowExclusiveLock);
+
+   /* make update visible */
+   CommandCounterIncrement();
 }
 
 
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 504806b25b..9ca632865b 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1003,6 +1003,9 @@ DefineIndex(Oid relationId,
ReleaseSysCache(tup);
heap_close(pg_index, RowExclusiveLock);
heap_freetuple(newtup);
+
+   /* make update visible */
+   CommandCounterIncrement();
}
}
else
@@ -2512,5 +2515,8 @@ IndexSetParentIndex(Relation partitionIdx, Oid parentOid)
 
recordDependencyOn(, , 
DEPENDENCY_AUTO);
}
+
+   /* make our updates visible */
+   CommandCounterIncrement();
}
 }
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 74e020bffc..7ecfbc17a0 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -14571,8 +14571,6 @@ ATExecAttachPartitionIdx(List **wqueue, Relation 
parentIdx, RangeVar *name)
 
pfree(attmap);
 
-   CommandCounterIncrement();
-
validatePartitionedIndex(parentIdx, parentTbl);
}
 
-- 
2.11.0

>From 975902aa033877165d0aaec556edc09bb02808c7 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Tue, 21 Nov 2017 15:53:11 -0300
Subject: [PATCH v7 3/3] Allow FOR EACH ROW triggers on partitioned tables

---
 src/backend/catalog/heap.c |   1 +
 src/backend/catalog/index.c|   4 +-
 src/backend/catalog/pg_constraint.c|   3 +
 src/backend/commands/tablecmds.c   |  92 +-
 src/backend/commands/trigger.c | 183 +--
 src/backend/commands/typecmds.c|   1 +
 src/backend/tcop/utility.c |   3 +-
 src/include/catalog/indexing.h |   2 +
 src/include/catalog/pg_constraint.h|  39 ++--
 src/incl

Re: FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

Peter Eisentraut wrote:
> On 3/7/18 20:57, Alvaro Herrera wrote:
> > So, unless someone has a brilliant idea on how to construct a column
> > mapping from partitioned table to partition, I'm going back to the
> > design I was proposing earlier, ie., creating individual pg_trigger rows
> > for each partition that are essentially adjusted copies of the ones for
> > the partitioned table.
> 
> Yes, that seems easiest.
> 
> The idea of having only one pg_trigger entry was derived from the
> assumption that we wouldn't need the other ones for anything.  But if
> that doesn't apply, then it's better to just go with the straightforward
> way instead of bending the single-pg_trigger way to our will.

Agreed.

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

Re: FOR EACH ROW triggers on partitioned tables

[Peter Eisentraut]

On 3/7/18 20:57, Alvaro Herrera wrote:
> So, unless someone has a brilliant idea on how to construct a column
> mapping from partitioned table to partition, I'm going back to the
> design I was proposing earlier, ie., creating individual pg_trigger rows
> for each partition that are essentially adjusted copies of the ones for
> the partitioned table.

Yes, that seems easiest.

The idea of having only one pg_trigger entry was derived from the
assumption that we wouldn't need the other ones for anything.  But if
that doesn't apply, then it's better to just go with the straightforward
way instead of bending the single-pg_trigger way to our will.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 74e020bffc..7ecfbc17a0 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -14571,8 +14571,6 @@ ATExecAttachPartitionIdx(List **wqueue, Relation 
parentIdx, RangeVar *name)
 
pfree(attmap);
 
-   CommandCounterIncrement();
-
validatePartitionedIndex(parentIdx, parentTbl);
}
 
-- 
2.11.0

>From 40301760e580901379953f400c8c992917234556 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Tue, 21 Nov 2017 15:53:11 -0300
Subject: [PATCH v6 3/3] Allow FOR EACH ROW triggers on partitioned tables

---
 src/backend/catalog/index.c|   3 +-
 src/backend/commands/tablecmds.c   |  91 -
 src/backend/commands/trigger.c | 137 ++--
 src/backend/tcop/utility.c |   3 +-
 src/include/commands/trigger.h |   4 +-
 src/test/regress/expected/triggers.out | 228 +
 src/test/regress/sql/triggers.sql  | 142 ++--
 7 files changed, 550 insertions(+), 58 deletions(-)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 431bc31969..1195064954 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1361,7 +1361,8 @@ index_constraint_create(Relation heapRelation,
trigger->constrrel = NULL;
 
(void) CreateTrigger(trigger, NULL, 
RelationGetRelid(heapRelation),
-InvalidOid, conOid, 
indexRelationId, true);
+InvalidOid, conOid, 
indexRelationId, InvalidOid,
+InvalidOid, true);
}
 
/*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7ecfbc17a0..480f1b3996 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -487,6 +487,7 @@ static void ValidatePartitionConstraints(List **wqueue, 
Relation scanrel,
 List *scanrel_children,
 List *partConstraint,
 bool validate_default);
+static void CloneRowTriggersToPartition(Oid parentId, Oid partitionId);
 static ObjectAddress ATExecDetachPartition(Relation rel, RangeVar *name);
 static ObjectAddress ATExecAttachPartitionIdx(List **wqueue, Relation rel,
 RangeVar *name);
@@ -916,9 +917,11 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
}
 
/*
-* If we're creating a partition, create now all the indexes defined in
-* the parent.  We can't do it earlier, because DefineIndex wants to 
know
-* the partition key which we just stored.
+* If we're creating a partition, create now all the indexes and 
triggers
+* defined in the parent.
+*
+* We can't do it earlier, because DefineIndex wants to know the 
partition
+* key which we just stored.
 */
if (stmt->partbound)
{
@@ -959,6 +962,14 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
}
 
list_free(idxlist);
+
+   /*
+* If there are any row-level triggers, clone them to the new
+* partition.
+*/
+   if (parent->trigdesc != NULL)
+   CloneRowTriggersToPartition(RelationGetRelid(parent), 
relationId);
+
heap_close(parent, NoLock);
}
 
@@ -8455,7 +8466,7 @@ CreateFKCheckTrigger(Oid myRelOid, Oid refRelOid, 
Constraint *fkconstraint,
fk_trigger->args = NIL;
 
(void) CreateTrigger(fk_trigger, NULL, myRelOid, refRelOid, 
constraintOid,
-indexOid, true);
+indexOid, InvalidOid, 
InvalidOid, true);
 
/* Make changes-so-far visible */
CommandCounterIncrement();
@@ -8529,7 +8540,7 @@ createForeignKeyTriggers(Relation rel, Oid refRelOid, 
Constraint *fkconstraint,
fk_trigger->args = NIL;
 
(void) CreateTrigger(fk_trigger, NULL, refRelOid, myRelOid, 
constraintOid,
-indexOid, true);
+indexOid, InvalidOid, 
InvalidOid, true);
 
/* Make changes-so-far visible */
CommandCounterIncrement();
@@ -8584,7 +8595,7 @@ createForeignKeyTriggers(Relation rel, Oid refRelOid, 
Constraint *fkconstraint,
fk_trigger->args = NIL;
 
(void) CreateTrigger(fk_trigger, NULL, refRelOid, myRelOid, 
constraintOid,
-indexOid, true);
+  

Re: FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

Alvaro Herrera wrote:

> I reserve the right to revise this further, as I'm going to spend a
> couple of hours looking at it this afternoon, particularly to see how
> concurrent DDL behaves, but I don't see anything obviously wrong with
> it.

I do now.  TLDR; I'm afraid this cute idea crashed and burned, so I'm
back to the idea of just cloning the pg_trigger row for each partition.

The reason for the failure is pg_trigger->tgqual, which is an expression
tree.  In most cases, when set, that expression will contain references
to columns of the table, in the form of a varattno.  But this varattno
references the column number of the partitioned table; and if the
partition happens to require some attribute mapping, we're screwed
because there is no way to construct that without forming the
partitioned table's tuple descriptor.  But we can't do that without
grabbing a lock on the partitioned table; and we can't do that because
we would incur the deadlock risk Robert was talking about.

An example that causes the problem is:

create table parted_irreg (fd int, a int, fd2 int, b text) partition by range 
(b);
alter table parted_irreg drop column fd, drop column fd2;
create table parted1_irreg (b text, fd int, a int);
alter table parted1_irreg drop column fd;
alter table parted_irreg attach partition parted1_irreg for values from 
('') to ('');
create trigger parted_trig after insert on parted_irreg for each row when 
(new.a % 1 = 0) execute procedure trigger_notice_irreg();
insert into parted_irreg values (1, 'aardvark');
insert into parted1_irreg values ('aardwolf', 2);
drop table parted_irreg;
drop function trigger_notice_irreg();

Both inserts fail thusly:

ERROR:  attribute 2 of type parted1_irreg has been dropped

Now, I can fix the first failure by taking advantage of
ResultRelInfo->ri_PartitionRoot during trigger execution; it's easy and
trouble-free to call map_variable_attnos() using that relation.  But in
the second insert, ri_PartitionRoot is null (because of inserting into
the partition directly), so we have no relation to refer to for
map_variable_attnos().  I think it gets worse: if you have a three-level
partitioning scheme, and define the trigger in the second one, there is
no relation either.

Another option I think would be to always keep in the trigger descriptor
("somehow"), an open Relation on which the trigger is defined.  But this
has all sorts of problems also, so I'm not doing that.

I guess another option is to store a column map somewhere.


So, unless someone has a brilliant idea on how to construct a column
mapping from partitioned table to partition, I'm going back to the
design I was proposing earlier, ie., creating individual pg_trigger rows
for each partition that are essentially adjusted copies of the ones for
the partitioned table.  The only missing thing in that one was having
ALTER TABLE ENABLE/DISABLE for a trigger on the partitioned table
cascade to the partitions; I'll see about that.

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

Re: FOR EACH ROW triggers on partitioned tables

[Thomas Munro]

On Thu, Mar 8, 2018 at 6:17 AM, Alvaro Herrera  wrote:
> Here's another version of this patch.  It is virtually identical to the
> previous one, except for a small doc update and whitespace changes.

What is this test for?

+create trigger failed after update on parted_trig
+  referencing old table as old_table
+  for each statement execute procedure trigger_nothing();

It doesn't fail as you apparently expected.  Perhaps it was supposed
to be "for each row" so you could hit your new error with
errdetail("Triggers on partitioned tables cannot have transition
tables.")?

-- 
Thomas Munro
http://www.enterprisedb.com

Re: FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

und)
+   if (is_partition)
{
Oid parentId = linitial_oid(inheritOids);
Relationparent;
@@ -1692,6 +1698,8 @@ storage_name(char c)
  * 'supconstr' receives a list of constraints belonging to the parents,
  * updated as necessary to be valid for the child.
  * 'supOidCount' is set to the number of parents that have OID columns.
+ * 'hasTriggers' is set to true if any parent has inheritable triggers,
+ * false otherwise.
  *
  * Return value:
  * Completed schema list.
@@ -1738,7 +1746,7 @@ storage_name(char c)
 static List *
 MergeAttributes(List *schema, List *supers, char relpersistence,
bool is_partition, List **supOids, List 
**supconstr,
-   int *supOidCount)
+   int *supOidCount, bool *hasTriggers)
 {
ListCell   *entry;
List   *inhSchema = NIL;
@@ -1750,6 +1758,8 @@ MergeAttributes(List *schema, List *supers, char 
relpersistence,
static Node bogus_marker = {0}; /* marks conflicting defaults */
List   *saved_schema = NIL;
 
+   *hasTriggers = false;
+
/*
 * Check for and reject tables with too many columns. We perform this
 * check relatively early for two reasons: (a) we don't run the risk of
@@ -2147,6 +2157,23 @@ MergeAttributes(List *schema, List *supers, char 
relpersistence,
pfree(newattno);
 
/*
+* If this parent has triggers, and any of them is marked 
inheritable,
+* set *hastriggers.
+*/
+   if (relation->rd_rel->relhastriggers &&
+   relation->trigdesc != NULL &&
+   !*hasTriggers)
+   {
+   int trg;
+
+   for (trg = 0; trg < relation->trigdesc->numtriggers; 
trg++)
+   {
+   if 
(relation->trigdesc->triggers[trg].tginherits)
+   *hasTriggers = true;
+   }
+   }
+
+   /*
 * Close the parent rel, but keep our lock on it until xact 
commit.
 * That will prevent someone else from deleting or ALTERing the 
parent
 * before the child is committed.
@@ -14248,6 +14275,9 @@ AttachPartitionEnsureIndexes(Relation rel, Relation 
attachrel)
index_close(idxRel, AccessShareLock);
}
 
+   /* Make this all visible */
+   CommandCounterIncrement();
+
/* Clean up. */
for (i = 0; i < list_length(attachRelIdxs); i++)
index_close(attachrelIdxRels[i], AccessShareLock);
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index fbd176b5d0..e3e46814f9 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -24,6 +24,7 @@
 #include "catalog/objectaccess.h"
 #include "catalog/pg_constraint.h"
 #include "catalog/pg_constraint_fn.h"
+#include "catalog/pg_inherits.h"
 #include "catalog/pg_inherits_fn.h"
 #include "catalog/pg_proc.h"
 #include "catalog/pg_trigger.h"
@@ -100,7 +101,17 @@ static void AfterTriggerSaveEvent(EState *estate, 
ResultRelInfo *relinfo,
  List *recheckIndexes, Bitmapset 
*modifiedCols,
  TransitionCaptureState 
*transition_capture);
 static void AfterTriggerEnlargeQueryState(void);
+static void SendTriggerRelcacheInval(Relation inheritsRel, Relation rel);
 static bool before_stmt_triggers_fired(Oid relid, CmdType cmdType);
+static void recursively_update_relhastriggers(Relation pg_class, Oid relid,
+ bool recurse);
+static void add_triggers_to_array(Relation tgrel, Relation inhrel,
+ Oid tgrelid, bool all_triggers,
+ Trigger **triggers, int *numtrigs, 
int *maxtrigs,
+ bool *must_sort);
+static void add_trigger_to_array(TupleDesc tgdesc, HeapTuple tgtup,
+Trigger **triggers, int *numtrigs, int 
*maxtrigs);
+static int qsort_trigger_cmp(const void *a, const void *b);
 
 
 /*
@@ -133,6 +144,9 @@ static bool before_stmt_triggers_fired(Oid relid, CmdType 
cmdType);
  * relation, as well as ACL_EXECUTE on the trigger function.  For internal
  * triggers the caller must apply any required permission checks.
  *
+ * When called on partitioned tables, FOR EACH ROW triggers are marked as
+ * applying on partitions too (ie. tginherits), except if isInternal.
+ *
  * Note: can return InvalidObjectAddress if we decided to not create a trigger
  * at all, but a foreign-key co

Re: FOR EACH ROW triggers on partitioned tables

[Robert Haas]

On Thu, Feb 22, 2018 at 6:52 PM, Alvaro Herrera  wrote:
> Alvaro Herrera wrote:
>> Another option is to rethink this feature from the ground up: instead of
>> cloning catalog rows for each children, maybe we should have the trigger
>> lookup code, when running DML on the child relation (the partition),
>> obtain trigger entries not only for the child relation itself but also
>> for its parents recursively -- so triggers defined in the parent are
>> fired for the partitions, too.
>
> I have written this, and it seems to work fine; it's attached.
>
> Generally speaking, I like this better than my previously proposed
> patch: having duplicate pg_trigger rows seems lame, in hindsight.
>
> I haven't measured the performance loss, but we now scan pg_inherits
> each time we build a relcache entry and relhastriggers=on.  Can't be
> good.  In general, the pg_inherits stuff looks generally unnatural --
> manually doing scans upwards (find parents) and downwards (find
> partitions).  It's messy and there are no nice abstractions.
> Partitioning looks too much bolted-on still.

Elsewhere, we've put a lot of blood, sweat, and tears into making sure
that we only traverse the inheritance hierarchy from top to bottom.
Otherwise, we're adding deadlock hazards.  I think it's categorically
unacceptable to do traversals in the opposite order -- if you do, then
an UPDATE on a child could deadlock with a LOCK TABLE on the parent.
That will not win us any awards.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

Amit Langote wrote:
> On Fri, Feb 23, 2018 at 11:32 AM, Alvaro Herrera
>  wrote:

> > Uh, wow, how have I missed that all this time!  Yes, it probably does.
> > I'll rework this tomorrow ... and the already committed index patch too,
> > I think.
> 
> BTW, not sure if you'd noticed but I had emailed about setting
> relispartition on index partitions after you committed the first
> indexes patch.

I hadn't noticed.  These days sadly I'm not able to keep up with all
pgsql-hackers traffic, and I'm quite likely to miss things unless I'm
CCed.  This seems true for many others, too.

Thanks for pointing it out.

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

Re: FOR EACH ROW triggers on partitioned tables

[Amit Langote]

On Fri, Feb 23, 2018 at 11:32 AM, Alvaro Herrera
 wrote:
> Amit Langote wrote:
>> On 2018/02/23 8:52, Alvaro Herrera wrote:
>> > We could mitigate the performance loss to some extent by adding more to
>> > RelationData.  For example, a "is_partition" boolean would help: skip
>> > searching pg_inherits for a relation that is not a partition.
>>
>> Unless I'm missing something, doesn't rd_rel->relispartition help?
>
> Uh, wow, how have I missed that all this time!  Yes, it probably does.
> I'll rework this tomorrow ... and the already committed index patch too,
> I think.

BTW, not sure if you'd noticed but I had emailed about setting
relispartition on index partitions after you committed the first
indexes patch.

https://www.postgresql.org/message-id/12085bc4-0bc6-0f3a-4c43-57fe06817...@lab.ntt.co.jp

Thanks,
Amit

Re: FOR EACH ROW triggers on partitioned tables

[Amit Langote]

On 2018/02/23 8:52, Alvaro Herrera wrote:
> We could mitigate the performance loss to some extent by adding more to
> RelationData.  For example, a "is_partition" boolean would help: skip
> searching pg_inherits for a relation that is not a partition.
Unless I'm missing something, doesn't rd_rel->relispartition help?

Thanks,
Amit

Re: FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

riggers' is set to true if any parent has inheritable triggers,
+ * false otherwise.
  *
  * Return value:
  * Completed schema list.
@@ -1738,7 +1746,7 @@ storage_name(char c)
 static List *
 MergeAttributes(List *schema, List *supers, char relpersistence,
bool is_partition, List **supOids, List 
**supconstr,
-   int *supOidCount)
+   int *supOidCount, bool *hasTriggers)
 {
ListCell   *entry;
List   *inhSchema = NIL;
@@ -1750,6 +1758,8 @@ MergeAttributes(List *schema, List *supers, char 
relpersistence,
static Node bogus_marker = {0}; /* marks conflicting defaults */
List   *saved_schema = NIL;
 
+   *hasTriggers = false;
+
/*
 * Check for and reject tables with too many columns. We perform this
 * check relatively early for two reasons: (a) we don't run the risk of
@@ -2147,6 +2157,23 @@ MergeAttributes(List *schema, List *supers, char 
relpersistence,
pfree(newattno);
 
/*
+* If this parent has triggers, and any of them is marked 
inheritable,
+* set *hastriggers.
+*/
+   if (relation->rd_rel->relhastriggers &&
+   relation->trigdesc != NULL &&
+   !*hasTriggers)
+   {
+   int trg;
+
+   for (trg = 0; trg < relation->trigdesc->numtriggers; 
trg++)
+   {
+   if 
(relation->trigdesc->triggers[trg].tginherits)
+   *hasTriggers = true;
+   }
+   }
+
+   /*
 * Close the parent rel, but keep our lock on it until xact 
commit.
 * That will prevent someone else from deleting or ALTERing the 
parent
 * before the child is committed.
@@ -14248,6 +14275,9 @@ AttachPartitionEnsureIndexes(Relation rel, Relation 
attachrel)
index_close(idxRel, AccessShareLock);
}
 
+   /* Make this all visible */
+   CommandCounterIncrement();
+
/* Clean up. */
for (i = 0; i < list_length(attachRelIdxs); i++)
index_close(attachrelIdxRels[i], AccessShareLock);
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index fffc0095a7..23d669cf26 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -24,6 +24,7 @@
 #include "catalog/objectaccess.h"
 #include "catalog/pg_constraint.h"
 #include "catalog/pg_constraint_fn.h"
+#include "catalog/pg_inherits.h"
 #include "catalog/pg_inherits_fn.h"
 #include "catalog/pg_proc.h"
 #include "catalog/pg_trigger.h"
@@ -100,7 +101,17 @@ static void AfterTriggerSaveEvent(EState *estate, 
ResultRelInfo *relinfo,
  List *recheckIndexes, Bitmapset 
*modifiedCols,
  TransitionCaptureState 
*transition_capture);
 static void AfterTriggerEnlargeQueryState(void);
+static void SendTriggerRelcacheInval(Relation inheritsRel, Relation rel);
 static bool before_stmt_triggers_fired(Oid relid, CmdType cmdType);
+static void recursively_update_relhastriggers(Relation pg_class, Oid relid,
+ bool recurse);
+static void add_triggers_to_array(Relation tgrel, Relation inhrel,
+ Oid tgrelid, bool all_triggers,
+ Trigger **triggers, int *numtrigs, 
int *maxtrigs,
+ bool *must_sort);
+static void add_trigger_to_array(TupleDesc tgdesc, HeapTuple tgtup,
+Trigger **triggers, int *numtrigs, int 
*maxtrigs);
+static int qsort_trigger_cmp(const void *a, const void *b);
 
 
 /*
@@ -133,6 +144,9 @@ static bool before_stmt_triggers_fired(Oid relid, CmdType 
cmdType);
  * relation, as well as ACL_EXECUTE on the trigger function.  For internal
  * triggers the caller must apply any required permission checks.
  *
+ * When called on partitioned tables, FOR EACH ROW triggers are marked as
+ * applying on partitions too (ie. tginherits), except if isInternal.
+ *
  * Note: can return InvalidObjectAddress if we decided to not create a trigger
  * at all, but a foreign-key constraint.  This is a kluge for backwards
  * compatibility.
@@ -149,6 +163,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
Node   *whenClause;
List   *whenRtable;
char   *qual;
+   booltginherits;
Datum   values[Natts_pg_trigger];
boolnulls[Natts_pg_trigger];
Relationrel;
@@ -179,8 +194,7 @@ CreateTr

Re: FOR EACH ROW triggers on partitioned tables

[Peter Eisentraut]

On 2/15/18 16:55, Alvaro Herrera wrote:
> Amit Langote wrote:
>> Do you mean to fire these triggers only if the parent table (not a child
>> table/partition) is addressed in the DML, right?  If the table directly
>> addressed in the DML is a partition whose parent has a row-level trigger,
>> then that trigger should not get fired I suppose.
> 
> No, I think that would be strange and cause data inconsistencies.
> Inserting directly into the partition is seen as a performance
> optimization (compared to inserted into the partitioned table), so we
> don't get to skip firing the triggers defined on the parent because the
> behavior would become different.  In other words, the performance
> optimization breaks the database.
> 
> Example: suppose the trigger is used to maintain an audit record trail.

Although this situation could probably be addressed by not giving
permission to write directly into the partitions, I can't think of an
example where one would want a trigger that is only fired when writing
into the partition root rather than into the partition directly.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: FOR EACH ROW triggers on partitioned tables

[Amit Langote]

On 2018/02/16 6:55, Alvaro Herrera wrote:
> Amit Langote wrote:
>> On 2018/02/15 6:26, Alvaro Herrera wrote:
>>> Another option is to rethink this feature from the ground up: instead of
>>> cloning catalog rows for each children, maybe we should have the trigger
>>> lookup code, when running DML on the child relation (the partition),
>>> obtain trigger entries not only for the child relation itself but also
>>> for its parents recursively -- so triggers defined in the parent are
>>> fired for the partitions, too.  I'm not sure what implications this has
>>> for constraint triggers.
>>>
>>> The behavior should be the same, except that you cannot modify the
>>> trigger (firing conditions, etc) on the partition individually -- it
>>> works at the level of the whole partitioned table instead.
>>
>> Do you mean to fire these triggers only if the parent table (not a child
>> table/partition) is addressed in the DML, right?  If the table directly
>> addressed in the DML is a partition whose parent has a row-level trigger,
>> then that trigger should not get fired I suppose.
> 
> No, I think that would be strange and cause data inconsistencies.
> Inserting directly into the partition is seen as a performance
> optimization (compared to inserted into the partitioned table), so we
> don't get to skip firing the triggers defined on the parent because the
> behavior would become different.  In other words, the performance
> optimization breaks the database.

OK, that makes sense.

Thanks,
Amit

Re: FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

Amit Langote wrote:
> On 2018/02/15 6:26, Alvaro Herrera wrote:
> > Another option is to rethink this feature from the ground up: instead of
> > cloning catalog rows for each children, maybe we should have the trigger
> > lookup code, when running DML on the child relation (the partition),
> > obtain trigger entries not only for the child relation itself but also
> > for its parents recursively -- so triggers defined in the parent are
> > fired for the partitions, too.  I'm not sure what implications this has
> > for constraint triggers.
> >
> > The behavior should be the same, except that you cannot modify the
> > trigger (firing conditions, etc) on the partition individually -- it
> > works at the level of the whole partitioned table instead.
> 
> Do you mean to fire these triggers only if the parent table (not a child
> table/partition) is addressed in the DML, right?  If the table directly
> addressed in the DML is a partition whose parent has a row-level trigger,
> then that trigger should not get fired I suppose.

No, I think that would be strange and cause data inconsistencies.
Inserting directly into the partition is seen as a performance
optimization (compared to inserted into the partitioned table), so we
don't get to skip firing the triggers defined on the parent because the
behavior would become different.  In other words, the performance
optimization breaks the database.

Example: suppose the trigger is used to maintain an audit record trail.

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

Re: FOR EACH ROW triggers on partitioned tables

[Amit Langote]

On 2018/02/15 6:26, Alvaro Herrera wrote:
> Another option is to rethink this feature from the ground up: instead of
> cloning catalog rows for each children, maybe we should have the trigger
> lookup code, when running DML on the child relation (the partition),
> obtain trigger entries not only for the child relation itself but also
> for its parents recursively -- so triggers defined in the parent are
> fired for the partitions, too.  I'm not sure what implications this has
> for constraint triggers.
>
> The behavior should be the same, except that you cannot modify the
> trigger (firing conditions, etc) on the partition individually -- it
> works at the level of the whole partitioned table instead.

Do you mean to fire these triggers only if the parent table (not a child
table/partition) is addressed in the DML, right?  If the table directly
addressed in the DML is a partition whose parent has a row-level trigger,
then that trigger should not get fired I suppose.

Thanks,
Amit

Re: FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

g1 BEFORE UPDATE for ROW
-NOTICE:  trigger on parted2_stmt_trig BEFORE UPDATE for STATEMENT
-NOTICE:  trigger on parted_stmt_trig1 AFTER UPDATE for ROW
-NOTICE:  trigger on parted_stmt_trig AFTER UPDATE for STATEMENT
-NOTICE:  trigger on parted2_stmt_trig AFTER UPDATE for STATEMENT
+NOTICE:  trigger trig_upd_before on parted_stmt_trig BEFORE UPDATE for 
STATEMENT
+NOTICE:  trigger trig_upd_before on parted_stmt_trig1 BEFORE UPDATE for ROW
+NOTICE:  trigger trig_upd_before on parted2_stmt_trig BEFORE UPDATE for 
STATEMENT
+NOTICE:  trigger trig_upd_after on parted_stmt_trig1 AFTER UPDATE for ROW
+NOTICE:  trigger trig_upd_after on parted_stmt_trig AFTER UPDATE for STATEMENT
+NOTICE:  trigger trig_upd_after on parted2_stmt_trig AFTER UPDATE for STATEMENT
 delete from parted_stmt_trig;
-NOTICE:  trigger on parted_stmt_trig BEFORE DELETE for STATEMENT
-NOTICE:  trigger on parted_stmt_trig AFTER DELETE for STATEMENT
+NOTICE:  trigger trig_del_before on parted_stmt_trig BEFORE DELETE for 
STATEMENT
+NOTICE:  trigger trig_del_after on parted_stmt_trig AFTER DELETE for STATEMENT
 -- insert via copy on the parent
 copy parted_stmt_trig(a) from stdin;
-NOTICE:  trigger on parted_stmt_trig BEFORE INSERT for STATEMENT
-NOTICE:  trigger on parted_stmt_trig1 BEFORE INSERT for ROW
-NOTICE:  trigger on parted_stmt_trig1 AFTER INSERT for ROW
-NOTICE:  trigger on parted_stmt_trig AFTER INSERT for STATEMENT
+NOTICE:  trigger trig_ins_before on parted_stmt_trig BEFORE INSERT for 
STATEMENT
+NOTICE:  trigger trig_ins_before on parted_stmt_trig1 BEFORE INSERT for ROW
+NOTICE:  trigger trig_ins_after on parted_stmt_trig1 AFTER INSERT for ROW
+NOTICE:  trigger trig_ins_after on parted_stmt_trig AFTER INSERT for STATEMENT
 -- insert via copy on the first partition
 copy parted_stmt_trig1(a) from stdin;
-NOTICE:  trigger on parted_stmt_trig1 BEFORE INSERT for ROW
-NOTICE:  trigger on parted_stmt_trig1 AFTER INSERT for ROW
+NOTICE:  trigger trig_ins_before on parted_stmt_trig1 BEFORE INSERT for ROW
+NOTICE:  trigger trig_ins_after on parted_stmt_trig1 AFTER INSERT for ROW
 drop table parted_stmt_trig, parted2_stmt_trig;
 --
 -- Test the interaction between transition tables and both kinds of
diff --git a/src/test/regress/sql/triggers.sql 
b/src/test/regress/sql/triggers.sql
index dba9bdd98b..ae8349ccbf 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1317,7 +1317,7 @@ create table parted2_stmt_trig2 partition of 
parted2_stmt_trig for values in (2)
 
 create or replace function trigger_notice() returns trigger as $$
   begin
-raise notice 'trigger on % % % for %', TG_TABLE_NAME, TG_WHEN, TG_OP, 
TG_LEVEL;
+raise notice 'trigger % on % % % for %', TG_NAME, TG_TABLE_NAME, TG_WHEN, 
TG_OP, TG_LEVEL;
 if TG_LEVEL = 'ROW' then
return NEW;
 end if;
-- 
2.11.0

>From 2ba9e8eaa53284eed1eefb137501ed1a22f4320b Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Tue, 21 Nov 2017 15:53:11 -0300
Subject: [PATCH v3 2/3] Allow FOR EACH ROW triggers on partitioned tables

---
 src/backend/catalog/index.c|   3 +-
 src/backend/commands/tablecmds.c   |  87 +--
 src/backend/commands/trigger.c | 124 +++---
 src/backend/tcop/utility.c |   3 +-
 src/include/commands/trigger.h |   2 +-
 src/test/regress/expected/triggers.out | 154 -
 src/test/regress/sql/triggers.sql  |  87 ---
 7 files changed, 405 insertions(+), 55 deletions(-)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index f2cb6d7fb8..04f70d9a86 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1321,7 +1321,8 @@ index_constraint_create(Relation heapRelation,
trigger->constrrel = NULL;
 
(void) CreateTrigger(trigger, NULL, 
RelationGetRelid(heapRelation),
-InvalidOid, conOid, 
indexRelationId, true);
+InvalidOid, conOid, 
indexRelationId, InvalidOid,
+InvalidOid, true);
}
 
/*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 89454d8e80..a9d551d8c7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -487,6 +487,7 @@ static void ValidatePartitionConstraints(List **wqueue, 
Relation scanrel,
 List *scanrel_children,
 List *partConstraint,
 bool validate_default);
+static void CloneRowTriggersOnPartition(Oid parentId, Oid partitionId);
 static ObjectAddress ATExecDetachPartition(Relation rel, RangeVar *name);
 static ObjectAddress ATExecAttachPartitionIdx(List **wqueue, Relation

Re: FOR EACH ROW triggers on partitioned tables

[Peter Eisentraut]

Moved to next commit fest.

There is some work to be done, but there appears to be a straight path
to success.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [Sender Address Forgery]Re: FOR EACH ROW triggers on partitioned tables

[Amit Langote]

On 2018/01/31 9:44, Peter Eisentraut wrote:
> On 1/30/18 04:49, Amit Langote wrote:
>>> If you are writing a generic
>>> trigger function, maybe to dump out all columns, you want to know the
>>> physical table and its actual columns.  It's easy[citation needed] to
>>> get the partition root for a given table, if the trigger code needs
>>> that.  The other way around is not possible.
>>
>> I guess you mean the root where a trigger originated, that is, ancestor
>> table on which an inherited trigger was originally defined.  It is
>> possible for a trigger to be defined on an intermediate parent and not the
>> topmost root in a partition tree.
> 
> OK, so maybe not so "easy".
> 
> But this muddies the situation even further.  You could be updating
> table A, which causes an update in intermediate partition B, which
> causes an update in leaf partition C, which fires a trigger that was
> logically defined on B and has a local child on C.  Under this proposal,
> the trigger will see TG_RELNAME = C.  You could make arguments that the
> trigger should also somehow know about B (where the trigger was defined)
> and A (what the user actually targeted in their statement).  I'm not
> sure how useful these would be.  But if you want to cover everything,
> you'll need three values.
> 
> I think the patch can go ahead as proposed, and the other things could
> be future separate additions.

Yeah, I see no problem with going ahead with the patch as it for now.

Thanks,
Amit

Re: FOR EACH ROW triggers on partitioned tables

[Peter Eisentraut]

On 1/30/18 04:49, Amit Langote wrote:
>> If you are writing a generic
>> trigger function, maybe to dump out all columns, you want to know the
>> physical table and its actual columns.  It's easy[citation needed] to
>> get the partition root for a given table, if the trigger code needs
>> that.  The other way around is not possible.
> 
> I guess you mean the root where a trigger originated, that is, ancestor
> table on which an inherited trigger was originally defined.  It is
> possible for a trigger to be defined on an intermediate parent and not the
> topmost root in a partition tree.

OK, so maybe not so "easy".

But this muddies the situation even further.  You could be updating
table A, which causes an update in intermediate partition B, which
causes an update in leaf partition C, which fires a trigger that was
logically defined on B and has a local child on C.  Under this proposal,
the trigger will see TG_RELNAME = C.  You could make arguments that the
trigger should also somehow know about B (where the trigger was defined)
and A (what the user actually targeted in their statement).  I'm not
sure how useful these would be.  But if you want to cover everything,
you'll need three values.

I think the patch can go ahead as proposed, and the other things could
be future separate additions.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: FOR EACH ROW triggers on partitioned tables

[Amit Langote]

On 2018/01/30 5:30, Peter Eisentraut wrote:
> On 1/23/18 17:10, Alvaro Herrera wrote:
>> The main question is this: when running the trigger function, it is
>> going to look as it is running in the context of the partition, not in
>> the context of the parent partitioned table (TG_RELNAME etc).  That
>> seems mildly ugly: some users may be expecting that the partitioning
>> stuff is invisible to the rest of the system, so if you have triggers on
>> a regular table and later on decide to partition that table, the
>> behavior of triggers will change, which is maybe unexpected.  Maybe this
>> is not really a problem, but I'm not sure and would like further
>> opinions.
> 
> One could go either way on this, but I think reporting the actual table
> partition is acceptable and preferable.

+1

> If you are writing a generic
> trigger function, maybe to dump out all columns, you want to know the
> physical table and its actual columns.  It's easy[citation needed] to
> get the partition root for a given table, if the trigger code needs
> that.  The other way around is not possible.

I guess you mean the root where a trigger originated, that is, ancestor
table on which an inherited trigger was originally defined.  It is
possible for a trigger to be defined on an intermediate parent and not the
topmost root in a partition tree.

I see that the only parent-child relationship for triggers created
recursively is recorded in the form of a dependency.  I wonder why not a
flag in, say, pg_trigger to indicate that a trigger may have been created
recursively.  With the committed for inherited indexes, I can see that
inheritance is explicitly recorded in pg_inherits because indexes are
relations, so it's possible in the indexes' case to get the parent in
which a given inherited index originated.

> Similarly, transition tables should be OK.  You only get the current
> partition to look at, of course.

+1

> The function name CloneRowTriggersOnPartition() confused me.  A more
> accurate phrasing might be CloneRowTriggersToPartition(), or maybe
> reword altogether.

CloneRowTriggers*For*Partition()?

Thanks,
Amit

Re: FOR EACH ROW triggers on partitioned tables

[Peter Eisentraut]

On 1/23/18 17:10, Alvaro Herrera wrote:
> The main question is this: when running the trigger function, it is
> going to look as it is running in the context of the partition, not in
> the context of the parent partitioned table (TG_RELNAME etc).  That
> seems mildly ugly: some users may be expecting that the partitioning
> stuff is invisible to the rest of the system, so if you have triggers on
> a regular table and later on decide to partition that table, the
> behavior of triggers will change, which is maybe unexpected.  Maybe this
> is not really a problem, but I'm not sure and would like further
> opinions.

One could go either way on this, but I think reporting the actual table
partition is acceptable and preferable.  If you are writing a generic
trigger function, maybe to dump out all columns, you want to know the
physical table and its actual columns.  It's easy[citation needed] to
get the partition root for a given table, if the trigger code needs
that.  The other way around is not possible.

Some other comments are reading the patch:

It seems to generally follow the patterns of the partitioned indexes
patch, which is good.

I think WHEN clauses on partition triggers should be OK.  I don't see a
reason to disallow them.

Similarly, transition tables should be OK.  You only get the current
partition to look at, of course.

The function name CloneRowTriggersOnPartition() confused me.  A more
accurate phrasing might be CloneRowTriggersToPartition(), or maybe
reword altogether.

New CommandCounterIncrement() call in AttachPartitionEnsureIndexes()
should be explained.  Or maybe it belongs in ATExecAttachPartition()
between the calls to AttachPartitionEnsureIndexes() and
CloneRowTriggersOnPartition()?

Prohibition against constraint triggers is unclear.  The subsequent
foreign-key patches mess with that further.  It's not clear to me why
constraint triggers shouldn't be allowed like normal triggers.

Obvious missing things: documentation, pg_dump, psql updates

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: FOR EACH ROW triggers on partitioned tables

[Robert Haas]

On Tue, Jan 23, 2018 at 5:10 PM, Alvaro Herrera  wrote:
> The main question is this: when running the trigger function, it is
> going to look as it is running in the context of the partition, not in
> the context of the parent partitioned table (TG_RELNAME etc).  That
> seems mildly ugly: some users may be expecting that the partitioning
> stuff is invisible to the rest of the system, so if you have triggers on
> a regular table and later on decide to partition that table, the
> behavior of triggers will change, which is maybe unexpected.  Maybe this
> is not really a problem, but I'm not sure and would like further
> opinions.

It doesn't seem either great or horrible.

Also, what about logical replication?  Amit just raised this issue for
the UPDATE row movement patch, and it seems like the issues are
similar here.  If somebody's counting on the same kinds of per-row
triggers to fire during logical replication as we do during the
original operation, they will be disappointed.

>> Also, does ALTER TABLE ... ENABLE/DISABLE TRIGGER do the right things on
>> partitioned tables?
>
> Haven't done this yet, either.  I like Simon's suggestion of outright
> disallowing this.

Why not just make it work?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: FOR EACH ROW triggers on partitioned tables

[Simon Riggs]

On 3 January 2018 at 03:12, Peter Eisentraut
<peter.eisentr...@2ndquadrant.com> wrote:
> On 12/29/17 17:53, Alvaro Herrera wrote:
>> This patch enables FOR EACH ROW triggers on partitioned tables.
>>
>> As presented, this patch is sufficient to discuss the semantics that we
>> want for triggers on partitioned tables, which is the most pressing
>> question here ISTM.
>
> This seems pretty straightforward.  What semantics questions do you have?

I see the patch imposes these restrictions

* AFTER TRIGGERS only

* No transition tables

* No WHEN clause

All of which might be removed/extended at some later date

So that's all good... there's not much here, so easy to commit soon.

>> However, this is incomplete: it doesn't create triggers when you do
>> ALTER TABLE ATTACH PARTITION or by CREATE TABLE PARTITION OF.  I'm using
>> this as a basis on which to try foreign keys for partitioned tables.
>> Getting this to committable status requires adding those features.
>
> Yeah that, and also perhaps preventing the removal of triggers from
> partitions if they are supposed to be on the whole partition hierarchy.

+1

> And then make pg_dump do the right things.  That's all mostly legwork, I
> think.
>
> Also, does ALTER TABLE ... ENABLE/DISABLE TRIGGER do the right things on
> partitioned tables?

Not sure I care about that, since it just breaks FKs and other things,
but we can add it later.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: FOR EACH ROW triggers on partitioned tables

[Peter Eisentraut]

On 12/29/17 17:53, Alvaro Herrera wrote:
> This patch enables FOR EACH ROW triggers on partitioned tables.
> 
> As presented, this patch is sufficient to discuss the semantics that we
> want for triggers on partitioned tables, which is the most pressing
> question here ISTM.

This seems pretty straightforward.  What semantics questions do you have?

> However, this is incomplete: it doesn't create triggers when you do
> ALTER TABLE ATTACH PARTITION or by CREATE TABLE PARTITION OF.  I'm using
> this as a basis on which to try foreign keys for partitioned tables.
> Getting this to committable status requires adding those features.

Yeah that, and also perhaps preventing the removal of triggers from
partitions if they are supposed to be on the whole partition hierarchy.
And then make pg_dump do the right things.  That's all mostly legwork, I
think.

Also, does ALTER TABLE ... ENABLE/DISABLE TRIGGER do the right things on
partitioned tables?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

FOR EACH ROW triggers on partitioned tables

[Alvaro Herrera]

This patch enables FOR EACH ROW triggers on partitioned tables.

As presented, this patch is sufficient to discuss the semantics that we
want for triggers on partitioned tables, which is the most pressing
question here ISTM.

However, this is incomplete: it doesn't create triggers when you do
ALTER TABLE ATTACH PARTITION or by CREATE TABLE PARTITION OF.  I'm using
this as a basis on which to try foreign keys for partitioned tables.
Getting this to committable status requires adding those features.

This is essentially the same patch I posted as 0003 in
https://postgr.es/m/20171220194937.pldcecyx7yrwmgkg@alvherre.pgsql

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 4caccf4dc0ab25de37a109170052f98273450468 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Tue, 21 Nov 2017 15:53:11 -0300
Subject: [PATCH v1] Allow FOR EACH ROW triggers on partitioned tables

---
 src/backend/commands/trigger.c | 86 +++---
 src/test/regress/expected/triggers.out | 83 +++-
 src/test/regress/sql/triggers.sql  | 63 -
 3 files changed, 202 insertions(+), 30 deletions(-)

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 92ae3822d8..eb6b25b28c 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -133,6 +133,10 @@ static bool before_stmt_triggers_fired(Oid relid, CmdType 
cmdType);
  * relation, as well as ACL_EXECUTE on the trigger function.  For internal
  * triggers the caller must apply any required permission checks.
  *
+ * When called on partitioned tables, this function recurses to create the
+ * trigger on all the partitions, except if isInternal is true, in which
+ * case caller is expected to execute recursion on its own.
+ *
  * Note: can return InvalidObjectAddress if we decided to not create a trigger
  * at all, but a foreign-key constraint.  This is a kluge for backwards
  * compatibility.
@@ -179,8 +183,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
 * Triggers must be on tables or views, and there are additional
 * relation-type-specific restrictions.
 */
-   if (rel->rd_rel->relkind == RELKIND_RELATION ||
-   rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+   if (rel->rd_rel->relkind == RELKIND_RELATION)
{
/* Tables can't have INSTEAD OF triggers */
if (stmt->timing != TRIGGER_TYPE_BEFORE &&
@@ -190,13 +193,59 @@ CreateTrigger(CreateTrigStmt *stmt, const char 
*queryString,
 errmsg("\"%s\" is a table",

RelationGetRelationName(rel)),
 errdetail("Tables cannot have INSTEAD 
OF triggers.")));
-   /* Disallow ROW triggers on partitioned tables */
-   if (stmt->row && rel->rd_rel->relkind == 
RELKIND_PARTITIONED_TABLE)
+   }
+   else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+   {
+   /* Partitioned tables can't have INSTEAD OF triggers */
+   if (stmt->timing != TRIGGER_TYPE_BEFORE &&
+   stmt->timing != TRIGGER_TYPE_AFTER)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-errmsg("\"%s\" is a partitioned table",
+errmsg("\"%s\" is a table",

RelationGetRelationName(rel)),
-errdetail("Partitioned tables cannot 
have ROW triggers.")));
+errdetail("Tables cannot have INSTEAD 
OF triggers.")));
+   /*
+* FOR EACH ROW triggers have further restrictions
+*/
+   if (stmt->row)
+   {
+   /*
+* Disallow WHEN clauses; I think it's okay, but 
disallow for now
+* to reduce testing surface.
+*/
+   if (stmt->whenClause)
+   ereport(ERROR,
+   
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+errmsg("\"%s\" is a 
partitioned table",
+   
RelationGetRelationName(rel)),
+errdetail("Triggers FOR EACH 
ROW on partitioned table cannot have WHEN clauses.")));
+
+