Insert Statement / Stored Proc dead locks

Question

I have an insert statement that was deadlocking using linq. So I placed it in a stored proc incase the surrounding statements were affecting it.

Now the Stored Proc is dead locked. Something about the insert statement is locking itself according to the Server Profiler. It claims that two of those insert statements were waiting for the PK index to be freed:

When I placed the code in the stored procedure it is now stating that this stored proc has deadlocked with another instance of this stored proc.

Here is the code. The select statement is similar to that used by linq when it did its own query. I simply want to see if the item exists and if not then insert it. I can find the system by either the PK or by some lookup values.

       SET NOCOUNT ON;
       BEGIN TRY
        SET TRANSACTION ISOLATION LEVEL SERIALIZABLE

        BEGIN TRANSACTION SPFindContractMachine
        DECLARE @id int;
        set @id = (select [m].pkID from Machines as [m]
                        WHERE ([m].[fkContract] = @fkContract) AND ((
                        (CASE 
                            WHEN @bByID = 1 THEN 
                                (CASE 
                                    WHEN [m].[pkID] = @nMachineID THEN 1
                                    WHEN NOT ([m].[pkID] = @nMachineID) THEN 0
                                    ELSE NULL
                                 END)
                            ELSE 
                                (CASE 
                                    WHEN ([m].[iA_Metric] = @lA) AND ([m].[iB_Metric] = @lB) AND ([m].[iC_Metric] = @lC) THEN 1
                                    WHEN NOT (([m].[iA_Metric] = @lA) AND ([m].[iB_Metric] = @lB) AND ([m].[iC_Metric] = @lC)) THEN 0
                                    ELSE NULL
                                 END)
                         END)) = 1));
        if (@id IS NULL)
        begin
            Insert into Machines(fkContract, iA_Metric, iB_Metric, iC_Metric, dteFirstAdded) 
                values (@fkContract, @lA, @lB, @lC, GETDATE());

            set @id = SCOPE_IDENTITY();
        end

        COMMIT TRANSACTION SPFindContractMachine

        return @id;

    END TRY
    BEGIN CATCH
        if @@TRANCOUNT > 0
            ROLLBACK TRANSACTION SPFindContractMachine
    END CATCH

Answer 1

Get rid of the transaction. It's not really helping you, instead it is hurting you. That should clear up your problem.

Answer 2

Any procedure that follows the pattern:

BEGIN TRAN
check if row exists with SELECT
if row doesn't exist INSERT
COMMIT

is going to run into trouble in production because there is nothing to prevent two treads doing the check simultaneously and both reach the conclusion that they should insert. In particular, under serialization isolation level (as in your case), this pattern is guaranteed to deadlock.

A much better pattern is to use database unique constraints and always INSERT, capture duplicate key violation errors. This is also significantly more performant.

Another alternative is to use the MERGE statement:

create procedure usp_getOrCreateByMachineID
    @nMachineId int output,
    @fkContract int,
    @lA int,
    @lB int,
    @lC int,
    @id int output
as
begin
    declare @idTable table (id int not null);
    merge Machines as target
        using (values (@nMachineID, @fkContract, @lA, @lB, @lC, GETDATE()))
            as source (MachineID, ContractID, lA, lB, lC, dteFirstAdded)
    on (source.MachineID = target.MachineID)
    when matched then
        update set @id = target.MachineID
    when not matched then
        insert (ContractID, iA_Metric, iB_Metric, iC_Metric, dteFirstAdded)
        values (source.contractID, source.lA, source.lB, source.lC, source.dteFirstAdded)
    output inserted.MachineID into @idTable;
    select @id = id from @idTable;
end 
go

create procedure usp_getOrCreateByMetrics
    @nMachineId int output,
    @fkContract int,
    @lA int,
    @lB int,
    @lC int,
    @id int output
as
begin
    declare @idTable table (id int not null);
    merge Machines as target
        using (values (@nMachineID, @fkContract, @lA, @lB, @lC, GETDATE()))
            as source (MachineID, ContractID, lA, lB, lC, dteFirstAdded)
    on (target.iA_Metric = source.lA
        and target.iB_Metric = source.lB
        and target.iC_Metric = source.lC)
    when matched then
        update set @id = target.MachineID
    when not matched then
        insert (ContractID, iA_Metric, iB_Metric, iC_Metric, dteFirstAdded)
        values (source.contractID, source.lA, source.lB, source.lC, source.dteFirstAdded)
    output inserted.MachineID into @idTable;
    select @id = id from @idTable;
end 
go

This example separates the two cases, since T-SQL queries should never attempt to resolve two different solutions in one single query (the result is never optimizable). Since the two tasks at hand (get by mahcine id and get by metrics) are completely separate, the should be separate procedures and the caller should call the apropiate one, rather than passing a flag. This example shouws how to achieve the (probably) desired result using MERGE, but of course, a correct and optimal solution depends on the actual schema (table definition, indexes and cosntraints in place) and on the actual requirements (is not clear what the procedure is expected to do if the criteria is already matched, not output and @id?).

By eliminating the SERIALIZABLE isolation, this is no longer guaranteed to deadlock, but it may still deadlock. Solving the deadlock is, of course, completely dependent on the schema which was not specified, so a solution to the deadlock cannotactually be provided in this context. There is a sledge hammer of locking all candidate row (force UPDLOCK or even TABLOCX) but such a solution would kill throughput on heavy use, so I cannot recommend it w/o knowing the use case.

Answer 3

I wonder if adding an UPDLOCK hint to the earlier SELECT(s) would fix this; it should avoid sone deadlock scenarios be preventing another spud getting a read-lock on the data you are about to mutate.

Answer 4

How about this SQL? It moves the check for existing data and the insert into a single statement. This way, when two threads are running they're not deadlocked waiting for each other. At best, thread two is deadlocked waiting for thread one, but as soon as thread one finishes, thread two can run.

BEGIN TRY

  BEGIN TRAN SPFindContractMachine

  INSERT INTO Machines (fkContract, iA_Metric, iB_Metric, iC_Metric, dteFirstAdded)
    SELECT @fkContract, @lA, @lB, @lC, GETDATE()
      WHERE NOT EXISTS (
        SELECT * FROM Machines
        WHERE fkContract = @fkContract
        AND ((@bByID = 1 AND pkID = @nMachineID)
             OR
             (@bByID <> 1 AND iA_Metric = @lA AND iB_Metric = @lB AND iC_Metric = @lC))

  DECLARE @id INT

  SET @id = (
    SELECT pkID FROM Machines
    WHERE fkContract = @fkContract
    AND ((@bByID = 1 AND pkID = @nMachineID)
         OR
         (@bByID <> 1 AND iA_Metric = @lA AND iB_Metric = @lB AND iC_Metric = @lC)))

  COMMIT TRAN SPFindContractMachine

  RETURN @id

END TRY
BEGIN CATCH
  IF @@TRANCOUNT > 0
    ROLLBACK TRAN SPFindContractMachine
END CATCH

I also changed those CASE statements to ORed clauses just because they were easier to read to me. If I recall my SQL theory, ORing might make this query a little slower.