RE: extension patch of CREATE OR REPLACE TRIGGER

Hi


I spent too much time to respond to this e-mail. Sorry.
Actually, I got stuck to deal with achieving both
error detection of internal trigger case and pending trigger case.

> * 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).
Yeah, you are right. tgisinternal works to detect an invalid cases.
I added a new check condition in my patch to prohibit
the replacement of internal triggers by an user,
which protects FK trigger and child trigger from being replaced directly.


> * 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.)
Hmm, actually, when I just put a code of CheckTableNotInUse() in CreateTrigger(),
it throws error like "cannot CREATE OR REPLACE
TRIGGER because it is being used by active queries in this session".
This causes an break of the protection for internal cases above
and a contradiction of already passed test cases.
I though adding a condition of in_partition==false to call
CheckTableNotInUse(). But this doesn't work in a corner case that
child trigger generated internally is pending and
we don't want to allow the replacement of this kind of trigger.
Did you have any good idea to achieve both points at the same time ?


> * 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.
While waiting for a new reply, I'll doing those 2 refactorings.


Regards,
    Takamichi Osumi