Hello
> > * 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.
I'm done with those refactorings. Please have a look at the changes
of the latest patch.
> > * 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 ?
Still, in terms of this point, I'm waiting for a comment !
Regards,
Takamichi Osumi
CREATE_OR_REPLACE_TRIGGER_v13.patch
Description: CREATE_OR_REPLACE_TRIGGER_v13.patch
