[DMS-373] Investigate deadlocks (#297)

Ed-Fi-Alliance-OSS · Oct 8, 2024 · cc27a94 · cc27a94
1 parent 482a760
commit cc27a94
Show file tree

Hide file tree

Showing 32 changed files with 541 additions and 807 deletions.
diff --git a/.vscode/settings.json b/.vscode/settings.json
@@ -74,7 +74,9 @@
     "allowidentityupdates",
     "appsettings",
     "begindate",
+    "correlationid",
     "decapitalizer",
+    "documentpartitionkey",
     "documentpathsmapping",
     "Edfi",
     "equalityconstraints",
@@ -84,6 +86,7 @@
     "isshoolyearenumeration",
     "issubclass",
     "jsonschemaforinsert",
+    "lastmodifiedat",
     "noallowidentityupdates",
     "nodocumentpathsmappings",
     "noequalityconstraints",
@@ -102,11 +105,16 @@
     "openapi",
     "opensearch",
     "Overpost",
+    "parentdocumentid",
+    "parentdocumentpartitionkey",
     "pluralizer",
+    "referenceddocumentid",
+    "referenceddocumentpartitionkey",
     "Reqnroll",
     "resourcename",
     "resourceschema",
     "resourceschemas",
+    "Retryable",
     "Serilog",
     "Shoolyear",
     "subclassidentitydocumentkey",

diff --git a/src/backend/EdFi.DataManagementService.Backend.Postgresql.Test.Integration/DatabaseTest.cs b/src/backend/EdFi.DataManagementService.Backend.Postgresql.Test.Integration/DatabaseTest.cs
@@ -126,7 +126,7 @@ public void ConnectionTeardown()
 
     protected static SqlAction CreateSqlAction()
     {
-        return new SqlAction(NullLogger<SqlAction>.Instance);
+        return new SqlAction();
     }
 
     protected static UpsertDocument CreateUpsert()

diff --git a/src/backend/EdFi.DataManagementService.Backend.Postgresql.Test.Integration/DeleteTests.cs b/src/backend/EdFi.DataManagementService.Backend.Postgresql.Test.Integration/DeleteTests.cs
@@ -123,9 +123,9 @@ public void It_should_be_a_successful_delete_for_1st_transaction()
         }
 
         [Test]
-        public void It_should_be_not_exists_for_2nd_transaction_due_to_retry()
+        public void It_should_be_a_write_conflict_for_2nd_transaction()
         {
-            _deleteResult2!.Should().BeOfType<DeleteResult.DeleteFailureNotExists>();
+            _deleteResult2!.Should().BeOfType<DeleteResult.DeleteFailureWriteConflict>();
         }
     }
 
@@ -184,9 +184,9 @@ public void It_should_be_a_successful_delete_for_1st_transaction()
         }
 
         [Test]
-        public void It_should_be_an_update_not_exists_for_2nd_transaction_due_to_retry()
+        public void It_should_be_an_update_write_conflict_for_2nd_transaction()
         {
-            _updateResult.Should().BeOfType<UpdateResult.UpdateFailureNotExists>();
+            _updateResult.Should().BeOfType<UpdateResult.UpdateFailureWriteConflict>();
         }
     }
 
@@ -245,9 +245,9 @@ public void It_should_be_a_successful_update_for_1st_transaction()
         }
 
         [Test]
-        public void It_should_be_a_successful_delete_for_2nd_transaction_due_to_retry()
+        public void It_should_be_a_delete_write_conflict_for_2nd_transaction()
         {
-            _deleteResult.Should().BeOfType<DeleteResult.DeleteSuccess>();
+            _deleteResult.Should().BeOfType<DeleteResult.DeleteFailureWriteConflict>();
         }
     }
 
@@ -306,9 +306,9 @@ public void It_should_be_a_successful_delete_for_1st_transaction()
         }
 
         [Test]
-        public void It_should_be_a_successful_insert_for_2nd_transaction_due_to_retry()
+        public void It_should_be_an_update_write_conflict_for_2nd_transaction()
         {
-            _upsertResult.Should().BeOfType<UpsertResult.InsertSuccess>();
+            _upsertResult.Should().BeOfType<UpsertResult.UpsertFailureWriteConflict>();
         }
     }
 
@@ -367,9 +367,9 @@ public void It_should_be_a_successful_update_for_1st_transaction()
         }
 
         [Test]
-        public void It_should_be_a_successful_delete_for_2nd_transaction_due_to_retry()
+        public void It_should_be_a_delete_write_conflict_for_2nd_transaction()
         {
-            _deleteResult.Should().BeOfType<DeleteResult.DeleteSuccess>();
+            _deleteResult.Should().BeOfType<DeleteResult.DeleteFailureWriteConflict>();
         }
     }
 

diff --git a/src/backend/EdFi.DataManagementService.Backend.Postgresql.Test.Integration/UpdateTests.cs b/src/backend/EdFi.DataManagementService.Backend.Postgresql.Test.Integration/UpdateTests.cs
@@ -296,20 +296,19 @@ public void It_should_be_a_successful_update_for_1st_transaction()
         }
 
         [Test]
-        public void It_should_be_a_successful_update_for_2nd_transaction_due_to_retry()
+        public void It_should_be_a_conflict_failure_for_2nd_transaction()
         {
-            _updateResult2!.Should().BeOfType<UpdateResult.UpdateSuccess>();
+            _updateResult2!.Should().BeOfType<UpdateResult.UpdateFailureWriteConflict>();
         }
 
         [Test]
-        public async Task It_should_be_the_2nd_update_found_by_get()
+        public async Task It_should_be_the_1st_update_found_by_get()
         {
             IGetRequest getRequest = CreateGetRequest(_defaultResourceName, _documentUuidGuid);
             GetResult? _getResult = await CreateGetById().GetById(getRequest, Connection!, Transaction!);
 
             (_getResult! as GetResult.GetSuccess)!.DocumentUuid.Value.Should().Be(_documentUuidGuid);
-            (_getResult! as GetResult.GetSuccess)!.EdfiDoc.ToJsonString().Should().Be(_edFiDocString3);
-            (_getResult! as GetResult.GetSuccess)!.LastModifiedTraceId.Should().Be(traceId.Value);
+            (_getResult! as GetResult.GetSuccess)!.EdfiDoc.ToJsonString().Should().Be(_edFiDocString2);
         }
     }
 
@@ -654,9 +653,9 @@ public void It_should_be_a_successful_update_for_1st_transaction()
         }
 
         [Test]
-        public void It_should_be_a_successful_update_for_2nd_transaction_due_to_retry()
+        public void It_should_be_a_conflict_failure_for_2nd_transaction()
         {
-            _updateResult2!.Should().BeOfType<UpdateResult.UpdateSuccess>();
+            _updateResult2!.Should().BeOfType<UpdateResult.UpdateFailureWriteConflict>();
         }
     }
 

diff --git a/src/backend/EdFi.DataManagementService.Backend.Postgresql.Test.Integration/UpsertTests.cs b/src/backend/EdFi.DataManagementService.Backend.Postgresql.Test.Integration/UpsertTests.cs
@@ -323,19 +323,19 @@ public void It_should_be_a_successful_update_for_1st_transaction()
         }
 
         [Test]
-        public void It_should_be_a_successful_update_for_2nd_transaction_due_to_retry()
+        public void It_should_be_a_write_conflict_for_2nd_transaction()
         {
-            _upsertResult2!.Should().BeOfType<UpsertResult.UpdateSuccess>();
+            _upsertResult2!.Should().BeOfType<UpsertResult.UpsertFailureWriteConflict>();
         }
 
         [Test]
-        public async Task It_should_be_the_2nd_updated_document_found_by_get()
+        public async Task It_should_be_the_1st_updated_document_found_by_get()
         {
             IGetRequest getRequest = CreateGetRequest(_defaultResourceName, _documentUuidGuid);
             GetResult? getResult = await CreateGetById().GetById(getRequest, Connection!, Transaction!);
 
             (getResult! as GetResult.GetSuccess)!.DocumentUuid.Value.Should().Be(_documentUuidGuid);
-            (getResult! as GetResult.GetSuccess)!.EdfiDoc.ToJsonString().Should().Contain("\"abc\":8");
+            (getResult! as GetResult.GetSuccess)!.EdfiDoc.ToJsonString().Should().Contain("\"abc\":7");
         }
     }
 

diff --git a/src/backend/EdFi.DataManagementService.Backend.Postgresql/Deploy/DatabaseDeploy.cs b/src/backend/EdFi.DataManagementService.Backend.Postgresql/Deploy/DatabaseDeploy.cs
@@ -31,7 +31,9 @@ public DatabaseDeployResult DeployDatabase(string connectionString)
             .Build();
 
         if (!upgrader.TryConnect(out string error))
+        {
             return new DatabaseDeployResult.DatabaseDeployFailure(new Exception(error));
+        }
 
         var result = upgrader.PerformUpgrade();
         return result.Successful

diff --git a/src/backend/EdFi.DataManagementService.Backend.Postgresql/Operation/DeleteDocumentById.cs b/src/backend/EdFi.DataManagementService.Backend.Postgresql/Operation/DeleteDocumentById.cs
@@ -69,6 +69,11 @@ NpgsqlTransaction transaction
             _logger.LogDebug(pe, "Transaction conflict on DeleteById - {TraceId}", deleteRequest.TraceId);
             return new DeleteResult.DeleteFailureWriteConflict();
         }
+        catch (PostgresException pe) when (pe.SqlState == PostgresErrorCodes.DeadlockDetected)
+        {
+            _logger.LogDebug(pe, "Transaction deadlock on DeleteById - {TraceId}", deleteRequest.TraceId);
+            return new DeleteResult.DeleteFailureWriteConflict();
+        }
         catch (PostgresException pe) when (pe.SqlState == PostgresErrorCodes.ForeignKeyViolation)
         {
             // Restore transaction save point to continue using transaction
@@ -79,7 +84,6 @@ NpgsqlTransaction transaction
                 documentPartitionKey,
                 connection,
                 transaction,
-                LockOption.BlockUpdateDelete,
                 deleteRequest.TraceId
             );
             _logger.LogDebug(pe, "Foreign key violation on Delete - {TraceId}", deleteRequest.TraceId);

diff --git a/src/backend/EdFi.DataManagementService.Backend.Postgresql/Operation/GetDocumentById.cs b/src/backend/EdFi.DataManagementService.Backend.Postgresql/Operation/GetDocumentById.cs
@@ -45,7 +45,6 @@ NpgsqlTransaction transaction
                 PartitionKeyFor(getRequest.DocumentUuid),
                 connection,
                 transaction,
-                LockOption.None,
                 getRequest.TraceId
             );
 
@@ -61,6 +60,11 @@ NpgsqlTransaction transaction
                 document.LastModifiedTraceId
             );
         }
+        catch (PostgresException pe) when (pe.SqlState == PostgresErrorCodes.DeadlockDetected)
+        {
+            _logger.LogDebug(pe, "Transaction deadlock on query - {TraceId}", getRequest.TraceId);
+            return new GetResult.GetFailureRetryable();
+        }
         catch (Exception ex)
         {
             _logger.LogError(ex, "GetDocumentById failure - {TraceId}", getRequest.TraceId);

diff --git a/src/backend/EdFi.DataManagementService.Backend.Postgresql/Operation/ISqlAction.cs b/src/backend/EdFi.DataManagementService.Backend.Postgresql/Operation/ISqlAction.cs
@@ -24,7 +24,6 @@ public interface ISqlAction
         PartitionKey partitionKey,
         NpgsqlConnection connection,
         NpgsqlTransaction transaction,
-        LockOption lockOption,
         TraceId traceId
     );
 
@@ -33,7 +32,6 @@ TraceId traceId
         PartitionKey partitionKey,
         NpgsqlConnection connection,
         NpgsqlTransaction transaction,
-        LockOption lockOption,
         TraceId traceId
     );
 
@@ -75,7 +73,6 @@ public Task<UpdateDocumentValidationResult> UpdateDocumentValidation(
         PartitionKey referentialPartitionKey,
         NpgsqlConnection connection,
         NpgsqlTransaction transaction,
-        LockOption lockOption,
         TraceId traceId
     );
 
@@ -124,7 +121,6 @@ public Task<string[]> FindReferencingResourceNamesByDocumentUuid(
         PartitionKey documentPartitionKey,
         NpgsqlConnection connection,
         NpgsqlTransaction transaction,
-        LockOption lockOption,
         TraceId traceId
     );
 
@@ -133,7 +129,6 @@ public Task<Document[]> FindReferencingDocumentsByDocumentId(
         short documentPartitionKey,
         NpgsqlConnection connection,
         NpgsqlTransaction transaction,
-        LockOption lockOption,
         TraceId traceId
     );
 }
diff --git a/src/backend/EdFi.DataManagementService.Backend.Postgresql/Operation/LockOption.cs b/src/backend/EdFi.DataManagementService.Backend.Postgresql/Operation/LockOption.cs
@@ -19,9 +19,4 @@ public enum LockOption
     /// A read lock that blocks update/delete of the result rows but not other reads
     /// </summary>
     BlockUpdateDelete,
-
-    /// <summary>
-    /// A read lock in preparation for an update/delete of the result rows in the same transaction
-    /// </summary>
-    BlockAll
 }
diff --git a/src/backend/EdFi.DataManagementService.Backend.Postgresql/Operation/QueryDocument.cs b/src/backend/EdFi.DataManagementService.Backend.Postgresql/Operation/QueryDocument.cs
@@ -49,6 +49,15 @@ await _sqlAction.GetAllDocumentsByResourceName(
                     : null
             );
         }
+        catch (PostgresException pe) when (pe.SqlState == PostgresErrorCodes.DeadlockDetected)
+        {
+            _logger.LogDebug(
+                pe,
+                "Transaction deadlock on query - {TraceId}",
+                queryRequest.TraceId
+            );
+            return new QueryResult.QueryFailureRetryable();
+        }
         catch (Exception ex)
         {
             _logger.LogError(ex, "QueryDocument.QueryDocuments failure - {TraceId}", queryRequest.TraceId);

diff --git a/src/backend/EdFi.DataManagementService.Backend.Postgresql/Operation/Resilience.cs b/src/backend/EdFi.DataManagementService.Backend.Postgresql/Operation/Resilience.cs