May 25, 2011 at 10:22 am
I have a trigger set to populate a project list - determined by a comma-delimited string of IDs (cimageid) .Regularly, the trigger is firing off twice, close enough together, so that when the two grab the information already in the string, they're getting the same info. The end result being that the first trigger execution finishes its update, then the second one finishes its update, losing the id that the first one had just inserted. Is there some way I can tell the trigger to wait if it's already executing? Or a better solution to this?
example:
initial members = 123
new id = 124
new id = 125
new id = 126
trigger fires first time, members updates to 123,124
trigger fires second time, members polled as 123,124, set members = 123,124,125
trigger fires while second is still running, members polled as 123,124, set members = 123,124,126. Loss of ID 125.
Here's the trigger. The problem is with setting @members.
CREATE TRIGGER TR_PopulateProjectOwners_U on MyTable FOR UPDATE AS
BEGIN
IF (select count(cimageid) from inserted where indexed = 1) =1
BEGIN
-- update ProjectOwner folder with content
Declare @ProjectName varchar(32)-- the ProjectName
, @ProjectOwner varchar (128) -- ProjectOwner of entry
, @ID int-- ProjectOwner project ID
, @cimageid int-- the new cimageid
, @C_ID int-- the MyProject ID of the ProjectOwner project
, @Members varchar(max)-- members of the ProjectOwner project
-- @ProjectName
SELECT @ProjectName = ProjectName from cases with (nolock) where caseid =( select caseid from inserted where indexed = 1 )
-- @ID
SELECT @ID = id from MyProject with (nolock) where path = 'ProjectOwners'
and parent = (select id from MyProject with (nolock) where path = @ProjectName)
--@cimageid
Select @cimageid = min(cimageid) from INSERTED where indexed = 1
-- debug
Raiserror ('Cust TR ProjectOwner project update executed for cimageid: %d.',10,1,@cimageid) with nowait, log
-- end debug
-- @ProjectOwner - accomodate for full or empty ProjectOwner
IF (select len(ProjectOwner) from inserted where indexed = 1 )= 0
BEGINSELECT @ProjectOwner = 'No ProjectOwner Info'
-- debug
Raiserror ('Cust TR ProjectOwner for cimageid %d has been set to No ProjectOwner Info.',10,1,@cimageid) with nowait,log
-- end debug
END
ELSE
BEGIN
SELECT @ProjectOwner = ProjectOwner from inserted where indexed = 1
END
-- @C_ID
Select @C_ID = ID from MyProject with (nolock) WHERE Path = convert(varchar(32),@ProjectOwner) and parent = @ID
-- @Members
Select @Members = Convert(varchar(max),Members) from MyProject with (nolock) where id = @C_ID
IF (select @members ) is NULL BEGIN Select @Members = '0' END
UPDATE MyProject
SET Members = rtrim(@Members) + ','+convert(varchar(20),@cimageid)
FROM MyProject
WHERE ID = @C_ID
INSERT Projmembers (imageid, projid)
VALUES (@cimageid,@C_ID)
END -- end of triggered execution
END -- end of trigger statement
go
May 25, 2011 at 12:43 pm
The whole trigger is designed to only work with one inserted row.
The heavy use of NOLOCK hints provide a risk of dirty reads which might be the root cause.
Rewrite the trigger to
a) use a set based method and
b) avoid the NOLOCK hints.
As a side note: the transaction inserting the rows will remain open until the code inside the trigger finished. This might significantly affect performance.
May 25, 2011 at 12:58 pm
I'd have to see the tables to really rewrite this, but this trigger has a fair number of "don't ever do that" kind of code in it.
First, you're denormalizing data by doing this in the first place. Why a comma-delimited list of member IDs? Why not a normalized list with them in separate rows? That would solve the whole problem for you right there, all by itself.
If you absolutely must do it this way, regardless of usual design considerations, you need to change to locking the target table for the duration of the trigger execution. That means, at the very least, taking an Intent Exclusive lock on it.
It honestly looks like you could do the whole trigger in a single update statement with the right From clause. (Using Merge in 2008 would be even better, but this is the 2005 forum, so I'm assuming you don't have that.) That would take and hold a lock on the row you need.
Keep in mind that anything other than normalizing this data will make it so you either have data errors or concurrency issues. Normalizing it will handle both of those. Keeping it denormalized like it currently is, you'll have one, the other, or both. Right now, you have both.
- Gus "GSquared", RSVP, OODA, MAP, NMVP, FAQ, SAT, SQL, DNA, RNA, UOI, IOU, AM, PM, AD, BC, BCE, USA, UN, CF, ROFL, LOL, ETC
Property of The Thread
"Nobody knows the age of the human race, but everyone agrees it's old enough to know better." - Anon
May 25, 2011 at 1:26 pm
I tend to agree with GSquared, you're tackling this the wrong way and asking for problems. I'd move this to a set based storage, where you are keeping the various user ids in another table.
May 26, 2011 at 6:27 am
Unfortunately I can't control the comma-delimited string, that's the way the developers designed it and the end applications read off that field, NOT the Projmembers table where they do get inserted as a list. It's an awful design but one I have no control of. And I'm not sure I can do the string off the list after the fact because there's no knowing when the list is "done".... or maybe not. Hmm... I have the trigger doing two updates - the comma-delimited string and the table list. I haven't had any hiccups in the table list. So maybe I should try having it just do the list then put an additional trigger over there to update the comma-delimited string. It would be much quicker since I wouldn't have to gather variables to figure out the Ids of the projects.
I have it set to only fire for one row transactions so as to avoid the trigger firing off for batch updates which come from other applications and user activity that would be unnecessary overhead and possibly undesirable. But perhaps I can change it to fire off based on the context_info of the app's spid to identify it instead of the row count in inserted. Some of our other triggers by other staff are written that way.
And your comment on the nolocks makes total sense. Thanks for all the feedback, I have a lot more ideas now.
May 26, 2011 at 9:09 am
Good luck, and I feel for you on the bad design.
Moving to one trigger that simplifies the update sounds like a better idea if you're stuck here. The less processing time here, the less chance that it will collide with another one.
Viewing 6 posts - 1 through 5 (of 5 total)
You must be logged in to reply to this topic. Login to reply