pg_trigger.tgparentid

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

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

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