Thread: Fix disabled triggers with deferred constraints
About as obscure a bug as you can get - without the patch, disabled triggers for deferred constraints get run anyways. The patch is simple and works, but the "right" (and more complicated) fix may involve not adding the trigger to event->dte_item to begin with. -- Elliot
Attachment
Elliot Lee <sopwith@redhat.com> writes:
> About as obscure a bug as you can get - without the patch, disabled
> triggers for deferred constraints get run anyways. The patch is simple and
> works, but the "right" (and more complicated) fix may involve not adding
> the trigger to event->dte_item to begin with.
I remember looking at this issue and not doing anything because I
couldn't decide whether the test for enabled status should occur when
the trigger is queued or when it is executed --- or, perhaps, both?
Is there anything in the standard about it?
regards, tom lane
Tom Lane wrote: > Elliot Lee <sopwith@redhat.com> writes: > > About as obscure a bug as you can get - without the patch, disabled > > triggers for deferred constraints get run anyways. The patch is simple and > > works, but the "right" (and more complicated) fix may involve not adding > > the trigger to event->dte_item to begin with. > > I remember looking at this issue and not doing anything because I > couldn't decide whether the test for enabled status should occur when > the trigger is queued or when it is executed --- or, perhaps, both? > Is there anything in the standard about it? Was there any agreement on this? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian <pgman@candle.pha.pa.us> writes: > Tom Lane wrote: > > Elliot Lee <sopwith@redhat.com> writes: > > > About as obscure a bug as you can get - without the patch, disabled > > > triggers for deferred constraints get run anyways. The patch is simple and > > > works, but the "right" (and more complicated) fix may involve not adding > > > the trigger to event->dte_item to begin with. > > > > I remember looking at this issue and not doing anything because I > > couldn't decide whether the test for enabled status should occur when > > the trigger is queued or when it is executed --- or, perhaps, both? > > Is there anything in the standard about it? > > Was there any agreement on this? Any update on this? Cheers, Neil -- Neil Conway <neilconway@rogers.com> PGP Key ID: DB3C29FC
Neil Conway <nconway@klamath.dyndns.org> writes:
> Elliot Lee <sopwith@redhat.com> writes:
> About as obscure a bug as you can get - without the patch, disabled
> triggers for deferred constraints get run anyways. The patch is simple and
> works, but the "right" (and more complicated) fix may involve not adding
> the trigger to event->dte_item to begin with.
>
> I remember looking at this issue and not doing anything because I
> couldn't decide whether the test for enabled status should occur when
> the trigger is queued or when it is executed --- or, perhaps, both?
> Is there anything in the standard about it?
>>
>> Was there any agreement on this?
> Any update on this?
I think we're still waiting for someone to figure out what the behavior
should be per spec.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes:
> Neil Conway <nconway@klamath.dyndns.org> writes:
> > Elliot Lee <sopwith@redhat.com> writes:
> > I remember looking at this issue and not doing anything because I
> > couldn't decide whether the test for enabled status should occur when
> > the trigger is queued or when it is executed --- or, perhaps, both?
> > Is there anything in the standard about it?
[...]
> I think we're still waiting for someone to figure out what the behavior
> should be per spec.
I took a brief look at SQL99, but I couldn't find anything regarding
this issue (AFAICS it doesn't mention "disabled triggers" at all). But
given my prior track record for divining information from the
standards, perhaps someone should double-check :-)
I did notice some behavior which we don't implement AFAIK:
If the constraint mode is /deferred/, then the constraint is
effectively checked when the constraint mode is changed to
/immediate/ either explicitely by execution of a <set
constraints mode statement>, or implicitely at the end of the
current SQL-transaction.
(SQL99, Section 4.17.1, paragraph 3)
We don't recheck any outstanding deferred constraints when the
constraint mode is explicitly switched back to IMMEDIATE, as the
standard says we should.
Cheers,
Neil
--
Neil Conway <neilconway@rogers.com>
PGP Key ID: DB3C29FC
On 7 Aug 2002, Neil Conway wrote: > Tom Lane <tgl@sss.pgh.pa.us> writes: > > Neil Conway <nconway@klamath.dyndns.org> writes: > > > Elliot Lee <sopwith@redhat.com> writes: > > > I remember looking at this issue and not doing anything because I > > > couldn't decide whether the test for enabled status should occur when > > > the trigger is queued or when it is executed --- or, perhaps, both? > > > Is there anything in the standard about it? > > [...] > > > I think we're still waiting for someone to figure out what the behavior > > should be per spec. > > I took a brief look at SQL99, but I couldn't find anything regarding > this issue (AFAICS it doesn't mention "disabled triggers" at all). But > given my prior track record for divining information from the > standards, perhaps someone should double-check :-) > > I did notice some behavior which we don't implement AFAIK: > > If the constraint mode is /deferred/, then the constraint is > effectively checked when the constraint mode is changed to > /immediate/ either explicitely by execution of a <set > constraints mode statement>, or implicitely at the end of the > current SQL-transaction. > > (SQL99, Section 4.17.1, paragraph 3) > > We don't recheck any outstanding deferred constraints when the > constraint mode is explicitly switched back to IMMEDIATE, as the > standard says we should. (have been out since last wed, responding now) Seems to work for me in some cases if I understand correctly. sszabo=# create table iia(a int primary key); NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index 'iia_pkey' for table 'iia' CREATE TABLE sszabo=# create table iib(a int constraint foo1 references iia(a) deferrable initially deferred); NOTICE: CREATE TABLE will create implicit trigger(s) for FOREIGN KEY check(s) CREATE TABLE sszabo=# begin; BEGIN sszabo=# insert into iib values (1); INSERT 9678819 1 sszabo=# end; ERROR: foo1 referential integrity violation - key referenced from iib not found in iia sszabo=# begin; BEGIN sszabo=# insert into iib values (1); INSERT 9678820 1 sszabo=# set constraints all immediate; ERROR: foo1 referential integrity violation - key referenced from iib not found in iia
On Mon, 12 Aug 2002, Stephan Szabo wrote: > On 7 Aug 2002, Neil Conway wrote: > > > Tom Lane <tgl@sss.pgh.pa.us> writes: > > > Neil Conway <nconway@klamath.dyndns.org> writes: > > > > Elliot Lee <sopwith@redhat.com> writes: > > > > I remember looking at this issue and not doing anything because I > > > > couldn't decide whether the test for enabled status should occur when > > > > the trigger is queued or when it is executed --- or, perhaps, both? > > > > Is there anything in the standard about it? > > > > [...] > > > > > I think we're still waiting for someone to figure out what the behavior > > > should be per spec. > > > > I took a brief look at SQL99, but I couldn't find anything regarding > > this issue (AFAICS it doesn't mention "disabled triggers" at all). But > > given my prior track record for divining information from the > > standards, perhaps someone should double-check :-) > > > > I did notice some behavior which we don't implement AFAIK: > > > > If the constraint mode is /deferred/, then the constraint is > > effectively checked when the constraint mode is changed to > > /immediate/ either explicitely by execution of a <set > > constraints mode statement>, or implicitely at the end of the > > current SQL-transaction. > > > > (SQL99, Section 4.17.1, paragraph 3) > > > > We don't recheck any outstanding deferred constraints when the > > constraint mode is explicitly switched back to IMMEDIATE, as the > > standard says we should. > > (have been out since last wed, responding now) > Seems to work for me in some cases if I understand correctly. Replying to myself. This is before doing a cvs update for the patch you sent.
On 7 Aug 2002, Neil Conway wrote:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
> > Neil Conway <nconway@klamath.dyndns.org> writes:
> > > Elliot Lee <sopwith@redhat.com> writes:
> > > I remember looking at this issue and not doing anything because I
> > > couldn't decide whether the test for enabled status should occur when
> > > the trigger is queued or when it is executed --- or, perhaps, both?
> > > Is there anything in the standard about it?
>
> [...]
>
> > I think we're still waiting for someone to figure out what the behavior
> > should be per spec.
>
> I took a brief look at SQL99, but I couldn't find anything regarding
> this issue (AFAICS it doesn't mention "disabled triggers" at all). But
> given my prior track record for divining information from the
> standards, perhaps someone should double-check :-)
I had a pretty hard look around SQL99. It does not appear to say anything
explicit about disabling triggers. This should be clear from page 90: 4.35
Triggers. This specifies the trigger descriptor. Those familiar with SQL99
know that it just about mandates that all state information about any
object in the system is recorded in its descriptor. The fact that
enabled/disabled state information is not recorded in the trigger
descriptor suggests that it is only ever enabled.
More over there is no case when a trigger is not executed, according to
10.12 'Execution of triggers'.
I dug deeper, wondering if it may be implicitly disabled given the
disabling of its 'dependencies', shall we call them. Namely: the base
table or the procedure used in the trigger action. Tables cannot be
disabled or made in active. As for the procedure, <SQL procedure
statement>, this expands to SQL which, itself, cannot be 'disabled'.
The spec is a large one and I didn't look at all references to triggers --
since there are hundreds -- but I don't believe that there is any
precedent for an implementation of DISABLE TRIGGER.
FWIW, i think that in the case of deferred triggers they should all be
added to the queue and whether they are executed or not should be
evaluated inside DeferredTriggerExecute() with:
if(LocTriggerData.tg_trigger->tgenabled == false)
return;
Gavin
Gavin Sherry <swm@linuxworld.com.au> writes:
> ...The spec is a large one and I didn't look at all references to triggers --
> since there are hundreds -- but I don't believe that there is any
> precedent for an implementation of DISABLE TRIGGER.
Thanks for the dig. I was hoping we could get some guidance from the
spec, but it looks like not. How about other implementations --- does
Oracle support disabled triggers? DB2? etc?
> FWIW, i think that in the case of deferred triggers they should all be
> added to the queue and whether they are executed or not should be
> evaluated inside DeferredTriggerExecute() with:
> if(LocTriggerData.tg_trigger->tgenabled == false)
> return;
So check the state at execution, not when the triggering event occurs.
I don't have any strong reason to object to that, but I have a gut
feeling that it still needs to be thought about...
Let's see, I guess there are several possible changes of state for a
deferred trigger between the triggering event and the end of
transaction:
* Trigger deleted. Surely the trigger shouldn't be executed, but should
we raise an error or just silently ignore it? (I suspect right now we
crash :-()
* Trigger created. In some ideal world we might think that such a
trigger should be fired, but in reality that ain't gonna happen; we're
not going to record every possible event on the speculation that some
trigger for it might be created later in the transaction.
* Trigger disabled. Your proposal is to not fire it. Okay, comports
with the deleted case, if we make that behavior be silently-ignore.
* Trigger enabled. Your proposal is to fire it. Seems not to comport
with the creation case --- does that bother anyone?
* Trigger changed from not-deferred to deferred. If we already fired it
for the event, we surely shouldn't fire it again. I believe the code
gets this case right.
* Trigger changed from deferred to not-deferred. As Neil was pointing
out recently, this really should cause the trigger to be fired for the
pending event immediately, but we don't get that right at the moment.
(I suppose a stricter interpretation would be to raise an error because
we can't do anything that really comports with the intended behavior
of either case.)
How do these various cases relate? Can we come up with a unified
rationale?
regards, tom lane
Tom Lane wrote: > Gavin Sherry <swm@linuxworld.com.au> writes: >>...The spec is a large one and I didn't look at all references to triggers -- >>since there are hundreds -- but I don't believe that there is any >>precedent for an implementation of DISABLE TRIGGER. > > Thanks for the dig. I was hoping we could get some guidance from the > spec, but it looks like not. How about other implementations --- does > Oracle support disabled triggers? DB2? etc? Oracle does for sure. With a complex app (i.e. Oracle Applications) being able to disable triggers from time-to-time is *indispensable*. Not sure about DB2. My knowledge of MSSQL is getting dated, but as of MSSQL7 I don't *think* you can disable a trigger. Joe
On Tue, 13 Aug 2002, Tom Lane wrote: > Gavin Sherry <swm@linuxworld.com.au> writes: > > ...The spec is a large one and I didn't look at all references to triggers -- > > since there are hundreds -- but I don't believe that there is any > > precedent for an implementation of DISABLE TRIGGER. > > Thanks for the dig. I was hoping we could get some guidance from the > spec, but it looks like not. How about other implementations --- does > Oracle support disabled triggers? DB2? etc? Oracle 8 (and I presume 9) allows you to disable/enable triggers through alter table and alter trigger. My 8.1.7 documentation is silent on the cases you mention below and I do not have an oracle installation handy to test. Anyone? > > > FWIW, i think that in the case of deferred triggers they should all be > > added to the queue and whether they are executed or not should be > > evaluated inside DeferredTriggerExecute() with: > > if(LocTriggerData.tg_trigger->tgenabled == false) > > return; > > So check the state at execution, not when the triggering event occurs. > I don't have any strong reason to object to that, but I have a gut > feeling that it still needs to be thought about... > > Let's see, I guess there are several possible changes of state for a > deferred trigger between the triggering event and the end of > transaction: > > * Trigger deleted. Surely the trigger shouldn't be executed, but should > we raise an error or just silently ignore it? (I suspect right now we > crash :-() > > * Trigger created. In some ideal world we might think that such a > trigger should be fired, but in reality that ain't gonna happen; we're > not going to record every possible event on the speculation that some > trigger for it might be created later in the transaction. It doesn't need to be an ideal world. We're only talking about deferred triggers after all. Why couldn't CreateTrgger() just have a look through deftrig_events, check for its relid and if its in there, call deferredTriggerAddEvent(). > * Trigger disabled. Your proposal is to not fire it. Okay, comports > with the deleted case, if we make that behavior be silently-ignore. > > * Trigger enabled. Your proposal is to fire it. Seems not to comport > with the creation case --- does that bother anyone? > > * Trigger changed from not-deferred to deferred. If we already fired it > for the event, we surely shouldn't fire it again. I believe the code > gets this case right. Agreed. > * Trigger changed from deferred to not-deferred. As Neil was pointing > out recently, this really should cause the trigger to be fired for the > pending event immediately, but we don't get that right at the moment. > (I suppose a stricter interpretation would be to raise an error because > we can't do anything that really comports with the intended behavior > of either case.) I think this should generate an error as it doesn't sit well with the spec IMHO. Gavin
Where are we on this? Does someone have a patch? --------------------------------------------------------------------------- Gavin Sherry wrote: > On Tue, 13 Aug 2002, Tom Lane wrote: > > > Gavin Sherry <swm@linuxworld.com.au> writes: > > > ...The spec is a large one and I didn't look at all references to triggers -- > > > since there are hundreds -- but I don't believe that there is any > > > precedent for an implementation of DISABLE TRIGGER. > > > > Thanks for the dig. I was hoping we could get some guidance from the > > spec, but it looks like not. How about other implementations --- does > > Oracle support disabled triggers? DB2? etc? > > Oracle 8 (and I presume 9) allows you to disable/enable triggers through > alter table and alter trigger. My 8.1.7 documentation is silent on the > cases you mention below and I do not have an oracle installation handy to > test. Anyone? > > > > > > FWIW, i think that in the case of deferred triggers they should all be > > > added to the queue and whether they are executed or not should be > > > evaluated inside DeferredTriggerExecute() with: > > > if(LocTriggerData.tg_trigger->tgenabled == false) > > > return; > > > > So check the state at execution, not when the triggering event occurs. > > I don't have any strong reason to object to that, but I have a gut > > feeling that it still needs to be thought about... > > > > Let's see, I guess there are several possible changes of state for a > > deferred trigger between the triggering event and the end of > > transaction: > > > > * Trigger deleted. Surely the trigger shouldn't be executed, but should > > we raise an error or just silently ignore it? (I suspect right now we > > crash :-() > > > > * Trigger created. In some ideal world we might think that such a > > trigger should be fired, but in reality that ain't gonna happen; we're > > not going to record every possible event on the speculation that some > > trigger for it might be created later in the transaction. > > It doesn't need to be an ideal world. We're only talking about deferred > triggers after all. Why couldn't CreateTrgger() just have a look through > deftrig_events, check for its relid and if its in there, call > deferredTriggerAddEvent(). > > > * Trigger disabled. Your proposal is to not fire it. Okay, comports > > with the deleted case, if we make that behavior be silently-ignore. > > > > * Trigger enabled. Your proposal is to fire it. Seems not to comport > > with the creation case --- does that bother anyone? > > > > * Trigger changed from not-deferred to deferred. If we already fired it > > for the event, we surely shouldn't fire it again. I believe the code > > gets this case right. > > Agreed. > > > * Trigger changed from deferred to not-deferred. As Neil was pointing > > out recently, this really should cause the trigger to be fired for the > > pending event immediately, but we don't get that right at the moment. > > (I suppose a stricter interpretation would be to raise an error because > > we can't do anything that really comports with the intended behavior > > of either case.) > > I think this should generate an error as it doesn't sit well with the > spec IMHO. > > Gavin > > > ---------------------------(end of broadcast)--------------------------- > TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Is this resolved? I think so. --------------------------------------------------------------------------- Tom Lane wrote: > Elliot Lee <sopwith@redhat.com> writes: > > About as obscure a bug as you can get - without the patch, disabled > > triggers for deferred constraints get run anyways. The patch is simple and > > works, but the "right" (and more complicated) fix may involve not adding > > the trigger to event->dte_item to begin with. > > I remember looking at this issue and not doing anything because I > couldn't decide whether the test for enabled status should occur when > the trigger is queued or when it is executed --- or, perhaps, both? > Is there anything in the standard about it? > > regards, tom lane > > ---------------------------(end of broadcast)--------------------------- > TIP 6: Have you searched our list archives? > > http://archives.postgresql.org > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Is this resolved?
No, we still haven't decided what to do (ie, when should the test
for disabled status occur).
Disabling triggers is not and never has been a supported feature,
so I don't consider this a "must fix" issue for 7.3 ...
regards, tom lane
Added to TODO:
* Allow triggers to be disabled [trigger]
---------------------------------------------------------------------------
Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Is this resolved?
>
> No, we still haven't decided what to do (ie, when should the test
> for disabled status occur).
>
> Disabling triggers is not and never has been a supported feature,
> so I don't consider this a "must fix" issue for 7.3 ...
>
> regards, tom lane
>
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073