Removing conflict check when the last conflict happened on a journal #1882

danieldoglas · 2024-09-30T17:09:06Z

Details

The normal flow for a write command is to try to commit it. If that fails because of a conflict, we will retry the commits three times before sending them to the blocking queue.

When we have a conflict on a page that belongs to a table, that means that some other command is also changing that same page. We'll store the page that conflicted:

Bedrock/sqlitecluster/SQLite.cpp

Line 286 in 67b55ff

_conflictPage = atol(offset);

And then we'll wait until that page is not locked anymore before we try to execute the command again:

Bedrock/BedrockServer.cpp

Lines 910 to 913 in 67b55ff

    
           PageLockGuard pageLock(lastConflictPage); 
        
           if (lastConflictPage) { 
        
               SINFO("Waited " << (STimeNow() - conflictLockStartTime) << "us for lock on db page " << lastConflictPage << "."); 
        
           }

When that happens for any tables in the system, it is not a problem. But when we have a conflict that happened on a journal table, then we're blocking the next commit from happening without reason. The journals are chosen when the commit is being executed, and the chances that we select the same journal on the next commit are low. that considered, we don't need to do a PageLockGuard if the previous conflict was on a journal table.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/432114

Tests

Ran the ConflictSpam test and confirmed that the changed log line is also logging the tables:

2024-09-30T18:35:02.974407+00:00 expensidev2004 bedrock10019: xxxxxx (SQLite.cpp:757) commit [replicate517] [info] part of last conflict page: 31, conflict table: test

then I applied these changes to make the cluster use page conflicts

diff --git a/BedrockServer.cpp b/BedrockServer.cpp
index 544d3e5b..59469959 100644
--- a/BedrockServer.cpp
+++ b/BedrockServer.cpp
@@ -1029,6 +1029,9 @@ void BedrockServer::runCommand(unique_ptr<BedrockCommand>&& _command, bool isBlo
                                 // don't need to lock our next commit on this page conflict.
                                 if (!SStartsWith(lastConflictTable, "journal")) {
                                     lastConflictPage = db.getLastConflictPage();
+                                    SINFO(format("Not skipping lock on journal table {}", lastConflictTable));
+                                } else {
+                                    SINFO(format("Skipping lock on journal table {}", lastConflictTable));
                                 }
                             }
                         }
diff --git a/test/clustertest/BedrockClusterTester.h b/test/clustertest/BedrockClusterTester.h
index d8d11bc4..fb8dbe54 100644
--- a/test/clustertest/BedrockClusterTester.h
+++ b/test/clustertest/BedrockClusterTester.h
@@ -129,6 +129,7 @@ ClusterTester<T>::ClusterTester(ClusterSize size,
             {"-peerList", peerString},
             {"-plugins", pluginsToLoad},
             {"-overrideProcessName", "bedrock" + to_string(nodePort)},
+            {"-enableConflictPageLocks", "true"},
         };
 
         // Now, if any args were supplied, we'll use those instead of these ones (note that this overwrites our

And confirmed that the logs were successfully doing what we wanted them to do:

2024-09-30T19:04:37.866701+00:00 expensidev2004 bedrock10013: xGhV1t (BedrockServer.cpp:1032) runCommand [socket27] [info] Not skipping lock on journal table test
2024-09-30T19:04:37.866977+00:00 expensidev2004 bedrock10013: QZeN1R (BedrockServer.cpp:1034) runCommand [socket32] [info] Skipping lock on journal table journal0004
2024-09-30T19:04:37.867260+00:00 expensidev2004 bedrock10013: DHoH5U (BedrockServer.cpp:1032) runCommand [socket5] [info] Not skipping lock on journal table test

Internal Testing Reminder: when changing bedrock, please compile auth against your new changes

danieldoglas · 2024-09-30T17:11:49Z

sqlitecluster/SQLite.cpp

+        const char* tableOffset = strstr(zMsg, "part of db table") + 17;
+
+        // Check if the tableOffset exists since not all conflicts are on tables
+        if (tableOffset) {
+            // Based on the SQLite log line, we should always have ';' after the table name,
+            // so let's use it to finish this loop.
+            int i = 0;
+            while (tableOffset[i] != ';') {
+                _conflictTable += tableOffset[i];
+                i++;
+            }
+        }


I've confirmed this works as expected:

I'm using this log line as the base for this logic: https://github.com/Expensify/Bedrock/blob/676f804ed01ac09ffaedb2f3732069f0bbf50500/libstuff/sqlite3.c#L84720

BedrockServer.cpp

tylerkaraszewski · 2024-09-30T17:17:15Z

sqlitecluster/SQLite.cpp

@@ -284,6 +285,20 @@ void SQLite::_sqliteLogCallback(void* pArg, int iErrCode, const char* zMsg) {
        // 17 is the length of "conflict at page" and the following space.
        const char* offset = strstr(zMsg, "conflict at page") + 17;
        _conflictPage = atol(offset);
+
+        // 17 is the length of "part of db table" and the following space.
+        const char* tableOffset = strstr(zMsg, "part of db table") + 17;


You can start from offset, we already know tablerOffset will be after that.

Also, renaming offset to pageOffset might be clearer.

sqlitecluster/SQLite.cpp

danieldoglas · 2024-09-30T17:44:25Z

sqlitecluster/SQLite.cpp

+
+            // Let's add this check in case the SQLite log changes and we don't notice it
+            // since this would generate a runtime error.
+            if (semicolonOffset) {


added this IF since the string initializer would generate a runtime error in case semicollongOffset was null:

I don't understand this comment. You need the if, yes, because otherwise semicolonOffset - tableOffset will be negative.

The error you posted looks like you were setting msg by hand, and that's the error you're seeing.

Below that that msg error we have a terminate called after throwing an instance of std::length-error`, I just wanted to show that it will fail if we don't have the if

tylerkaraszewski

One more suggested change.

sqlitecluster/SQLite.cpp

tylerkaraszewski · 2024-09-30T17:55:26Z

sqlitecluster/SQLite.cpp

+
+            // Let's add this check in case the SQLite log changes and we don't notice it
+            // since this would generate a runtime error.
+            if (semicolonOffset) {


I don't understand this comment. You need the if, yes, because otherwise semicolonOffset - tableOffset will be negative.

The error you posted looks like you were setting msg by hand, and that's the error you're seeing.

BedrockServer.cpp

flodnv · 2024-10-01T09:03:25Z

BedrockServer.cpp

@@ -778,6 +778,7 @@ void BedrockServer::runCommand(unique_ptr<BedrockCommand>&& _command, bool isBlo
    }

    int64_t lastConflictPage = 0;
+    string lastConflictTable;


Why is this defined so far up?

danieldoglas added 7 commits September 30, 2024 16:35

adding new properties

33a90a3

adding new properties

c94484a

adding new functions

7eeddaf

checking for commit table before locking

0d84e65

removing vscode settings folder from commitable files

56abe2b

removing vs code folder

ec7a49d

capturing table name from log

676f804

danieldoglas self-assigned this Sep 30, 2024

danieldoglas changed the title ~~Removing conflicts checks on journals~~ Removing conflict check when the last conflict happened on a journal Sep 30, 2024

danieldoglas commented Sep 30, 2024

View reviewed changes

flodnv requested changes Sep 30, 2024

View reviewed changes

BedrockServer.cpp Show resolved Hide resolved

tylerkaraszewski requested changes Sep 30, 2024

View reviewed changes

danieldoglas added 6 commits September 30, 2024 17:30

reseting _conflictTable and fixing logs

5c99651

adding comment

48f8345

clarifying comment

1a306b2

addressing comments

786e21b

adding check if offset exists

9d35fc0

adding comment

27642d6

danieldoglas commented Sep 30, 2024

View reviewed changes

danieldoglas requested review from tylerkaraszewski and flodnv September 30, 2024 17:46

tylerkaraszewski requested changes Sep 30, 2024

View reviewed changes

tylerkaraszewski previously approved these changes Sep 30, 2024

View reviewed changes

tylerkaraszewski reviewed Sep 30, 2024

View reviewed changes

BedrockServer.cpp Outdated Show resolved Hide resolved

style change

9fcee06

danieldoglas dismissed tylerkaraszewski’s stale review via 9fcee06 September 30, 2024 18:30

tylerkaraszewski previously approved these changes Sep 30, 2024

View reviewed changes

adding enableConflictPageLocks on cluster test

202c584

danieldoglas dismissed tylerkaraszewski’s stale review via 202c584 September 30, 2024 19:16

tylerkaraszewski approved these changes Sep 30, 2024

View reviewed changes

flodnv approved these changes Oct 1, 2024

View reviewed changes

flodnv merged commit 294538c into main Oct 1, 2024
1 check passed

flodnv deleted the dsilva_removingConflictsChecksOnJournals branch October 1, 2024 09:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing conflict check when the last conflict happened on a journal #1882

Removing conflict check when the last conflict happened on a journal #1882

danieldoglas commented Sep 30, 2024 •

edited

Loading

danieldoglas Sep 30, 2024

danieldoglas Sep 30, 2024

tylerkaraszewski Sep 30, 2024

danieldoglas Sep 30, 2024

danieldoglas Sep 30, 2024

tylerkaraszewski Sep 30, 2024

danieldoglas Sep 30, 2024

tylerkaraszewski left a comment

tylerkaraszewski Sep 30, 2024

flodnv Oct 1, 2024

	PageLockGuard pageLock(lastConflictPage);
	if (lastConflictPage) {
	SINFO("Waited " << (STimeNow() - conflictLockStartTime) << "us for lock on db page " << lastConflictPage << ".");
	}

Removing conflict check when the last conflict happened on a journal #1882

Removing conflict check when the last conflict happened on a journal #1882

Conversation

danieldoglas commented Sep 30, 2024 • edited Loading

Details

Fixed Issues

Tests

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

tylerkaraszewski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieldoglas commented Sep 30, 2024 •

edited

Loading