June 11, 2013 at 8:54 am
I'm trying to troubleshoot some inherited code that uses cursors and seems to be causing blocking.
The cursor gets loaded from a select statement, and eventually does an insert into a production table. I think it loops around and does 1 insert per record in the cursor.
Is a lock held on the table being inserted until the cursor loop completes ? Or is there a lock-release-lock-release for each loop in the cursor ?
Other users are trying to issue updates against the same table but seem to be blocked for long periods of time.
Or am I being to vague to get an answer ?
tia
June 11, 2013 at 8:57 am
It could be locking especially if there is a transaction outside the loop. From the brief description it sounds like you just make that cursor go the way of the dodo bird. It will run faster which help eliminate the blocking. It is a win-win. 😀
_______________________________________________________________
Need help? Help us help you.
Read the article at http://www.sqlservercentral.com/articles/Best+Practices/61537/ for best practices on asking questions.
Need to split a string? Try Jeff Modens splitter http://www.sqlservercentral.com/articles/Tally+Table/72993/.
Cross Tabs and Pivots, Part 1 – Converting Rows to Columns - http://www.sqlservercentral.com/articles/T-SQL/63681/
Cross Tabs and Pivots, Part 2 - Dynamic Cross Tabs - http://www.sqlservercentral.com/articles/Crosstab/65048/
Understanding and Using APPLY (Part 1) - http://www.sqlservercentral.com/articles/APPLY/69953/
Understanding and Using APPLY (Part 2) - http://www.sqlservercentral.com/articles/APPLY/69954/
June 11, 2013 at 9:03 am
I only see a transaction inside the loop, doing a commit/rollback based on error code.
June 11, 2013 at 9:07 am
Well, seeing as we can't see what you see it is hard to give you a really good answer instead of a shot in the dark.
June 11, 2013 at 9:43 am
You asked for it 😛
Here's the code as I found it. The table being locked from other users is WebSiteMessageRecipient
CREATE PROCEDURE [dbo].[spWebSiteMessageDumpProcess]
AS
BEGIN
-- SET NOCOUNT ON added to prevent extra result sets from
-- interfering with SELECT statements.
SET NOCOUNT ON;
declare @status int
declare @StatusDescription varchar(512)
declare @MessageID int
declare @sender int
declare@recipients varchar(max)
declare@recipients_status varchar(max)
declare @sentdate datetime
declare @subject varchar(max)
declare @body varchar(max)
declare @recipientcount int
declare @ONSMBodyID int
DECLARE @ONSMSenderID int
declare @ONSMMDID int
DECLARE @currentRecipient INT
DECLARE @currentRecipient_status VARCHAR(10)
DECLARE @RCOUNT INT
DECLARE @ONSMMEMID INT
DEClare @processStartDate datetime
DECLARE @processEndDate datetime
DECLARE @ONSMRecipientMemID INT
DECLARE @splitRecipient TABLE (
id int,
DATA NVARCHAR(max) )
DECLARE @splitRecipient_status TABLE (
id int,
DATA NVARCHAR(max) )
SET @RCOUNT = 0
set @processStartDate = getDate()
delete from WebSiteMessageDump where ProcessStatus <> -1 and DateDiff(day,processDate, getDate()) > 40;
DECLARE WebSiteMessageDump_cursor CURSOR FOR
SELECTONSMMD_ID,Message_ID, sender, Recipients,recipients_status, sent_date,
subject,body, recipient_count,Message_ID
FROMWebSiteMessageDump
WHEREProcessStatus = -1
ORDER BY CreatedDate;
open WebSiteMessageDump_cursor;
fetch next fromWebSiteMessageDump_cursor
into @ONSMMDID,@MessageID, @sender,@recipients,@recipients_status, @sentdate, @subject, @body,@recipientcount,@MESSAGEID;
WHILE @@FETCH_STATUS = 0
BEGIN
begin transaction WebSiteMessgeInsert
insert into WebSiteMessageBody
(Body,subject,Message_ID)
values(@body,@subject,@MESSAGEID)
IF @@ERROR <> 0
BEGIN
SET @status = 1
set @StatusDescription ='Error Inserting into WebSiteMessageBody'
goto ExitPoint
END
select @ONSMBodyID = @@identity
IF @ONSMBodyID is NULL
BEGIN
SET @status = 2
set @StatusDescription ='Error Inserting into WebSiteMessageBody: IDENTITY COLLUM IS NULL'
goto ExitPoint
END
insert into WebSiteMessageSender
(Sender, recipient_count, MessageSentDate)
values(@Sender,@recipientcount, @sentdate)
IF @@ERROR <> 0
BEGIN
SET @status = 3
set @StatusDescription ='Error Inserting into WebSiteMessageSender'
goto ExitPoint
END
select @ONSMSenderID = @@identity
IF @ONSMSenderID is NULL
BEGIN
SET @status = 4
set @StatusDescription ='Error Inserting into WebSiteMessageSender: IDENTITY COLLUM IS NULL'
goto ExitPoint
END
insert into @splitRecipient(id,data)
select id, data
from dbo.split(@recipients,',')
insert into @splitRecipient_status(id,data)
select id, data
from dbo.split(@recipients_status,',')
SET @RCOUNT = 1
WHILE @RCOUNT <= @recipientcount
BEGIN
SELECT @currentRecipient = DATA FROM @splitRecipient WHERE ID = @RCOUNT
SELECT @currentRecipient_status = replace(replace(DATA,@currentRecipient,''),':','') FROM @splitRecipient_status WHERE LEFT(data, Charindex(':', data) - 1) = @currentRecipient
--SELECT @currentRecipient_status = replace(replace(DATA,@currentRecipient,''),':','') FROM @splitRecipient_status WHERE ID = @RCOUNT
SELECT @ONSMMEMID = dbo.fnGetMemberID(@Sender)
SELECT @ONSMRecipientMemID = dbo.fnGetMemberID(@currentRecipient)
INSERT INTO WebSiteMessageRecipient
(ONSMSender_ID,
Sender,
ONSMMEM_ID,
ONSMRecipient_ID,
recipients_status,
ONSMRecipientMem_ID,
ONSMBody_ID)
VALUES(@ONSMSenderID,
@Sender,
@ONSMMEMID,
@currentRecipient,
@currentRecipient_status,
@ONSMRecipientMemID,
@ONSMBodyID)
IF @@ERROR <> 0
BEGIN
SET @status = 5
set @StatusDescription ='Error Inserting into WebSiteMessageRecipient for recipientID # '+ convert(varchar(30),@currentRecipient)
goto ExitPoint
END
set @RCOUNT = @RCOUNT + 1
END
set @status = 0
set @StatusDescription ='Success Processing'
ExitPoint:
IF @status = 0
COMMIT transaction WebSiteMessgeInsert
ELSE
rollback transaction WebSiteMessgeInsert
set @processEndDate = getDate()
update WebSiteMessageDump set ProcessStatus =@Status , ProcessStatusDescription = @StatusDescription, processDate = @processEndDate where ONSMMD_ID = @ONSMMDID
fetch next fromWebSiteMessageDump_cursor
into @ONSMMDID,@MessageID, @sender,@recipients,@recipients_status, @sentdate, @subject, @body,@recipientcount,@MESSAGEID;
END
CLOSE WebSiteMessageDump_cursor;
DEALLOCATE WebSiteMessageDump_cursor;
-- Insert statements for procedure here
select * from WebSiteMessageDump WHERE ProcessStatus <> 0 and processDate >=@processStartDate and processDate <=@processEndDate
END
June 11, 2013 at 9:56 am
Once I see named code blocks and GOTOs all over the place my eyes started bleeding.
I know that you inherited this so it won't offend you when I say this thing does not need to be fixed. It needs to be taken out to pasture and shot like a horse with a broken leg. A complete 100% rewrite is the only thing that can fix this.
Scalar functions, a split function (that might be horrible, or not).
That table is being inserted in a loop inside of a transaction. It is no surprise that is being blocked.
_______________________________________________________________
Need help? Help us help you.
Read the article at http://www.sqlservercentral.com/articles/Best+Practices/61537/ for best practices on asking questions.
Need to split a string? Try Jeff Modens splitter http://www.sqlservercentral.com/articles/Tally+Table/72993/.
Cross Tabs and Pivots, Part 1 – Converting Rows to Columns - http://www.sqlservercentral.com/articles/T-SQL/63681/
Cross Tabs and Pivots, Part 2 - Dynamic Cross Tabs - http://www.sqlservercentral.com/articles/Crosstab/65048/
Understanding and Using APPLY (Part 1) - http://www.sqlservercentral.com/articles/APPLY/69953/
Understanding and Using APPLY (Part 2) - http://www.sqlservercentral.com/articles/APPLY/69954/
June 11, 2013 at 10:40 am
Nice Reply 😀
I don't really want to take on anything else right now, but may have to.
Back to my original question, would you expect that table to be locked until the entire process completes ?
June 11, 2013 at 10:58 am
my guess would be the WHILE loop *cough* being the reason for locking.
Depending on the value of @recipientcount it might take a while to perform all those single row inserts.
Step 1 would be to replace that WHILE stuff with a set base solution. And once you're changing the code anyway, get rid of the outer c.u.r.s.o.r. *cough* 😉
Afther those two steps you won't have to rewrite the code 😀
June 11, 2013 at 11:02 am
homebrew01 (6/11/2013)
Nice Reply 😀I don't really want to take on anything else right now, but may have to.
Back to my original question, would you expect that table to be locked until the entire process completes ?
Given that the transaction begins at the beginning of each iteration through the cursor and then starts a loop to insert into that table it will be blocked for most of the time this is running. There will be a small window that other processes should be able to access that table but they should be able to jump in between iterations.
_______________________________________________________________
Need help? Help us help you.
Read the article at http://www.sqlservercentral.com/articles/Best+Practices/61537/ for best practices on asking questions.
Need to split a string? Try Jeff Modens splitter http://www.sqlservercentral.com/articles/Tally+Table/72993/.
Cross Tabs and Pivots, Part 1 – Converting Rows to Columns - http://www.sqlservercentral.com/articles/T-SQL/63681/
Cross Tabs and Pivots, Part 2 - Dynamic Cross Tabs - http://www.sqlservercentral.com/articles/Crosstab/65048/
Understanding and Using APPLY (Part 1) - http://www.sqlservercentral.com/articles/APPLY/69953/
Understanding and Using APPLY (Part 2) - http://www.sqlservercentral.com/articles/APPLY/69954/
June 11, 2013 at 11:14 am
LutzM (6/11/2013)
my guess would be the WHILE loop *cough* being the reason for locking.Depending on the value of @recipientcount it might take a while to perform all those single row inserts.
Step 1 would be to replace that WHILE stuff with a set base solution. And once you're changing the code anyway, get rid of the outer c.u.r.s.o.r. *cough* 😉
Afther those two steps you won't have to rewrite the code 😀
Even if you didn't choose to get rid of the cursor - you can still get rid of the WHILE loop. You've already got a table variable being used for the intermediate results for the recipient: if you did the rest of the the intermediate processing that is being done in the WHILE, but do that in the table variable then do the insert of the entire contents of the table variable in set fashion, your lock on that recipients table would be for a much smaller window.
That said - don't take that as encouragement NOT to kill the cursor. You'll be better off once that thing hits the trash heap.
----------------------------------------------------------------------------------
Your lack of planning does not constitute an emergency on my part...unless you're my manager...or a director and above...or a really loud-spoken end-user..All right - what was my emergency again?
June 12, 2013 at 2:46 am
Another couple of things I noticed that you might want to change, the use of @@Identity which isnt thread safe, so you might want to consider switching it to use SCOPE_IDENTITY().
http://msdn.microsoft.com/en-us/library/ms190315.aspx
How often is this SP run?
This looks like it was written by an Application developer rather than an SQL developer, Personally I would be marking this procedure as an amber category (probably Red) SP for refactoring as the issues are only going to get worse.
_________________________________________________________________________
SSC Guide to Posting and Best Practices
June 12, 2013 at 5:51 am
You know, I can say I've seen a lot worse. Yes, there are problems with it, but it isn't even close to the biggest offender I've seen. There was once a trigger that was over 1400 lines long. It had lots of named locations, goto statements everywhere, conditional transactions, cursors, implicit casts and dismal performance. It was the best example of spaghetti logic I've ever seen. It kept track of hours allocation on jobs and the data was used for costing and billing. It was written by several rambo-style coders - a term I use to refer to people who just keep going and adding more code until it eventually works. No finesse; just brute force.
Anyway, the trigger needed to be updated, but nobody wanted to even try because it was so ridiculous. I took the challenge and modularized the whole thing. It took a couple of weeks to pull off, but in the end, it worked and was significantly shorter. Yes, this type of thing can be done, but it shouldn't be entered into thinking it'll be done quickly and I don't need to emphasize the importance of testing.
With all that being said, when you refactor this and the approach you take is completely up to you. You may decide that, in the interest of timing, you're going to tackle the dbo.split function first. You may decide that other things need to wait and the whole thing needs to be redone now. Either way, there will probably be some pain, so just be ready for that. In the grand scheme of things, getting it to work with acceptable performance is the goal and its importance has to be weighed against the other things you have on your plate. I know this isn't a solution to the problem, but sometimes just the perspective of the situation from others is a benefit to me on something like this.
June 12, 2013 at 7:52 am
Thanks for all the replies.
It runs every day at 4:00 am, and usually take 10 minutes. It processes a list of members and their recipeints of emails for reporting purposes. 1 user accidentally somehow managed to select all members for emails, and sent out 150,000 + emails, and this reporting job really bogged down trying to process them all, so it was still running during business hours.
June 12, 2013 at 7:59 am
If its only called once a day then its not too bad, so probably an Amber for refactoring, it it was being run more regulararly then you may want to look at it with a little more urgancy.
To prevent someone sending out 150K+ emails you might want to consider putting a hard cap on on the process, say 10,000 emails or what ever the daily avg is +50% to prevent it happening again, but log that it was a truncated run.
_________________________________________________________________________
SSC Guide to Posting and Best Practices
Viewing 14 posts - 1 through 13 (of 13 total)
You must be logged in to reply to this topic. Login to reply