Please start any new threads on our new
site at https://forums.sqlteam.com. We've got lots of great SQL Server
experts to answer whatever question you can come up with.
| Author |
Topic |
|
EliW
Starting Member
5 Posts |
Posted - 2010-05-10 : 12:38:59
|
TIA for your help! I am working with a table that will lock FoobarID for a given user for five minutes. (I want to keep a lock history, so it'll be impractical to use a primary key, or to delete old records, and so on.)I need to prevent multiple users from creating a lock on the same FoobarID at the same time. So far, I've just resorted to making the INSERT statement "atomic", which I think will help, but I probably need to do more. Here is my code so far; it's just a mock-up, that will become a stored proc with parameters when it grows up :DECLARE @LockID INTDECLARE @FoobarID INTDECLARE @UserID INTSET @FoobarID = 1SET @UserID = 1INSERT tblLock (LockTypeID ,LockObjectID ,StartTime ,EndTime ,UserID)SELECT LockTypeID = 1 ,LockObjectID = @FoobarID ,StartTime = GETDATE() ,EndTime = DATEADD(MINUTE, 5, GETDATE()) ,UserID = @UserIDWHERE NOT EXISTS (SELECT 1 FROM tblLock WHERE LockTypeID = 1 AND LockObjectID = @FoobarID AND GETDATE() BETWEEN StartTime AND EndTime)IF @@ROWCOUNT = 1 SET @LockID = SCOPE_IDENTITY()ELSE SET @LockID = -1SELECT @LockIDSELECT * FROM tblLock There will also be a separate stored proc for extending the lock for five additional minutes, thus:UPDATE tblLockSET EndTime = DATEADD(MINUTE, 5, GETDATE())WHERE LockID = @LockID Here is my code again, with several suggestions added (marked A, B, C, D and E). Which of these should I use, and which ones would be superfluous and/or will just degrade performance?Also, should any of these items (A,B,C,D,E) be added to the second stored proc (the one that extends the lock for five minutes)?SET TRANSACTION ISOLATION LEVEL REPEATABLE READ --(A) Is this necessary?SET TRANSACTION ISOLATION LEVEL SERIALIZABLE --(B) This is probably too much.DECLARE @LockID INTDECLARE @FoobarID INTDECLARE @UserID INTSET @FoobarID = 1SET @UserID = 1BEGIN TRANSACTION --(C) Isn't the INSERT statement atomic, making this unnecessary?INSERT tblLock (LockTypeID ,LockObjectID ,StartTime ,EndTime ,UserID)SELECT LockTypeID = 1 ,LockObjectID = @FoobarID ,StartTime = GETDATE() ,EndTime = DATEADD(MINUTE, 5, GETDATE()) ,UserID = @UserIDWHERE NOT EXISTS (SELECT 1 FROM tblLock (UPDLOCK, ROWLOCK) --(D) and (E), is (E) necessary? WHERE LockTypeID = 1 AND LockObjectID = @FoobarID AND GETDATE() BETWEEN StartTime AND EndTime)COMMIT TRANSACTION --(C) againIF @@ROWCOUNT = 1 SET @LockID = SCOPE_IDENTITY()ELSE SET @LockID = -1SELECT @LockIDSELECT * FROM tblLock |
|
|
Kristen
Test
22859 Posts |
Posted - 2010-05-10 : 12:59:29
|
The INSERT statement is atomic, and thus I don't think you need the transaction isolation settings, not the UPDLOCK / ROWLOCK hints, nor the transaction block (although that is benign)I've done this type of ID-lock before, although not for a given time-limit. We had routines to remove locks (after a PC crash, for example), a routine that will tell a user [trying to lock record] who currently held the lock, and when we updated a record we double-checked that the lock was still in place at that time:UPDATE USET Col1 = 'XXX'FROM MyTable AS UWHERE U.ColID = @FoobarIDAND EXISTS (SELECT * FROM tblLock WHERE LockTypeID = 1 AND LockObjectID = @FoobarID AND UserID = @UserID) this makes sure that the lock has not been released (some how) whilst the application thought iut was "Owned" by the user. (You would need to include your expiry date test) |
 |
|
|
Lamprey
Master Smack Fu Yak Hacker
4614 Posts |
|
|
EliW
Starting Member
5 Posts |
Posted - 2010-05-10 : 17:00:34
|
quote: Originally posted by Lamprey The INSERT is atomic, but the EXISTS can still cause a deadlock without UPDLOCK, HOLDLOCK:http://weblogs.sqlteam.com/dang/archive/2007/10/28/Conditional-INSERTUPDATE-Race-Condition.aspx
1) So even though the EXISTS query is embedded in the INSERT query, deadlocking can still occur and this is not quite as "atomic" as I thought it was?2) I've got a co-worker insisting I should still have some kind of SET TRANSACTION ISOLATION LEVEL at the top. Wouldn't "WITH (UPDLOCK, HOLDLOCK)" be enough?3) Throughout most of our app, we start every stored proc with SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED. If I added WITH (UPDLOCK, HOLDLOCK) to the EXISTS query, wouldn't I still be able to get away with that setting?4) Double-checking just to make sure: Absolutely NONE of this should be applied to the UPDATE statement in which I extend the lock's end time? |
 |
|
|
Lamprey
Master Smack Fu Yak Hacker
4614 Posts |
Posted - 2010-05-10 : 17:49:19
|
quote: Originally posted by EliW
quote: Originally posted by Lamprey The INSERT is atomic, but the EXISTS can still cause a deadlock without UPDLOCK, HOLDLOCK:http://weblogs.sqlteam.com/dang/archive/2007/10/28/Conditional-INSERTUPDATE-Race-Condition.aspx
1) So even though the EXISTS query is embedded in the INSERT query, deadlocking can still occur and this is not quite as "atomic" as I thought it was?2) I've got a co-worker insisting I should still have some kind of SET TRANSACTION ISOLATION LEVEL at the top. Wouldn't "WITH (UPDLOCK, HOLDLOCK)" be enough?3) Throughout most of our app, we start every stored proc with SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED. If I added WITH (UPDLOCK, HOLDLOCK) to the EXISTS query, wouldn't I still be able to get away with that setting?4) Double-checking just to make sure: Absolutely NONE of this should be applied to the UPDATE statement in which I extend the lock's end time?
1. Kind of; My understanding is that without the UPDLOCK, HOLDLOCK on the NOT EXISTS clause that you could get TWO inserts because of the race condition on the NOT EXISTS select part. Thus the second insert would fail (assuming proper constraints).2. I think it's a good idea to put SET TRANSACTION ISOLATION LEVEL at the top of the stored procedure. But, two things. 1) yes, the WITH (UPDLOCK, HOLDLOCK) will over ride the SET statement at the top. and 2) If you are using READ UNCOMMITTED be careful with bad/duplicate data..!3. See #2.4. Correct, there shouldn't be a need to do any locking on the update itself. The one caveat to that is if you need to do other updates on that same row. Then you might need SERIALIZEABLE or something. |
 |
|
|
Kristen
Test
22859 Posts |
Posted - 2010-05-11 : 02:46:43
|
Interesting link, thanks.I see the article suggests that MERGE might solve that - but wouldn't an OUTER JOIN be better?INSERT tblLock (LockTypeID ,LockObjectID ,StartTime ,EndTime ,UserID)SELECT LockTypeID = 1 ,LockObjectID = @FoobarID ,StartTime = GETDATE() ,EndTime = DATEADD(MINUTE, 5, GETDATE()) ,UserID = @UserIDFROM (SELECT 1 AS X) AS X LEFT OUTER JOIN tblLock AS L ON L.LockTypeID = 1 AND L.LockObjectID = @FoobarID AND GETDATE() BETWEEN L.StartTime AND L.EndTimeWHERE L.LockTypeID IS NULL |
 |
|
|
Kristen
Test
22859 Posts |
Posted - 2010-05-11 : 02:57:21
|
| Re: 1 - it is atomic (I think?) just it might cause a deadlock (so if the EXISTS test succeeds [target NOT found] then the INSERT will proceed, otherwise not). Thus no race condition?? |
 |
|
|
Lamprey
Master Smack Fu Yak Hacker
4614 Posts |
Posted - 2010-05-11 : 10:51:34
|
| There shouldn't be a deadlock. Like you describe, the EXISTS check succeeds so both threads/processes perform the INSERT, of which one will fail. The Race Condition is on the EXISTS SELECT statement itself. |
 |
|
|
|
|
|
|
|