Thread: 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
Attachment
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
Attachment
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