From: Arne Roland <A.Roland@index.de>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
CC: David Steele <david@pgmasters.net>, 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/wutkWbcRQhC03q0KnbGQ2sgAAECBOATmSTgIBhAeKAgBDG6bOAACCNT4AAKOcAgAAj1kM=
Date: Mon, 29 Mar 2021 14:02:35 +0000
Message-ID: <6e04673973bf4922bf30fe22d5b0f067@index.de>
References: 
 <e93f8494c6314b58ad52b553b0e50779@index.de>,<20210329132536.GA9798@alvherre.pgsql>
In-Reply-To: <20210329132536.GA9798@alvherre.pgsql>
Accept-Language: en-US, de-DE
Content-Language: en-US
Content-Type: multipart/alternative;
	boundary="_000_6e04673973bf4922bf30fe22d5b0f067indexde_"
MIME-Version: 1.0
Archived-At: 
 <https://www.postgresql.org/message-id/6e04673973bf4922bf30fe22d5b0f067%40index.de>
Precedence: bulk

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

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=3D<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


--_000_6e04673973bf4922bf30fe22d5b0f067indexde_
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>Thank you for the quick reply!<br>
<br>
Alvaro Herrera wrote:<br>
&gt; I think this is not what we want to do.&nbsp; What you're doing here a=
s I<br>
&gt; understand is traversing the inheritance hierarchy down using the<br>
&gt; trigger name, and then fail if the trigger with that name has a<br>
&gt; different parent or if no trigger with that name exists in the child.<=
br>
<br>
The basic concept here was to fail in any circumstance where the trigger on=
 the child isn't as expected.<br>
May it be the name or the parent for that matter.<br>
<br>
&gt; What we really want to have happen, is to search the list of triggers =
in<br>
&gt; the child, see which one has tgparentid=3D&lt;the one we're renaming&g=
t;, and<br>
&gt;rename that one -- regardless of what name it had originally.<br>
<br>
So if we have a trigger named t42 can be renamed to my_trigger by <br>
ALTER TRIGGER sometrigg ON my_table RENAME TO my_trigger;?<br>
Equivalently there is the question whether we just want to silently ignore<=
br>
if the corresponding child doesn't have a corresponding trigger.<br>
I am not convinced this is really what we want, but I could agree to raise =
a notice in these cases.<br>
<br>
&gt; Also, you added grammar support for the ONLY keyword in the command, b=
ut<br>
&gt; it doesn't do anything different when given that flag, does it?&nbsp; =
At<br>
&gt; least, I see no change or addition to recursion behavior in ATExecCmd.=
<br>
&gt; I would expect that if I say &quot;ALTER TRIGGER .. ON ONLY tab&quot; =
then it<br>
&gt; renames the trigger on table &quot;tab&quot; but not on its child tabl=
es; only if<br>
&gt; I remove the ONLY from the command it recurses.<br>
<br>
The syntax does work via <br>
&#43;&nbsp;&nbsp;&nbsp; if (stmt-&gt;relation-&gt;inh &amp;&amp; has_subcla=
ss(relid))<br>
in renametrigHelper (src/backend/commands/trigger.c line 1543).<br>
I am not sure which sort of addition in ATExecCmd you expected.<br>
Maybe I can make this more obvious one way or the other. You input would be=
 appreciated.<br>
<br>
&gt; I think it would be good to have a new test for pg_dump that verifies<=
br>
&gt; that this stuff is doing the right thing.<br>
<br>
Good idea, I will add a case involving pg_dump.</div>
<div><br>
</div>
<div>Regards<br>
</div>
<div>Arne<br>
<br>
</div>
<br>
</div>
</body>
</html>

--_000_6e04673973bf4922bf30fe22d5b0f067indexde_--