Обсуждение: pg_trigger.tgparentid

As mentioned in https://postgr.es/m/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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> As mentioned in https://postgr.es/m/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 https://postgr.es/m/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