SQL Multiple queries at the same time creating deadlock (UPDLOCK)

Question

So I have a problem with my database giving me a deadlock.

System.Data.SqlClient.SqlException : Transaction (Process ID 57) was deadlocked on lock resources with another process and has been chosen as the deadlock victim. Rerun the transaction.

I have a project that starts to run and then splits into x amounts of instances that run parallel (usually between 4 and 8). Each of these instances run the setup queries which a small part of looks like this:

    public void AddNewUserAndAssignPermission(
        string userid, string username, string normalizedUsername, string password, string security, string concurrency, string rolename)
    {
        InsertRecordIntoUserTable(userid,username,normalizedUsername,password,security,concurrency);
        InsertRecordIntoPersonTable(username, userid);
        //If the new user's role ID has not been specified, then skip over the assignment of role permission queries
        if(rolename!= "")
        {
            InsertRecordIntoUserRolesTable(userid, rolename);
        }
    }

    public void InsertRecordIntoUserTable(
        string userid, string username, string normalizedUsername, string password, string security, string concurrency)
    {
        var query = @"IF NOT EXISTS(
                        SELECT * FROM AspNetUsers WHERE UserName = @username)
                        BEGIN
                            INSERT INTO AspNetUsers(Id,UserName,NormalizedUserName,Email,NormalizedEmail,EmailConfirmed,PasswordHash,SecurityStamp,ConcurrencyStamp,PhoneNumber,PhoneNumberConfirmed,TwoFactorEnabled,LockoutEnd,LockoutEnabled,AccessFailedCount)
                            VALUES (@userid, @username, @normalizedUsername, NULL, NULL, 0, @password, @security, @concurrency, NULL, 0, 0, NULL, 0, 0)
                        END";
        var param_id = NewParam("@userid", userid);
        var param_username = NewParam("@username", username);
        var param_normalizedUsername = NewParam("@normalizedUsername", normalizedUsername);
        var param_password = NewParam("@password", password);
        var param_security = NewParam("@security", security);
        var param_concurrency = NewParam("@concurrency", concurrency);
        SqlDataLib sql = new SqlDataLib();
        sql.ExecuteNonQuery(Settings.AppConnectionString, query, param_id, param_username, param_normalizedUsername, param_password, param_security, param_concurrency);
    }

    public void InsertRecordIntoPersonTable( string lastname, string userid)
    {
        var query = @"IF NOT EXISTS(SELECT * FROM crm.Person WHERE IdentityUserId = @userid)
                        BEGIN
                            INSERT INTO crm.Person (FirstName, LastName, IdentityUserId, Inactive) VALUES ('Testing', @lastname, @userid, 0)
                        END";
        var param_userid = NewParam("@userid", userid);
        var param_lastname = NewParam("@lastname", lastname);
        SqlDataLib sql = new SqlDataLib();
        sql.ExecuteNonQuery(Settings.AppConnectionString, query, param_userid, param_lastname);
    }

    public void InsertRecordIntoUserRolesTable(string userid, string rolename)
    {
        var query = @"DECLARE @roleId nvarchar(450); SET @roleId = (SELECT Id from AspNetRoles WHERE Name = @rolename)
                        IF NOT EXISTS(SELECT * FROM AspNetUserRoles WHERE UserId = @userid AND RoleId = @roleid)
                        BEGIN
                            INSERT INTO AspNetUserRoles (UserId, RoleId) VALUES (@userid, @roleid)
                        END";
        var param_userid = NewParam("@userid", userid);
        var param_rolename = NewParam("@rolename", rolename);
        SqlDataLib sql = new SqlDataLib();
        sql.ExecuteNonQuery(Settings.AppConnectionString, query, param_userid, param_rolename);
    }

This code however fails giving me errors about trying to insert duplicate keys - I have investigated this and believe it to be that since these do not lock the database in anyway while executing the code that one actually adds in the record while the other already has checked it doesn't exist and then also tries to add it. So I rewrote these queries using UPDLOCK to sort out that problem. The new queries look like this:

INSERT INTO AspNetUsers(Id,UserName,NormalizedUserName,Email,NormalizedEmail,EmailConfirmed,PasswordHash,SecurityStamp,ConcurrencyStamp,PhoneNumber,PhoneNumberConfirmed,TwoFactorEnabled,LockoutEnd,LockoutEnabled,AccessFailedCount)
                      Select TOP 1 @userid, @username, @normalizedUsername, NULL, NULL, 0, @password, @security, @concurrency, NULL, 0, 0, NULL, 0, 0 FROM AspNetUsers
                      WHERE not exists ( SELECT * FROM AspNetUsers WITH (UPDLOCK)
                                         WHERE UserName = @username )
 
 INSERT INTO crm.Person (FirstName, LastName, IdentityUserId, Inactive)
                      SELECT TOP 1 'Testing', @lastname, @userid, 0 FROM crm.Person
                      WHERE not exists ( SELECT * FROM crm.Person WITH (UPDLOCK)
                                         WHERE IdentityUserId = @userid )

 DECLARE @roleId nvarchar(450); SET @roleId = (SELECT Id from dbo.AspNetRoles WHERE Name = @rolename)
                      INSERT INTO dbo.AspNetUserRoles (UserId, RoleId)
                      SELECT TOP 1 @userid, @roleid FROM dbo.AspNetUserRoles
                      WHERE not exists ( SELECT * FROM dbo.AspNetUserRoles WITH (UPDLOCK)
                                         WHERE UserId = @userid AND RoleId = @roleid )

This however made them still sometimes still fail on having tried to insert duplicate keys while mostly failing on this:

System.Data.SqlClient.SqlException : Transaction (Process ID 57) was deadlocked on lock resources with another process and has been chosen as the deadlock victim

So I rewrote them once more (this I believe isn't how they are supposed to look but this was me experimenting around). Notice the UPDLOCK on the main statement:

INSERT INTO AspNetUsers(Id,UserName,NormalizedUserName,Email,NormalizedEmail,EmailConfirmed,PasswordHash,SecurityStamp,ConcurrencyStamp,PhoneNumber,PhoneNumberConfirmed,TwoFactorEnabled,LockoutEnd,LockoutEnabled,AccessFailedCount)
                      Select TOP 1 @userid, @username, @normalizedUsername, NULL, NULL, 0, @password, @security, @concurrency, NULL, 0, 0, NULL, 0, 0 FROM AspNetUsers WITH (UPDLOCK)
                      WHERE not exists ( SELECT * FROM AspNetUsers 
                                         WHERE UserName = @username )

 INSERT INTO crm.Person (FirstName, LastName, IdentityUserId, Inactive)
                      SELECT TOP 1 'Testing', @lastname, @userid, 0 FROM crm.Person WITH (UPDLOCK)
                      WHERE not exists ( SELECT * FROM crm.Person
                                         WHERE IdentityUserId = @userid )

 DECLARE @roleId nvarchar(450); SET @roleId = (SELECT Id from dbo.AspNetRoles WHERE Name = @rolename)
                      INSERT INTO dbo.AspNetUserRoles (UserId, RoleId)
                      SELECT TOP 1 @userid, @roleid FROM dbo.AspNetUserRoles WITH (UPDLOCK)
                      WHERE not exists ( SELECT * FROM dbo.AspNetUserRoles
                                         WHERE UserId = @userid AND RoleId = @roleid )

This worked just fine but was locking the database for too long making usually one of the multiple instances fail on a SQL timeout saying the database didn't respond while the others passed. (no more duplicate key issues).

I would also like to add if I run these statements singly or only a single instance of the C# code I can run them as often as I want and they will always create the row if it doesn't exist otherwise do nothing as expected. They only start erroring when they are run basically at the same time by my C# code.

My guess is that I am using the UPDLOCK slightly wrong but I have tried everything in my knowledge. Any help would be appreciated.

Answer 1

Your code is an absolute nonsense ! You use what we call a spaghetti code...

If you want to have uniqueness, you have to add a UNIQUE constraint into your tables, and you don't have to worry about the potential existence of a duplicate, because there will never be one...

So modify the DB structure with :

ALTER TABLE AspNetUsers ADD CONSTRAINT UK_AspNetUsers_Username UNIQUE (UserName);
ALTER TABLE crm.Person ADD CONSTRAINT UK_Person_IdentityUserId UNIQUE (IdentityUserId);
ALTER TABLE dbo.AspNetUserRoles ADD CONSTRAINT UK_AspNetUserRoles_UserId_RoleId UNIQUE (UserId, RoleId);

And now simplify your queries to :

INSERT INTO AspNetUsers(Id,UserName,NormalizedUserName,Email,NormalizedEmail,EmailConfirmed,PasswordHash,SecurityStamp,ConcurrencyStamp,PhoneNumber,PhoneNumberConfirmed,TwoFactorEnabled,LockoutEnd,LockoutEnabled,AccessFailedCount)
VALUES (@userid, @username, @normalizedUsername, NULL, NULL, 0, @password, @security, @concurrency, NULL, 0, 0, NULL, 0, 0)
 
INSERT INTO crm.Person (FirstName, LastName, IdentityUserId, Inactive)
VALUES ('Testing', @lastname, @userid, 0 )

INSERT INTO dbo.AspNetUserRoles (UserId, RoleId)
SELECT TOP 1 @userid, Id from dbo.AspNetRoles WHERE Name = @rolename

You will better use an transacted procedure to do so:

CREATE PROCEDURE P_SETUP @userid INT, @username VARCHAR(32), @normalizedUsername VARCHAR(32), ...
AS
SET NOCOUNT ON;
BEGIN TRANSACTION;
BEGIN TRY
   INSERT INTO AspNetUsers(Id,UserName,NormalizedUserName,Email,NormalizedEmail,EmailConfirmed,PasswordHash,SecurityStamp,ConcurrencyStamp,PhoneNumber,PhoneNumberConfirmed,TwoFactorEnabled,LockoutEnd,LockoutEnabled,AccessFailedCount)
   VALUES (@userid, @username, @normalizedUsername, NULL, NULL, 0, @password, @security, @concurrency, NULL, 0, 0, NULL, 0, 0);
 
   INSERT INTO crm.Person (FirstName, LastName, IdentityUserId, Inactive)
   VALUES ('Testing', @lastname, @userid, 0 );

   INSERT INTO dbo.AspNetUserRoles (UserId, RoleId)
   SELECT TOP 1 @userid, Id from dbo.AspNetRoles WHERE Name = @rolename;

   COMMIT;
END TRY
BEGIN CATCH
   IF XACT_STATE() <> 0
      ROLLBACK;
   THROW;
END CATCH
GO

Answer 2

Your code doesn't really make sense as it stands, and I wonder if that may be exacerbating the locking issue.

Each of your statements goes

select top 1 @param1, @param2 from someTable where not exists ...

which is nonsensical. We don't care in the select part as to what exists already, only in the not exists part. So it should just be

select @param1, @param2 where not exists ...

Also, as mentioned by others, you need to make it SERIALIZABLE (alternative syntax is HOLDLOCK). @DavidBrowne is somewhat unclear about this. The point is that it both takes out a range lock to prevent inserts, and that it holds that lock until the end of the transaction. It could use a table lock to satisfy that, it just needs to hold it until the end.

So what we need is as follows:

INSERT INTO AspNetUsers
    (Id,UserName,NormalizedUserName,Email,NormalizedEmail,EmailConfirmed,PasswordHash,SecurityStamp,ConcurrencyStamp,PhoneNumber,PhoneNumberConfirmed,TwoFactorEnabled,LockoutEnd,LockoutEnabled,AccessFailedCount)
Select @userid, @username, @normalizedUsername, NULL, NULL, 0, @password, @security, @concurrency, NULL, 0, 0, NULL, 0, 0
WHERE not exists ( SELECT 1
    FROM AspNetUsers WITH (UPDLOCK, HOLDLOCK)
    WHERE UserName = @username );

INSERT INTO crm.Person
    (FirstName, LastName, IdentityUserId, Inactive)
SELECT 'Testing', @lastname, @userid, 0
WHERE not exists ( SELECT 1
    FROM crm.Person WITH (UPDLOCK, HOLDLOCK)
    WHERE IdentityUserId = @userid );

DECLARE @roleId nvarchar(450) = (SELECT Id from dbo.AspNetRoles WHERE Name = @rolename);

INSERT INTO dbo.AspNetUserRoles
    (UserId, RoleId)
SELECT @userid, @roleId
WHERE not exists ( SELECT 1
    FROM dbo.AspNetUserRoles WITH (UPDLOCK, HOLDLOCK)
    WHERE UserId = @userid AND RoleId = @roleid );

---

If you are still getting deadlocking after doing this, I suggest you share your deadlock graph, along with the full table and index definitions.

There are many other issues that can cause deadlocks, and a definitive answer can only be given after seeing the graph

Answer 3

You should just not have all the parallel threads running the same setup inserts - do it in your serialised part only.

Otherwise, instead of attempting the locking, you could just catch the duplicate key error and move on - your current logic says that nothing needs to happen if there is already a row that has the same name (assuming that is the unique key). This demo is from Only inserting a row if it's not already there

BEGIN TRY
   INSERT etc
END TRY
BEGIN CATCH
    IF ERROR_NUMBER() <> 2627
      RAISERROR etc
END CATCH

Answer 4

The problem with this pattern:

WHERE not exists ( SELECT * FROM AspNetUsers WITH (UPDLOCK)
                 WHERE UserName = @username )

If there's no rows found in the subquery, nothing gets locked. Under the default concurrency models locks are always Row/Key/Page/Table locks. It doesn't lock empty key ranges. SERIALIZABLE does, however. So change the lock hint to:

 WHERE not exists ( SELECT * FROM AspNetUsers WITH (UPDLOCK, SERIALIZABLE)
                     WHERE UserName = @username )