From 4ecc826de0424e5804614f3fcd1864214c66624e Mon Sep 17 00:00:00 2001 From: Andriy Svyryd Date: Fri, 3 Sep 2021 21:41:14 -0700 Subject: [PATCH] Always sort deletes before inserts for the same table to avoid deadlocks. This dependency isn't hard, so cycle breaking is introduced. Make the implementations of Multigraph TopologicalSort and BatchingTopologicalSort more alike and faster Fixes #14371 --- .../Update/Internal/CommandBatchPreparer.cs | 31 +- src/Shared/Multigraph.cs | 282 ++++++++++-------- .../Internal/MigrationsModelDifferTest.cs | 11 +- .../BatchingTest.cs | 67 +++++ test/EFCore.Tests/Utilities/MultigraphTest.cs | 4 +- 5 files changed, 257 insertions(+), 138 deletions(-) diff --git a/src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs b/src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs index a70dedb1688..07f1ec9ca97 100644 --- a/src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs +++ b/src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs @@ -287,7 +287,9 @@ protected virtual IReadOnlyList> TopologicalS AddUniqueValueEdges(modificationCommandGraph); - return modificationCommandGraph.BatchingTopologicalSort(FormatCycle); + AddSameTableEdges(modificationCommandGraph); + + return modificationCommandGraph.BatchingTopologicalSort(static (_, _, edges) => edges.All(e => e is IEntityType), FormatCycle); } private string FormatCycle(IReadOnlyList>> data) @@ -765,5 +767,32 @@ private void AddUniqueValueEdges(Multigraph modificationCommandGraph) + { + var deletedDictionary = new Dictionary<(string, string?), List>(); + + foreach (var command in modificationCommandGraph.Vertices) + { + if (command.EntityState == EntityState.Deleted) + { + deletedDictionary.GetOrAddNew((command.TableName, command.Schema)).Add(command); + } + } + + foreach (var command in modificationCommandGraph.Vertices) + { + if (command.EntityState == EntityState.Added) + { + if (deletedDictionary.TryGetValue((command.TableName, command.Schema), out var deletedList)) + { + foreach (var deleted in deletedList) + { + modificationCommandGraph.AddEdge(deleted, command, command.Entries[0].EntityType); + } + } + } + } + } } } diff --git a/src/Shared/Multigraph.cs b/src/Shared/Multigraph.cs index 93c69e95033..f3b6a0437bb 100644 --- a/src/Shared/Multigraph.cs +++ b/src/Shared/Multigraph.cs @@ -136,123 +136,123 @@ public IReadOnlyList TopologicalSort( Func>>, string>? formatCycle, Func? formatException = null) { - var sortedQueue = new List(); - var predecessorCounts = new Dictionary(); - - foreach (var vertex in _vertices) + var queue = new List(); + var predecessorCounts = new Dictionary(_predecessorMap.Count); + foreach (var predecessor in _predecessorMap) { - foreach (var outgoingNeighbor in GetOutgoingNeighbors(vertex)) - { - if (predecessorCounts.ContainsKey(outgoingNeighbor)) - { - predecessorCounts[outgoingNeighbor]++; - } - else - { - predecessorCounts[outgoingNeighbor] = 1; - } - } + predecessorCounts[predecessor.Key] = predecessor.Value.Count; } foreach (var vertex in _vertices) { if (!predecessorCounts.ContainsKey(vertex)) { - sortedQueue.Add(vertex); + queue.Add(vertex); } } var index = 0; - while (sortedQueue.Count < _vertices.Count) + while (queue.Count < _vertices.Count) { - while (index < sortedQueue.Count) + while (index < queue.Count) { - var currentRoot = sortedQueue[index]; + var currentRoot = queue[index]; + index++; - foreach (var successor in GetOutgoingNeighbors(currentRoot).Where(neighbor => predecessorCounts.ContainsKey(neighbor))) + foreach (var successor in GetOutgoingNeighbors(currentRoot)) { - // Decrement counts for edges from sorted vertices and append any vertices that no longer have predecessors predecessorCounts[successor]--; if (predecessorCounts[successor] == 0) { - sortedQueue.Add(successor); - predecessorCounts.Remove(successor); + queue.Add(successor); } } - - index++; } // Cycle breaking - if (sortedQueue.Count < _vertices.Count) + if (queue.Count < _vertices.Count) { var broken = false; var candidateVertices = predecessorCounts.Keys.ToList(); var candidateIndex = 0; - // Iterate over the unsorted vertices while ((candidateIndex < candidateVertices.Count) && !broken && tryBreakEdge != null) { var candidateVertex = candidateVertices[candidateIndex]; + if (predecessorCounts[candidateVertex] != 1) + { + candidateIndex++; + continue; + } - // Find vertices in the unsorted portion of the graph that have edges to the candidate - var incomingNeighbors = GetIncomingNeighbors(candidateVertex) - .Where(neighbor => predecessorCounts.ContainsKey(neighbor)).ToList(); + // Find a vertex in the unsorted portion of the graph that has edges to the candidate + var incomingNeighbor = GetIncomingNeighbors(candidateVertex) + .First(neighbor => predecessorCounts.TryGetValue(neighbor, out var neighborPredecessors) + && neighborPredecessors > 0); - foreach (var incomingNeighbor in incomingNeighbors) + if (tryBreakEdge(incomingNeighbor, candidateVertex, _successorMap[incomingNeighbor][candidateVertex])) { - // Check to see if the edge can be broken - if (tryBreakEdge(incomingNeighbor, candidateVertex, _successorMap[incomingNeighbor][candidateVertex])) - { - predecessorCounts[candidateVertex]--; - if (predecessorCounts[candidateVertex] == 0) - { - sortedQueue.Add(candidateVertex); - predecessorCounts.Remove(candidateVertex); - broken = true; - break; - } - } + _successorMap[incomingNeighbor].Remove(candidateVertex); + _predecessorMap[candidateVertex].Remove(incomingNeighbor); + predecessorCounts[candidateVertex]--; + queue.Add(candidateVertex); + broken = true; + break; } candidateIndex++; } - if (!broken) + if (broken) { - // Failed to break the cycle - var currentCycleVertex = _vertices.First(v => predecessorCounts.ContainsKey(v)); - var cycle = new List { currentCycleVertex }; - var finished = false; - while (!finished) + continue; + } + + var currentCycleVertex = _vertices.First( + v => predecessorCounts.TryGetValue(v, out var predecessorCount) && predecessorCount != 0); + var cycle = new List { currentCycleVertex }; + var finished = false; + while (!finished) + { + foreach (var predecessor in GetIncomingNeighbors(currentCycleVertex)) { - // Find a cycle - foreach (var predecessor in GetIncomingNeighbors(currentCycleVertex) - .Where(neighbor => predecessorCounts.ContainsKey(neighbor))) + if (!predecessorCounts.TryGetValue(predecessor, out var predecessorCount) + || predecessorCount == 0) { - if (predecessorCounts[predecessor] != 0) - { - predecessorCounts[currentCycleVertex] = -1; - - currentCycleVertex = predecessor; - cycle.Add(currentCycleVertex); - finished = predecessorCounts[predecessor] == -1; - break; - } + continue; } + + predecessorCounts[currentCycleVertex] = -1; + + currentCycleVertex = predecessor; + cycle.Add(currentCycleVertex); + finished = predecessorCounts[predecessor] == -1; + break; } + } + + cycle.Reverse(); - cycle.Reverse(); + // Remove any tail that's not part of the cycle + var startingVertex = cycle[0]; + for (var i = cycle.Count - 1; i >= 0; i--) + { + if (cycle[i].Equals(startingVertex)) + { + break; + } - ThrowCycle(cycle, formatCycle, formatException); + cycle.RemoveAt(i); } + + ThrowCycle(cycle, formatCycle, formatException); } } - return sortedQueue; + return queue; } private void ThrowCycle( @@ -287,27 +287,17 @@ private void ThrowCycle( => vertex.ToString(); public IReadOnlyList> BatchingTopologicalSort() - => BatchingTopologicalSort(null); + => BatchingTopologicalSort(null, null); public IReadOnlyList> BatchingTopologicalSort( + Func, bool>? tryBreakEdge, Func>>, string>? formatCycle) { var currentRootsQueue = new List(); - var predecessorCounts = new Dictionary(); - - foreach (var vertex in _vertices) + var predecessorCounts = new Dictionary(_predecessorMap.Count); + foreach (var predecessor in _predecessorMap) { - foreach (var outgoingNeighbor in GetOutgoingNeighbors(vertex)) - { - if (predecessorCounts.ContainsKey(outgoingNeighbor)) - { - predecessorCounts[outgoingNeighbor]++; - } - else - { - predecessorCounts[outgoingNeighbor] = 1; - } - } + predecessorCounts[predecessor.Key] = predecessor.Value.Count; } foreach (var vertex in _vertices) @@ -320,85 +310,121 @@ public IReadOnlyList> BatchingTopologicalSort( var result = new List>(); var nextRootsQueue = new List(); - var currentRootIndex = 0; - while (currentRootIndex < currentRootsQueue.Count) + while (result.Sum(b => b.Count) != _vertices.Count) { - var currentRoot = currentRootsQueue[currentRootIndex]; - currentRootIndex++; - - // Remove edges from current root and add any exposed vertices to the next batch - foreach (var successor in GetOutgoingNeighbors(currentRoot)) + var currentRootIndex = 0; + while (currentRootIndex < currentRootsQueue.Count) { - predecessorCounts[successor]--; - if (predecessorCounts[successor] == 0) + var currentRoot = currentRootsQueue[currentRootIndex]; + currentRootIndex++; + + foreach (var successor in GetOutgoingNeighbors(currentRoot)) { - nextRootsQueue.Add(successor); + predecessorCounts[successor]--; + if (predecessorCounts[successor] == 0) + { + nextRootsQueue.Add(successor); + } } - } - // Roll lists over for next batch - if (currentRootIndex == currentRootsQueue.Count) - { - result.Add(currentRootsQueue); + // Roll lists over for next batch + if (currentRootIndex == currentRootsQueue.Count) + { + result.Add(currentRootsQueue); - currentRootsQueue = nextRootsQueue; - currentRootIndex = 0; + currentRootsQueue = nextRootsQueue; + currentRootIndex = 0; - if (currentRootsQueue.Count != 0) - { - nextRootsQueue = new List(); + if (currentRootsQueue.Count != 0) + { + nextRootsQueue = new List(); + } } } - } - if (result.Sum(b => b.Count) != _vertices.Count) - { - var currentCycleVertex = _vertices.First( - v => predecessorCounts.TryGetValue(v, out var predecessorNumber) && predecessorNumber != 0); - var cyclicWalk = new List { currentCycleVertex }; - var finished = false; - while (!finished) + // Cycle breaking + if (result.Sum(b => b.Count) != _vertices.Count) { - foreach (var predecessor in GetIncomingNeighbors(currentCycleVertex)) + var broken = false; + + var candidateVertices = predecessorCounts.Keys.ToList(); + var candidateIndex = 0; + + while ((candidateIndex < candidateVertices.Count) + && !broken + && tryBreakEdge != null) { - if (!predecessorCounts.TryGetValue(predecessor, out var predecessorCount)) + var candidateVertex = candidateVertices[candidateIndex]; + if (predecessorCounts[candidateVertex] != 1) { + candidateIndex++; continue; } - if (predecessorCount != 0) + // Find a vertex in the unsorted portion of the graph that has edges to the candidate + var incomingNeighbor = GetIncomingNeighbors(candidateVertex) + .First(neighbor => predecessorCounts.TryGetValue(neighbor, out var neighborPredecessors) + && neighborPredecessors > 0); + + if (tryBreakEdge(incomingNeighbor, candidateVertex, _successorMap[incomingNeighbor][candidateVertex])) + { + _successorMap[incomingNeighbor].Remove(candidateVertex); + _predecessorMap[candidateVertex].Remove(incomingNeighbor); + predecessorCounts[candidateVertex]--; + currentRootsQueue.Add(candidateVertex); + nextRootsQueue = new List(); + broken = true; + break; + } + + candidateIndex++; + } + + if (broken) + { + continue; + } + + var currentCycleVertex = _vertices.First( + v => predecessorCounts.TryGetValue(v, out var predecessorCount) && predecessorCount != 0); + var cycle = new List { currentCycleVertex }; + var finished = false; + while (!finished) + { + foreach (var predecessor in GetIncomingNeighbors(currentCycleVertex)) { + if (!predecessorCounts.TryGetValue(predecessor, out var predecessorCount) + || predecessorCount == 0) + { + continue; + } + predecessorCounts[currentCycleVertex] = -1; currentCycleVertex = predecessor; - cyclicWalk.Add(currentCycleVertex); + cycle.Add(currentCycleVertex); finished = predecessorCounts[predecessor] == -1; break; } } - } - cyclicWalk.Reverse(); + cycle.Reverse(); - var cycle = new List(); - var startingVertex = cyclicWalk.First(); - cycle.Add(startingVertex); - foreach (var vertex in cyclicWalk.Skip(1)) - { - if (!vertex.Equals(startingVertex)) - { - cycle.Add(vertex); - } - else + // Remove any tail that's not part of the cycle + var startingVertex = cycle[0]; + for (var i = cycle.Count - 1; i >= 0; i--) { - break; - } - } + if (cycle[i].Equals(startingVertex)) + { + break; + } - cycle.Add(startingVertex); + cycle.RemoveAt(i); + } - ThrowCycle(cycle, formatCycle); + ThrowCycle(cycle, formatCycle); + } } return result; diff --git a/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs b/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs index 69c96bd73e2..45aec7aaa3b 100644 --- a/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs +++ b/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs @@ -333,13 +333,10 @@ public void Model_differ_breaks_double_foreign_key_cycles_in_create_table_operat }, result => { - Assert.Equal(14, result.Count); - - var createBankTableOperation = Assert.IsType(result[0]); - Assert.Equal("Banks", createBankTableOperation.Name); - Assert.Empty(createBankTableOperation.ForeignKeys); - - Assert.Equal(4, result.OfType().Count()); + Assert.Equal(3, result.OfType().Count()); + Assert.Equal(7, result.OfType().Count()); + Assert.Equal(7, result.OfType().SelectMany(t => t.ForeignKeys).Count() + + result.OfType().Count()); }); } diff --git a/test/EFCore.SqlServer.FunctionalTests/BatchingTest.cs b/test/EFCore.SqlServer.FunctionalTests/BatchingTest.cs index 4fa323a94c0..889e056dc92 100644 --- a/test/EFCore.SqlServer.FunctionalTests/BatchingTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/BatchingTest.cs @@ -169,6 +169,73 @@ public void Insertion_order_is_preserved(int maxBatchSize) }); } + [ConditionalFact] + public void Deadlock_on_inserts_and_deletes_with_dependents_is_handled_correctly() + { + var blogs = new List(); + + using (var context = CreateContext()) + { + var owner1 = new Owner { Name = "0" }; + var owner2 = new Owner { Name = "1" }; + context.Owners.Add(owner1); + context.Owners.Add(owner2); + + blogs.Add(new Blog + { + Id = Guid.NewGuid(), + Owner = owner1, + Order = 1 + }); + blogs.Add(new Blog + { + Id = Guid.NewGuid(), + Owner = owner2, + Order = 2 + }); + blogs.Add(new Blog + { + Id = Guid.NewGuid(), + Owner = owner1, + Order = 3 + }); + blogs.Add(new Blog + { + Id = Guid.NewGuid(), + Owner = owner2, + Order = 4 + }); + + context.AddRange(blogs); + + context.SaveChanges(); + } + + for (var i = 0; i < 10; i++) + { + Parallel.ForEach(blogs, blog => + { + RemoveAndAddPosts(blog); + }); + } + + void RemoveAndAddPosts(Blog blog) + { + using var context = (BloggingContext)Fixture.CreateContext(useConnectionString: true); + + context.Attach(blog); + blog.Posts.Clear(); + + blog.Posts.Add(new Post { Comments = { new Comment() } }); + blog.Posts.Add(new Post { Comments = { new Comment() } }); + blog.Posts.Add(new Post { Comments = { new Comment() } }); + + context.SaveChanges(); + } + + Fixture.Reseed(); + } + [ConditionalFact] public void Deadlock_on_deletes_with_dependents_is_handled_correctly() { diff --git a/test/EFCore.Tests/Utilities/MultigraphTest.cs b/test/EFCore.Tests/Utilities/MultigraphTest.cs index 1ed04245ed3..3310ab14ffe 100644 --- a/test/EFCore.Tests/Utilities/MultigraphTest.cs +++ b/test/EFCore.Tests/Utilities/MultigraphTest.cs @@ -434,7 +434,7 @@ string formatter(IEnumerable>> data) Assert.Equal( CoreStrings.CircularDependency(message), - Assert.Throws(() => graph.BatchingTopologicalSort(formatter)).Message); + Assert.Throws(() => graph.BatchingTopologicalSort(null, formatter)).Message); Assert.Equal(3, cycleData.Count()); @@ -485,7 +485,7 @@ string formatter(IEnumerable>> data) Assert.Equal( CoreStrings.CircularDependency(message), - Assert.Throws(() => graph.BatchingTopologicalSort(formatter)).Message); + Assert.Throws(() => graph.BatchingTopologicalSort(null, formatter)).Message); Assert.Equal(2, cycleData.Count);