September 30, 2006 at 2:42 pm
Hi!
We have a proc that is called from a .NET client within a transaction.
The client is responsible for beginning and ending the transaction.
Occasionally we get deadlocks when the procedure is called.
A deadlock trace is included in the post, as well as the outline of the proc.
I have renamed the columns and object names.
At first we did not have the hint WITH(ROWLOCK,UPDLOCK,HOLDLOCK), and were receiving deadlocks,
placing the hint in the proc does not seem to have solved the problem.
As far as I can tell it has not made any difference.
Maybe it will be more obvious to someone here, how a deadlock can occur ?
Many thanks in advance,
rockmoose
CREATE PROCEDURE dbo.myProc
(
@keyCOL1 INT,
@keyCOL2 INT,
@keyCOL3 INT,
@valueCOL1 DECIMAL(9,2),
@ValueCOL2 INT
)
AS
SET NOCOUNT ON
DECLARE @TimeStamp DATETIME
SET @TimeStamp = GETUTCDATE()
IF NOT EXISTS(SELECT * FROM myTable WITH(ROWLOCK,UPDLOCK,HOLDLOCK)
WHEREkeyCOL1 = @keyCOL1
ANDkeyCOL2 = @keyCOL2
ANDkeyCOL3 = @keyCOL3)
BEGIN
INSERT INTO myTable (
keyCOL1,
keyCOL2,
valueCOL1,
valueCOL1_LastUpdated,
ValueCOL2,
ValueCOL2_LastUpdated,
keyCOL3)
VALUES (
@keyCOL1,
@keyCOL2,
@valueCOL1,
@TimeStamp,
@ValueCOL2,
@TimeStamp,
@keyCOL3)
END
ELSE
BEGIN
DECLARE @OldValueCOL1 DECIMAL(9,2)
DECLARE @OldValueCOL2 INT
SELECT @OldValueCOL1 = valueCOL1,
@OldValueCOL2 = ValueCOL2,
FROM myTable -- WITH(ROWLOCK,UPDLOCK,HOLDLOCK) <- tried this, but seemed to make deadlocking issues worse!!
WHEREkeyCOL1 = @keyCOL1
ANDkeyCOL2 = @keyCOL2
ANDkeyCOL3 = @keyCOL3
IF ((@OldValueCOL1 <> @valueCOL1) OR
(@OldValueCOL2 <> @ValueCOL2))
BEGIN
UPDATEmyTable
SETvalueCOL1 = @valueCOL1,
ValueCOL2 = @ValueCOL2
WHEREkeyCOL1 = @keyCOL1
ANDkeyCOL2 = @keyCOL2
ANDkeyCOL3 = @keyCOL3
IF (@OldValueCOL1 <> @valueCOL1)
BEGIN
UPDATEmyTable
SETvalueCOL1_LastUpdated = @TimeStamp
WHEREkeyCOL1 = @keyCOL1
ANDkeyCOL2 = @keyCOL2
ANDkeyCOL3 = @keyCOL3
END
IF (@OldValueCOL2 <> @ValueCOL2)
BEGIN
UPDATEmyTable
SETValueCOL2_LastUpdated = @TimeStamp
WHEREkeyCOL1 = @keyCOL1
ANDkeyCOL2 = @keyCOL2
ANDkeyCOL3 = @keyCOL3
END
END
END
2006-09-30 20:54:48.73 spid4
2006-09-30 20:54:48.73 spid4
2006-09-30 20:54:48.73 spid4 Wait-for graph
2006-09-30 20:54:48.73 spid4
2006-09-30 20:54:48.73 spid4 Node:1
2006-09-30 20:54:48.73 spid4 KEY: 9:978102525:1 (ba0018b14283) CleanCnt:2 Mode: U Flags: 0x0
2006-09-30 20:54:48.73 spid4 Grant List 2::
2006-09-30 20:54:48.73 spid4 Owner:0x8d99df40 Mode: U Flg:0x0 Ref:1 Life:02000000 SPID:177 ECID:0
2006-09-30 20:54:48.73 spid4 SPID: 177 ECID: 0 Statement Type: CONDITIONAL Line #: 26
2006-09-30 20:54:48.73 spid4 Input Buf: RPC Event: myProc;1
2006-09-30 20:54:48.73 spid4 Requested By:
2006-09-30 20:54:48.73 spid4 ResType:LockOwner Stype:'OR' Mode: U SPID:115 ECID:0 Ec: (0x94A9B5A8) Value:0xacc66540 Cost: (0/0)
2006-09-30 20:54:48.73 spid4
2006-09-30 20:54:48.73 spid4 Node:2
2006-09-30 20:54:48.73 spid4 KEY: 9:978102525:1 (da001b4664b8) CleanCnt:2 Mode: U Flags: 0x0
2006-09-30 20:54:48.73 spid4 Grant List 3::
2006-09-30 20:54:48.73 spid4 Owner:0x971d1aa0 Mode: U Flg:0x0 Ref:1 Life:02000000 SPID:115 ECID:0
2006-09-30 20:54:48.73 spid4 SPID: 115 ECID: 0 Statement Type: CONDITIONAL Line #: 26
2006-09-30 20:54:48.73 spid4 Input Buf: RPC Event: myProc;1
2006-09-30 20:54:48.73 spid4 Requested By:
2006-09-30 20:54:48.73 spid4 ResType:LockOwner Stype:'OR' Mode: U SPID:177 ECID:0 Ec: (0xE05EB5A8) Value:0xc92bd2a0 Cost: (0/0)
2006-09-30 20:54:48.73 spid4 Victim Resource Owner:
2006-09-30 20:54:48.73 spid4 ResType:LockOwner Stype:'OR' Mode: U SPID:177 ECID:0 Ec: (0xE05EB5A8) Value:0xc92bd2a0 Cost: (0/0)
You must unlearn what You have learnt
September 30, 2006 at 2:47 pm
I'd love to format the post better, how is that done ? 😉
You must unlearn what You have learnt
September 30, 2006 at 9:44 pm
I'm thinking that formatting the message is much less important that formatting the code for readability... It only took about ten minutes to fix the format of your code and to write some meaningful comments. It wouldn't take you anytime at all if you wrote code in a standard format (including CASEing of different types of words) and added comments as you went to begin with. Like this...
CREATE PROCEDURE dbo.myProc --===== Declare I/O parameters ( @pKeyCOL1 INT, @pKeyCOL2 INT, @pKeyCOL3 INT, @pValueCol1 DECIMAL(9,2), @pValueCol2 INT ) AS
--===== Declare local variables and presets SET NOCOUNT ON DECLARE @TimeStamp DATETIME SET @TimeStamp = GETUTCDATE()
--======================================================================================== --===== If the row does not exist, insert a new row --======================================================================================== IF NOT EXISTS(SELECT * FROM myTable WITH(ROWLOCK,UPDLOCK,HOLDLOCK) WHERE KeyCOL1 = @pKeyCOL1 AND KeyCOL2 = @pKeyCOL2 AND KeyCOL3 = @pKeyCOL3) BEGIN --Start Insert INSERT INTO myTable ( KeyCOL1,KeyCOL2,KeyCOL3 ValueCol1,ValueCol1_LastUpdated, ValueCol2,ValueCol2_LastUpdated, ) VALUES ( @pKeyCOL1,@pKeyCOL2,@pKeyCOL3, @pValueCol1,@TimeStamp, @pValueCol2,@TimeStamp ) END --End Insert ELSE
--======================================================================================== --===== The row already exists, do an update --======================================================================================== BEGIN --Start update --===== Declare still more variables here DECLARE @OldValueCol1 DECIMAL(9,2) DECLARE @OldValueCol2 INT
--===== Get the old values SELECT @OldValueCol1 = ValueCol1, @OldValueCol2 = ValueCol2, FROM myTable WHERE KeyCOL1 = @pKeyCOL1 AND KeyCOL2 = @pKeyCOL2 AND KeyCOL3 = @pKeyCOL3
--===== If the both old values don't match the new values, update both values IF (@OldValueCol1 <> @pValueCol1 OR @OldValueCol2 <> @pValueCol2) BEGIN --Start 2 column update UPDATE myTable SET ValueCol1 = @pValueCol1, ValueCol2 = @pValueCol2 WHERE KeyCOL1 = @pKeyCOL1 AND KeyCOL2 = @pKeyCOL2 AND KeyCOL3 = @pKeyCOL3
--===== If only column 1 was changed, just update column 1 IF @OldValueCol1 <> @pValueCol1 BEGIN --Start column 1 timestamp update UPDATE myTable SET ValueCol1_LastUpdated = @TimeStamp WHERE KeyCOL1 = @pKeyCOL1 AND KeyCOL2 = @pKeyCOL2 AND KeyCOL3 = @pKeyCOL3 END --End column 1 timestamp update
--===== If only column 2 was changed, just update column 2 IF (@OldValueCol2 <> @pValueCol2) BEGIN --Start column 2 timestamp update UPDATE myTable SET ValueCol2_LastUpdated = @TimeStamp WHERE KeyCOL1 = @pKeyCOL1 AND KeyCOL2 = @pKeyCOL2 AND KeyCOL3 = @pKeyCOL3 END --End column 2 timestamp update END --End 2 column update --DOESN'T THIS SEEM A BIT OUT OF PLACE TO YOU!!! END --End of update
Here's a couple of suggestions...
With all that in mind, here's how I'd write this "UPSERT" right after I took the transaction out of the app...
CREATE PROCEDURE dbo.myProc --===== Declare I/O parameters ( @pKeyCOL1 INT, @pKeyCOL2 INT, @pKeyCOL3 INT, @pValueCol1 DECIMAL(9,2), @pValueCol2 INT ) AS
--===== Declare local variables and conditions SET NOCOUNT ON DECLARE @OldValueCol1 DECIMAL(9,2) DECLARE @OldValueCol2 INT DECLARE @RowExists INT DECLARE @TimeStamp DATETIME
--===== Preset required local variables SET @TimeStamp = GETUTCDATE()
--======================================================================================== --===== Simultaneously check for row existance and, if available, set the old values --======================================================================================== SELECT @OldValueCol1=ValueCol1, @OldValueCol2=ValueCol2, @RowExists =1 FROM dbo.myTable WITH (UPDLOCK) WHERE KeyCOL1 = @pKeyCOL1 AND KeyCOL2 = @pKeyCOL2 AND KeyCOL3 = @pKeyCOL3)
--======================================================================================== --===== If the row does not exist, insert a new row --======================================================================================== IF @RowExists IS NULL BEGIN --Start Insert INSERT INTO dbo.myTable ( KeyCOL1,KeyCOL2,KeyCOL3 ValueCol1,ValueCol1_LastUpdated, ValueCol2,ValueCol2_LastUpdated, ) VALUES ( @pKeyCOL1,@pKeyCOL2,@pKeyCOL3, @pValueCol1,@TimeStamp, @pValueCol2,@TimeStamp ) END --End Insert ELSE
--======================================================================================== --===== The row already exists, do an update --======================================================================================== BEGIN --Start update UPDATE dbo.myTable WITH (ROWLOCK) SET ValueCol1 = @pValueCol1, ValueCol2 = @pValueCol2, ValueCol1_LastUpdated = CASE WHEN @OldValueCol1 = @pValueCol1 THEN @TimeStamp ELSE ValueCol1_LastUpdated END, ValueCol2_LastUpdated = CASE WHEN @OldValueCol2 = @pValueCol2 THEN @TimeStamp ELSE ValueCol2_LastUpdated END WHERE KeyCOL1 = @pKeyCOL1 AND KeyCOL2 = @pKeyCOL2 AND KeyCOL3 = @pKeyCOL3 END --End of update
--Jeff Moden
Change is inevitable... Change for the better is not.
October 1, 2006 at 2:05 am
Wow thanks Jeff!
That quite some reply :-), and I am very happy!
Great points, I will rewrite the proc for better upsert - processing logic with some of the modifications you suggested.
The original question to as how deadlocks are possible still remains unsolved in my mind though.
What nags me is that with the sample code posted I don't really see how a deadlock would be possible.
Even if the proc is inefficiently written etc, it does take a UPDLOCK on the row in the first select.
I don't see under what conditions a deadlock could occur ?
PS.
How do you format code in this forum?
You must unlearn what You have learnt
October 1, 2006 at 9:43 am
Even though their occurance is really unacceptable, deadlocks are about as normal (I didn't say "as frequent") as regular locking anytime you have both a SELECT and an UPDATE in the same transaction. This seems to defy what Books Online says about how a deadlock works (see [Deadlocks][Avoiding] in Books Online) because procs like this seem to deadlock the same proc in a different session. Even the very short/fast upsert code I wrote for you has the potential for a deadlock if it's embedded in an explicit transaction.
Your code was a little different and a little "worse"... in the worst case, you did a SELECT followed by another SELECT followed by an UPDATE followed by two conditional UPDATES. That's not only way too long a transaction, there is no guarantee the two instances of the same proc will see things in the same order. If proc "B" runs after proc "A" starts, it may "intrude" on a SELECT/SELECT, a SELECT/UPDATE, or an UPDATE/UPDATE the way you had it written. That's why I took the time to reduce the worst case to a single SELECT followed by a single UPDATE. It will reduce the number of deadlocks but, despite what Books Online says, may not totally eliminate deadlocks if the whole proc is embedded in an explicit transaction.
If you are going to use mixed transactions (SELECT/UPDATE or UPDATE/UPDATE), you have to have code that tests for and handles error 1205 (see [Deadlocks][Detecting and Ending] in Books Online). You can either have the proc resubmit the transaction or send a flag back to the app telling it to resubmit the transaction. If you don't have this type of code in place, then you really don't care about a transaction and should probably remove the explicit declaration of a transaction.
You said that your app is creating the transaction... bad idea... again, see [Deadlocks][Avoiding] in Books Online paying particular attention to the section that says "Avoid User Interaction in Transactions".
Last but not least, I think the UPDLOCK and ROWLOCK hints I added to my version will help with the number of deadlocks a lot... but you still have to remove the responsibility for the explicit transaction from the app and, if you do put an explicit transaction in the proc, you must do error checking especially for deadlock error #1205.
--Jeff Moden
Change is inevitable... Change for the better is not.
October 1, 2006 at 1:52 pm
Thanks for all the input Jeff!
and yes, the original proc can be coded more efficiently 😉
"if you do put an explicit transaction in the proc, you must do error checking especially for deadlock error #1205."
I need to check how they handle this, but do expect the worse!
I'd like to take this to a more conceptual level.
Here this is the situation today:
A client (.NET) calls the proc in bursts of several hundred times a second.
Multiple executions (12) are done in parallell (same starttime according to profiler).
We provide the sproc as an interface to the db, and how clients use it, is more or less out of our control.
This particular client runs the sproc in a client-controlled transaction.
But that may change.
So now, what I REALLY want to do, if possible, is to be able to write the proc in such a way that deadlocks
do not occur.
Any input / pseudocode etc on how to best do this is very welcome.
Here is the outline of the proc now, But we are still getting deadlocks!
And I don't really understand exactly HOW that is possible!
=============================================================================
create proc myProc
@pKey int
,@pVal int
as
set nocount on
declare @OldVal int
declare @exists bit
set @exists = 0
declare @InUsertrans bit
if @@trancount > 0
begin
set @InUserTrans = 1
end
else
begin
set @InUserTrans = 0
begin transaction
end
-- holdlock (or set isolation level serializable) because PK violations are possible
select@OldVal = Val
,@exists = 1
frommyTable with(rowlock,updlock,holdlock)
where[Key] = @pKey
if @exists = 0
begin
insertmyTable
(
[Key]
,Val
)
values
(
@pKey
,@pVal
)
if @@error 0
begin
if @InUserTrans = 0 and @@trancount > 0
rollback transaction
return 1
end
end
else
begin
if( @OldVal @pVal )
begin
updatemyTable with(rowlock)
setVal = @pVal
where[Key] = @pKey
if @@error 0
begin
if @InUserTrans = 0 and @@trancount > 0
rollback transaction
return 1
end
end
end
if @InUserTrans = 0 and @@trancount > 0
commit transaction
=============================================================================
PS. how do you indent on this forum? where is the formatting FAQ?
You must unlearn what You have learnt
October 1, 2006 at 8:36 pm
The way you indent on this forum is to use leading spaces or use the "Increase Indent" tool at the top of the message box when you are creating your message. The real problem is, you shouldn't have to indent your code... it should already be nicely formatted and it's not. And instead of us having to scan the code to see which section does what, the occasional meaningful comment would go a long way for both you and me.
First, I'm thinking that we're not looking at all the code that comes into play when this proc is called... does the target table, per chance, have any triggers on it? If so, that could definitely be the problem.
Even if there are no triggers, like I said, anytime there is a mixed transaction, it's possible to get a deadlock. So, let's get rid of the mix altogether especially since you're not really using @OldValue for anything worthwhile (a bit harder to read when I format it like you and without comments don'cha' think ?)...
create procedure myproc
@pkey int,
@pval int
as
set nocount on
declare @mycount int
declare @myerror int
declare @mytrans int
if @@trancount = 0
begin
set @mytrans = 1
begin transaction
end
else
begin
set @mytrans = 0
end
update mytable with (rowlock)
set val = @pval
where = @pkey
select @mycount = @@rowcount,
@myerror = @@error
if @myerror <> 0
begin
rollback
return 1
end
if @mycount = 0
begin
insert into mytable (val)
values (val)
end
select @mycount = @@rowcount,
@myerror = @@error
if @myerror <> 0
or @mycount <> 1
begin
rollback
return 1
end
if @@trancount = 1
and @mytrans = 1
commit
return 0
Do everyone a favor and start formatting your code like this ESPECIALLY if you have having problems with the code... the following is the same code as the new solution I posted above...
CREATE PROCEDURE myProc
@pKey INT,
@pVal INT
AS
SET NOCOUNT ON
DECLARE @myCount INT
DECLARE @myError INT
DECLARE @myTrans INT
IF @@TRANCOUNT = 0
BEGIN
SET @myTrans = 1
BEGIN TRANSACTION
END
ELSE
BEGIN
SET @myTrans = 0
END
UPDATE myTable WITH (ROWLOCK)
SET Val = @pVal
WHERE [Key] = @pKey
SELECT @myCount = @@ROWCOUNT,
@myError = @@ERROR
IF @myError <> 0
BEGIN
ROLLBACK
RETURN 1
END
IF @myCount = 0
BEGIN
INSERT INTO myTable (Val)
VALUES (Val)
END
SELECT @myCount = @@ROWCOUNT,
@myError = @@ERROR
IF @myError <> 0
OR @myCount <> 1
BEGIN
ROLLBACK
RETURN 1
END
IF @@TRANCOUNT = 1
AND @myTrans = 1
COMMIT
RETURN 0
--Jeff Moden
Change is inevitable... Change for the better is not.
October 2, 2006 at 1:23 am
You are a real formatting fascist 🙂
It gave me an idea for an editor that has 3 fields,
where the first one is CAPITALIZED by default and right-aligned,
the middle one is a separator field (tabs or spaces),
the third one is the code field.
Like this:
KEYWORD COLUMN | SEP | CODE COLUMN
-------------->| |<-----------
SELECT| |col1
FROM| |theTable
| |
Wouldn't that be nice?!
Ok, after all you great input, I think I will just rewrite the logic,
and try to get rid of the select clause.
Yes, there is some more logic involved, that's why there were several selects + updates in the first place.
I suspect that the proc has been rewritten a couple of times as new fields have been added.
Anyway, some of that logic might not be necessary at all.
I have one question left though,
In your example you use this hint in the UPDATE;
UPDATE myTable WITH (ROWLOCK)
SET Val = @pVal
WHERE [Key] = @pKey
Which means that (according to my testing) 2 simultaneous requests with the same non-existant key,
could get past that section and try to insert the same key (PK violation)
But if I do;
UPDATE myTable WITH (ROWLOCK,HOLDLOCK)
SET Val = @pVal
WHERE [Key] = @pKey
No PK violations occur.
You must unlearn what You have learnt
October 2, 2006 at 1:27 am
Like this:
KEYWORD COLUMN| SEP |CODE COLUMN
| |
SELECT| |col1
FROM| |theTable
| |
Sorry about all the formatting Jeff
I just opened the topic in INTERNET EXPLORER, which does give me
quite a few more options than FIREFOX.
(Now I can see the editor!)
Regards!
You must unlearn what You have learnt
October 2, 2006 at 2:05 am
Why you need all those IF's?
INSERT INTO myTable (
keyCOL1, keyCOL2, valueCOL1, valueCOL1_LastUpdated,
ValueCOL2, ValueCOL2_LastUpdated, keyCOL3)
SELECT @keyCOL1, @keyCOL2,
@valueCOL1, @TimeStamp,
@ValueCOL2,@TimeStamp,@keyCOL3
WHERE NOT EXISTS( SELECT * FROM myTable WITH(ROWLOCK,UPDLOCK,HOLDLOCK)
WHERE keyCOL1 = @keyCOL1 AND keyCOL2 = @keyCOL2 AND keyCOL3 = @keyCOL3)
UPDATE myTable
SET valueCOL1 = @valueCOL1,
ValueCOL2 = @ValueCOL2
WHERE keyCOL1 = @keyCOL1
AND keyCOL2 = @keyCOL2
AND keyCOL3 = @keyCOL3
AND (ValueCOL1 <> @valueCOL1 OR ValueCOL2 <> @ValueCOL2)
Jeff, I'm really surprised you did not suggest this.
Why?
_____________
Code for TallyGenerator
October 2, 2006 at 6:34 am
I'm trying to stay away from SELECT at all, Serqiy...
--Jeff Moden
Change is inevitable... Change for the better is not.
October 2, 2006 at 6:36 am
If the ROWLOCK,HOLDLOCK works to keep the PK violations out, that'll work... I was peeling 1 potato at a time How's the deadlock situation?
So far as formatting goes... the "gutter" format I use is difficult for some... takes a bit of getting used to when typing but I've gotten very fast at it. The real key is to use SOME format and the total absence of a format along with the total absence of comments simply cannot be called formatted code. And, yeah, I guess I am a bit of a formatting fascist... Thanks for the compliment
--Jeff Moden
Change is inevitable... Change for the better is not.
October 2, 2006 at 6:49 am
Don't use NOT EXISTS. It forces the whole table to be looked at because it has to check EVERY row to make sure the row doesn't exist. |
Where did you get this from Jeff.
I do not believe the statement is correct.
EXISTS 'Returns TRUE if a subquery contains any rows'
NOT 'Negates a Boolean input'
Therefore the use of NOT will not affect the subquery, only the subquery itself (database design) can determine whether a table scan, index scan or index seek is utilised
Far away is close at hand in the images of elsewhere.
Anon.
October 2, 2006 at 6:53 am
I guess that's where I was going with that, David... I'm trying to stay away from the sub-query and it came out wrong.
--Jeff Moden
Change is inevitable... Change for the better is not.
October 2, 2006 at 7:07 am
Ah, OK
Besides any solution will require a SELECT/JOIN because of the INSERT
Far away is close at hand in the images of elsewhere.
Anon.
Viewing 15 posts - 1 through 15 (of 21 total)
You must be logged in to reply to this topic. Login to reply