Thread: Rename of triggers for partitioned tables

Hi Arne,

On Fri, Nov 27, 2020 at 9:20 AM Arne Roland <A.Roland@index.de> wrote:
>
> I'm sorry for the spam. Here a patch with updated comments, I missed one before.
>

You sent in your patch, recursive_tgrename.2.patch to pgsql-hackers on
Nov 27, but you did not post it to the next CommitFest[1].  If this
was intentional, then you need to take no action.  However, if you
want your patch to be reviewed as part of the upcoming CommitFest,
then you need to add it yourself before 2021-01-01 AoE[2]. Thanks for
your contributions.

Regards,

[1] https://commitfest.postgresql.org/31/
[2] https://en.wikipedia.org/wiki/Anywhere_on_Earth

Regards,

-- 
Masahiko Sawada
EnterpriseDB:  https://www.enterprisedb.com/

On 2020-Nov-27, Arne Roland wrote:

> I got too annoyed at building queries for gexec all the time. So wrote
> a patch to fix the issue that the rename of partitioned trigger
> doesn't affect children.

As you say, triggers on children don't necessarily have to have the same
name as on parent; this already happens when the trigger is renamed in
the child table but not on parent.  In that situation the search on the
child will fail, which will cause the whole thing to fail I think.

We now have the column pg_trigger.tgparentid, and I think it would be
better (more reliable) to search for the trigger in the child by the
tgparentid column instead, rather than by name.

Also, I think it would be good to have
 ALTER TRIGGER .. ON ONLY parent RENAME TO ..
to avoid recursing to children.  This seems mostly pointless, but since
we've allowed changing the name of the trigger in children thus far,
then we've implicitly made it supported to have triggers that are named
differently.  (And it's not entirely academic, since the trigger name
determines firing order.)

Alternatively to this last point, we could decide to disallow renaming
of triggers on children (i.e. if trigger has tgparentid set, then
renaming is disallowed).  I don't have a problem with that, but it would
have to be an explicit decision to take.

I think you did not register the patch in commitfest, so I did that for
you: https://commitfest.postgresql.org/32/2943/

Thanks!

-- 
Álvaro Herrera       Valdivia, Chile
"¿Cómo puedes confiar en algo que pagas y que no ves,
y no confiar en algo que te dan y te lo muestran?" (Germán Poo)

On 1/15/21 5:26 PM, Alvaro Herrera wrote:
> On 2020-Nov-27, Arne Roland wrote:
> 
>> I got too annoyed at building queries for gexec all the time. So wrote
>> a patch to fix the issue that the rename of partitioned trigger
>> doesn't affect children.
> 
> As you say, triggers on children don't necessarily have to have the same
> name as on parent; this already happens when the trigger is renamed in
> the child table but not on parent.  In that situation the search on the
> child will fail, which will cause the whole thing to fail I think.
> 
> We now have the column pg_trigger.tgparentid, and I think it would be
> better (more reliable) to search for the trigger in the child by the
> tgparentid column instead, rather than by name.
> 
> Also, I think it would be good to have
>   ALTER TRIGGER .. ON ONLY parent RENAME TO ..
> to avoid recursing to children.  This seems mostly pointless, but since
> we've allowed changing the name of the trigger in children thus far,
> then we've implicitly made it supported to have triggers that are named
> differently.  (And it's not entirely academic, since the trigger name
> determines firing order.)
> 
> Alternatively to this last point, we could decide to disallow renaming
> of triggers on children (i.e. if trigger has tgparentid set, then
> renaming is disallowed).  I don't have a problem with that, but it would
> have to be an explicit decision to take.

Arne, thoughts on Álvaro's comments?

Marking this patch as Waiting for Author.

Regards,
-- 
-David
david@pgmasters.net

On 2021-Mar-29, Arne Roland wrote:

> @@ -1475,7 +1467,12 @@ renametrig(RenameStmt *stmt)
>          tuple = heap_copytuple(tuple);    /* need a modifiable copy */
>          trigform = (Form_pg_trigger) GETSTRUCT(tuple);
>          tgoid = trigform->oid;
> -
> +        if (tgparent != 0 && tgparent != trigform->tgparentid) {
> +            ereport(ERROR,
> +                    (errcode(ERRCODE_UNDEFINED_OBJECT),
> +                     errmsg("trigger \"%s\" for table \"%s\" is not dependent on the trigger on \"%s\"",
> +                            stmt->subname, RelationGetRelationName(targetrel), stmt->relation->relname)));
> +        }

I think this is not what we want to do.  What you're doing here as I
understand is traversing the inheritance hierarchy down using the
trigger name, and then fail if the trigger with that name has a
different parent or if no trigger with that name exists in the child.

What we really want to have happen, is to search the list of triggers in
the child, see which one has tgparentid=<the one we're renaming>, and
rename that one -- regardless of what name it had originally.

Also, you added grammar support for the ONLY keyword in the command, but
it doesn't do anything different when given that flag, does it?  At
least, I see no change or addition to recursion behavior in ATExecCmd.
I would expect that if I say "ALTER TRIGGER .. ON ONLY tab" then it
renames the trigger on table "tab" but not on its child tables; only if
I remove the ONLY from the command it recurses.

I think it would be good to have a new test for pg_dump that verifies
that this stuff is doing the right thing.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"Siempre hay que alimentar a los dioses, aunque la tierra esté seca" (Orual)

On 2021-Mar-29, Arne Roland wrote:

> Alvaro Herrera wrote:
> > I think this is not what we want to do.  What you're doing here as I
> > understand is traversing the inheritance hierarchy down using the
> > trigger name, and then fail if the trigger with that name has a
> > different parent or if no trigger with that name exists in the child.
> 
> The basic concept here was to fail in any circumstance where the
> trigger on the child isn't as expected.
> May it be the name or the parent for that matter.

Consider this.  You have table p and partition p1.  Add trigger t to p,
and do
ALTER TRIGGER t ON p1 RENAME TO q;

What I'm saying is that if you later do
ALTER TRIGGER t ON ONLY p RENAME TO r;
then the trigger on parent is changed, and the trigger on child stays q.
If you later do
ALTER TRIGGER r ON p RENAME TO s;
then triggers on both tables end up with name s.

> > What we really want to have happen, is to search the list of triggers in
> > the child, see which one has tgparentid=<the one we're renaming>, and
> >rename that one -- regardless of what name it had originally.
> 
> So if we have a trigger named t42 can be renamed to my_trigger by
> ALTER TRIGGER sometrigg ON my_table RENAME TO my_trigger;?
> Equivalently there is the question whether we just want to silently ignore
> if the corresponding child doesn't have a corresponding trigger.

I think the child cannot not have a corresponding trigger.  If you try
to drop the trigger on child, it should say that it cannot be dropped
because the trigger on parent requires it.  So I don't think there's any
case where altering name of trigger in parent would raise an user-facing
error.  If you can't find the trigger in child, that's a case of catalog
corruption and should be reported with something like

elog(ERROR, "cannot find trigger cloned from trigger %s in partition %s")

> > Also, you added grammar support for the ONLY keyword in the command, but
> > it doesn't do anything different when given that flag, does it?  At
> > least, I see no change or addition to recursion behavior in ATExecCmd.
> > I would expect that if I say "ALTER TRIGGER .. ON ONLY tab" then it
> > renames the trigger on table "tab" but not on its child tables; only if
> > I remove the ONLY from the command it recurses.
> 
> The syntax does work via
> +    if (stmt->relation->inh && has_subclass(relid))
> in renametrigHelper (src/backend/commands/trigger.c line 1543).
> I am not sure which sort of addition in ATExecCmd you expected.
> Maybe I can make this more obvious one way or the other. You input would be appreciated.

Hmm, ok, maybe this is sufficient, I didn't actually test it.  I think
you do need to add a few test cases to ensure everything is sane.  Make
sure to verify what happens if you have multi-level partitioning
(grandparent, parent, child) and you ALTER the one in the middle.  Also
things like if parent has two children and one child has multiple
children.

Also, please make sure that it works correctly with INHERITS (legacy
inheritance).  We probably don't want to cause recursion in that case.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W

On 2021-Jun-26, Arne Roland wrote:

> Hello Álvaro Herrera,
> 
> I am sorry to bother, but I am still annoyed by this. So I wonder if you had a look.

I'll have a look during the commitfest which starts late next week.

(However, I'm fairly sure that it won't be in released versions ...
it'll only be fixed in the version to be released next year, postgres
15.  Things that are clear bug fixes can be applied
in released versions, but this patch probably changes behavior in ways
that are not acceptable in released versions.  Sorry about that.)

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
Al principio era UNIX, y UNIX habló y dijo: "Hello world\n".
No dijo "Hello New Jersey\n", ni "Hello USA\n".

On Mon, Jun 28, 2021 at 3:46 PM Arne Roland <A.Roland@index.de> wrote:
>
> Hi!
>
>
> From: Zhihong Yu <zyu@yugabyte.com>
> Sent: Saturday, June 26, 2021 20:32
> Subject: Re: Rename of triggers for partitioned tables
>
> > Hi, Arne:
> > It seems the patch no longer applies cleanly on master branch.
> > Do you mind updating the patch ?
> >
> > Thanks
>
> Thank you for having a look! Please let me know if the attached rebased patch works for you.

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

I found this coding too convoluted, so I rewrote it in a different way.
You tests pass with this, but I admit I haven't double-checked them yet;
I'll do that next.

I don't think we need to give a NOTICE when the trigger name does not
match; it doesn't really matter that the trigger was named differently
before the command, does it?

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/

On 2021-Jul-20, Arne Roland wrote:

> Is your patch based on master? It doesn't apply at my end.

It does ... master being dd498998a3 here,

$ patch -p1 < /tmp/renametrig-8.patch 
patching file src/backend/commands/trigger.c
patching file src/backend/parser/gram.y
patching file src/test/regress/expected/triggers.out
patching file src/test/regress/sql/triggers.sql

applies fine.

>> I don't think we need to give a NOTICE when the trigger name does not
>> match; it doesn't really matter that the trigger was named differently
>> before the command, does it?

> I'd expect the command
> ALTER TRIGGER name ON table_name RENAME TO new_name;
> to rename a trigger named "name". We are referring the trigger via it's name after all. If a child is named
differentlywe break with that assumption. I think informing the user about that, is very valuable.
 

Well, it does rename a trigger named 'name' on the table 'table_name',
as well as all its descendant triggers.  I guess I am surprised that
anybody would rename a descendant trigger in the first place.  I'm not
wedded to the decision of removing the NOTICE, though  ... are there any
other votes for that, anyone?

Thanks

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/

On 2021-Jul-20, Arne Roland wrote:

> Thank you! I get a compile time error

> trigger.c:1588:17: note: in expansion of macro ‘PG_USED_FOR_ASSERTS_ONLY’
>   int  found = 0 PG_USED_FOR_ASSERTS_ONLY;
>                  ^~~~~~~~~~~~~~~~~~~~~~~~

Oh, I misplaced the annotation of course.  It has to be

   int  found PG_USED_FOR_ASSERTS_ONLY = 0;

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/

On 2021-Jul-19, Alvaro Herrera wrote:

> Well, it does rename a trigger named 'name' on the table 'table_name',
> as well as all its descendant triggers.  I guess I am surprised that
> anybody would rename a descendant trigger in the first place.  I'm not
> wedded to the decision of removing the NOTICE, though  ... are there any
> other votes for that, anyone?

I put it back, mostly because I don't really care and it's easily
removed if people don't want it.  (I used a different wording though,
not necessarily final.)

I also realized that if you rename a trigger and the target name is
already occupied, we'd better throw a nicer error message than failure
by violation of a unique constraint; so I moved the check for the name
to within renametrig_internal().  Added a test for this.

Also added a test to ensure that nothing happens with statement
triggers.  This doesn't need any new code, because those triggers don't
have tgparentid set.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/

... now, thinking about how does pg_dump deal with this, I think this
thread is all wrong, or at least mostly wrong.  We cannot have triggers
in partitions with different names from the triggers in the parents.
pg_dump would only dump the trigger in the parent and totally ignore the
ones in children if they have different names.

So we still need the recursion bits; but

1. if we detect that we're running in a trigger which has tgparentid
set, we have to raise an error,
2. if we are running on a trigger in a partitioned table, then ONLY must
not be specified.

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"Puedes vivir sólo una vez, pero si lo haces bien, una vez es suficiente"

On 2021-Jul-22, Arne Roland wrote:

> 
> Hi,
> 
> looking at the patch, I realized the renametrig_partition could  use an index leading with tgparentid, without the
needto traverse the child tables. Since we still need to lock them, there is likely no practical performance gain. But
Iam surprised there is no unique index on (tgparentid, tgrelid), which sounds like a decent sanity check to have
anyways.

If we have good use for such an index, I don't see why we can't add it.
But I'm not sure that it is justified -- certainly if the only benefit
is to make ALTER TRIGGER RENAME recurse faster on partitioned tables, it
is not justified.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Nunca se desea ardientemente lo que solo se desea por razón" (F. Alexandre)

On 2021-Jul-22, Arne Roland wrote:

> Since it is sort of the same problem, I think it might be worthwhile
> to address it as well within this patch. Adding two to four ereports
> doesn't sound like scope creeping to me, even though it touches
> completely different code. I'll look into that as well.

I don't understand what you mean.  But here's an updated patch, with the
following changes

1. support for ONLY is removed, since evidently the only thing it is
good for is introduce inconsistencies

2. recursion to partitioned tables always occurs; no more conditionally
on relation->inh.  This is sensible because due to point 1 above, inh
can no longer be false.

3. renaming a trigger that's not topmost is forbidden.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/

On 2021-Jul-22, Arne Roland wrote:

> I just noticed that apparently the
> ALTER TABLE [ IF EXISTS ] [ ONLY ] name [ ENABLE | DISABLE ] TRIGGER ...
> syntax albeit looking totally different and it already recurses, it
> has precisely the same issue with pg_dump. In that case the ONLY
> syntax isn't optional and the 2. from your above post does inevitably
> apply.
> 
> Since it is sort of the same problem, I think it might be worthwhile
> to address it as well within this patch. Adding two to four ereports
> doesn't sound like scope creeping to me, even though it touches
> completely different code. I'll look into that as well.

Oh!  For some reason I failed to notice that you were talking about
ENABLE / DISABLE, not RENAME anymore.  It turns out that I recently
spent some time in this area; see commits below (these are from branch
REL_11_STABLE).  Are you talking about something that need to be handled
*beyond* these fixes?

commit ccfc3cbb341abf515d930097c4851d9e2152504f
Author:     Alvaro Herrera <alvherre@alvh.no-ip.org> [Álvaro Herrera <alvherre@alvh.no-ip.org>]
AuthorDate: Fri Jul 16 17:29:22 2021 -0400
CommitDate: Fri Jul 16 17:29:22 2021 -0400

    Fix pg_dump for disabled triggers on partitioned tables
    
    pg_dump failed to preserve the 'enabled' flag (which can be not only
    disabled, but also REPLICA or ALWAYS) for partitions which had it
    changed from their respective parents.  Attempt to handle that by
    including a definition for such triggers in the dump, but replace the
    standard CREATE TRIGGER line with an ALTER TRIGGER line.
    
    Backpatch to 11, where these triggers can exist.  In branches 11 and 12,
    pick up a few test lines from commit b9b408c48724 to verify that
    pg_upgrade is okay with these arrangements.
    
    Co-authored-by: Justin Pryzby <pryzby@telsasoft.com>
    Co-authored-by: Álvaro Herrera <alvherre@alvh.no-ip.org>
    Discussion: https://postgr.es/m/20200930223450.GA14848@telsasoft.com

commit fed35bd4a65032f714af6bc88c102d0e90422dee
Author:     Alvaro Herrera <alvherre@alvh.no-ip.org> [Álvaro Herrera <alvherre@alvh.no-ip.org>]
AuthorDate: Fri Jul 16 13:01:43 2021 -0400
CommitDate: Fri Jul 16 13:01:43 2021 -0400

    Preserve firing-on state when cloning row triggers to partitions
    
    When triggers are cloned from partitioned tables to their partitions,
    the 'tgenabled' flag (origin/replica/always/disable) was not propagated.
    Make it so that the flag on the trigger on partition is initially set to
    the same value as on the partitioned table.
    
    Add a test case to verify the behavior.
    
    Backpatch to 11, where this appeared in commit 86f575948c77.
    
    Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
    Reported-by: Justin Pryzby <pryzby@telsasoft.com>
    Discussion: https://postgr.es/m/20200930223450.GA14848@telsasoft.com

commit bbb927b4db9b3b449ccd0f76c1296de382a2f0c1
Author:     Alvaro Herrera <alvherre@alvh.no-ip.org> [Álvaro Herrera <alvherre@alvh.no-ip.org>]
AuthorDate: Tue Oct 20 19:22:09 2020 -0300
CommitDate: Tue Oct 20 19:22:09 2020 -0300

    Fix ALTER TABLE .. ENABLE/DISABLE TRIGGER recursion
    
    More precisely, correctly handle the ONLY flag indicating not to
    recurse.  This was implemented in 86f575948c77 by recursing in
    trigger.c, but that's the wrong place; use ATSimpleRecursion instead,
    which behaves properly.  However, because legacy inheritance has never
    recursed in that situation, make sure to do that only for new-style
    partitioning.
    
    I noticed this problem while testing a fix for another bug in the
    vicinity.
    
    This has been wrong all along, so backpatch to 11.
    
    Discussion: https://postgr.es/m/20201016235925.GA29829@alvherre.pgsql

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/

On 2021-Jul-22, Arne Roland wrote:

> alter table middle disable trigger b;
> creates the same kind of inconsistency
> alter trigger b on middle rename to something;
> does.
> With other words: enableing/disabling non-topmost triggers should be forbidden as well.

I'm not so sure about that ... I think enabling/disabling triggers on
individual partitions is a valid use case.  Renaming them, not so much.

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"No necesitamos banderas
 No reconocemos fronteras"                  (Jorge González)

I pushed this patch with some minor corrections (mostly improved the
message wording.)

Thanks

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"En las profundidades de nuestro inconsciente hay una obsesiva necesidad
de un universo lógico y coherente. Pero el universo real se halla siempre
un paso más allá de la lógica" (Irulan)

On 2021-Jul-22, Alvaro Herrera wrote:

> I pushed this patch with some minor corrections (mostly improved the
> message wording.)

Hmm, the sorting fails for Czech or something like that.  Will fix ...

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I pushed this patch with some minor corrections (mostly improved the
> message wording.)

Coverity does not like this bit, and I fully agree:

/srv/coverity/git/pgsql-git/postgresql/src/backend/commands/trigger.c: 1639 in renametrig_partition()
>>>     CID 1489387:  Incorrect expression  (ASSERT_SIDE_EFFECT)
>>>     Argument "found++" of Assert() has a side effect.  The containing function might work differently in a
non-debugbuild. 
1639             Assert(found++ <= 0);

Perhaps there's no actual bug there, but it's still horrible coding.
For one thing, the implication that found could be negative is extremely
confusing to readers.  A boolean might be better.  However, I wonder why
you bothered with a flag in the first place.  The usual convention if
we know there can be only one match is to just not write a loop at all,
with a suitable comment, like this pre-existing example elsewhere in
trigger.c:

        /* There should be at most one matching tuple */
        if (HeapTupleIsValid(tuple = systable_getnext(tgscan)))

If you're not quite convinced there can be only one match, then it
still shouldn't be an Assert --- a real test-and-elog would be better.

            regards, tom lane

On 2021-Jul-25, Tom Lane wrote:

> Perhaps there's no actual bug there, but it's still horrible coding.
> For one thing, the implication that found could be negative is extremely
> confusing to readers.  A boolean might be better.  However, I wonder why
> you bothered with a flag in the first place.  The usual convention if
> we know there can be only one match is to just not write a loop at all,
> with a suitable comment, like this pre-existing example elsewhere in
> trigger.c:
> 
>         /* There should be at most one matching tuple */
>         if (HeapTupleIsValid(tuple = systable_getnext(tgscan)))
> 
> If you're not quite convinced there can be only one match, then it
> still shouldn't be an Assert --- a real test-and-elog would be better.

I agree that coding there was dubious.  I've removed the flag and assert.

Arne complained that there should be a unique constraint on (tgrelid,
tgparentid) which would sidestep the need for this to be a loop.  I
don't think it's really necessary, and I'm not sure how to create a
system index WHERE tgparentid <> 0.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
"Saca el libro que tu religión considere como el indicado para encontrar la
oración que traiga paz a tu alma. Luego rebootea el computador
y ve si funciona" (Carlos Duclós)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Arne complained that there should be a unique constraint on (tgrelid,
> tgparentid) which would sidestep the need for this to be a loop.  I
> don't think it's really necessary, and I'm not sure how to create a
> system index WHERE tgparentid <> 0.

Yeah, we don't support partial indexes on catalogs, and this example
doesn't make me feel like we ought to open that can of worms.  But
then maybe this function shouldn't assume there's only one match?

            regards, tom lane