RE: extension patch of CREATE OR REPLACE TRIGGER

Hi,

From: Tsunakawa, Takayuki < tsunakawa(dot)takay(at)fujitsu(dot)com>
> From: osumi(dot)takamichi(at)fujitsu(dot)com <osumi(dot)takamichi(at)fujitsu(dot)com>
> > > > * 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 !
>
> I understand this patch is intended for helping users to migrate from other
> DBMSs (mainly Oracle?) because they can easily alter some trigger attributes
> (probably the trigger action and WHEN condition in practice.) OTOH, the
> above issue seems to be associated with the Postgres-specific constraint
> trigger that is created with CONSTRAINT clause. (Oracle and the SQL
> standard doesn't have an equivalent feature.)
>
> So, how about just disallowing the combination of REPLACE and
> CONSTRAINT? I think nobody would be crippled with that. If someone
> wants the combination by all means, that can be a separate enhancement.
I didn't notice this kind of perspective and you are right.
In order to achieve the purpose to help database migration from Oracle to Postgres,
prohibitting the usage of OR REPLACE for constraint trigger is no problem.

Thanks for your great advice. I fixed and created new version.
Also, the size of this patch becomes much smaller.

Best,
Takamichi Osumi