June 12, 2015 at 3:22 am
Guys i am confused once more - multi vms running - each call the below proc multi times (all have unique record ID's, 1 user has only ever has 1 record), sometimes i get a deadlock - most of the time i dont - any ideas?
ALTER PROCEDURE [dbo].[MarkAsKO] @Username varchar(25),@Reason varchar(1024), @LastURL varchar(1024)
AS
Set NOCOUNT ON
declare @RecordID as bigint
set @RecordId = (select recordid from tbl_main with(nolock) where lockedby = @Username)
begin tran
update tbl_main set inprogress = 0,caseworked = 1,success = 0,Exception = 1,ExceptionReason = @reason,DateWorked = getdate(), LastURL = @LastURL where recordid = @RecordID
--add a comment
insert into tblaudit(RecordID,StepDetail,StepTime,StepBy) values(@RecordID,@reason,getdate(), @Username)
commit tran
SET NOCOUNT OFF
June 12, 2015 at 3:25 am
i have a feeling with(nolock) could be causing this!!
June 12, 2015 at 3:31 am
Probably same as before: unneeded reads before you write.
This is how I would do it:
CREATE PROCEDURE [dbo].[MarkAsKO]
@Username varchar(25),
@Reason varchar(1024),
@LastURL varchar(1024)
AS
BEGIN
SET NOCOUNT,
XACT_ABORT,
QUOTED_IDENTIFIER,
ANSI_NULLS,
ANSI_PADDING,
ANSI_WARNINGS,
ARITHABORT,
CONCAT_NULL_YIELDS_NULL ON;
SET NUMERIC_ROUNDABORT OFF;
DECLARE @RecordID as bigint;
BEGIN TRAN;
BEGIN TRY;
UPDATE tbl_main
SET inprogress = 0,
caseworked = 1,
success = 0,
Exception = 1,
ExceptionReason = @reason,
DateWorked = getdate(),
LastURL = @LastURL,
@RecordID = RecordID
WHERE lockedby = @Username;
--add a comment
INSERT INTO tblaudit(RecordID,StepDetail,StepTime,StepBy)
VALUES(@RecordID,@reason,getdate(), @Username);
IF XACT_STATE() = 1
COMMIT;
END TRY
BEGIN CATCH;
IF XACT_STATE() <> 0
ROLLBACK;
THROW;
END CATCH
END
BTW, if you could post the deadlock graph, that would help a lot.
-- Gianluca Sartori
June 12, 2015 at 3:31 am
No. Nolock could cause other things, like incorrect data, but it won't cause a deadlock. That said, do remove it.
Deadlock graph please, you can get it from the system health extended events session, table definitions and index definitions.
Gail Shaw
Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability
June 12, 2015 at 3:33 am
spaghettidba (6/12/2015)
This is how I would do it:
That isn't identical. If there are multiple rows in tbl_main locked by the same user, the original will update one of them (which one, no way to say). Your will update all of them.
Gail Shaw
Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability
June 12, 2015 at 3:37 am
BTW, your code assumes that only one row at a time can be locked by a particular user. The same assumption is applied in my rewrite, but you should probably enforce it with something like this:
CREATE PROCEDURE [dbo].[MarkAsKO]
@Username varchar(25),
@Reason varchar(1024),
@LastURL varchar(1024)
AS
BEGIN
SET NOCOUNT,
XACT_ABORT,
QUOTED_IDENTIFIER,
ANSI_NULLS,
ANSI_PADDING,
ANSI_WARNINGS,
ARITHABORT,
CONCAT_NULL_YIELDS_NULL ON;
SET NUMERIC_ROUNDABORT OFF;
DECLARE @RecordID as bigint;
BEGIN TRAN;
BEGIN TRY;
UPDATE tbl_main
SET inprogress = 0,
caseworked = 1,
success = 0,
Exception = 1,
ExceptionReason = @reason,
DateWorked = getdate(),
LastURL = @LastURL,
@RecordID = RecordID
WHERE lockedby = @Username;
IF @@ROWCOUNT > 1
BEGIN;
THROW 50001, 'Only one row at a time should be locked by a user', 1;
END
IF @RecordID IS NULL
BEGIN;
THROW 50002, 'No locked row found for this user', 1;
END
--add a comment
INSERT INTO tblaudit(RecordID,StepDetail,StepTime,StepBy)
VALUES(@RecordID,@reason,getdate(), @Username);
IF XACT_STATE() = 1
COMMIT;
END TRY
BEGIN CATCH;
IF XACT_STATE() <> 0
ROLLBACK;
THROW;
END CATCH
END
-- Gianluca Sartori
June 12, 2015 at 3:38 am
thanks guys, going to try spaghettidba solution - alos have started a profiler so will post a graph as soon as it occours again.
Ta
June 12, 2015 at 3:38 am
GilaMonster (6/12/2015)
spaghettidba (6/12/2015)
This is how I would do it:That isn't identical. If there are multiple rows in tbl_main locked by the same user, the original will update one of them (which one, no way to say). Your will update all of them.
Yes, I noticed that after posting. Updating a random row is not probably what he wants to do here, so I think it should be treated as an error.
-- Gianluca Sartori
June 12, 2015 at 3:41 am
spaghettidba (6/12/2015)
GilaMonster (6/12/2015)
spaghettidba (6/12/2015)
This is how I would do it:That isn't identical. If there are multiple rows in tbl_main locked by the same user, the original will update one of them (which one, no way to say). Your will update all of them.
Yes, I noticed that after posting. Updating a random row is not probably what he wants to do here, so I think it should be treated as an error.
It is worth mentioning that if only one row at a time can be locked by a user, it should be enforced by a UNIQUE contraint on the lockedby column.
-- Gianluca Sartori
June 12, 2015 at 3:43 am
Should be ok - as stated each user will only ever have 1 record locked 😀
spaghettidba (6/12/2015)
BTW, your code assumes that only one row at a time can be locked by a particular user. The same assumption is applied in my rewrite, but you should probably enforce it with something like this:
CREATE PROCEDURE [dbo].[MarkAsKO]
@Username varchar(25),
@Reason varchar(1024),
@LastURL varchar(1024)
AS
BEGIN
SET NOCOUNT,
XACT_ABORT,
QUOTED_IDENTIFIER,
ANSI_NULLS,
ANSI_PADDING,
ANSI_WARNINGS,
ARITHABORT,
CONCAT_NULL_YIELDS_NULL ON;
SET NUMERIC_ROUNDABORT OFF;
DECLARE @RecordID as bigint;
BEGIN TRAN;
BEGIN TRY;
UPDATE tbl_main
SET inprogress = 0,
caseworked = 1,
success = 0,
Exception = 1,
ExceptionReason = @reason,
DateWorked = getdate(),
LastURL = @LastURL,
@RecordID = RecordID
WHERE lockedby = @Username;
IF @@ROWCOUNT > 1
BEGIN;
THROW 50001, 'Only one row at a time should be locked by a user', 1;
END
IF @RecordID IS NULL
BEGIN;
THROW 50002, 'No locked row found for this user', 1;
END
--add a comment
INSERT INTO tblaudit(RecordID,StepDetail,StepTime,StepBy)
VALUES(@RecordID,@reason,getdate(), @Username);
IF XACT_STATE() = 1
COMMIT;
END TRY
BEGIN CATCH;
IF XACT_STATE() <> 0
ROLLBACK;
THROW;
END CATCH
END
June 12, 2015 at 3:45 am
matthew.green 36969 (6/12/2015)
thanks guys, going to try spaghettidba solution - alos have started a profiler so will post a graph as soon as it occours again.
You don't need profiler. In SQL 2012 and above deadlock graphs are captured by default by the system health extended events session.
Gail Shaw
Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability
June 12, 2015 at 3:47 am
matthew.green 36969 (6/12/2015)
Should be ok - as stated each user will only ever have 1 record locked 😀
So you do have a unique index on the LockedBy column?
Gail Shaw
Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability
June 12, 2015 at 4:03 am
sorry for delay - my profilers not picking up the graphs - will ppost soon - the deadlock issue is worse with the new code
June 12, 2015 at 4:09 am
matthew.green 36969 (6/12/2015)
sorry for delay - my profilers not picking up the graphs - will ppost soon - the deadlock issue is worse with the new code
No need to capture the deadlocks with profiler: they're already captured by the system_health Extended Events session. You just need to extract them.
The easiest way is with this script: http://troubleshootingsql.com/2012/09/06/system-health-session-and-deadlocks/
-- Gianluca Sartori
June 12, 2015 at 4:16 am
fyi deadlock graph attached
the 2 conflicting statments in this instance are:
303: MarkAsCompleted 'C0239484'
192: MarkAsKO 'C0239494'
As such seems to be conflicting with another proc - im reviewing now
Viewing 15 posts - 1 through 15 (of 28 total)
You must be logged in to reply to this topic. Login to reply