SERVER-39517 Only use Collection MODE_X for index creation and drop

8 files changed, 103 insertions, 86 deletions

diff --git a/jstests/core/txns/indexing_not_blocked_by_txn.js b/jstests/core/txns/indexing_not_blocked_by_txn.js
new file mode 100644
index 00000000000..47bfaebe83d
--- /dev/null
+++ b/jstests/core/txns/indexing_not_blocked_by_txn.js
@@ -0,0 +1,37 @@
+/*
+ * This test ensures that creating and dropping an index on a collection
+ * does not conflict with multi-statement transactions on different collections
+ * as a result of taking strong database locks. Additionally tests that a
+ * createIndexes for an index that already exist does not conflict with an open
+ * transaction on that same collection.
+ * @tags: [uses_transactions, assumes_unsharded_collection]
+ */
+(function() {
+    "use strict";
+    var dbName = 'indexing_not_blocked_by_txn';
+    var mydb = db.getSiblingDB(dbName);
+    const wcMajority = {writeConcern: {w: "majority"}};
+
+    mydb.foo.drop(wcMajority);
+    mydb.bar.drop(wcMajority);
+    assert.commandWorked(mydb.createCollection("foo", wcMajority));
+    assert.commandWorked(mydb.foo.createIndex({x: 1}));
+    assert.commandWorked(mydb.createCollection("bar", wcMajority));
+
+    var session = db.getMongo().startSession();
+    var sessionDb = session.getDatabase(dbName);
+
+    session.startTransaction();
+    assert.commandWorked(sessionDb.foo.insert({x: 1}));
+
+    // Creating already existing index is a no-op that shouldn't take strong locks.
+    assert.commandWorked(mydb.foo.createIndex({x: 1}));
+
+    // Creating an index on a different collection should not conflict.
+    assert.commandWorked(mydb.bar.createIndex({x: 1}));
+    // Dropping shouldn't either.
+    assert.commandWorked(mydb.bar.dropIndex({x: 1}));
+
+    assert.commandWorked(session.commitTransaction_forTesting());
+    session.endSession();
+}());
diff --git a/src/mongo/db/catalog/drop_indexes.cpp b/src/mongo/db/catalog/drop_indexes.cpp
index 56143447308..ef6074eaaff 100644
--- a/src/mongo/db/catalog/drop_indexes.cpp
+++ b/src/mongo/db/catalog/drop_indexes.cpp
@@ -200,7 +200,7 @@ Status dropIndexes(OperationContext* opCtx,
                    const BSONObj& cmdObj,
                    BSONObjBuilder* result) {
     return writeConflictRetry(opCtx, "dropIndexes", nss.db(), [opCtx, &nss, &cmdObj, result] {
-        AutoGetDb autoDb(opCtx, nss.db(), MODE_X);
+        AutoGetCollection autoColl(opCtx, nss, MODE_IX, MODE_X);
 
         bool userInitiatedWritesAndNotPrimary = opCtx->writesAreReplicated() &&
             !repl::ReplicationCoordinator::get(opCtx)->canAcceptWritesFor(opCtx, nss);
@@ -215,9 +215,9 @@ Status dropIndexes(OperationContext* opCtx,
         }
 
         // If db/collection does not exist, short circuit and return.
-        Database* db = autoDb.getDb();
-        Collection* collection = db ? db->getCollection(opCtx, nss) : nullptr;
-        if (!db || !collection) {
+        Database* db = autoColl.getDb();
+        Collection* collection = autoColl.getCollection();
+        if (!collection) {
             if (db && ViewCatalog::get(db)->lookup(opCtx, nss.ns())) {
                 return Status(ErrorCodes::CommandNotSupportedOnView,
                               str::stream() << "Cannot drop indexes on view " << nss);
diff --git a/src/mongo/db/catalog/index_build_block.cpp b/src/mongo/db/catalog/index_build_block.cpp
index af1bd77af68..46231723f62 100644
--- a/src/mongo/db/catalog/index_build_block.cpp
+++ b/src/mongo/db/catalog/index_build_block.cpp
@@ -50,7 +50,7 @@ IndexCatalogImpl::IndexBuildBlock::IndexBuildBlock(IndexCatalogImpl* catalog,
                                                    const NamespaceString& nss,
                                                    const BSONObj& spec,
                                                    IndexBuildMethod method)
-    : _catalog(catalog), _ns(nss.ns()), _spec(spec.getOwned()), _method(method), _entry(nullptr) {}
+    : _catalog(catalog), _nss(nss), _spec(spec.getOwned()), _method(method), _entry(nullptr) {}
 
 void IndexCatalogImpl::IndexBuildBlock::deleteTemporaryTables(OperationContext* opCtx) {
     if (_indexBuildInterceptor) {
@@ -137,14 +137,7 @@ void IndexCatalogImpl::IndexBuildBlock::fail(OperationContext* opCtx,
     invariant(opCtx->lockState()->inAWriteUnitOfWork());
     fassert(17204, collection->ok());  // defensive
 
-    NamespaceString ns(_indexNamespace);
-    // TODO(SERVER-39520): Once createCollection does not need database IX lock, 'system.views' will
-    // be no longer a special case.
-    if (ns.coll().startsWith(NamespaceString::kSystemDotViewsCollectionName)) {
-        invariant(opCtx->lockState()->isDbLockedForMode(ns.db(), MODE_IX));
-    } else {
-        invariant(opCtx->lockState()->isDbLockedForMode(ns.db(), MODE_X));
-    }
+    invariant(opCtx->lockState()->isCollectionLockedForMode(_nss, MODE_X));
 
     if (_entry) {
         invariant(_catalog->_dropIndex(opCtx, _entry).isOK());
@@ -161,14 +154,7 @@ void IndexCatalogImpl::IndexBuildBlock::success(OperationContext* opCtx, Collect
     invariant(opCtx->lockState()->inAWriteUnitOfWork());
 
     fassert(17207, collection->ok());
-    NamespaceString ns(_indexNamespace);
-    // TODO(SERVER-39520): Once createCollection does not need database IX lock, 'system.views' will
-    // be no longer a special case.
-    if (ns.coll().startsWith(NamespaceString::kSystemDotViewsCollectionName)) {
-        invariant(opCtx->lockState()->isDbLockedForMode(ns.db(), MODE_IX));
-    } else {
-        invariant(opCtx->lockState()->isDbLockedForMode(ns.db(), MODE_X));
-    }
+    invariant(opCtx->lockState()->isCollectionLockedForMode(_nss, MODE_X));
 
     if (_indexBuildInterceptor) {
         // An index build should never be completed with writes remaining in the interceptor.
@@ -178,7 +164,7 @@ void IndexCatalogImpl::IndexBuildBlock::success(OperationContext* opCtx, Collect
         invariant(_indexBuildInterceptor->areAllConstraintsChecked(opCtx));
     }
 
-    log() << "index build: done building index " << _indexName << " on ns " << _ns;
+    log() << "index build: done building index " << _indexName << " on ns " << _nss;
 
     collection->indexBuildSuccess(opCtx, _entry);
 
diff --git a/src/mongo/db/catalog/index_catalog_impl.h b/src/mongo/db/catalog/index_catalog_impl.h
index 05d790b46b3..f7736b642fa 100644
--- a/src/mongo/db/catalog/index_catalog_impl.h
+++ b/src/mongo/db/catalog/index_catalog_impl.h
@@ -298,7 +298,7 @@ public:
 
     private:
         IndexCatalogImpl* const _catalog;
-        const std::string _ns;
+        const NamespaceString& _nss;
 
         BSONObj _spec;
         IndexBuildMethod _method;
diff --git a/src/mongo/db/commands/create_indexes.cpp b/src/mongo/db/commands/create_indexes.cpp
index 5156edad90b..368298d286c 100644
--- a/src/mongo/db/commands/create_indexes.cpp
+++ b/src/mongo/db/commands/create_indexes.cpp
@@ -265,32 +265,31 @@ bool runCreateIndexes(OperationContext* opCtx,
         return true;
     };
 
-    // Before potentially taking an exclusive database lock, check if all indexes already exist
-    // while holding an intent lock. Only continue if new indexes need to be built and the
-    // database should be re-locked in exclusive mode.
+    // Before potentially taking an exclusive database or collection lock, check if all indexes
+    // already exist while holding an intent lock. Only continue if new indexes need to be built
+    // and the collection or database should be re-locked in exclusive mode.
     {
-        AutoGetCollection autoColl(opCtx, ns, MODE_IX);
-        if (auto collection = autoColl.getCollection()) {
+        boost::optional<AutoGetCollection> autoColl(boost::in_place_init, opCtx, ns, MODE_IX);
+        if (auto collection = autoColl->getCollection()) {
             auto specsCopy = resolveDefaultsAndRemoveExistingIndexes(opCtx, collection, specs);
             if (specsCopy.size() == 0) {
                 return indexesAlreadyExist(collection->getIndexCatalog()->numIndexesTotal(opCtx));
             }
+        } else {
+            // We'll need to create a collection and maybe even a new database. Temporarily
+            // releases the Database lock while holding a Global IX lock. This prevents replication
+            // from changing, but requires abandoning the current snapshot in case indexes change
+            // during the period of time where no database lock is held.
+            autoColl.reset();
+            opCtx->recoveryUnit()->abandonSnapshot();
+            dbLock.relockWithMode(MODE_X);
         }
     }
 
-    // Relocking temporarily releases the Database lock while holding a Global IX lock. This
-    // prevents the replication state from changing, but requires abandoning the current
-    // snapshot in case indexes change during the period of time where no database lock is held.
-    opCtx->recoveryUnit()->abandonSnapshot();
-    dbLock.relockWithMode(MODE_X);
-
-    // Allow the strong lock acquisition above to be interrupted, but from this point forward do
-    // not allow locks or re-locks to be interrupted.
-    UninterruptibleLockGuard noInterrupt(opCtx->lockState());
-
     auto databaseHolder = DatabaseHolder::get(opCtx);
     auto db = databaseHolder->getDb(opCtx, ns.db());
     if (!db) {
+        invariant(opCtx->lockState()->isDbLockedForMode(ns.db(), MODE_X));
         db = databaseHolder->openDb(opCtx, ns.db());
     }
 
@@ -300,10 +299,18 @@ bool runCreateIndexes(OperationContext* opCtx,
         dss.checkDbVersion(opCtx, dssLock);
     }
 
+    opCtx->recoveryUnit()->abandonSnapshot();
+    boost::optional<Lock::CollectionLock> exclusiveCollectionLock(
+        boost::in_place_init, opCtx, ns.ns(), MODE_X);
+
+    // Allow the strong lock acquisition above to be interrupted, but from this point forward do
+    // not allow locks or re-locks to be interrupted.
+    UninterruptibleLockGuard noInterrupt(opCtx->lockState());
+
     Collection* collection = db->getCollection(opCtx, ns);
-    if (collection) {
-        result.appendBool("createdCollectionAutomatically", false);
-    } else {
+    bool createCollectionAutomatically = collection == nullptr;
+    result.appendBool("createdCollectionAutomatically", createCollectionAutomatically);
+    if (createCollectionAutomatically) {
         if (ViewCatalog::get(db)->lookup(opCtx, ns.ns())) {
             errmsg = "Cannot create indexes on a view";
             uasserted(ErrorCodes::CommandNotSupportedOnView, errmsg);
@@ -317,7 +324,6 @@ bool runCreateIndexes(OperationContext* opCtx,
             invariant(collection);
             wunit.commit();
         });
-        result.appendBool("createdCollectionAutomatically", true);
     }
 
     // Use AutoStatsTracker to update Top.
@@ -354,6 +360,10 @@ bool runCreateIndexes(OperationContext* opCtx,
 
     // The 'indexer' can throw, so ensure the build cleanup occurs.
     ON_BLOCK_EXIT([&] {
+        if (!exclusiveCollectionLock) {
+            opCtx->recoveryUnit()->abandonSnapshot();
+            exclusiveCollectionLock.emplace(opCtx, ns.ns(), MODE_X);
+        }
         if (MONGO_FAIL_POINT(leaveIndexBuildUnfinishedForShutdown)) {
             // Set a flag to leave the persisted index build state intact when cleanUpAfterBuild()
             // is called below. The index build will be found on server startup.
@@ -363,7 +373,6 @@ bool runCreateIndexes(OperationContext* opCtx,
             // commit() clears the state.
             indexer.abortWithoutCleanup(opCtx);
         }
-        invariant(opCtx->lockState()->isDbLockedForMode(dbname, MODE_X));
         indexer.cleanUpAfterBuild(opCtx, collection);
     });
 
@@ -376,32 +385,19 @@ bool runCreateIndexes(OperationContext* opCtx,
                              MultiIndexBlock::makeTimestampedIndexOnInitFn(opCtx, collection)));
         });
 
-    // If we're a background index, replace exclusive db lock with an intent lock, so that
-    // other readers and writers can proceed during this phase.
+    // Don't hold an exclusive collection lock during background indexing, so that other readers
+    // and writers can proceed during this phase. A BackgroundOperation has been registered on the
+    // namespace, so the collection cannot be removed after yielding the lock.
     if (indexer.isBackgroundBuilding()) {
+        invariant(BackgroundOperation::inProgForNs(ns));
         opCtx->recoveryUnit()->abandonSnapshot();
-        dbLock.relockWithMode(MODE_IX);
-    }
-
-    auto relockOnErrorGuard = makeGuard([&] {
-        // Must have exclusive DB lock before we clean up the index build via the
-        // destructor of 'indexer'.
-        if (indexer.isBackgroundBuilding()) {
-            try {
-                // This function cannot throw today, but we will preemptively prepare for
-                // that day, to avoid data corruption due to lack of index cleanup.
-                opCtx->recoveryUnit()->abandonSnapshot();
-                dbLock.relockWithMode(MODE_X);
-            } catch (...) {
-                std::terminate();
-            }
-        }
-    });
+        exclusiveCollectionLock.reset();
+    }
 
     // Collection scan and insert into index, followed by a drain of writes received in the
     // background.
     {
-        Lock::CollectionLock colLock(opCtx, ns.ns(), MODE_IX);
+        Lock::CollectionLock colLock(opCtx, ns.ns(), MODE_IS);
         uassertStatusOK(indexer.insertAllDocumentsInCollection(opCtx, collection));
     }
 
@@ -429,7 +425,6 @@ bool runCreateIndexes(OperationContext* opCtx,
     {
         opCtx->recoveryUnit()->abandonSnapshot();
         Lock::CollectionLock colLock(opCtx, ns.ns(), MODE_S);
-
         uassertStatusOK(indexer.drainBackgroundWrites(opCtx));
     }
 
@@ -438,26 +433,21 @@ bool runCreateIndexes(OperationContext* opCtx,
         MONGO_FAIL_POINT_PAUSE_WHILE_SET(hangAfterIndexBuildSecondDrain);
     }
 
-    relockOnErrorGuard.dismiss();
-
-    // Need to return db lock back to exclusive, to complete the index build.
+    // Need to get exclusive collection lock back to complete the index build.
     if (indexer.isBackgroundBuilding()) {
         opCtx->recoveryUnit()->abandonSnapshot();
-        dbLock.relockWithMode(MODE_X);
-
-        auto db = databaseHolder->getDb(opCtx, ns.db());
-        if (db) {
-            auto& dss = DatabaseShardingState::get(db);
-            auto dssLock = DatabaseShardingState::DSSLock::lock(opCtx, &dss);
-            dss.checkDbVersion(opCtx, dssLock);
-        }
+        exclusiveCollectionLock.emplace(opCtx, ns.ns(), MODE_X);
+    }
 
-        invariant(db);
-        invariant(db->getCollection(opCtx, ns));
+    db = databaseHolder->getDb(opCtx, ns.db());
+    invariant(db->getCollection(opCtx, ns));
+    {
+        auto& dss = DatabaseShardingState::get(db);
+        auto dssLock = DatabaseShardingState::DSSLock::lock(opCtx, &dss);
+        dss.checkDbVersion(opCtx, dssLock);
     }
 
-    // Perform the third and final drain after releasing a shared lock and reacquiring an
-    // exclusive lock on the database.
+    // Perform the third and final drain while holding the exclusive collection lock.
     uassertStatusOK(indexer.drainBackgroundWrites(opCtx));
 
     // This is required before completion.
diff --git a/src/mongo/db/concurrency/d_concurrency.cpp b/src/mongo/db/concurrency/d_concurrency.cpp
index bdd07a1d5e0..f49c225a807 100644
--- a/src/mongo/db/concurrency/d_concurrency.cpp
+++ b/src/mongo/db/concurrency/d_concurrency.cpp
@@ -265,8 +265,9 @@ void Lock::DBLock::relockWithMode(LockMode newMode) {
     // 2PL would delay the unlocking
     invariant(!_opCtx->lockState()->inAWriteUnitOfWork());
 
-    // Not allowed to change global intent
-    invariant(!isSharedLockMode(_mode) || isSharedLockMode(newMode));
+    // Not allowed to change global intent, so check when going from shared to exclusive.
+    if (isSharedLockMode(_mode) && !isSharedLockMode(newMode))
+        invariant(_opCtx->lockState()->isWriteLocked());
 
     _opCtx->lockState()->unlock(_id);
     _mode = newMode;
diff --git a/src/mongo/db/index/index_access_method.cpp b/src/mongo/db/index/index_access_method.cpp
index f017dd20f88..0b3c6abcce5 100644
--- a/src/mongo/db/index/index_access_method.cpp
+++ b/src/mongo/db/index/index_access_method.cpp
@@ -583,7 +583,11 @@ Status AbstractIndexAccessMethod::BulkBuilderImpl::insert(OperationContext* opCt
     BSONObjSet keys = SimpleBSONObjComparator::kInstance.makeBSONObjSet();
     MultikeyPaths multikeyPaths;
 
-    _real->getKeys(obj, options.getKeysMode, &keys, &_multikeyMetadataKeys, &multikeyPaths);
+    try {
+        _real->getKeys(obj, options.getKeysMode, &keys, &_multikeyMetadataKeys, &multikeyPaths);
+    } catch (...) {
+        return exceptionToStatus();
+    }
 
     if (!multikeyPaths.empty()) {
         if (_indexMultikeyPaths.empty()) {
diff --git a/src/mongo/dbtests/validate_tests.cpp b/src/mongo/dbtests/validate_tests.cpp
index 290176b500f..9fb912f9538 100644
--- a/src/mongo/dbtests/validate_tests.cpp
+++ b/src/mongo/dbtests/validate_tests.cpp
@@ -679,8 +679,8 @@ public:
             wunit.commit();
         }
 
-        // Create a partial geo index that indexes the document. This should throw an error.
-        ASSERT_THROWS(dbtests::createIndexFromSpec(&_opCtx,
+        // Create a partial geo index that indexes the document. This should return an error.
+        ASSERT_NOT_OK(dbtests::createIndexFromSpec(&_opCtx,
                                                    coll->ns().ns(),
                                                    BSON("name"
                                                         << "partial_index"
@@ -694,8 +694,7 @@ public:
                                                         << "background"
                                                         << false
                                                         << "partialFilterExpression"
-                                                        << BSON("a" << BSON("$eq" << 2)))),
-                      AssertionException);
+                                                        << BSON("a" << BSON("$eq" << 2)))));
 
         // Create a partial geo index that does not index the document.
         auto status = dbtests::createIndexFromSpec(&_opCtx,