Thread: Re: [GENERAL] Cascades Failing
[ redirected to -hackers ]
I wrote:
> This suggests that we need a way to prevent immediate execution of
> freshly queued triggers at the end of a command fired by an FK trigger.
> If we could move them to the end of the trigger queue that the FK
> operation itself is in, things would work reasonably well I think.
After a quick look through the code, it seems like the way to do this
is to add an extra bool parameter "nest_triggers" to _SPI_pquery, which
when false would simply suppress its calls to AfterTriggerBeginQuery
and AfterTriggerEndQuery --- thus causing any queued triggers to be
queued in the same trigger list the FK is in. We'd then expose this
parameter (only) via SPI_execute_snapshot, which is intended only for
RI trigger use anyway.
I think this would take some generalization of afterTriggerInvokeEvents,
which now might or might not find the target rel in the EState it's
passed, but otherwise it doesn't seem too invasive. Thoughts?
regards, tom lane
On Tue, 16 Aug 2005, Tom Lane wrote: > [ redirected to -hackers ] > > I wrote: > > This suggests that we need a way to prevent immediate execution of > > freshly queued triggers at the end of a command fired by an FK trigger. > > If we could move them to the end of the trigger queue that the FK > > operation itself is in, things would work reasonably well I think. > > After a quick look through the code, it seems like the way to do this > is to add an extra bool parameter "nest_triggers" to _SPI_pquery, which > when false would simply suppress its calls to AfterTriggerBeginQuery > and AfterTriggerEndQuery --- thus causing any queued triggers to be > queued in the same trigger list the FK is in. We'd then expose this > parameter (only) via SPI_execute_snapshot, which is intended only for > RI trigger use anyway. This seems right to me. I'd thought that SQL wanted the user triggers to be run after the updating directly, but reading it again, SQL03 at least seems to just talk about adding state changes for after triggers to the current trigger context AFAICS which means that the above seems to be what is requested by the spec in general. > I think this would take some generalization of afterTriggerInvokeEvents, > which now might or might not find the target rel in the EState it's > passed, but otherwise it doesn't seem too invasive. Thoughts? That doesn't seem too bad really, looking at afterTriggerInvokeEvents it doesn't look like it'd be that much work to change it to handle that case. I can put a patch together to see what it looks like.
On Tuesday 16 August 2005 09:17, Stephan Szabo wrote: > On Tue, 16 Aug 2005, Tom Lane wrote: > > [ redirected to -hackers ] > > > > I wrote: > > > This suggests that we need a way to prevent immediate execution of > > > freshly queued triggers at the end of a command fired by an FK trigger. > > > If we could move them to the end of the trigger queue that the FK > > > operation itself is in, things would work reasonably well I think. > > > > After a quick look through the code, it seems like the way to do this > > is to add an extra bool parameter "nest_triggers" to _SPI_pquery, which > > when false would simply suppress its calls to AfterTriggerBeginQuery > > and AfterTriggerEndQuery --- thus causing any queued triggers to be > > queued in the same trigger list the FK is in. We'd then expose this > > parameter (only) via SPI_execute_snapshot, which is intended only for > > RI trigger use anyway. > > This seems right to me. I'd thought that SQL wanted the user triggers to > be run after the updating directly, but reading it again, SQL03 at least > seems to just talk about adding state changes for after triggers to the > current trigger context AFAICS which means that the above seems to be what > is requested by the spec in general. > > > I think this would take some generalization of afterTriggerInvokeEvents, > > which now might or might not find the target rel in the EState it's > > passed, but otherwise it doesn't seem too invasive. Thoughts? > > That doesn't seem too bad really, looking at afterTriggerInvokeEvents it > doesn't look like it'd be that much work to change it to handle that case. > I can put a patch together to see what it looks like. I have a realworld test case of delete cascade (approx 90 cascaded tables, some more than 8 levels deep) failing on 8.0.3 (and 8.1) , this is one of a few issues that is preventing me from upgrading a couple of 7.4 boxen to 8.x, if you need testers for this patch, please let me know and I'll be glad to try it out and see if it solves the cascade problems I am experiencing. > > ---------------------------(end of broadcast)--------------------------- > TIP 2: Don't 'kill -9' the postmaster -- Darcy Buskermolen Wavefire Technologies Corp. http://www.wavefire.com ph: 250.717.0200 fx: 250.763.1759
On Tue, 16 Aug 2005, Stephan Szabo wrote: > On Tue, 16 Aug 2005, Tom Lane wrote: > > > I think this would take some generalization of afterTriggerInvokeEvents, > > which now might or might not find the target rel in the EState it's > > passed, but otherwise it doesn't seem too invasive. Thoughts? > > That doesn't seem too bad really, looking at afterTriggerInvokeEvents it > doesn't look like it'd be that much work to change it to handle that case. > I can put a patch together to see what it looks like. I did some work on this, and I'm getting a couple of other failures from other parts of the foreign key regression test (specifically an error that is no longer erroring in a multi-column on update set default). I'm going to need to look more closely to see if I can figure out why.
On Wed, 17 Aug 2005, Stephan Szabo wrote: > > On Tue, 16 Aug 2005, Stephan Szabo wrote: > > > On Tue, 16 Aug 2005, Tom Lane wrote: > > > > > I think this would take some generalization of afterTriggerInvokeEvents, > > > which now might or might not find the target rel in the EState it's > > > passed, but otherwise it doesn't seem too invasive. Thoughts? > > > > That doesn't seem too bad really, looking at afterTriggerInvokeEvents it > > doesn't look like it'd be that much work to change it to handle that case. > > I can put a patch together to see what it looks like. > > I did some work on this, and I'm getting a couple of other failures from > other parts of the foreign key regression test (specifically an error > that is no longer erroring in a multi-column on update set default). I'm > going to need to look more closely to see if I can figure out why. I think I see at least part of what's going on. It looks to me that events are being added, but not fired because they weren't marked. The event sequence seems to be: after trigger begin query add events for the actual statement after trigger end query fire trigger add events for the triggered statement finish trigger skip event added for triggered statement because it's not marked. Is the correct answer to continue marking and running the triggers until there are no immediate triggers left to run for this case?
Stephan Szabo <sszabo@megazone.bigpanda.com> writes:
> Is the correct answer to continue marking and running the triggers until
> there are no immediate triggers left to run for this case?
Hmm ... my recollection is that we put in the concept of marking because
we needed it for correct behavior in some cases. I don't recall exactly
why though.
regards, tom lane
On Fri, 19 Aug 2005, Tom Lane wrote:
> Stephan Szabo <sszabo@megazone.bigpanda.com> writes:
> > Is the correct answer to continue marking and running the triggers until
> > there are no immediate triggers left to run for this case?
>
> Hmm ... my recollection is that we put in the concept of marking because
> we needed it for correct behavior in some cases. I don't recall exactly
> why though.
The comment there talks about someone doing a set constraints immediate
inside a trigger function.
/* * Process all immediate-mode triggers queued by the query, and
move * the deferred ones to the main list of deferred events. * * Notice that we decide which ones
willbe fired, and put the
deferred * ones on the main list, before anything is actually fired. This * ensures reasonably sane
behaviorif a trigger function does * SET CONSTRAINTS ... IMMEDIATE: all events we have decided to
defer * will be available for it to fire. * * If we find no firable events, we don't have to
increment
firing_counter. */
I think we were worried about it either skipping events or potentially
running events twice in that case, but I don't remember exactly either.
I'm not sure that looping would affect that though, it'd be basically
mark (0)
increment firing id (0->1)
run triggers using the old id (0)
- if the set constraints immediate was run here, it'd mark using the
- incremented id (hopefully incrementing again - will need to check) and
- run using that id (1->2) and (1)
mark (2)
increment firing id (2->3)
run triggers using (2)
There might be some other reason that's not enshrined in the comment
though.
On Fri, 19 Aug 2005, Stephan Szabo wrote: > On Fri, 19 Aug 2005, Tom Lane wrote: > > > Stephan Szabo <sszabo@megazone.bigpanda.com> writes: > > > Is the correct answer to continue marking and running the triggers until > > > there are no immediate triggers left to run for this case? > > > > Hmm ... my recollection is that we put in the concept of marking because > > we needed it for correct behavior in some cases. I don't recall exactly > > why though. > Hmm, there's an issue with before triggers as well. We add the checks for the updates to the end of the current statement's queue and shouldn't run them until all the cascaded updates are done. However, if a before on update trigger of the fk side also updates an fk row that is in the middle of a series of these updates with a direct update statement, that statement's check will happen inside the before trigger, which will fail. It's not necessarily a triggered data change violation if the change happens to not change the key values or sets them to what the have already or will become. We could get around this by somehow inheriting the state of our direct parent trigger (whether or not it was in a new query), but that seems like it'd break other cases because the triggers would line up in the pre-8.0 sequence in that case. Or, is it correct to fail in this case because the statement is trying to update in a new query to a set of invalid keys?
On Wed, Aug 17, 2005 at 08:53:28AM -0700, Darcy Buskermolen wrote: > I have a realworld test case of delete cascade (approx 90 cascaded tables, > some more than 8 levels deep) failing on 8.0.3 (and 8.1) , this is one of a > few issues that is preventing me from upgrading a couple of 7.4 boxen to 8.x, > if you need testers for this patch, please let me know and I'll be glad to > try it out and see if it solves the cascade problems I am experiencing. Is this something we can put into the regression tests? -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com 512-569-9461