From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: pg_trigger.tgparentid
Date: Mon, 24 Feb 2020 15:58:50 -0300
Message-ID: <20200224185850.GA30899@alvherre.pgsql> (raw)
In-Reply-To: <CA+HiwqHCcE57ARm0ZtU5nDg7VZgBTf8BU1hn7H7Om7h7wserVg@mail.gmail.com>
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
Attachments:
[text/x-diff] v2-0001-Record-parents-of-triggers.patch (5.3K, ../20200224185850.GA30899@alvherre.pgsql/2-v2-0001-Record-parents-of-triggers.patch)
download | inline diff:
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.sgmlindex a10b66569b..34bc0d0526 100644--- a/doc/src/sgml/catalogs.sgml+++ b/doc/src/sgml/catalogs.sgml@@ -6951,6 +6951,17 @@ SCRAM-SHA-256$<replaceable><iteration count></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.cindex 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.cindex 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.hindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@2ndquadrant.com, amitlangote09@gmail.com, pgsql-hackers@lists.postgresql.org, tgl@sss.pgh.pa.us
Subject: Re: pg_trigger.tgparentid
In-Reply-To: <20200224185850.GA30899@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox