SERVER-36961 createIndexes command should check if indexes already exist with weak lock

3 files changed, 109 insertions, 30 deletions

diff --git a/jstests/core/txns/noop_createIndexes_not_blocked_by_txn.js b/jstests/core/txns/noop_createIndexes_not_blocked_by_txn.js
new file mode 100644
index 00000000000..2fd5d32f905
--- /dev/null
+++ b/jstests/core/txns/noop_createIndexes_not_blocked_by_txn.js
@@ -0,0 +1,47 @@
+// Tests that no-op createIndex commands do not block behind transactions.
+// @tags: [uses_transactions]
+(function() {
+    "use strict";
+
+    const dbName = 'noop_createIndexes_not_blocked';
+    const collName = 'test';
+    const testDB = db.getSiblingDB(dbName);
+
+    testDB.runCommand({drop: collName, writeConcern: {w: "majority"}});
+
+    const session = db.getMongo().startSession({causalConsistency: false});
+    const sessionDB = session.getDatabase(dbName);
+
+    const createIndexesCommand = {createIndexes: collName, indexes: [{key: {a: 1}, name: "a_1"}]};
+    assert.commandWorked(sessionDB.runCommand(createIndexesCommand));
+
+    session.startTransaction();
+    assert.commandWorked(sessionDB[collName].insert({a: 5, b: 6}));
+
+    // This should not block because an identical index exists.
+    let res = testDB.runCommand(createIndexesCommand);
+    assert.commandWorked(res);
+    assert.eq(res.numIndexesBefore, res.numIndexesAfter);
+
+    // This should not block but return an error because the index exists with different options.
+    res = testDB.runCommand({
+        createIndexes: collName,
+        indexes: [{key: {a: 1}, name: "unique_a_1", unique: true}],
+    });
+    assert.commandFailedWithCode(res, ErrorCodes.IndexOptionsConflict);
+
+    // This should block and time out because the index does not already exist.
+    res = testDB.runCommand(
+        {createIndexes: collName, indexes: [{key: {b: 1}, name: "b_1"}], maxTimeMS: 500});
+    assert.commandFailedWithCode(res, ErrorCodes.MaxTimeMSExpired);
+
+    // This should block and time out because one of the indexes does not already exist.
+    res = testDB.runCommand({
+        createIndexes: collName,
+        indexes: [{key: {a: 1}, name: "a_1"}, {key: {b: 1}, name: "b_1"}],
+        maxTimeMS: 500
+    });
+    assert.commandFailedWithCode(res, ErrorCodes.MaxTimeMSExpired);
+
+    assert.commandWorked(session.commitTransaction_forTesting());
+}());
diff --git a/src/mongo/db/catalog/index_create_impl.cpp b/src/mongo/db/catalog/index_create_impl.cpp
index 0adb92849d9..e20b190b265 100644
--- a/src/mongo/db/catalog/index_create_impl.cpp
+++ b/src/mongo/db/catalog/index_create_impl.cpp
@@ -309,7 +309,7 @@ StatusWith<std::vector<BSONObj>> MultiIndexBlockImpl::init(const std::vector<BSO
         log() << "Index build interrupted due to 'crashAfterStartingIndexBuild' failpoint. Exiting "
                  "after waiting for changes to become durable.";
         Locker::LockSnapshot lockInfo;
-        _opCtx->lockState()->saveLockStateAndUnlock(&lockInfo);
+        invariant(_opCtx->lockState()->saveLockStateAndUnlock(&lockInfo));
         if (_opCtx->recoveryUnit()->waitUntilDurable()) {
             quickExit(EXIT_TEST);
         }
@@ -440,7 +440,7 @@ Status MultiIndexBlockImpl::insertAllDocumentsInCollection() {
     if (MONGO_FAIL_POINT(hangAfterStartingIndexBuildUnlocked)) {
         // Unlock before hanging so replication recognizes we've completed.
         Locker::LockSnapshot lockInfo;
-        _opCtx->lockState()->saveLockStateAndUnlock(&lockInfo);
+        invariant(_opCtx->lockState()->saveLockStateAndUnlock(&lockInfo));
         while (MONGO_FAIL_POINT(hangAfterStartingIndexBuildUnlocked)) {
             log() << "Hanging index build with no locks due to "
                      "'hangAfterStartingIndexBuildUnlocked' failpoint";
diff --git a/src/mongo/db/commands/create_indexes.cpp b/src/mongo/db/commands/create_indexes.cpp
index 940454d6d8e..a4edf0b000e 100644
--- a/src/mongo/db/commands/create_indexes.cpp
+++ b/src/mongo/db/commands/create_indexes.cpp
@@ -206,6 +206,32 @@ StatusWith<std::vector<BSONObj>> resolveCollectionDefaultProperties(
     return indexSpecsWithDefaults;
 }
 
+/**
+ * Returns a vector of index specs with the filled in collection default options and removes any
+ * indexes that already exist on the collection. If the returned vector is empty after returning, no
+ * new indexes need to be built. Throws on error.
+ */
+std::vector<BSONObj> resolveDefaultsAndRemoveExistingIndexes(OperationContext* opCtx,
+                                                             const Collection* collection,
+                                                             std::vector<BSONObj> validatedSpecs) {
+    auto swDefaults =
+        resolveCollectionDefaultProperties(opCtx, collection, std::move(validatedSpecs));
+    uassertStatusOK(swDefaults.getStatus());
+
+    auto specs = std::move(swDefaults.getValue());
+    for (size_t i = 0; i < specs.size(); i++) {
+        Status status =
+            collection->getIndexCatalog()->prepareSpecForCreate(opCtx, specs.at(i)).getStatus();
+        if (status.code() == ErrorCodes::IndexAlreadyExists) {
+            specs.erase(specs.begin() + i);
+            i--;
+            continue;
+        }
+        uassertStatusOK(status);
+    }
+    return specs;
+}
+
 }  // namespace
 
 /**
@@ -254,14 +280,40 @@ public:
         uassertStatusOK(specsWithStatus.getStatus());
         auto specs = std::move(specsWithStatus.getValue());
 
-        // now we know we have to create index(es)
-        // Do not use AutoGetOrCreateDb because we may relock the DbLock in mode IX.
-        Lock::DBLock dbLock(opCtx, ns.db(), MODE_X);
+        // Do not use AutoGetOrCreateDb because we may relock the database in mode X.
+        Lock::DBLock dbLock(opCtx, ns.db(), MODE_IX);
         if (!repl::ReplicationCoordinator::get(opCtx)->canAcceptWritesFor(opCtx, ns)) {
             uasserted(ErrorCodes::NotMaster,
                       str::stream() << "Not primary while creating indexes in " << ns.ns());
         }
 
+        const auto indexesAlreadyExist = [&result](int numIndexes) {
+            result.append("numIndexesBefore", numIndexes);
+            result.append("numIndexesAfter", numIndexes);
+            result.append("note", "all indexes already exist");
+            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.
+        {
+            AutoGetCollection autoColl(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));
+                }
+            }
+        }
+
+        // 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());
@@ -298,27 +350,21 @@ public:
         const boost::optional<int> dbProfilingLevel = boost::none;
         statsTracker.emplace(opCtx, ns, Top::LockType::WriteLocked, dbProfilingLevel);
 
-        auto indexSpecsWithDefaults =
-            resolveCollectionDefaultProperties(opCtx, collection, std::move(specs));
-        uassertStatusOK(indexSpecsWithDefaults.getStatus());
-        specs = std::move(indexSpecsWithDefaults.getValue());
-
-        const int numIndexesBefore = collection->getIndexCatalog()->numIndexesTotal(opCtx);
-        result.append("numIndexesBefore", numIndexesBefore);
 
         MultiIndexBlock indexer(opCtx, collection);
         indexer.allowBackgroundBuilding();
         indexer.allowInterruption();
 
         const size_t origSpecsSize = specs.size();
-        indexer.removeExistingIndexes(&specs);
+        specs = resolveDefaultsAndRemoveExistingIndexes(opCtx, collection, std::move(specs));
 
+        const int numIndexesBefore = collection->getIndexCatalog()->numIndexesTotal(opCtx);
         if (specs.size() == 0) {
-            result.append("numIndexesAfter", numIndexesBefore);
-            result.append("note", "all indexes already exist");
-            return true;
+            return indexesAlreadyExist(numIndexesBefore);
         }
 
+        result.append("numIndexesBefore", numIndexesBefore);
+
         if (specs.size() != origSpecsSize) {
             result.append("note", "index already exists");
         }
@@ -341,11 +387,6 @@ public:
         if (indexer.getBuildInBackground()) {
             opCtx->recoveryUnit()->abandonSnapshot();
             dbLock.relockWithMode(MODE_IX);
-            if (!repl::ReplicationCoordinator::get(opCtx)->canAcceptWritesFor(opCtx, ns)) {
-                uasserted(ErrorCodes::NotMaster,
-                          str::stream() << "Not primary while creating background indexes in "
-                                        << ns.ns());
-            }
         }
 
         try {
@@ -364,12 +405,6 @@ public:
                 } catch (...) {
                     std::terminate();
                 }
-                uassert(ErrorCodes::NotMaster,
-                        str::stream() << "Not primary while creating background indexes in "
-                                      << ns.ns()
-                                      << ": cleaning up index build failure due to "
-                                      << e.toString(),
-                        repl::ReplicationCoordinator::get(opCtx)->canAcceptWritesFor(opCtx, ns));
             }
             throw;
         }
@@ -377,9 +412,6 @@ public:
         if (indexer.getBuildInBackground()) {
             opCtx->recoveryUnit()->abandonSnapshot();
             dbLock.relockWithMode(MODE_X);
-            uassert(ErrorCodes::NotMaster,
-                    str::stream() << "Not primary while completing index build in " << dbname,
-                    repl::ReplicationCoordinator::get(opCtx)->canAcceptWritesFor(opCtx, ns));
 
             Database* db = DatabaseHolder::getDatabaseHolder().get(opCtx, ns.db());
             if (db) {