SERVER-43858: Relax database creation lock acquisition from MODE_X to MODE_IX.

7 files changed, 24 insertions, 40 deletions

diff --git a/src/mongo/db/catalog/database_holder_impl.cpp b/src/mongo/db/catalog/database_holder_impl.cpp
index c105b6bc07b..7f75cbffe48 100644
--- a/src/mongo/db/catalog/database_holder_impl.cpp
+++ b/src/mongo/db/catalog/database_holder_impl.cpp
@@ -109,7 +109,7 @@ std::vector<std::string> DatabaseHolderImpl::getNames() {
 
 Database* DatabaseHolderImpl::openDb(OperationContext* opCtx, StringData ns, bool* justCreated) {
     const StringData dbname = _todb(ns);
-    invariant(opCtx->lockState()->isDbLockedForMode(dbname, MODE_X));
+    invariant(opCtx->lockState()->isDbLockedForMode(dbname, MODE_IX));
 
     if (justCreated)
         *justCreated = false;  // Until proven otherwise.
@@ -136,11 +136,8 @@ Database* DatabaseHolderImpl::openDb(OperationContext* opCtx, StringData ns, boo
                           << "]",
             duplicates.empty());
 
-
     // Do the catalog lookup and database creation outside of the scoped lock, because these may
-    // block. Only one thread can be inside this method for the same DB name, because of the
-    // requirement for X-lock on the database when we enter. So there is no way we can insert two
-    // different databases for the same name.
+    // block.
     lk.unlock();
 
     if (CollectionCatalog::get(opCtx).getAllCollectionUUIDsFromDb(dbname).empty()) {
@@ -156,7 +153,15 @@ Database* DatabaseHolderImpl::openDb(OperationContext* opCtx, StringData ns, boo
     removeDbGuard.dismiss();
     lk.lock();
     auto it = _dbs.find(dbname);
-    invariant(it != _dbs.end() && it->second == nullptr);
+    // Dropping a database requires a MODE_X lock, so the entry in the `_dbs` map cannot disappear.
+    invariant(it != _dbs.end());
+    if (it->second) {
+        // Creating databases only requires a DB lock in MODE_IX. Thus databases can concurrently
+        // created. If this thread "lost the race", return the database object that was persisted in
+        // the `_dbs` map.
+        return it->second;
+    }
+
     it->second = newDb.release();
     invariant(_getNamesWithConflictingCasing_inlock(dbname.toString()).empty());
 
diff --git a/src/mongo/db/catalog_raii.cpp b/src/mongo/db/catalog_raii.cpp
index 1a207786be2..41bf887d77b 100644
--- a/src/mongo/db/catalog_raii.cpp
+++ b/src/mongo/db/catalog_raii.cpp
@@ -135,18 +135,12 @@ AutoGetCollection::AutoGetCollection(OperationContext* opCtx,
 AutoGetOrCreateDb::AutoGetOrCreateDb(OperationContext* opCtx,
                                      StringData dbName,
                                      LockMode mode,
-                                     Date_t deadline) {
+                                     Date_t deadline)
+    : _autoDb(opCtx, dbName, mode, deadline) {
     invariant(mode == MODE_IX || mode == MODE_X);
 
-    _autoDb.emplace(opCtx, dbName, mode, deadline);
-    _db = _autoDb->getDb();
-
-    // If the database didn't exist, relock in MODE_X
+    _db = _autoDb.getDb();
     if (!_db) {
-        if (mode != MODE_X) {
-            _autoDb.emplace(opCtx, dbName, MODE_X, deadline);
-        }
-
         auto databaseHolder = DatabaseHolder::get(opCtx);
         _db = databaseHolder->openDb(opCtx, dbName, &_justCreated);
     }
diff --git a/src/mongo/db/catalog_raii.h b/src/mongo/db/catalog_raii.h
index f3fcae654ef..4172a54514b 100644
--- a/src/mongo/db/catalog_raii.h
+++ b/src/mongo/db/catalog_raii.h
@@ -148,8 +148,7 @@ private:
  * RAII-style class, which acquires a lock on the specified database in the requested mode and
  * obtains a reference to the database, creating it was non-existing. Used as a shortcut for
  * calls to DatabaseHolder::get(opCtx)->openDb(), taking care of locking details. The
- * requested mode must be MODE_IX or MODE_X. If the database needs to be created, the lock will
- * automatically be reacquired as MODE_X.
+ * requested mode must be MODE_IX or MODE_X.
  *
  * Use this when you are about to perform a write, and want to create the database if it doesn't
  * already exist.
@@ -176,7 +175,7 @@ public:
     }
 
 private:
-    boost::optional<AutoGetDb> _autoDb;
+    AutoGetDb _autoDb;
 
     Database* _db;
     bool _justCreated{false};
diff --git a/src/mongo/db/db_raii.cpp b/src/mongo/db/db_raii.cpp
index cdb3813f985..ede268a31f3 100644
--- a/src/mongo/db/db_raii.cpp
+++ b/src/mongo/db/db_raii.cpp
@@ -329,7 +329,6 @@ OldClientContext::OldClientContext(OperationContext* opCtx, const std::string& n
     : _opCtx(opCtx), _db(DatabaseHolder::get(opCtx)->getDb(opCtx, ns)) {
     if (!_db) {
         const auto dbName = nsToDatabaseSubstring(ns);
-        invariant(_opCtx->lockState()->isDbLockedForMode(dbName, MODE_X));
         _db = DatabaseHolder::get(opCtx)->openDb(_opCtx, dbName, &_justCreated);
         invariant(_db);
     }
diff --git a/src/mongo/db/repl/oplog_applier_impl_test.cpp b/src/mongo/db/repl/oplog_applier_impl_test.cpp
index 32df5b58f79..9338da454d6 100644
--- a/src/mongo/db/repl/oplog_applier_impl_test.cpp
+++ b/src/mongo/db/repl/oplog_applier_impl_test.cpp
@@ -117,7 +117,8 @@ void createCollection(OperationContext* opCtx,
                       const NamespaceString& nss,
                       const CollectionOptions& options) {
     writeConflictRetry(opCtx, "createCollection", nss.ns(), [&] {
-        Lock::DBLock dblk(opCtx, nss.db(), MODE_X);
+        Lock::DBLock dblk(opCtx, nss.db(), MODE_IX);
+        Lock::CollectionLock collLk(opCtx, nss, MODE_X);
         OldClientContext ctx(opCtx, nss.ns());
         auto db = ctx.db();
         ASSERT_TRUE(db);
@@ -210,7 +211,7 @@ TEST_F(OplogApplierImplTest, applyOplogEntryOrGroupedInsertsInsertDocumentCollec
     createDatabase(_opCtx.get(), nss.db());
     // Even though the collection doesn't exist, this is handled in the actual application function,
     // which in the case of this test just ignores such errors. This tests mostly that we don't
-    // implicitly create the collection and lock the database in MODE_X.
+    // implicitly create the collection.
     auto op = makeOplogEntry(OpTypeEnum::kInsert, nss, {});
     ASSERT_THROWS(_applyOplogEntryOrGroupedInsertsWrapper(
                       _opCtx.get(), &op, OplogApplication::Mode::kSecondary),
@@ -223,7 +224,7 @@ TEST_F(OplogApplierImplTest, applyOplogEntryOrGroupedInsertsDeleteDocumentCollec
     createDatabase(_opCtx.get(), nss.db());
     // Even though the collection doesn't exist, this is handled in the actual application function,
     // which in the case of this test just ignores such errors. This tests mostly that we don't
-    // implicitly create the collection and lock the database in MODE_X.
+    // implicitly create the collection.
     auto op = makeOplogEntry(OpTypeEnum::kDelete, nss, {});
     _testApplyOplogEntryOrGroupedInsertsCrudOperation(ErrorCodes::OK, op, false);
     ASSERT_FALSE(collectionExists(_opCtx.get(), nss));
@@ -279,7 +280,7 @@ TEST_F(OplogApplierImplTest, applyOplogEntryOrGroupedInsertsCommand) {
                                             const BSONObj&) {
         applyCmdCalled = true;
         ASSERT_TRUE(opCtx);
-        ASSERT_TRUE(opCtx->lockState()->isDbLockedForMode(nss.db(), MODE_X));
+        ASSERT_TRUE(opCtx->lockState()->isDbLockedForMode(nss.db(), MODE_IX));
         ASSERT_EQUALS(nss, collNss);
         return Status::OK();
     };
diff --git a/src/mongo/dbtests/dbtests.cpp b/src/mongo/dbtests/dbtests.cpp
index 64902947f37..e094c2ed223 100644
--- a/src/mongo/dbtests/dbtests.cpp
+++ b/src/mongo/dbtests/dbtests.cpp
@@ -157,22 +157,8 @@ WriteContextForTests::WriteContextForTests(OperationContext* opCtx, StringData n
     if (getCollection())
         return;
 
-    // If the database was just created, it is already locked in MODE_X so we can skip the relocking
-    // code below
-    if (_autoCreateDb->justCreated()) {
-        dassert(opCtx->lockState()->isDbLockedForMode(_nss.db(), MODE_X));
-        return;
-    }
-
-    // If the collection doesn't exists, put the context in a state where the database is locked in
-    // MODE_X so that the collection can be created
-    _clientContext.reset();
-    _collLock.reset();
-    _autoCreateDb.reset();
-    _autoCreateDb.emplace(opCtx, _nss.db(), MODE_X);
-
-    _clientContext.emplace(opCtx, _nss.ns(), _autoCreateDb->getDb());
     invariant(_autoCreateDb->getDb() == _clientContext->db());
+    _collLock.emplace(opCtx, _nss, MODE_X);
 }
 
 }  // namespace dbtests
diff --git a/src/mongo/dbtests/dbtests.h b/src/mongo/dbtests/dbtests.h
index f0b0166aecb..b40c46b8fb1 100644
--- a/src/mongo/dbtests/dbtests.h
+++ b/src/mongo/dbtests/dbtests.h
@@ -65,8 +65,8 @@ Status createIndexFromSpec(OperationContext* opCtx, StringData ns, const BSONObj
 /**
  * Combines AutoGetOrCreateDb and OldClientContext. If the requested 'ns' exists, the constructed
  * object will have both the database and the collection locked in MODE_IX. Otherwise, the database
- * will be locked in MODE_X and will be created (note, only the database will be created, but not
- * the collection).
+ * will be locked in MODE_IX and will be created, while the collection will be locked in MODE_X, but
+ * not created.
  */
 class WriteContextForTests {
     WriteContextForTests(const WriteContextForTests&) = delete;