[DMS-277] DB schema changes for delete performance #197

bradbanister · 2024-07-19T14:30:43Z

No description provided.

github-actions · 2024-07-19T14:36:17Z

Test Results

428 tests - 3 390 ✅ - 12 1m 48s ⏱️ -32s
5 suites ±0 38 💤 + 9
5 files ±0 0 ❌ ± 0

Results for commit 950bc88. ± Comparison against base commit 971b577.

This pull request removes 5 and adds 2 tests. Note that renamed tests count towards both.

EdFi.DataManagementService.Backend.Postgresql.Test.Integration.UpdateTests+Given_an_update_of_a_document_that_references_a_non_existent_document ‑ It_should_be_a_successful_inserts
EdFi.DataManagementService.Backend.Postgresql.Test.Integration.UpdateTests+Given_an_update_of_a_document_that_references_a_non_existent_document ‑ It_should_be_a_update_failure_reference
EdFi.DataManagementService.Backend.Postgresql.Test.Integration.UpdateTests+Given_an_update_of_a_document_that_references_an_existing_document ‑ It_should_be_a_successful_update
EdFi.DataManagementService.Backend.Postgresql.Test.Integration.UpdateTests+Given_an_update_of_a_document_with_one_existing_and_one_non_existent_reference ‑ It_should_be_a_successful_inserts
EdFi.DataManagementService.Backend.Postgresql.Test.Integration.UpdateTests+Given_an_update_of_a_subclass_document_referenced_by_an_existing_document_as_a_superclass ‑ It_should_be_a_successful_inserts

EdFi.DataManagementService.Backend.Postgresql.Test.Integration.UpdateTests+Given_an_update_of_a_document_to_reference_a_non_existent_document ‑ It_should_be_an_update_failure_reference
EdFi.DataManagementService.Backend.Postgresql.Test.Integration.UpdateTests+Given_an_update_of_a_document_to_reference_an_existing_document ‑ It_should_be_a_successful_update

This pull request skips 9 tests.

EdFi.DataManagementService.Backend.Postgresql.Test.Integration.DeleteTests+Given_a_delete_of_the_same_document_with_two_overlapping_requests ‑ It_should_be_a_write_conflict_for_2nd_transaction
EdFi.DataManagementService.Backend.Postgresql.Test.Integration.DeleteTests+Given_an_overlapping_delete_and_update_of_the_same_document_with_delete_committed_first ‑ It_should_be_an_update_write_conflict_for_2nd_transaction
EdFi.DataManagementService.Backend.Postgresql.Test.Integration.DeleteTests+Given_an_overlapping_delete_and_update_of_the_same_document_with_update_committed_first ‑ It_should_be_a_delete_write_conflict_for_2nd_transaction
EdFi.DataManagementService.Backend.Postgresql.Test.Integration.DeleteTests+Given_an_overlapping_delete_and_upsert_as_update_of_the_same_document_with_delete_committed_first ‑ It_should_be_an_update_write_conflict_for_2nd_transaction
EdFi.DataManagementService.Backend.Postgresql.Test.Integration.DeleteTests+Given_an_overlapping_delete_and_upsert_as_update_of_the_same_document_with_upsert_committed_first ‑ It_should_be_a_delete_write_conflict_for_2nd_transaction
EdFi.DataManagementService.Backend.Postgresql.Test.Integration.UpdateTests+Given_an_update_of_the_same_document_with_two_overlapping_request ‑ It_should_be_a_conflict_failure_for_2nd_transaction
EdFi.DataManagementService.Backend.Postgresql.Test.Integration.UpdateTests+Given_an_update_of_the_same_document_with_two_overlapping_request_but_also_with_different_references ‑ It_should_be_a_conflict_failure_for_2nd_transaction
EdFi.DataManagementService.Backend.Postgresql.Test.Integration.UpsertTests+Given_an_insert_of_the_same_new_document_with_two_overlapping_requests ‑ It_should_be_a_write_conflict_for_2nd_transaction
EdFi.DataManagementService.Backend.Postgresql.Test.Integration.UpsertTests+Given_an_update_of_the_same_document_with_two_overlapping_requests ‑ It_should_be_a_write_conflict_for_2nd_transaction

♻️ This comment has been updated with latest results.

bradbanister · 2024-07-22T19:50:11Z

src/backend/EdFi.DataManagementService.Backend.Postgresql.Test.Integration/DeleteTests.cs

@@ -122,6 +122,7 @@ public void It_should_be_a_successful_delete_for_1st_transaction()
        }

        [Test]
+        [Ignore("DMS-296 will resolve intermittent serialization failures causing UnknownFailure")]


See DMS-296, but ran into this several times. Not sure why.

bradbanister · 2024-07-22T19:51:03Z

src/backend/EdFi.DataManagementService.Backend.Postgresql.Test.Integration/UpdateTests.cs

@@ -235,76 +236,63 @@ public async Task It_should_be_the_1st_update_found_by_get()
    }

    [TestFixture]
-    public class Given_an_update_of_a_document_that_references_a_non_existent_document : UpdateTests
+    public class Given_an_update_of_a_document_to_reference_a_non_existent_document : UpdateTests


This test was not testing the right thing

bradbanister · 2024-07-22T19:51:51Z

src/backend/EdFi.DataManagementService.Backend.Postgresql.Test.Integration/UpdateTests.cs

        [Test]
        public void It_should_be_a_update_failure_reference()
        {
-            _updateResult.Should().BeOfType<UpdateResult.UpdateFailureImmutableIdentity>();
+            _updateResult.Should().BeOfType<UpdateResult.UpdateFailureReference>();


This test was not testing the right thing

bradbanister · 2024-07-22T19:52:57Z

src/backend/EdFi.DataManagementService.Backend.Postgresql.Test.Integration/UpdateTests.cs


-    // given an update of a document that references a nonexisting descriptor
+    // given an update of a document that tries to reference an existing descriptor


Rename to make scenario more clear

bradbanister · 2024-07-22T19:54:10Z

src/backend/EdFi.DataManagementService.Backend.Postgresql/Operation/SqlAction.cs

@@ -13,130 +13,18 @@

 namespace EdFi.DataManagementService.Backend.Postgresql.Operation;

-/// <summary>


This unnecessary duplication was getting to be too much.

bradbanister · 2024-07-22T19:55:15Z

src/backend/EdFi.DataManagementService.Backend.Postgresql/Operation/SqlAction.cs

@@ -177,6 +65,7 @@ LockOption lockOption
                }
            };

+        await command.PrepareAsync();


All SQL now cached by PostgreSQL as prepared statements

bradbanister · 2024-07-22T19:56:44Z

src/backend/EdFi.DataManagementService.Backend.Postgresql/Operation/SqlAction.cs

+              FROM unnest($1, $2, $3, $4) AS
+                  ids(documentId, documentPartitionKey, referentialId, referentialPartitionKey)
+              LEFT JOIN dms.Alias a ON
+                  ids.referentialId = a.referentialId and ids.referentialPartitionKey = a.referentialPartitionKey",


Inserts into references table now requires a join to get documentId of reference.

bradbanister · 2024-07-22T19:58:02Z

src/backend/EdFi.DataManagementService.Backend.Postgresql/Operation/SqlAction.cs

+                     INNER JOIN dms.Document d2 ON d2.Id = r.ReferencedDocumentId
+                       AND d2.DocumentPartitionKey = r.ReferencedDocumentPartitionKey
+                       WHERE d2.DocumentUuid = $1 AND d2.DocumentPartitionKey = $2) AS re
+                     ON re.ParentDocumentId = d.id AND re.ParentDocumentPartitionKey = d.DocumentPartitionKey


One less join now to get post-delete failure info. All joins are also now on indexes.

bradbanister · 2024-07-22T19:59:05Z

tests/RestClient/demo.http

+### Note, no descriptor validation yet
+
+### Setup - POST School Year 2025
+POST http://localhost:{{port}}/data/ed-fi/schoolYearTypes


Friday's demo, because why not?

This will be great for the Project Tanager workgroup meeting on Thursday.

bradbanister force-pushed the DMS-277 branch from 2db61d8 to 511d158 Compare July 22, 2024 16:33

bradbanister marked this pull request as ready for review July 22, 2024 19:48

bradbanister commented Jul 22, 2024

View reviewed changes

bradbanister force-pushed the DMS-277 branch from 376d255 to f285b31 Compare July 23, 2024 13:43

bradbanister added 6 commits July 24, 2024 09:09

[DMS-277] start

c35b7b2

[DMS-277] checkpoint

1d3fba1

[DMS-277] updated insert and delete SQL

f85f6c8

[DMS-277] sql fixup

1a3c72a

switch tests to repeatableread

b261711

disable write conflict tests

950bc88

bradbanister force-pushed the DMS-277 branch from f285b31 to 950bc88 Compare July 24, 2024 14:09

stephenfuqua approved these changes Jul 24, 2024

View reviewed changes

stephenfuqua merged commit fed2f61 into main Jul 24, 2024
12 checks passed

stephenfuqua deleted the DMS-277 branch July 24, 2024 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DMS-277] DB schema changes for delete performance #197

[DMS-277] DB schema changes for delete performance #197

bradbanister commented Jul 19, 2024

github-actions bot commented Jul 19, 2024 •

edited

Loading

bradbanister Jul 22, 2024

bradbanister Jul 22, 2024

bradbanister Jul 22, 2024

bradbanister Jul 22, 2024

bradbanister Jul 22, 2024

bradbanister Jul 22, 2024

bradbanister Jul 22, 2024

bradbanister Jul 22, 2024

bradbanister Jul 22, 2024 •

edited

Loading

stephenfuqua Jul 24, 2024


		// given an update of a document that references a nonexisting descriptor
		// given an update of a document that tries to reference an existing descriptor

		@@ -13,130 +13,18 @@

		namespace EdFi.DataManagementService.Backend.Postgresql.Operation;

		/// <summary>

[DMS-277] DB schema changes for delete performance #197

[DMS-277] DB schema changes for delete performance #197

Conversation

bradbanister commented Jul 19, 2024

github-actions bot commented Jul 19, 2024 • edited Loading

Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradbanister Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 19, 2024 •

edited

Loading

bradbanister Jul 22, 2024 •

edited

Loading