SERVER-94306 Move profile settings out of CollectionCatalog and impro…

…ve concurrency model (#26781) GitOrigin-RevId: 62be3adbe6e147166576e7f51c60829ddbe01f15
lakamson · Sep 19, 2024 · 0750c68 · 0750c68
1 parent 4b1a9e7
commit 0750c68
Show file tree

Hide file tree

Showing 56 changed files with 721 additions and 424 deletions.
diff --git a/etc/performance_thresholds.yml b/etc/performance_thresholds.yml
@@ -50,12 +50,12 @@ tests:
       - thread_level: 1
         metrics:
           - name: instructions_per_iteration_mean
-            value: 20759
+            value: 20106
             bound_direction: upper
       - thread_level: 32
         metrics:
           - name: instructions_per_iteration_mean
-            value: 20783
+            value: 20132
             bound_direction: upper
   ServiceEntryPointShardRoleBenchmarkFixture/BM_SEP_PING:
     al2023-arm64-sep-benchmark:
@@ -65,10 +65,10 @@ tests:
       - thread_level: 1
         metrics:
           - name: instructions_per_iteration_mean
-            value: 24034
+            value: 23232
             bound_direction: upper
       - thread_level: 32
         metrics:
           - name: instructions_per_iteration_mean
-            value: 24113
+            value: 23262
             bound_direction: upper
diff --git a/src/mongo/db/BUILD.bazel b/src/mongo/db/BUILD.bazel
@@ -763,6 +763,7 @@ mongo_cc_library(
         ":generic_cursor",
         ":index_commands_idl",  # TODO(SERVER-93876): Remove.
         ":prepare_conflict_tracker",
+        ":profile_settings",
         ":server_base",  # TODO(SERVER-93876): Remove.
         ":server_feature_flags",  # TODO(SERVER-93876): Remove.
         "//src/mongo/bson/mutable:mutable_bson",
@@ -1179,15 +1180,16 @@ idl_generator(
 )
 
 mongo_cc_library(
-    name = "profile_filter",
+    name = "profile_settings",
     srcs = [
-        "profile_filter.cpp",
+        "profile_settings.cpp",
     ],
     hdrs = [
         "profile_filter.h",
+        "profile_settings.h",
     ],
     deps = [
-        "//src/mongo:base",
+        "//src/mongo/db:server_base",
     ],
 )
 

diff --git a/src/mongo/db/SConscript b/src/mongo/db/SConscript
@@ -101,7 +101,17 @@ env.Benchmark(
     LIBDEPS=[
         "$BUILD_DIR/mongo/db/commands/profile_common",
         "$BUILD_DIR/mongo/util/processinfo",
-        "profile_filter",
+        "profile_settings",
+    ],
+)
+
+env.CppUnitTest(
+    target="profile_settings_test",
+    source=[
+        "profile_settings_test.cpp",
+    ],
+    LIBDEPS=[
+        "profile_settings",
     ],
 )
 
@@ -443,6 +453,7 @@ env.Library(
     LIBDEPS=[
         "catalog/collection_catalog",
         "catalog/database_holder",
+        "profile_settings",
         "shard_role_api",
     ],
     LIBDEPS_PRIVATE=[
@@ -1680,6 +1691,7 @@ env.Library(
         "change_streams_cluster_parameter",
         "commands/mongod",
         "commands/mongod_fsync",
+        "commands/profile_common",
         "commands/test_commands",
         "concurrency/flow_control_ticketholder",
         "fle_crud_mongod",

diff --git a/src/mongo/db/catalog/SConscript b/src/mongo/db/catalog/SConscript
@@ -182,7 +182,6 @@ env.Library(
         "$BUILD_DIR/mongo/db/audit",
         "$BUILD_DIR/mongo/db/concurrency/lock_manager",
         "$BUILD_DIR/mongo/db/index/index_access_method",
-        "$BUILD_DIR/mongo/db/profile_filter",
         "$BUILD_DIR/mongo/db/query/collation/collator_factory_interface",
         "$BUILD_DIR/mongo/db/query/query_stats/query_stats",
         "$BUILD_DIR/mongo/db/server_base",

diff --git a/src/mongo/db/catalog/collection_catalog.cpp b/src/mongo/db/catalog/collection_catalog.cpp
@@ -2138,35 +2138,6 @@ std::vector<DatabaseName> CollectionCatalog::getAllConsistentDbNamesForTenant(
     return ret;
 }
 
-void CollectionCatalog::setAllDatabaseProfileFilters(std::shared_ptr<ProfileFilter> filter) {
-    auto dbProfileSettingsWriter = _databaseProfileSettings.transient();
-    for (const auto& [dbName, settings] : _databaseProfileSettings) {
-        ProfileSettings clone = settings;
-        clone.filter = filter;
-        dbProfileSettingsWriter.set(dbName, std::move(clone));
-    }
-    _databaseProfileSettings = dbProfileSettingsWriter.persistent();
-}
-
-void CollectionCatalog::setDatabaseProfileSettings(
-    const DatabaseName& dbName, CollectionCatalog::ProfileSettings newProfileSettings) {
-    _databaseProfileSettings = _databaseProfileSettings.set(dbName, std::move(newProfileSettings));
-}
-
-CollectionCatalog::ProfileSettings CollectionCatalog::getDatabaseProfileSettings(
-    const DatabaseName& dbName) const {
-    const ProfileSettings* settings = _databaseProfileSettings.find(dbName);
-    if (settings) {
-        return *settings;
-    }
-
-    return {serverGlobalParams.defaultProfile, ProfileFilter::getDefault()};
-}
-
-void CollectionCatalog::clearDatabaseProfileSettings(const DatabaseName& dbName) {
-    _databaseProfileSettings = _databaseProfileSettings.erase(dbName);
-}
-
 void CollectionCatalog::addDropPending(const DatabaseName& dbName) {
     _dropPendingDatabases = _dropPendingDatabases.insert(dbName);
 }

diff --git a/src/mongo/db/catalog/collection_catalog.h b/src/mongo/db/catalog/collection_catalog.h
@@ -53,7 +53,6 @@
 #include "mongo/db/database_name.h"
 #include "mongo/db/namespace_string.h"
 #include "mongo/db/operation_context.h"
-#include "mongo/db/profile_filter.h"
 #include "mongo/db/service_context.h"
 #include "mongo/db/storage/durable_catalog_entry.h"
 #include "mongo/db/tenant_id.h"
@@ -112,26 +111,6 @@ class CollectionCatalog {
         DatabaseName _dbName;
     };
 
-    struct ProfileSettings {
-        int level;
-        std::shared_ptr<const ProfileFilter> filter;  // nullable
-
-        ProfileSettings(int level, std::shared_ptr<ProfileFilter> filter)
-            : level(level), filter(filter) {
-            // ProfileSettings represents a state, not a request to change the state.
-            // -1 is not a valid profiling level: it is only used in requests, to represent
-            // leaving the state unchanged.
-            invariant(0 <= level && level <= 2,
-                      str::stream() << "Invalid profiling level: " << level);
-        }
-
-        ProfileSettings() = default;
-
-        bool operator==(const ProfileSettings& other) const {
-            return level == other.level && filter == other.filter;
-        }
-    };
-
     /**
      * Returns a CollectionCatalog instance capable of returning Collection instances consistent
      * with the storage snapshot. Is the same as latest() below if no snapshot is opened.
@@ -588,40 +567,6 @@ class CollectionCatalog {
     std::vector<DatabaseName> getAllConsistentDbNamesForTenant(
         OperationContext* opCtx, boost::optional<TenantId> tenantId) const;
 
-    /**
-     * Updates the profile filter on all databases with non-default settings.
-     */
-    void setAllDatabaseProfileFilters(std::shared_ptr<ProfileFilter> filter);
-
-    /**
-     * Sets 'newProfileSettings' as the profiling settings for the database 'dbName'.
-     */
-    void setDatabaseProfileSettings(const DatabaseName& dbName, ProfileSettings newProfileSettings);
-
-    /**
-     * Fetches the profiling settings for database 'dbName'.
-     *
-     * Returns the server's default database profile settings if the database does not exist.
-     */
-    ProfileSettings getDatabaseProfileSettings(const DatabaseName& dbName) const;
-
-    /**
-     * Fetches the profiling level for database 'dbName'.
-     *
-     * Returns the server's default database profile settings if the database does not exist.
-     *
-     * There is no corresponding setDatabaseProfileLevel; use setDatabaseProfileSettings instead.
-     * This method only exists as a convenience.
-     */
-    int getDatabaseProfileLevel(const DatabaseName& dbName) const {
-        return getDatabaseProfileSettings(dbName).level;
-    }
-
-    /**
-     * Clears the database profile settings entry for 'dbName'.
-     */
-    void clearDatabaseProfileSettings(const DatabaseName& dbName);
-
     /**
      * Marks the given database as drop pending.
      */
@@ -887,7 +832,6 @@ class CollectionCatalog {
     using NamespaceCollectionMap =
         immutable::unordered_map<NamespaceString, std::shared_ptr<Collection>>;
     using UncommittedViewsSet = immutable::unordered_set<NamespaceString>;
-    using DatabaseProfileSettingsMap = immutable::unordered_map<DatabaseName, ProfileSettings>;
     using ViewsForDatabaseMap = immutable::unordered_map<DatabaseName, ViewsForDatabase>;
 
     CollectionCatalogMap _catalog;
@@ -931,13 +875,6 @@ class CollectionCatalog {
     // global lock in at least MODE_IS to read it.
     uint64_t _epoch = 0;
 
-    /**
-     * Contains non-default database profile settings. New collections, current collections and
-     * views must all be able to access the correct profile settings for the database in which they
-     * reside. Simple database name to struct ProfileSettings map.
-     */
-    DatabaseProfileSettingsMap _databaseProfileSettings;
-
     // Tracks usage of collection usage features (e.g. capped).
     Stats _stats;
 };

diff --git a/src/mongo/db/catalog/collection_catalog_test.cpp b/src/mongo/db/catalog/collection_catalog_test.cpp
@@ -686,30 +686,6 @@ TEST_F(CollectionCatalogTest, GetAllTenants) {
     catalog.deregisterAllCollectionsAndViews(getServiceContext());
 }
 
-// Test setting and fetching the profile level for a database.
-TEST_F(CollectionCatalogTest, DatabaseProfileLevel) {
-    DatabaseName testDBNameFirst =
-        DatabaseName::createDatabaseName_forTest(boost::none, "testdbfirst");
-    DatabaseName testDBNameSecond =
-        DatabaseName::createDatabaseName_forTest(boost::none, "testdbsecond");
-
-    // Requesting a profile level that is not in the _databaseProfileLevel map should return the
-    // default server-wide setting
-    ASSERT_EQ(catalog.getDatabaseProfileSettings(testDBNameFirst).level,
-              serverGlobalParams.defaultProfile);
-    // Setting the default profile level should have not change the result.
-    catalog.setDatabaseProfileSettings(testDBNameFirst,
-                                       {serverGlobalParams.defaultProfile, nullptr});
-    ASSERT_EQ(catalog.getDatabaseProfileSettings(testDBNameFirst).level,
-              serverGlobalParams.defaultProfile);
-
-    // Changing the profile level should make fetching it different.
-    catalog.setDatabaseProfileSettings(testDBNameSecond,
-                                       {serverGlobalParams.defaultProfile + 1, nullptr});
-    ASSERT_EQ(catalog.getDatabaseProfileSettings(testDBNameSecond).level,
-              serverGlobalParams.defaultProfile + 1);
-}
-
 TEST_F(CollectionCatalogTest, DropPendingDatabase) {
     auto dbName = DatabaseName::createDatabaseName_forTest(boost::none, "DropPendingDatabase");
     ASSERT_FALSE(catalog.isDropPending(dbName));

diff --git a/src/mongo/db/catalog/create_collection.cpp b/src/mongo/db/catalog/create_collection.cpp
@@ -74,6 +74,7 @@
 #include "mongo/db/op_observer/op_observer.h"
 #include "mongo/db/operation_context.h"
 #include "mongo/db/pipeline/change_stream_pre_and_post_images_options_gen.h"
+#include "mongo/db/profile_settings.h"
 #include "mongo/db/query/collation/collator_factory_interface.h"
 #include "mongo/db/query/query_knobs_gen.h"
 #include "mongo/db/query/write_ops/insert.h"
@@ -292,12 +293,12 @@ Status _createView(OperationContext* opCtx,
 
         WriteUnitOfWork wunit(opCtx);
 
-        AutoStatsTracker statsTracker(
-            opCtx,
-            nss,
-            Top::LockType::NotLocked,
-            AutoStatsTracker::LogMode::kUpdateTopAndCurOp,
-            CollectionCatalog::get(opCtx)->getDatabaseProfileLevel(nss.dbName()));
+        AutoStatsTracker statsTracker(opCtx,
+                                      nss,
+                                      Top::LockType::NotLocked,
+                                      AutoStatsTracker::LogMode::kUpdateTopAndCurOp,
+                                      DatabaseProfileSettings::get(opCtx->getServiceContext())
+                                          .getDatabaseProfileLevel(nss.dbName()));
 
         // If the view creation rolls back, ensure that the Top entry created for the view is
         // deleted.
@@ -563,7 +564,8 @@ Status _createTimeseries(OperationContext* opCtx,
             bucketsNs,
             Top::LockType::NotLocked,
             AutoStatsTracker::LogMode::kUpdateTopAndCurOp,
-            CollectionCatalog::get(opCtx)->getDatabaseProfileLevel(ns.dbName()));
+            DatabaseProfileSettings::get(opCtx->getServiceContext())
+                .getDatabaseProfileLevel(ns.dbName()));
 
         // If the buckets collection and time-series view creation roll back, ensure that their
         // Top entries are deleted.
@@ -716,12 +718,12 @@ Status _createCollection(
 
         WriteUnitOfWork wunit(opCtx);
 
-        AutoStatsTracker statsTracker(
-            opCtx,
-            nss,
-            Top::LockType::NotLocked,
-            AutoStatsTracker::LogMode::kUpdateTopAndCurOp,
-            CollectionCatalog::get(opCtx)->getDatabaseProfileLevel(nss.dbName()));
+        AutoStatsTracker statsTracker(opCtx,
+                                      nss,
+                                      Top::LockType::NotLocked,
+                                      AutoStatsTracker::LogMode::kUpdateTopAndCurOp,
+                                      DatabaseProfileSettings::get(opCtx->getServiceContext())
+                                          .getDatabaseProfileLevel(nss.dbName()));
 
         // If the collection creation rolls back, ensure that the Top entry created for the
         // collection is deleted.

diff --git a/src/mongo/db/catalog/database_holder_impl.cpp b/src/mongo/db/catalog/database_holder_impl.cpp
@@ -55,6 +55,7 @@
 #include "mongo/db/namespace_string.h"
 #include "mongo/db/op_observer/op_observer.h"
 #include "mongo/db/operation_context.h"
+#include "mongo/db/profile_settings.h"
 #include "mongo/db/repl/replication_coordinator.h"
 #include "mongo/db/service_context.h"
 #include "mongo/db/stats/top.h"
@@ -240,16 +241,14 @@ void DatabaseHolderImpl::dropDb(OperationContext* opCtx, Database* db) {
         Top::get(serviceContext).collectionDropped(coll->ns());
     }
 
-    // Clean up the in-memory database state.
-    CollectionCatalog::write(
-        opCtx, [&](CollectionCatalog& catalog) { catalog.clearDatabaseProfileSettings(name); });
-
     // close() is called as part of the onCommit handler as it frees the memory pointed to by 'db'.
     // We need to keep this memory valid until the transaction successfully commits.
-    shard_role_details::getRecoveryUnit(opCtx)->onCommit(
-        [this, name = name](OperationContext* opCtx, boost::optional<Timestamp>) {
-            close(opCtx, name);
-        });
+    shard_role_details::getRecoveryUnit(opCtx)->onCommit([this,
+                                                          name = name](OperationContext* opCtx,
+                                                                       boost::optional<Timestamp>) {
+        close(opCtx, name);
+        DatabaseProfileSettings::get(opCtx->getServiceContext()).clearDatabaseProfileSettings(name);
+    });
 
     auto const storageEngine = serviceContext->getStorageEngine();
     writeConflictRetry(opCtx, "dropDatabase", NamespaceString(name), [&] {