Re: extension patch of CREATE OR REPLACE TRIGGER

"osumi(dot)takamichi(at)fujitsu(dot)com" <osumi(dot)takamichi(at)fujitsu(dot)com> writes:
> [ CREATE_OR_REPLACE_TRIGGER_v11.patch ]

I took a quick look through this. I think there's still work left to do.

* I'm concerned by the fact that there doesn't seem to be any defense
against somebody replacing a foreign-key trigger with something that
does something else entirely, and thereby silently breaking their
foreign key constraint. I think it might be a good idea to forbid
replacing triggers for which tgisinternal is true; but I've not done
the legwork to see if that's exactly the condition we want.

* In the same vein, I'm not sure that the right things happen when fooling
with triggers attached to partitioned tables. We presumably don't want to
allow mucking directly with a child trigger. Perhaps refusing an update
when tgisinternal might fix this too (although we'll have to be careful to
make the error message not too confusing).

* I don't think that you've fully thought through the implications
of replacing a trigger for a table that the current transaction has
already modified. Is it really sufficient, or even useful, to do
this:

+ /*
+ * If this trigger has pending events, throw an error.
+ */
+ if (trigger_deferrable && AfterTriggerPendingOnRel(RelationGetRelid(rel)))

As an example, if we change a BEFORE trigger to an AFTER trigger,
that's not going to affect the fact that we *already* fired that
trigger. Maybe this is okay and we just need to document it, but
I'm not convinced.

* BTW, I don't think a trigger necessarily has to be deferrable
in order to have pending AFTER events. The existing use of
AfterTriggerPendingOnRel certainly doesn't assume that. But really,
I think we probably ought to be applying CheckTableNotInUse which'd
include that test. (Another way in which there's fuzzy thinking
here is that AfterTriggerPendingOnRel isn't specific to *this*
trigger.)

* A lesser point is that I think you're overcomplicating the
code by applying heap_modify_tuple. You might as well just
build the new tuple normally in all cases, and then apply
either CatalogTupleInsert or CatalogTupleUpdate.

* Also, the search for an existing trigger tuple is being
done the hard way. You're using an index on (tgrelid, tgname),
so you could include the name in the index key and expect that
there's at most one match.

regards, tom lane