From: Arne Roland <A.Roland@index.de>
To: David Steele <david@pgmasters.net>, Alvaro Herrera
	<alvherre@alvh.no-ip.org>
CC: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Rename of triggers for partitioned tables
Thread-Topic: Rename of triggers for partitioned tables
Thread-Index: AQHWxEk1+rora/wutkWbcRQhC03q0KnbGQ2sgAAECBOATmSTgIBhAeKAgBDG6bM=
Date: Mon, 29 Mar 2021 07:37:53 +0000
Message-ID: <63c8be480eda4ef28d17a019e79841e2@index.de>
References: 
 <20210115222641.GA26284@alvherre.pgsql>,<0e245ca1-bb16-2416-a9cb-7255907227d7@pgmasters.net>
In-Reply-To: <0e245ca1-bb16-2416-a9cb-7255907227d7@pgmasters.net>
Accept-Language: en-US, de-DE
Content-Language: en-US
Content-Type: multipart/alternative;
	boundary="_000_63c8be480eda4ef28d17a019e79841e2indexde_"
MIME-Version: 1.0
Archived-At: 
 <https://www.postgresql.org/message-id/63c8be480eda4ef28d17a019e79841e2%40index.de>
Precedence: bulk

--_000_63c8be480eda4ef28d17a019e79841e2indexde_
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable

David Steele wrote:
> Arne, thoughts on =C1lvaro's comments?
>
> Marking this patch as Waiting for Author.

Thank you for the reminder. I was to absorbed by other tasks. I should be m=
ore 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 h=
andle 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 sim=
ply ignoring the users earlier rename.
Silently renaming sounded a bit presumptuous to me, even though I don't car=
e 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 i=
n 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 w=
e 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


--_000_63c8be480eda4ef28d17a019e79841e2indexde_
Content-Type: text/html; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable

<html>
<head>
<meta http-equiv=3D"Content-Type" content=3D"text/html; charset=3Diso-8859-=
1">
<style type=3D"text/css" style=3D"display:none;"><!-- P {margin-top:0;margi=
n-bottom:0;} --></style>
</head>
<body dir=3D"ltr">
<div id=3D"divtagdefaultwrapper" style=3D"font-size:12pt;color:#000000;font=
-family:Calibri,Helvetica,sans-serif;" dir=3D"ltr">
<div>David Steele wrote:<br>
&gt; Arne, thoughts on =C1lvaro's comments?<br>
&gt; <br>
&gt; Marking this patch as Waiting for Author.<br>
<br>
Thank you for the reminder. I was to absorbed by other tasks. I should be m=
ore present in the upcoming weeks.<br>
<br>
&gt; I think you did not register the patch in commitfest, so I did that fo=
r<br>
&gt; you: https://commitfest.postgresql.org/32/2943/<br>
<br>
Thank you! I should have done that.<br>
<br>
&gt; As you say, triggers on children don't necessarily have to have the sa=
me<br>
&gt; name as on parent; this already happens when the trigger is renamed in=
<br>
&gt; the child table but not on parent.&nbsp; In that situation the search =
on the<br>
&gt; child will fail, which will cause the whole thing to fail I think.<br>
&gt; <br>
&gt; We now have the column pg_trigger.tgparentid, and I think it would be<=
br>
&gt; better (more reliable) to search for the trigger in the child by the<b=
r>
&gt; tgparentid column instead, rather than by name.<br>
<br>
The last patch already checks tgparentid and errors out if either of those =
do not match.<br>
The reasoning behind that was, that I don't see a clear reasonable way to h=
andle this scenario.<br>
If the user choose to rename the child trigger, I am at a loss what to do.<=
br>
I thought to pass it back to the user to solve the situation instead of sim=
ply ignoring the users earlier rename.<br>
Silently renaming sounded a bit presumptuous to me, even though I don't car=
e that much either way.<br>
<br>
Do you think silently renaming is better than yielding an error?<br>
<br>
&gt; Also, I think it would be good to have<br>
&gt;&nbsp;&nbsp; ALTER TRIGGER .. ON ONLY parent RENAME TO ..<br>
&gt; to avoid recursing to children.&nbsp; This seems mostly pointless, but=
 since<br>
&gt; we've allowed changing the name of the trigger in children thus far,<b=
r>
&gt; then we've implicitly made it supported to have triggers that are name=
d<br>
&gt; differently.&nbsp; (And it's not entirely academic, since the trigger =
name<br>
&gt; determines firing order.)<br>
<br>
If it would be entirely academic, I wouldn't have noticed the whole thing i=
n the first place.<br>
<br>
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.<br>
<br>
&gt; Alternatively to this last point, we could decide to disallow renaming=
<br>
&gt; of triggers on children (i.e. if trigger has tgparentid set, then<br>
&gt; renaming is disallowed).&nbsp; I don't have a problem with that, but i=
t would<br>
&gt; have to be an explicit decision to take.<br>
<br>
I rejected that idea, because I was unsure what to do with migrations. If w=
e would be discussing this a few years back, this would have been likely my=
 vote, but I don't see it happening now.<br>
<br>
Regards<br>
Arne<br>
<br>
</div>
</div>
</body>
</html>

--_000_63c8be480eda4ef28d17a019e79841e2indexde_--