deadlock issue
-
We're currently developing a multithreading windows service which is simply dispatching work to the server farm.
The deadlock occured in one of the stored procedures. The sp is pretty simple, it first gets the minimum id in the table, then set machine name and windows service name for that row. Finally return that row.
here is the sp:
-----------------------------------------------------
CREATE PROCEDURE dbo.GetNextUnprocessedMessage
@StateNew varchar(20),
@StateNext varchar(20),
@WinServiceName varchar(20),
@MachineName varchar(20)
AS
DECLARE @id int
BEGIN TRANSACTION
SET @id = -1;
-- grab the next available message, if present
SET @id = (
SELECT MIN(unprocessed_message_id)
FROM unprocessed_message
WITH (HOLDLOCK, ROWLOCK)
WHERE state = @StateNew
)
-- update the state of the row we just selected (and locked) so that it can't
-- be grabbed by anyone else using this procedure
UPDATE unprocessed_message
WITH(ROWLOCK)
SET state = @StateNext,
win_service_name = @WinServiceName,
machine_name = @MachineName
WHERE unprocessed_message_id = @id
IF (@@ERROR > 0)
BEGIN
ROLLBACK TRANSACTION
RAISERROR('Error fetching next message', 15, -1, @@ERROR)
END
-- return the newly updated row
SELECT *
FROM unprocessed_message
WHERE unprocessed_message_id = @id
COMMIT TRANSACTION
GO
--------------------------------------------------
we set clustered index (unprocessed_message_id) on unprocessed_message, and the isolation level is default (read committed).
please help me on this issue. Thank you.
David
-
Using 'with HoldLock' raises the isolation level to Serializable and will place a range lock on the index for the column 'state'. If 'state' is not indexed then an excessive amout of locks may be caused to be taken.
In your case I think you can get away with an isolation level of 'Repeatable Read' which would only lock the one row retrieved not all rows with state = @StateNew. I think 'with updlock' may give you the equivalent of 'Repeatalbe Read' but I'm not certain about that. Perhaps someone else can confirm.
A minor point, the commit statement can be moved up in the code to just before the last 'Select' that returns the newly updated row to the client. This will reduce the amount of time the locks are held.
Hope that helps - ron
-
Perhaps you could get away with just an update, and not having to deal with transactions and locking rows to prevent others from reading in 'mid-process' of the retrieval/update..
Here's an idea, basically based on the idea that you don't necessarily need to get the lowest id, but just any single record with the current state that needs to be processed..
create table #x
( id int not null primary key,
state int not null,
name varchar(20) null
)
insert #x select 1, 1, 'foobar'
insert #x select 2, 0, null
insert #x select 3, 0, null
declare @id int, @stateNew int, @stateNext int, @name varchar(20)
select @stateNew = 0, @stateNext = 1, @name = 'bozo'
set rowcount 1
update #x
set @id = id,
state = @stateNext,
name = @name
where state = @stateNew
set rowcount 0
select @id --contains the id that was updated
..then one can take @id and do further stuff, now knowing which row was updated...
/Kenneth
-
Ron,
Thank you for the idea. I may try that to see if it works.
Ken,
Thank you for the help too.
In our case, we have to get the min id, because the processing order of messages are critical. We can't simply retrieve one message and do the update. Sorry for not clearify that.
-
Personally, I would persue Ken's Idea and combine both (select + update) into one statement that way you could get rid of the locking problem:
ex:
UPDATE unprocessed_message
SET state = @StateNext,
@id = id,
win_service_name = @WinServiceName,
machine_name = @MachineName
WHERE unprocessed_message_id = SELECT MIN(unprocessed_message_id)
FROM unprocessed_message
WHERE state = @StateNew
hth
* Noel -
Noel,
BOL:
Transactions must be run at an isolation level of repeatable read or higher to prevent lost updates that can occur when two transactions each retrieve the same row, and then later update the row based on the originally retrieved values. If the two transactions update rows using a single UPDATE statement and do not base the update on the previously retrieved values, lost updates cannot occur at the default isolation level of read committed.
So your suggestion will work fine but its not really avoiding the locking issue as the lock is implied and taken by the system to prevent a lost update situation. That is, other users using an update statement are blocked from reading/updating the row until the first update is complete.
Of course doing it all in one statement is more efficient and allows for shorter period of time that the lock is held.
-
Noel,
I'm not at a machine where I test at the moment but is that syntax correct? I haven't seen it before. If so, live and learn.
UPDATE unprocessed_message
SET state = @StateNext,
@id = id, <-------------
...
EDIT: never mind -- see post below.
-
I guess I can answer my own question from BOL:
UPDATE (T-SQL)
Changes existing data in a table.
Syntax
UPDATE
{
table_name WITH ( <table_hint_limited> [...n])
| view_name
| rowset_function_limited
}
SET
{column_name = {expression | DEFAULT | NULL}
| @variable = expression
| @variable = column = expression } [,...n]
-
The point with this 'backward' notation, is that you can update (the other columns) and assign values from columns not updated (ie the PK) in one go.
The net effect is the same as doing update + select from the row updated (for this you need to use explicit transactions in order to prevent others from reading your row in 'mid-process').
However, if all you do is just a single update, then it's impossible for others to read the same key value, since only one at a time can update a row.
The same technique can be used for generating your own keyvalues based on a counter table - all issues with risks assigning the same value to simultaneous readers will just automagically dissappear.
/Kenneth
-
I am interested to know if noeld's elegant solution really does avoid deadlock.:
UPDATE unprocessed_message
SET state = @StateNext,
@id = id,
win_service_name = @WinServiceName,
machine_name = @MachineName
WHERE unprocessed_message_id = SELECT MIN(unprocessed_message_id)
FROM unprocessed_message
WHERE state = @StateNew
I think the deadlock in the original sproc occurs if two instances acquire shared locks on the select before either can attempt to acquire the exclusive locks on the update.
Im not sure if noeld's version is dealock proof, so to be on the safe side I would add a UPDLOCK hint on the SELECT subquery.
Im pretty sure if ZPHillM had used UPDLOCK instead of HOLDLOCK in the original sproc, then there would be no deadlock issue.
-
Kenneth,
Thanks for your explanation.
The point with this 'backward' notation, is that you can update (the other columns) and assign values from columns not updated (ie the PK) in one go. ...
-ron
-
Renato,
I lean towards your idea of putting the UPDLOCK hint on the Select sub query just to play it safe.
This seems like it would be difficult to test and although the documentation suggests it will work without a deadlock I have yet to see any explicit documentation on this type of update (i.e. Update with a SELECT sub query).
-ron
-
Renato said:
I think the deadlock in the original sproc occurs if two instances acquire shared locks on the select before either can attempt to acquire the exclusive locks on the update.
Im not sure if noeld's version is dealock proof, so to be on the safe side I would add a UPDLOCK hint on the SELECT subquery.
Im pretty sure if ZPHillM had used UPDLOCK instead of HOLDLOCK in the original sproc, then there would be no deadlock issue.
I have tested the situation shown in the original proc and Renato is correct. Using UPDLOCK (instead of HOLDLOCK) avoids the deadlock by acquiring an exclusive lock during the initial SELECT.
--ron
Viewing 13 posts - 1 through 12 (of 12 total)
You must be logged in to reply to this topic. Login to reply