Date: Mon, 29 Mar 2021 12:16:57 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Arne Roland <A.Roland@index.de>
Cc: David Steele <david@pgmasters.net>,
	Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Rename of triggers for partitioned tables
Message-ID: <20210329151657.GA423@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <6e04673973bf4922bf30fe22d5b0f067@index.de>
User-Agent: Mutt/1.10.1 (2018-07-13)
Archived-At: 
 <https://www.postgresql.org/message-id/20210329151657.GA423%40alvherre.pgsql>
Precedence: bulk

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