5

I am seeing constant deadlocks in my app, even though it performs no select statements, no delete statements, and no update statements. It is only inserting completely new data.

TL;DR: It seems to be related to the foreign key. If I remove that then I don't get any deadlocks at all. But that is not an acceptable solution for obvious reasons.

Given the following table structure

CREATE TABLE [dbo].[IncomingFile]
(
    [Id] UNIQUEIDENTIFIER NOT NULL, 
    [ConcurrencyVersion] RowVersion NOT NULL,
    CONSTRAINT [PK_IncomingFile] PRIMARY KEY CLUSTERED([Id])
)
GO

CREATE TABLE [dbo].[IncomingFileEvent]
(
    [Id] UNIQUEIDENTIFIER NOT NULL, 
    [ConcurrencyVersion] RowVersion NOT NULL,
    [IncomingFileId] UNIQUEIDENTIFIER NOT NULL,
    CONSTRAINT [PK_IncomingFileEvent] PRIMARY KEY CLUSTERED([Id]),
    CONSTRAINT [FK_IncomingFileEvent_IncomingFileId]
        FOREIGN KEY ([IncomingFileId])
        REFERENCES [dbo].[IncomingFile] ([Id])
)
GO

When I hit a number of concurrent tasks inserting data, I always see a deadlock. READ_COMMITTED_SNAPSHOT is enabled in my DB options (even though I am not reading anyway).

Here is the code that will reproduce the problem. If you do not experience the problem, increase the NumberOfTasksPerCpu constant at the top of the program.

using System;
using System.Collections.Generic;
using System.Data.SqlClient;
using System.Diagnostics;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

namespace SqlServerDeadlockRepro
{
    class Program
    {
        private const int NumberOfTasksPerCpu = 8; // Keep increasing this by one if you do not get a deadlock!
        private const int NumberOfChildRows = 1_000;
        private const string MSSqlConnectionString = "Server=DESKTOP-G05BF1U;Database=EFCoreConcurrencyTest;Trusted_Connection=True;";

        private static int NumberOfConcurrentTasks;

        static async Task Main(string[] args)
        {
            NumberOfConcurrentTasks = Environment.ProcessorCount * NumberOfTasksPerCpu;

            var readySignals = new Queue<ManualResetEvent>();
            var trigger = new ManualResetEvent(false);
            var processingTasks = new List<Task>();
            for (int index = 0; index < NumberOfConcurrentTasks; index++)
            {
                var readySignal = new ManualResetEvent(false);
                readySignals.Enqueue(readySignal);
                var task = CreateDataWithSqlCommand(trigger, readySignal);
                processingTasks.Add(task);
            }
            Console.WriteLine("Waiting for tasks to become ready");
            while (readySignals.Count > 0)
            {
                var readySignalBatch = new List<WaitHandle>();
                for(int readySignalCount = 0; readySignals.Count > 0 && readySignalCount < 64; readySignalCount++)
                {
                    readySignalBatch.Add(readySignals.Dequeue());
                }
                WaitHandle.WaitAll(readySignalBatch.ToArray());
            }
            Console.WriteLine("Saving data");
            var sw = Stopwatch.StartNew();
            trigger.Set();
            await Task.WhenAll(processingTasks.ToArray());
            sw.Stop();
            Console.WriteLine("Finished - " + sw.ElapsedMilliseconds);
        }

        private static int TaskNumber = 0;
        private static async Task CreateDataWithSqlCommand(ManualResetEvent trigger, ManualResetEvent readySignal)
        {
            await Task.Yield();
            using var connection = new SqlConnection(MSSqlConnectionString);
            await connection.OpenAsync().ConfigureAwait(false);
            var transaction = (SqlTransaction)await connection.BeginTransactionAsync(System.Data.IsolationLevel.ReadCommitted).ConfigureAwait(false);

            Console.WriteLine("Task " + Interlocked.Increment(ref TaskNumber) + $" of {NumberOfConcurrentTasks}  ready ");
            readySignal.Set();
            trigger.WaitOne();
            Guid parentId = Guid.NewGuid();
            string fileCommandSql = "insert into IncomingFile (Id) values (@Id)";
            using var fileCommand = new SqlCommand(fileCommandSql, connection, transaction);
            fileCommand.Parameters.Add("@Id", System.Data.SqlDbType.UniqueIdentifier).Value = parentId;
            await fileCommand.ExecuteNonQueryAsync().ConfigureAwait(false);

            using var fileEventCommand = new SqlCommand
            {
                Connection = connection,
                Transaction = transaction
            };
            var commandTextBulder = new StringBuilder("INSERT INTO [IncomingFileEvent] ([Id], [IncomingFileId]) VALUES ");
            for (var i = 1; i <= NumberOfChildRows * 2; i += 2)
            {
                commandTextBulder.Append($"(@p{i}, @p{i + 1})");
                if (i < NumberOfChildRows * 2 - 1)
                    commandTextBulder.Append(',');

                fileEventCommand.Parameters.AddWithValue($"@p{i}", Guid.NewGuid());
                fileEventCommand.Parameters.AddWithValue($"@p{i + 1}", parentId);
            }

            fileEventCommand.CommandText = commandTextBulder.ToString();
            await fileEventCommand.ExecuteNonQueryAsync().ConfigureAwait(false);
            await transaction.CommitAsync().ConfigureAwait(false);
        }
    }
}

UPDATE

Also tried making the primary key NONCLUSTERED and adding a CLUSTERED index based on the current date and time.

CREATE TABLE [dbo].[IncomingFile]
(
    [Id] UNIQUEIDENTIFIER NOT NULL, 
    [ConcurrencyVersion] RowVersion NOT NULL,
    [CreatedUtc] DateTime2 DEFAULT GETDATE(),
    CONSTRAINT [PK_IncomingFile] PRIMARY KEY NONCLUSTERED([Id])
)
GO
CREATE CLUSTERED INDEX [IX_IncomingFile_CreatedUtc] on [dbo].[IncomingFile]([CreatedUtc])
GO

CREATE TABLE [dbo].[IncomingFileEvent]
(
    [Id] UNIQUEIDENTIFIER NOT NULL, 
    [ConcurrencyVersion] RowVersion NOT NULL,
    [IncomingFileId] UNIQUEIDENTIFIER NOT NULL,
    [CreatedUtc] DateTime2 DEFAULT GETDATE(),
    CONSTRAINT [PK_IncomingFileEvent] PRIMARY KEY NONCLUSTERED([Id]),
    CONSTRAINT [FK_IncomingFileEvent_IncomingFileId]
        FOREIGN KEY ([IncomingFileId])
        REFERENCES [dbo].[IncomingFile] ([Id])
)
GO
CREATE CLUSTERED INDEX [IX_IncomingFileEvent_CreatedUtc] on [dbo].[IncomingFileEvent]([CreatedUtc])
GO

UPDATE 2

I tried a sequential guid taken from here, which made no difference.

UPDATE 3

It seems to be related to the foreign key. If I remove that then I don't get any deadlocks at all.

UPDATE 4

A reply from Sql Server Product Group with some suggestions has been posted on my original github issue.

https://github.com/dotnet/efcore/issues/21899#issuecomment-683404734​​​​​​​

Peter Morris
  • 20,174
  • 9
  • 81
  • 146
  • Options, make the primary keys as ` PRIMARY KEY NONCLUSTERED` - this will make tables heaps, you may still want to add artificial clustered index(identity, ...). Second option - if guids are not needed to be random, you could generate sequential guids from your code. – Lukasz Szozda Aug 08 '20 at 10:20
  • Just add an index or redo the primary key as suggested in the accepted answer. – ErikEJ Aug 14 '20 at 15:03
  • Added the index, it made no difference. I also tried the composite PK and that also didn't fix it. Made the PK indexes NON CLUSTERED, still no difference. – Peter Morris Aug 14 '20 at 19:19

2 Answers2

4

The deadlock is due to the execution plan needed to check referential integrity. A full table scan of the IncomingFile table is performed when inserting a large number (1K) rows into the related IncomingFileEvent table. The scan acquires a shared table lock that's held for the duration of the transaction and leads to the deadlock when different sessions each hold an exclusive row lock on the just inserted IncomingFile row and are blocked by another sessions exclusive row lock.

Below is the execution plan that shows this:

plan with full scan

One way to avoid the deadlock is with an OPTION (LOOP JOIN) query hint on the IncomingFileEvent insert query:

    var commandTextBulder = new StringBuilder("INSERT INTO [IncomingFileEvent] ([Id], [IncomingFileId]) VALUES ");
    for (var i = 1; i <= NumberOfChildRows * 2; i += 2)
    {
        commandTextBulder.Append($"(@p{i}, @p{i + 1})");
        if (i < NumberOfChildRows * 2 - 1)
            commandTextBulder.Append(',');

        fileEventCommand.Parameters.AddWithValue($"@p{i}", Guid.NewGuid());
        fileEventCommand.Parameters.AddWithValue($"@p{i + 1}", parentId);
    }
    commandTextBulder.Append(" OPTION (LOOP JOIN);");

This is the plan with the hint:

plan with hint

On a side note, consider the changing the existing primary key to the one below. This is more correct from a data modeling perspective (identifying relationship) and will improve performance of both insert and selects since related rows are physically clustered together.

CONSTRAINT [PK_IncomingFileEvent] PRIMARY KEY CLUSTERED(IncomingFileId, Id)
Dan Guzman
  • 43,250
  • 3
  • 46
  • 71
  • Thank you! Is this the only way (other than deleting the FK)? I ask because everywhere I look it is recommended not to pass join hints to the optimiser. – Peter Morris Aug 08 '20 at 22:44
  • @PeterMorris, although not ideal, the [documentation](https://learn.microsoft.com/en-us/sql/t-sql/queries/hints-transact-sql-query) states "we recommend only using hints as a last resort for experienced developers and database administrators". The hint is needed to avoid deadlocks when the `IncomingFile` table has only a few rows but will not be needed when the table is larger because SQL Server will then create the same plan as hinted version. – Dan Guzman Aug 09 '20 at 04:40
  • Why should sql server need to be told this at all? Other databases I tested didn't have this problem. – Peter Morris Aug 09 '20 at 08:25
  • @PeterMorris, I seldom need to resort to hints in SQL Server but this is a worse-case scenario of dozens (on my workstation) of synchronized threads all hitting empty/small tables where the plan is optimized for performance rather than maximizing concurrency. There are other ways to improve performance without deadlocks. For example, I ran a single-threaded test of 1K iterations, each inserting an `IncomingFile` row and 1K related `IncomingFileEvent` rows via a table-valued parameter. The test ran in about 10 seconds, inserting 1M rows with the FK in place. – Dan Guzman Aug 09 '20 at 14:04
  • The problem is as soon as you have multiple threads updating at the same time. A fast single-user database isn't much use in 99.9% is cases :) I think I should report this as a bug. – Peter Morris Aug 09 '20 at 16:21
  • 1
    It's arguable that this is a bug but feel free to report it on the [feedback forum](https://feedback.azure.com/forums/908035-sql-server). Some application/schema designs are prone to deadlocks. – Dan Guzman Aug 09 '20 at 16:40
3

I wrote the following extension to solve the problem for EF Core.

protected override void OnConfiguring(DbContextOptionsBuilder options)
{
    base.OnConfiguring(options);
    options.UseLoopJoinQueries();
}

Using this code...

    public static class UseLoopJoinQueriesExtension
    {
        public static DbContextOptionsBuilder UseLoopJoinQueries(this DbContextOptionsBuilder builder)
        {
            if (builder is null)
                throw new ArgumentNullException(nameof(builder));

            builder.AddInterceptors(new OptionLoopJoinCommandInterceptor());
            return builder;
        }
    }

    internal class OptionLoopJoinCommandInterceptor : DbCommandInterceptor
    {
        public override Task<InterceptionResult<DbDataReader>> ReaderExecutingAsync(DbCommand command, CommandEventData eventData, InterceptionResult<DbDataReader> result, CancellationToken cancellationToken = default)
        {
            AppendOptionToSql(command);
            return Task.FromResult(result);
        }

        public override InterceptionResult<DbDataReader> ReaderExecuting(DbCommand command, CommandEventData eventData, InterceptionResult<DbDataReader> result)
        {
            AppendOptionToSql(command);
            return result;
        }

        private static void AppendOptionToSql(DbCommand command)
        {
            const string OPTION_TEXT = " OPTION (LOOP JOIN)";
            string[] commands = command.CommandText.Split(";");

            for (int index = 0; index < commands.Length; index++)
            {
                string sql = commands[index].Trim();
                if (sql.StartsWith("insert into ", StringComparison.InvariantCultureIgnoreCase)
                    || sql.StartsWith("select ", StringComparison.InvariantCultureIgnoreCase)
                    || sql.StartsWith("delete ", StringComparison.InvariantCultureIgnoreCase)
                    || sql.StartsWith("merge ", StringComparison.InvariantCultureIgnoreCase))
                {
                    commands[index] += OPTION_TEXT;
                }
            }

#pragma warning disable CA2100 // Review SQL queries for security vulnerabilities
            command.CommandText = string.Join(";\r\n", commands);
#pragma warning restore CA2100 // Review SQL queries for security vulnerabilities
        }
    }
Peter Morris
  • 20,174
  • 9
  • 81
  • 146