Date: Mon, 24 Feb 2020 15:58:50 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>,
	Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: pg_trigger.tgparentid
Message-ID: <20200224185850.GA30899@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="vkogqOf2sHV7VnPd"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CA+HiwqHCcE57ARm0ZtU5nDg7VZgBTf8BU1hn7H7Om7h7wserVg@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk


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

On 2020-Feb-19, Amit Langote wrote:

> Or:
> 
>          * However, if our parent is a partition itself, there might be
>          * internal triggers that must not be skipped.  For example, triggers
>          * on the parent that were in turn cloned from its own parent are
>          * marked internal, which must be cloned to the partition.

Thanks for pointing this out -- I agree it needed rewording.  I slightly
adjusted your text like this:

		 * Internal triggers require careful examination.  Ideally, we don't
		 * clone them.  However, if our parent is itself a partition, there
		 * might be internal triggers that must not be skipped; for example,
		 * triggers on our parent that are in turn clones from its parent (our
		 * grandparent) are marked internal, yet they are to be cloned.

Is this okay for you?

Tom Lane wrote:

> It'd be nice if the term "parent trigger" were defined somewhere in
> this.  Seems all right otherwise.

Fair point.  I propose to patch catalog.sgml like this

      <entry>
       Parent trigger that this trigger is cloned from, zero if not a clone;
       this happens when partitions are created or attached to a partitioned
       table.
      </entry>

It's perhaps not great to have to explain the parentage concept in the
catalog docs, so I'm going to go over the other documentation pages
(trigger.sgml and ref/create_trigger.sgml) to see whether they need any
patching; it's possible that we neglected to update them properly when
the partitioning-related commits went it.

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

--vkogqOf2sHV7VnPd
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v2-0001-Record-parents-of-triggers.patch"

From ff13d7004c9ee77ba411ace405224188aa0c353c Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Mon, 17 Feb 2020 17:34:47 -0300
Subject: [PATCH v2] Record parents of triggers

This let us get rid of a recently introduced ugly hack (commit
1fa846f1c9af).
---
 doc/src/sgml/catalogs.sgml       | 11 ++++++
 src/backend/commands/tablecmds.c | 59 +++-----------------------------
 src/backend/commands/trigger.c   |  1 +
 src/include/catalog/pg_trigger.h |  1 +
 4 files changed, 18 insertions(+), 54 deletions(-)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index a10b66569b..34bc0d0526 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -6951,6 +6951,17 @@ SCRAM-SHA-256$<replaceable>&lt;iteration count&gt;</replaceable>:<replaceable>&l
       <entry>The table this trigger is on</entry>
      </row>
 
+     <row>
+      <entry><structfield>tgparentid</structfield></entry>
+      <entry><type>oid</type></entry>
+      <entry><literal><link linkend="catalog-pg-trigger"><structname>pg_trigger</structname></link>.oid</literal></entry>
+      <entry>
+       Parent trigger that this trigger is cloned from, zero if not a clone;
+       this happens when partitions are created or attached to a partitioned
+       table.
+      </entry>
+     </row>
+
      <row>
       <entry><structfield>tgname</structfield></entry>
       <entry><type>name</type></entry>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b7c8d663fc..02a7c04fdb 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16447,54 +16447,6 @@ out:
 	MemoryContextDelete(cxt);
 }
 
-/*
- * isPartitionTrigger
- *		Subroutine for CloneRowTriggersToPartition: determine whether
- *		the given trigger has been cloned from another one.
- *
- * We use pg_depend as a proxy for this, since we don't have any direct
- * evidence.  This is an ugly hack to cope with a catalog deficiency.
- * Keep away from children.  Do not stare with naked eyes.  Do not propagate.
- */
-static bool
-isPartitionTrigger(Oid trigger_oid)
-{
-	Relation	pg_depend;
-	ScanKeyData key[2];
-	SysScanDesc scan;
-	HeapTuple	tup;
-	bool		found = false;
-
-	pg_depend = table_open(DependRelationId, AccessShareLock);
-
-	ScanKeyInit(&key[0], Anum_pg_depend_classid,
-				BTEqualStrategyNumber,
-				F_OIDEQ,
-				ObjectIdGetDatum(TriggerRelationId));
-	ScanKeyInit(&key[1], Anum_pg_depend_objid,
-				BTEqualStrategyNumber,
-				F_OIDEQ,
-				ObjectIdGetDatum(trigger_oid));
-
-	scan = systable_beginscan(pg_depend, DependDependerIndexId,
-							  true, NULL, 2, key);
-	while ((tup = systable_getnext(scan)) != NULL)
-	{
-		Form_pg_depend dep = (Form_pg_depend) GETSTRUCT(tup);
-
-		if (dep->refclassid == TriggerRelationId)
-		{
-			found = true;
-			break;
-		}
-	}
-
-	systable_endscan(scan);
-	table_close(pg_depend, AccessShareLock);
-
-	return found;
-}
-
 /*
  * CloneRowTriggersToPartition
  *		subroutine for ATExecAttachPartition/DefineRelation to create row
@@ -16537,11 +16489,10 @@ CloneRowTriggersToPartition(Relation parent, Relation partition)
 
 		/*
 		 * Internal triggers require careful examination.  Ideally, we don't
-		 * clone them.
-		 *
-		 * However, if our parent is a partitioned relation, there might be
-		 * internal triggers that need cloning.  In that case, we must skip
-		 * clone it if the trigger on parent depends on another trigger.
+		 * clone them.  However, if our parent is itself a partition, there
+		 * might be internal triggers that must not be skipped; for example,
+		 * triggers on our parent that are in turn clones from its parent (our
+		 * grandparent) are marked internal, yet they are to be cloned.
 		 *
 		 * Note we dare not verify that the other trigger belongs to an
 		 * ancestor relation of our parent, because that creates deadlock
@@ -16549,7 +16500,7 @@ CloneRowTriggersToPartition(Relation parent, Relation partition)
 		 */
 		if (trigForm->tgisinternal &&
 			(!parent->rd_rel->relispartition ||
-			 !isPartitionTrigger(trigForm->oid)))
+			 !OidIsValid(trigForm->tgparentid)))
 			continue;
 
 		/*
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index b9b1262e30..6e8b7223fe 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -847,6 +847,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
 
 	values[Anum_pg_trigger_oid - 1] = ObjectIdGetDatum(trigoid);
 	values[Anum_pg_trigger_tgrelid - 1] = ObjectIdGetDatum(RelationGetRelid(rel));
+	values[Anum_pg_trigger_tgparentid - 1] = ObjectIdGetDatum(parentTriggerOid);
 	values[Anum_pg_trigger_tgname - 1] = DirectFunctionCall1(namein,
 															 CStringGetDatum(trigname));
 	values[Anum_pg_trigger_tgfoid - 1] = ObjectIdGetDatum(funcoid);
diff --git a/src/include/catalog/pg_trigger.h b/src/include/catalog/pg_trigger.h
index 2f30c7c2f9..9612b9bdd6 100644
--- a/src/include/catalog/pg_trigger.h
+++ b/src/include/catalog/pg_trigger.h
@@ -35,6 +35,7 @@ CATALOG(pg_trigger,2620,TriggerRelationId)
 {
 	Oid			oid;			/* oid */
 	Oid			tgrelid;		/* relation trigger is attached to */
+	Oid			tgparentid;		/* OID of parent trigger, if any */
 	NameData	tgname;			/* trigger's name */
 	Oid			tgfoid;			/* OID of function to be called */
 	int16		tgtype;			/* BEFORE/AFTER/INSTEAD, UPDATE/DELETE/INSERT,
-- 
2.20.1


--vkogqOf2sHV7VnPd--