Rename of triggers for partitioned tables

Started by Arne Rolandover 5 years ago42 messageshackers
Jump to latest
#1Arne Roland
A.Roland@index.de

Hello,

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.

In case there isn't a dependent trigger with the same name on the child the function errors out. If the user touched the trigger of the child, it's not a sensitive idea to mess with it.

I'm not happy with the interface change in trigger.h. Yet I don't like the idea of creating another layer of indirection either.

Even though it's a small patch, there might be other things I'm missing, so I'd appreciate feedback.

Regards

Arne

Attachments:

recursive_tgrename.patchtext/x-patch; name=recursive_tgrename.patchDownload+95-30
partitioned_tgrename.sqlapplication/sql; name=partitioned_tgrename.sqlDownload
#2Arne Roland
A.Roland@index.de
In reply to: Arne Roland (#1)
Re: Rename of triggers for partitioned tables

I'm too unhappy with the interface change, so please view attached patch instead.

Regards

Arne

________________________________
From: Arne Roland <A.Roland@index.de>
Sent: Friday, November 27, 2020 12:28:31 AM
To: Pg Hackers
Subject: Rename of triggers for partitioned tables

Hello,

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.

In case there isn't a dependent trigger with the same name on the child the function errors out. If the user touched the trigger of the child, it's not a sensitive idea to mess with it.

I'm not happy with the interface change in trigger.h. Yet I don't like the idea of creating another layer of indirection either.

Even though it's a small patch, there might be other things I'm missing, so I'd appreciate feedback.

Regards

Arne

Attachments:

recursive_tgrename.1.patchtext/x-patch; name=recursive_tgrename.1.patchDownload+97-28
#3Arne Roland
A.Roland@index.de
In reply to: Arne Roland (#2)
Re: Rename of triggers for partitioned tables

I'm sorry for the spam. Here a patch with updated comments, I missed one before.

Regards

Arne

________________________________
From: Arne Roland <A.Roland@index.de>
Sent: Friday, November 27, 2020 1:05:02 AM
To: Pg Hackers
Subject: Re: Rename of triggers for partitioned tables

I'm too unhappy with the interface change, so please view attached patch instead.

Regards

Arne

________________________________
From: Arne Roland <A.Roland@index.de>
Sent: Friday, November 27, 2020 12:28:31 AM
To: Pg Hackers
Subject: Rename of triggers for partitioned tables

Hello,

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.

In case there isn't a dependent trigger with the same name on the child the function errors out. If the user touched the trigger of the child, it's not a sensitive idea to mess with it.

I'm not happy with the interface change in trigger.h. Yet I don't like the idea of creating another layer of indirection either.

Even though it's a small patch, there might be other things I'm missing, so I'd appreciate feedback.

Regards

Arne

Attachments:

recursive_tgrename.2.patchtext/x-patch; name=recursive_tgrename.2.patchDownload+97-28
#4Masahiko Sawada
sawada.mshk@gmail.com
In reply to: Arne Roland (#3)
Re: 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]https://commitfest.postgresql.org/31/. 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]https://en.wikipedia.org/wiki/Anywhere_on_Earth. 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/

#5Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Arne Roland (#3)
Re: Rename of triggers for partitioned tables

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)

#6David Steele
david@pgmasters.net
In reply to: Alvaro Herrera (#5)
Re: Rename of triggers for partitioned tables

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

#7Arne Roland
A.Roland@index.de
In reply to: David Steele (#6)
Re: Rename of triggers for partitioned tables

David Steele wrote:

Arne, thoughts on Álvaro's comments?

Marking this patch as Waiting for Author.

Thank you for the reminder. I was to absorbed by other tasks. I should be more present in the upcoming weeks.

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

Thank you! I should have done that.

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.

The last patch already checks tgparentid and errors out if either of those do not match.
The reasoning behind that was, that I don't see a clear reasonable way to handle this scenario.
If the user choose to rename the child trigger, I am at a loss what to do.
I thought to pass it back to the user to solve the situation instead of simply ignoring the users earlier rename.
Silently renaming sounded a bit presumptuous to me, even though I don't care that much either way.

Do you think silently renaming is better than yielding an error?

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

If it would be entirely academic, I wouldn't have noticed the whole thing in the first place.

The on only variant sounds like a good idea to me. Even though hardly worth any efford, it seems simple enough. I will work on that.

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 rejected that idea, because I was unsure what to do with migrations. If we would be discussing this a few years back, this would have been likely my vote, but I don't see it happening now.

Regards
Arne

#8Arne Roland
A.Roland@index.de
In reply to: Arne Roland (#7)
Re: Rename of triggers for partitioned tables

Attached a rebased patch with the suggested "ONLY ON" change.

Regards

Arne

________________________________
From: Arne Roland <A.Roland@index.de>
Sent: Monday, March 29, 2021 9:37:53 AM
To: David Steele; Alvaro Herrera
Cc: Pg Hackers
Subject: Re: Rename of triggers for partitioned tables

David Steele wrote:

Arne, thoughts on Álvaro's comments?

Marking this patch as Waiting for Author.

Thank you for the reminder. I was to absorbed by other tasks. I should be more present in the upcoming weeks.

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

Thank you! I should have done that.

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.

The last patch already checks tgparentid and errors out if either of those do not match.
The reasoning behind that was, that I don't see a clear reasonable way to handle this scenario.
If the user choose to rename the child trigger, I am at a loss what to do.
I thought to pass it back to the user to solve the situation instead of simply ignoring the users earlier rename.
Silently renaming sounded a bit presumptuous to me, even though I don't care that much either way.

Do you think silently renaming is better than yielding an error?

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

If it would be entirely academic, I wouldn't have noticed the whole thing in the first place.

The on only variant sounds like a good idea to me. Even though hardly worth any efford, it seems simple enough. I will work on that.

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 rejected that idea, because I was unsure what to do with migrations. If we would be discussing this a few years back, this would have been likely my vote, but I don't see it happening now.

Regards
Arne

Attachments:

recursive_tgrename.4.patchtext/x-patch; name=recursive_tgrename.4.patchDownload+98-29
#9Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Arne Roland (#8)
Re: Rename of triggers for partitioned tables

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)

#10Arne Roland
A.Roland@index.de
In reply to: Alvaro Herrera (#9)
Re: Rename of triggers for partitioned tables

Thank you for the quick reply!

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.

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 am not convinced this is really what we want, but I could agree to raise a notice in these cases.

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.

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

Good idea, I will add a case involving pg_dump.

Regards
Arne

#11Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Arne Roland (#10)
Re: Rename of triggers for partitioned tables

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

#12Arne Roland
A.Roland@index.de
In reply to: Alvaro Herrera (#11)
Re: Rename of triggers for partitioned tables

Alvaro Herrera wrote:

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

Ah, I didn't know that. That changes my reasoning about that. I only knew, that the alter table ... detach partition ... doesn't detach the child trigger from the partitioned trigger, but the the attach partition seem to add one. Maybe we should be able to make sure that there is a one to one correspondence for child triggers and child partitions.

Currently upon detaching we keep the trigger within the partitioned trigger of the parent relation. (So the trigger remains in place and can only be dropped with the parent one.) Only we try to attach the partition again any partitioned triggers are simply removed.

One idea would be to detach partitioned triggers there in a similar manner we detach partitioned indexes. That could give above guarantee. (At least if one would be willing to write a migration for that.) But then we can't tell anymore where the trigger stems from. That hence would break our attach/detach workflow.

I was annoyed by the inability to drop partitioned triggers from detached partitions, but it seems I just got a workaround. Ugh.

This is a bit of a sidetrack discussion, but it is related.

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.

If we get into the mindset of expecting one trigger on the child, we have the following edge case:

- The trigger is renamed. You seem to favor the silent rename in that case over raising a notice (or even an error). I am not convinced a notice wouldn't the better in that case.
- The trigger is outside of partitioned table. That still means that the trigger might still be in the inheritance tree, but likely isn't. Should we rename it anyways, or skip that? Should be raise a notice about what we are doing here? I think that would be helpful to the end user.

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.

The test I had somewhere upwards this thread, already tested that.

I am not sure how to add a test for pg_dump though. Can you point me to the location in pg_regress for pg_dump?

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

That works, but I will add a test to make sure it stays that way. Thank you for your input!

Regards
Arne

#13Arne Roland
A.Roland@index.de
In reply to: Arne Roland (#12)
Re: Rename of triggers for partitioned tables

Hi,

attached a patch with some cleanup and a couple of test cases. The lookup is now done with the parentid and the renaming affects detached partitions as well.

The test cases are not perfectly concise, but only add about 0.4 % to the total runtime of regression tests at my machine. So I didn't bother to much. They only consist of basic sql tests.

Further feedback appreciated! Thank you!

Regards

Arne

________________________________
From: Arne Roland <A.Roland@index.de>
Sent: Thursday, April 1, 2021 4:38:59 PM
To: Alvaro Herrera
Cc: David Steele; Pg Hackers
Subject: Re: Rename of triggers for partitioned tables

Alvaro Herrera wrote:

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

Ah, I didn't know that. That changes my reasoning about that. I only knew, that the alter table ... detach partition ... doesn't detach the child trigger from the partitioned trigger, but the the attach partition seem to add one. Maybe we should be able to make sure that there is a one to one correspondence for child triggers and child partitions.

Currently upon detaching we keep the trigger within the partitioned trigger of the parent relation. (So the trigger remains in place and can only be dropped with the parent one.) Only we try to attach the partition again any partitioned triggers are simply removed.

One idea would be to detach partitioned triggers there in a similar manner we detach partitioned indexes. That could give above guarantee. (At least if one would be willing to write a migration for that.) But then we can't tell anymore where the trigger stems from. That hence would break our attach/detach workflow.

I was annoyed by the inability to drop partitioned triggers from detached partitions, but it seems I just got a workaround. Ugh.

This is a bit of a sidetrack discussion, but it is related.

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.

If we get into the mindset of expecting one trigger on the child, we have the following edge case:

- The trigger is renamed. You seem to favor the silent rename in that case over raising a notice (or even an error). I am not convinced a notice wouldn't the better in that case.
- The trigger is outside of partitioned table. That still means that the trigger might still be in the inheritance tree, but likely isn't. Should we rename it anyways, or skip that? Should be raise a notice about what we are doing here? I think that would be helpful to the end user.

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.

The test I had somewhere upwards this thread, already tested that.

I am not sure how to add a test for pg_dump though. Can you point me to the location in pg_regress for pg_dump?

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

That works, but I will add a test to make sure it stays that way. Thank you for your input!

Regards
Arne

Attachments:

recursive_tgrename.5.patchtext/x-patch; name=recursive_tgrename.5.patchDownload+359-47
#14Arne Roland
A.Roland@index.de
In reply to: Arne Roland (#13)
Re: Rename of triggers for partitioned tables

Hello Álvaro Herrera,

I am sorry to bother, but I am still annoyed by this. So I wonder if you had a look.

Regards
Arne

________________________________
From: Arne Roland <A.Roland@index.de>
Sent: Friday, April 2, 2021 7:55:16 PM
To: Alvaro Herrera
Cc: David Steele; Pg Hackers
Subject: Re: Rename of triggers for partitioned tables

Hi,

attached a patch with some cleanup and a couple of test cases. The lookup is now done with the parentid and the renaming affects detached partitions as well.

The test cases are not perfectly concise, but only add about 0.4 % to the total runtime of regression tests at my machine. So I didn't bother to much. They only consist of basic sql tests.

Further feedback appreciated! Thank you!

Regards

Arne

________________________________
From: Arne Roland <A.Roland@index.de>
Sent: Thursday, April 1, 2021 4:38:59 PM
To: Alvaro Herrera
Cc: David Steele; Pg Hackers
Subject: Re: Rename of triggers for partitioned tables

Alvaro Herrera wrote:

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

Ah, I didn't know that. That changes my reasoning about that. I only knew, that the alter table ... detach partition ... doesn't detach the child trigger from the partitioned trigger, but the the attach partition seem to add one. Maybe we should be able to make sure that there is a one to one correspondence for child triggers and child partitions.

Currently upon detaching we keep the trigger within the partitioned trigger of the parent relation. (So the trigger remains in place and can only be dropped with the parent one.) Only we try to attach the partition again any partitioned triggers are simply removed.

One idea would be to detach partitioned triggers there in a similar manner we detach partitioned indexes. That could give above guarantee. (At least if one would be willing to write a migration for that.) But then we can't tell anymore where the trigger stems from. That hence would break our attach/detach workflow.

I was annoyed by the inability to drop partitioned triggers from detached partitions, but it seems I just got a workaround. Ugh.

This is a bit of a sidetrack discussion, but it is related.

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.

If we get into the mindset of expecting one trigger on the child, we have the following edge case:

- The trigger is renamed. You seem to favor the silent rename in that case over raising a notice (or even an error). I am not convinced a notice wouldn't the better in that case.
- The trigger is outside of partitioned table. That still means that the trigger might still be in the inheritance tree, but likely isn't. Should we rename it anyways, or skip that? Should be raise a notice about what we are doing here? I think that would be helpful to the end user.

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.

The test I had somewhere upwards this thread, already tested that.

I am not sure how to add a test for pg_dump though. Can you point me to the location in pg_regress for pg_dump?

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

That works, but I will add a test to make sure it stays that way. Thank you for your input!

Regards
Arne

#15Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Arne Roland (#14)
Re: Rename of triggers for partitioned tables

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

#16Arne Roland
A.Roland@index.de
In reply to: Alvaro Herrera (#15)
Re: Rename of triggers for partitioned tables

From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Sent: Saturday, June 26, 2021 18:36
Subject: Re: Rename of triggers for partitioned tables

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

Thank you!

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

I never expected any backports. Albeit I don't know anyone, who would mind, I agree with you on that assessment. This is very annoying, but not really breaking the product.

Sure, I was hoping for pg14 initially. But I will survive another year of gexec.
I am grateful you agreed to have a look!

Regards
Arne

#17Zhihong Yu
zyu@yugabyte.com
In reply to: Arne Roland (#16)
Re: Rename of triggers for partitioned tables

On Sat, Jun 26, 2021 at 10:08 AM Arne Roland <A.Roland@index.de> wrote:

*From:* Alvaro Herrera <alvherre@alvh.no-ip.org>
*Sent:* Saturday, June 26, 2021 18:36
*Subject:* Re: Rename of triggers for partitioned tables

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

Thank you!

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

I never expected any backports. Albeit I don't know anyone, who would
mind, I agree with you on that assessment. This is very annoying, but not
really breaking the product.

Sure, I was hoping for pg14 initially. But I will survive another year of
gexec.
I am grateful you agreed to have a look!

Regards
Arne

Hi, Arne:

It seems the patch no longer applies cleanly on master branch.
Do you mind updating the patch ?

Thanks

1 out of 7 hunks FAILED -- saving rejects to file
src/backend/commands/trigger.c.rej
patching file src/backend/parser/gram.y
Hunk #1 succeeded at 8886 (offset 35 lines).
patching file src/backend/utils/adt/ri_triggers.c
Reversed (or previously applied) patch detected! Assume -R? [n]
Apply anyway? [n]
Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file
src/backend/utils/adt/ri_triggers.c.rej

#18Arne Roland
A.Roland@index.de
In reply to: Zhihong Yu (#17)
Re: Rename of triggers for partitioned tables

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.

Regards
Arne

Attachments:

recursive_tgrename.6.patchtext/x-patch; name=recursive_tgrename.6.patchDownload+363-44
#19Zhihong Yu
zyu@yugabyte.com
In reply to: Arne Roland (#14)
Re: Rename of triggers for partitioned tables

On Mon, Jun 28, 2021 at 11:45 AM Arne Roland <A.Roland@index.de> wrote:

Show quoted text

Hi,

*From:* Zhihong Yu <zyu@yugabyte.com>
*Sent:* Monday, June 28, 2021 15:28
*Subject:* Re: Rename of triggers for partitioned tables

-                                void *arg)
+callbackForRenameTrigger(char *relname, Oid relid)

Since this method is only called by RangeVarCallbackForRenameTrigger(),

it seems the method can be folded into RangeVarCallbackForRenameTrigger.

that seems obvious. I have no idea why I didn't do that.

+ * renametrig - changes the name of a trigger on a possibly

partitioned relation recurisvely

...
+renametrigHelper(RenameStmt *stmt, Oid tgoid, Oid relid)

Comment doesn't match the name of method. I think it is better to use _

instead of camel case.

The other functions in this file seem to be in mixed case (camel case with
lower first character). I don't have a strong point either way, but the
consistency seems to be worth it to me.

-renametrig(RenameStmt *stmt)
+renameonlytrig(RenameStmt *stmt, Relation tgrel, Oid relid, Oid

tgparent)

Would renametrig_unlocked be better name for the method ?

I think the name renametrig_unlocked might be confusing. I am not sure my
name is good. But upon calling this function everything is locked already
and stays locked. So unlocked seems a confusing name to me.
renameOnlyOneTriggerWithoutAccountingForChildrenWhereAllLocksAreAquiredAlready
sadly isn't very concise anymore. Do you have another idea?

strcmp(stmt->subname, NameStr(trigform->tgname)) can be saved in a

variable since it is used after the if statement.

It's always needed later. I did miss it due to the short circuiting
expression. Thanks!

+   tgrel = table_open(TriggerRelationId, RowExclusiveLock);
...
+   ReleaseSysCache(tuple);     /* We are still holding the
+                                * AccessExclusiveLock. */

The lock mode in comment doesn't seem to match the lock taken.

I made the comment slightly more verbose. I hope it's clear now.

Thank you very much for the feedback! I attached a new version of the
patch based on your suggestions.

Regards
Arne

Adding mailing list.

#20vignesh C
vignesh21@gmail.com
In reply to: Arne Roland (#18)
Re: Rename of triggers for partitioned tables

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

#21Arne Roland
A.Roland@index.de
In reply to: vignesh C (#20)
#22Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Arne Roland (#21)
#23Arne Roland
A.Roland@index.de
In reply to: Alvaro Herrera (#22)
#24Arne Roland
A.Roland@index.de
In reply to: Arne Roland (#23)
#25Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Arne Roland (#23)
#26Arne Roland
A.Roland@index.de
In reply to: Alvaro Herrera (#25)
#27Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Arne Roland (#26)
#28Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Alvaro Herrera (#25)
#29Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Alvaro Herrera (#28)
#30Arne Roland
A.Roland@index.de
In reply to: Alvaro Herrera (#29)
#31Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Arne Roland (#30)
#32Arne Roland
A.Roland@index.de
In reply to: Alvaro Herrera (#31)
#33Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Arne Roland (#32)
#34Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Arne Roland (#32)
#35Arne Roland
A.Roland@index.de
In reply to: Alvaro Herrera (#33)
#36Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Arne Roland (#35)
#37Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Alvaro Herrera (#33)
#38Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Alvaro Herrera (#37)
#39Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#37)
#40Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Tom Lane (#39)
#41Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#40)
#42Arne Roland
A.Roland@index.de
In reply to: Tom Lane (#41)