From 1a12c44bf9f6c907bc1580af56c73a654cb269bc Mon Sep 17 00:00:00 2001 From: Florent De Neve Date: Thu, 9 Jan 2025 13:35:12 -0400 Subject: [PATCH 1/4] Don't page lock on onyx tables --- BedrockServer.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/BedrockServer.cpp b/BedrockServer.cpp index c0cad65f3..f9b76479e 100644 --- a/BedrockServer.cpp +++ b/BedrockServer.cpp @@ -1042,10 +1042,10 @@ void BedrockServer::runCommand(unique_ptr&& _command, bool isBlo if (_enableConflictPageLocks) { lastConflictTable = db.getLastConflictTable(); - // Journals are always chosen at the time of commit. So in case there was a conflict on the journal in - // the previous commit, the chances are very low (1/192) that we'll choose the same journal, thus, we + // Journals and Onyx tables are always chosen at the time of commit. So in case there was a conflict on these tables in + // the previous commit, the chances are very low that we'll choose the same table, thus, we // don't need to lock our next commit on this page conflict. - if (!SStartsWith(lastConflictTable, "journal")) { + if (!SStartsWith(lastConflictTable, "journal") && !SStartsWith(lastConflictTable, "onyx")) { lastConflictPage = db.getLastConflictPage(); } } From 2b0762d6b4a1026ec4d625688518cdc382a37956 Mon Sep 17 00:00:00 2001 From: Florent De'Neve Date: Thu, 9 Jan 2025 14:52:04 -0400 Subject: [PATCH 2/4] Update BedrockServer.cpp Co-authored-by: Ionatan Wiznia --- BedrockServer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/BedrockServer.cpp b/BedrockServer.cpp index f9b76479e..c4f32c34a 100644 --- a/BedrockServer.cpp +++ b/BedrockServer.cpp @@ -1045,7 +1045,7 @@ void BedrockServer::runCommand(unique_ptr&& _command, bool isBlo // Journals and Onyx tables are always chosen at the time of commit. So in case there was a conflict on these tables in // the previous commit, the chances are very low that we'll choose the same table, thus, we // don't need to lock our next commit on this page conflict. - if (!SStartsWith(lastConflictTable, "journal") && !SStartsWith(lastConflictTable, "onyx")) { + if (!SStartsWith(lastConflictTable, "journal") && !SStartsWith(lastConflictTable, "onyxUpdates_")) { lastConflictPage = db.getLastConflictPage(); } } From a457f093d046d71cb8750c69b8e9b51df7455e6f Mon Sep 17 00:00:00 2001 From: Florent De Neve Date: Tue, 14 Jan 2025 16:21:15 -0400 Subject: [PATCH 3/4] Expose shouldLockCommitPageOnTableConflict to plugins --- BedrockCommand.cpp | 4 ++++ BedrockCommand.h | 4 +++- BedrockPlugin.cpp | 4 ++++ BedrockPlugin.h | 3 +++ BedrockServer.cpp | 7 ++++--- 5 files changed, 18 insertions(+), 4 deletions(-) diff --git a/BedrockCommand.cpp b/BedrockCommand.cpp index bd533310f..b39b95d79 100644 --- a/BedrockCommand.cpp +++ b/BedrockCommand.cpp @@ -57,6 +57,10 @@ const string& BedrockCommand::getName() const { return defaultPluginName; } +const BedrockPlugin* BedrockCommand::getPlugin() const { + return _plugin; +} + int64_t BedrockCommand::_getTimeout(const SData& request, const uint64_t scheduledTime) { // Timeout is the default, unless explicitly supplied, or if Connection: forget is set. int64_t timeout = DEFAULT_TIMEOUT; diff --git a/BedrockCommand.h b/BedrockCommand.h index 8beb3a96b..e14e77959 100644 --- a/BedrockCommand.h +++ b/BedrockCommand.h @@ -85,6 +85,8 @@ class BedrockCommand : public SQLiteCommand { // Return the name of the plugin for this command. const string& getName() const; + const BedrockPlugin* getPlugin() const; + // Take all of the HTTPS requests attached to this object, and serialize them to a string. string serializeHTTPSRequests(); @@ -99,7 +101,7 @@ class BedrockCommand : public SQLiteCommand { } // Bedrock will call this before writing to the database after it has prepared a transaction for each plugin to allow it to - // enable a handler function for prepare If a plugin would like to perform operations after prepare but before commit, this should + // enable a handler function for prepare If a plugin would like to perform operations after prepare but before commit, this should // return true, and it should set the prepareHandler it would like to use. virtual bool shouldEnableOnPrepareNotification(const SQLite& db, void (**onPrepareHandler)(SQLite& _db, int64_t tableID)) { return false; diff --git a/BedrockPlugin.cpp b/BedrockPlugin.cpp index 007e568d1..c57c634bf 100644 --- a/BedrockPlugin.cpp +++ b/BedrockPlugin.cpp @@ -69,3 +69,7 @@ bool BedrockPlugin::preventAttach() { void BedrockPlugin::timerFired(SStopwatch* timer) {} void BedrockPlugin::upgradeDatabase(SQLite& db) {} + +bool BedrockPlugin::shouldLockCommitPageOnTableConflict(const string& tableName) const { + return true; +} diff --git a/BedrockPlugin.h b/BedrockPlugin.h index 90f3757b6..ad9943c03 100644 --- a/BedrockPlugin.h +++ b/BedrockPlugin.h @@ -78,6 +78,9 @@ class BedrockPlugin { // Called when the sync thread is finishing, before destroying DB handles. virtual void serverStopping() {} + // Should a conflict on the given tableName result in locking the associated database page when we try to commit again? + virtual bool shouldLockCommitPageOnTableConflict(const string& tableName) const; + // Map of plugin names to functions that will return a new plugin of the given type. static map> g_registeredPluginList; diff --git a/BedrockServer.cpp b/BedrockServer.cpp index c4f32c34a..af506b572 100644 --- a/BedrockServer.cpp +++ b/BedrockServer.cpp @@ -1042,10 +1042,11 @@ void BedrockServer::runCommand(unique_ptr&& _command, bool isBlo if (_enableConflictPageLocks) { lastConflictTable = db.getLastConflictTable(); - // Journals and Onyx tables are always chosen at the time of commit. So in case there was a conflict on these tables in - // the previous commit, the chances are very low that we'll choose the same table, thus, we + // Journals are always chosen at the time of commit. So in case there was a conflict on the journal in + // the previous commit, the chances are very low that we'll choose the same journal, thus, we // don't need to lock our next commit on this page conflict. - if (!SStartsWith(lastConflictTable, "journal") && !SStartsWith(lastConflictTable, "onyxUpdates_")) { + // Plugins may define other tables on which we should not lock our next commit. + if (!SStartsWith(lastConflictTable, "journal") && command->getPlugin()->shouldLockCommitPageOnTableConflict(lastConflictTable)) { lastConflictPage = db.getLastConflictPage(); } } From 4a1382c707466a3bce7b30cf0512cf4c6714c06d Mon Sep 17 00:00:00 2001 From: Florent De Neve Date: Thu, 16 Jan 2025 17:05:56 -0400 Subject: [PATCH 4/4] Protect against nullptr --- BedrockServer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/BedrockServer.cpp b/BedrockServer.cpp index af506b572..a779f41c1 100644 --- a/BedrockServer.cpp +++ b/BedrockServer.cpp @@ -1046,7 +1046,7 @@ void BedrockServer::runCommand(unique_ptr&& _command, bool isBlo // the previous commit, the chances are very low that we'll choose the same journal, thus, we // don't need to lock our next commit on this page conflict. // Plugins may define other tables on which we should not lock our next commit. - if (!SStartsWith(lastConflictTable, "journal") && command->getPlugin()->shouldLockCommitPageOnTableConflict(lastConflictTable)) { + if (!SStartsWith(lastConflictTable, "journal") && (command->getPlugin() == nullptr || command->getPlugin()->shouldLockCommitPageOnTableConflict(lastConflictTable))) { lastConflictPage = db.getLastConflictPage(); } }