RE: extension patch of CREATE OR REPLACE TRIGGER

From: "tsunakawa(dot)takay(at)fujitsu(dot)com" <tsunakawa(dot)takay(at)fujitsu(dot)com>
To: "osumi(dot)takamichi(at)fujitsu(dot)com" <osumi(dot)takamichi(at)fujitsu(dot)com>, 'Tom Lane' <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: 'Peter Smith' <smithpb2250(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Surafel Temesgen <surafel3000(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: RE: extension patch of CREATE OR REPLACE TRIGGER
Date: 2020-10-27 06:07:44
Message-ID: TYAPR01MB2990041076AC194015D77A0DFE160@TYAPR01MB2990.jpnprd01.prod.outlook.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

Regards
Takayuki Tsunakawa

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Langote 2020-10-27 06:39:53 Re: Parallel Append can break run-time partition pruning
Previous Message Amul Sul 2020-10-27 05:26:01 Re: Assertion failure when ATTACH partition followed by CREATE PARTITION.