pg_trigger.tgparentid
As mentioned in /messages/by-id/20191231194759.GA24692@alvherre.pgsql
I propose to add a new column to pg_trigger, which allows us to remove a
pg_depend scan when cloning triggers when adding/attaching partitions.
(It's not that I think the scan is a performance problem, but rather
than notionally we try not to depend on pg_depend contents for this kind
of semantic derivation.)
--
�lvaro Herrera 39�49'30"S 73�17'W
Attachments:
0001-Record-parents-of-triggers.patchtext/x-diff; charset=us-asciiDownload
From 8f1be4b2c24385bbfe2ecba941545ffa84f60b47 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] Record parents of triggers
This let us get rid of a recently introduced ugly hack (commit
1fa846f1c9af).
---
doc/src/sgml/catalogs.sgml | 7 +++++
src/backend/commands/tablecmds.c | 52 ++------------------------------
src/backend/commands/trigger.c | 1 +
src/include/catalog/pg_trigger.h | 1 +
4 files changed, 11 insertions(+), 50 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index a10b66569b..0bba96ee52 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -6951,6 +6951,13 @@ 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 of this trigger, if any</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..ae39bf1056 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
@@ -16541,7 +16493,7 @@ CloneRowTriggersToPartition(Relation parent, Relation partition)
*
* 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.
+ * cloning it if the trigger on parent depends on another trigger.
*
* Note we dare not verify that the other trigger belongs to an
* ancestor relation of our parent, because that creates deadlock
@@ -16549,7 +16501,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
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
As mentioned in /messages/by-id/20191231194759.GA24692@alvherre.pgsql
I propose to add a new column to pg_trigger, which allows us to remove a
pg_depend scan when cloning triggers when adding/attaching partitions.
(It's not that I think the scan is a performance problem, but rather
than notionally we try not to depend on pg_depend contents for this kind
of semantic derivation.)
It'd be nice if the term "parent trigger" were defined somewhere in
this. Seems all right otherwise.
regards, tom lane
On Tue, Feb 18, 2020 at 6:56 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
As mentioned in /messages/by-id/20191231194759.GA24692@alvherre.pgsql
I propose to add a new column to pg_trigger, which allows us to remove a
pg_depend scan when cloning triggers when adding/attaching partitions.
(It's not that I think the scan is a performance problem, but rather
than notionally we try not to depend on pg_depend contents for this kind
of semantic derivation.)
@@ -16541,7 +16493,7 @@ CloneRowTriggersToPartition(Relation parent,
Relation partition)
*
* However, if our parent is a partitioned relation, there might be
This is existing text, but should really be:
However, if our parent is a *partition* itself, there might be
(Sorry, I forgot to report this when the bug-fix went in couple months ago.)
* internal triggers that need cloning. In that case, we must skip
- * clone it if the trigger on parent depends on another trigger.
+ * cloning it if the trigger on parent depends on another trigger.2nd sentence seems unclear to me. Does the following say what needs
to be said here:
* However, if our parent is a partition itself, there might be
* internal triggers that need cloning. For example, triggers on the
* parent that were in turn cloned from its own parent are marked
* internal, which too must be cloned to the partition.
Thanks,
Amit
On Tue, Feb 18, 2020 at 1:11 PM Amit Langote <amitlangote09@gmail.com> wrote:
On Tue, Feb 18, 2020 at 6:56 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
@@ -16541,7 +16493,7 @@ CloneRowTriggersToPartition(Relation parent,
Relation partition)
*
* However, if our parent is a partitioned relation, there might beThis is existing text, but should really be:
However, if our parent is a *partition* itself, there might be
(Sorry, I forgot to report this when the bug-fix went in couple months ago.)
* internal triggers that need cloning. In that case, we must skip - * clone it if the trigger on parent depends on another trigger. + * cloning it if the trigger on parent depends on another trigger.2nd sentence seems unclear to me. Does the following say what needs
to be said here:* However, if our parent is a partition itself, there might be
* internal triggers that need cloning. For example, triggers on the
* parent that were in turn cloned from its own parent are marked
* internal, which too must be cloned to the partition.
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,
Amit
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:
v2-0001-Record-parents-of-triggers.patchtext/x-diff; charset=us-asciiDownload
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><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.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
Hi Alvaro,
On Tue, Feb 25, 2020 at 3:58 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
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?
Thanks. Your revised text looks good, except there is a typo:
in turn clones -> in turn cloned
Thanks,
Amit
On 2020-Feb-25, Amit Langote wrote:
On Tue, Feb 25, 2020 at 3:58 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
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?
Thanks. Your revised text looks good, except there is a typo:
in turn clones -> in turn cloned
Actually, that was on purpose ... (I also changed "were" to "are" to match.)
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Feb 25, 2020 at 11:01 PM Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
On 2020-Feb-25, Amit Langote wrote:
On Tue, Feb 25, 2020 at 3:58 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
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?
Thanks. Your revised text looks good, except there is a typo:
in turn clones -> in turn cloned
Actually, that was on purpose ... (I also changed "were" to "are" to match.)
Ah, got it.
Thanks,
Amit
Thanks both -- pushed. I also changed regress/sql/triggers to leave
tables around that have a non-zero tgparentid. This ensures that the
pg_upgrade test sees such objects, as well as findoidjoins.
I refrained from doing the findoidjoins dance itself, though; I got a
large number of false positives that I think are caused by some pg12-era
hacking.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Thanks both -- pushed. I also changed regress/sql/triggers to leave
tables around that have a non-zero tgparentid. This ensures that the
pg_upgrade test sees such objects, as well as findoidjoins.
I refrained from doing the findoidjoins dance itself, though; I got a
large number of false positives that I think are caused by some pg12-era
hacking.
Generally I try to update findoidjoins once per release cycle, after
feature freeze. I don't think it's worth messing with it more often
than that. But thanks for making sure there'll be data for it to find.
regards, tom lane