SERVER-40926 SERVER-40927 createIndexes cmd requests for indexes that are already being built wait for them to complete rather than return OK immediately

5 files changed, 85 insertions, 26 deletions

diff --git a/src/mongo/db/catalog/index_catalog.h b/src/mongo/db/catalog/index_catalog.h
index 304ef4a09f8..9cc22948090 100644
--- a/src/mongo/db/catalog/index_catalog.h
+++ b/src/mongo/db/catalog/index_catalog.h
@@ -356,7 +356,8 @@ public:
      * fields. Lastly, checks whether the spec conflicts with ready and in-progress indexes.
      *
      * Returns an error Status or the cleaned up version of the non-conflicting spec. Returns
-     * IndexAlreadyExists if the index either already exists or is already being built.
+     * IndexAlreadyExists if the index already exists; IndexBuildAlreadyInProgress if the index is
+     * already being built.
      */
     virtual StatusWith<BSONObj> prepareSpecForCreate(OperationContext* const opCtx,
                                                      const BSONObj& original) const = 0;
@@ -364,15 +365,21 @@ public:
     /**
      * Returns a copy of 'indexSpecsToBuild' that does not contain index specifications that already
      * exist or are already being built. If this is not done, an index build using
-     * 'indexSpecsToBuild' may fail with error code IndexAlreadyExists. If {buildIndexes:false} is
-     * set in the replica set config, also filters non-_id index specs out of the results.
+     * 'indexSpecsToBuild' may fail with an IndexAlreadyExists or IndexBuildAlreadyInProgress error.
+     * If {buildIndexes:false} is set in the replica set config, also filters non-_id index specs
+     * out of the results.
      *
-     * Additionally verifies the specs are valid and corrects any legacy fields. Throws on any spec
-     * validation errors or conflicts other than IndexAlreadyExists, which indicates that the index
-     * spec either already exists or is already being built and is what this function filters out.
+     * Additionally verifies the specs are valid. Throws on any spec validation errors or conflicts
+     * other than IndexAlreadyExists, which indicates that the index spec already exists is what
+     * this function filters out.
+     *
+     * 'removeIndexBuildsToo' controls whether in-progress index builds are also filtered out. If
+     * they are not, then IndexBuildAlreadyInProgress errors can be thrown.
      */
     virtual std::vector<BSONObj> removeExistingIndexes(
-        OperationContext* const opCtx, const std::vector<BSONObj>& indexSpecsToBuild) const = 0;
+        OperationContext* const opCtx,
+        const std::vector<BSONObj>& indexSpecsToBuild,
+        const bool removeIndexBuildsToo) const = 0;
 
     /**
      * Filters out ready and in-progress indexes that already exist and returns the remaining
diff --git a/src/mongo/db/catalog/index_catalog_impl.cpp b/src/mongo/db/catalog/index_catalog_impl.cpp
index 24287950d95..8d82670b009 100644
--- a/src/mongo/db/catalog/index_catalog_impl.cpp
+++ b/src/mongo/db/catalog/index_catalog_impl.cpp
@@ -392,11 +392,27 @@ StatusWith<BSONObj> IndexCatalogImpl::prepareSpecForCreate(OperationContext* opC
         return status;
     }
 
-    status = _doesSpecConflictWithExisting(opCtx, swValidatedAndFixed.getValue());
+    // First check against only the ready indexes for conflicts.
+    status = _doesSpecConflictWithExisting(opCtx, swValidatedAndFixed.getValue(), false);
     if (!status.isOK()) {
         return status;
     }
 
+    // Now we will check against all indexes, in-progress included.
+    //
+    // The index catalog cannot currently iterate over only in-progress indexes. So by previously
+    // checking against only ready indexes without error, we know that any errors encountered
+    // checking against all indexes occurred due to an in-progress index.
+    status = _doesSpecConflictWithExisting(opCtx, swValidatedAndFixed.getValue(), true);
+    if (!status.isOK()) {
+        if (ErrorCodes::IndexAlreadyExists == status.code()) {
+            // Callers need to be able to distinguish conflicts against ready indexes versus
+            // in-progress indexes.
+            return {ErrorCodes::IndexBuildAlreadyInProgress, status.reason()};
+        }
+        return status;
+    }
+
     return swValidatedAndFixed.getValue();
 }
 
@@ -413,7 +429,8 @@ std::vector<BSONObj> IndexCatalogImpl::removeExistingIndexesNoChecks(
 
         // _doesSpecConflictWithExisting currently does more work than we require here: we are only
         // interested in the index already exists error.
-        if (ErrorCodes::IndexAlreadyExists == _doesSpecConflictWithExisting(opCtx, spec)) {
+        if (ErrorCodes::IndexAlreadyExists ==
+            _doesSpecConflictWithExisting(opCtx, spec, true /*includeUnfinishedIndexes*/)) {
             continue;
         }
 
@@ -423,11 +440,14 @@ std::vector<BSONObj> IndexCatalogImpl::removeExistingIndexesNoChecks(
 }
 
 std::vector<BSONObj> IndexCatalogImpl::removeExistingIndexes(
-    OperationContext* const opCtx, const std::vector<BSONObj>& indexSpecsToBuild) const {
+    OperationContext* const opCtx,
+    const std::vector<BSONObj>& indexSpecsToBuild,
+    const bool removeIndexBuildsToo) const {
     std::vector<BSONObj> result;
     for (const auto& spec : indexSpecsToBuild) {
         auto prepareResult = prepareSpecForCreate(opCtx, spec);
-        if (prepareResult == ErrorCodes::IndexAlreadyExists) {
+        if (prepareResult == ErrorCodes::IndexAlreadyExists ||
+            (removeIndexBuildsToo && prepareResult == ErrorCodes::IndexBuildAlreadyInProgress)) {
             continue;
         }
         uassertStatusOK(prepareResult);
@@ -756,7 +776,8 @@ Status IndexCatalogImpl::_isSpecOk(OperationContext* opCtx, const BSONObj& spec)
 }
 
 Status IndexCatalogImpl::_doesSpecConflictWithExisting(OperationContext* opCtx,
-                                                       const BSONObj& spec) const {
+                                                       const BSONObj& spec,
+                                                       const bool includeUnfinishedIndexes) const {
     const char* name = spec.getStringField("name");
     invariant(name[0]);
 
@@ -764,8 +785,7 @@ Status IndexCatalogImpl::_doesSpecConflictWithExisting(OperationContext* opCtx,
     const BSONObj collation = spec.getObjectField("collation");
 
     {
-        const IndexDescriptor* desc =
-            findIndexByName(opCtx, name, true /*includeUnfinishedIndexes*/);
+        const IndexDescriptor* desc = findIndexByName(opCtx, name, includeUnfinishedIndexes);
         if (desc) {
             // index already exists with same name
 
@@ -809,8 +829,8 @@ Status IndexCatalogImpl::_doesSpecConflictWithExisting(OperationContext* opCtx,
     }
 
     {
-        const IndexDescriptor* desc = findIndexByKeyPatternAndCollationSpec(
-            opCtx, key, collation, true /*includeUnfinishedIndexes*/);
+        const IndexDescriptor* desc =
+            findIndexByKeyPatternAndCollationSpec(opCtx, key, collation, includeUnfinishedIndexes);
         if (desc) {
             LOG(2) << "Index already exists with a different name: " << name << " pattern: " << key
                    << " collation: " << collation;
diff --git a/src/mongo/db/catalog/index_catalog_impl.h b/src/mongo/db/catalog/index_catalog_impl.h
index 28a1233c58d..0b4ad27d872 100644
--- a/src/mongo/db/catalog/index_catalog_impl.h
+++ b/src/mongo/db/catalog/index_catalog_impl.h
@@ -191,9 +191,9 @@ public:
     StatusWith<BSONObj> prepareSpecForCreate(OperationContext* opCtx,
                                              const BSONObj& original) const override;
 
-    std::vector<BSONObj> removeExistingIndexes(
-        OperationContext* const opCtx,
-        const std::vector<BSONObj>& indexSpecsToBuild) const override;
+    std::vector<BSONObj> removeExistingIndexes(OperationContext* const opCtx,
+                                               const std::vector<BSONObj>& indexSpecsToBuild,
+                                               const bool removeIndexBuildsToo) const override;
 
     std::vector<BSONObj> removeExistingIndexesNoChecks(
         OperationContext* const opCtx,
@@ -446,9 +446,15 @@ private:
     /**
      * Checks whether there are any spec conflicts with existing ready indexes or in-progress index
      * builds. Also checks whether any limits set on this server would be exceeded by building the
-     * index.
+     * index. 'includeUnfinishedIndexes' dictates whether in-progress index builds are checked for
+     * conflicts, along with ready indexes.
+     *
+     * Returns IndexAlreadyExists for both ready and in-progress index builds. Can also return other
+     * errors.
      */
-    Status _doesSpecConflictWithExisting(OperationContext* opCtx, const BSONObj& spec) const;
+    Status _doesSpecConflictWithExisting(OperationContext* opCtx,
+                                         const BSONObj& spec,
+                                         const bool includeUnfinishedIndexes) const;
 
     /**
      * Returns true if the replica set member's config has {buildIndexes:false} set, which means
diff --git a/src/mongo/db/catalog/index_catalog_noop.h b/src/mongo/db/catalog/index_catalog_noop.h
index a1cb16bd9f4..9f9bb23c27b 100644
--- a/src/mongo/db/catalog/index_catalog_noop.h
+++ b/src/mongo/db/catalog/index_catalog_noop.h
@@ -147,10 +147,10 @@ public:
         return original;
     }
 
-    std::vector<BSONObj> removeExistingIndexes(
-        OperationContext* const opCtx,
-        const std::vector<BSONObj>& indexSpecsToBuild) const override {
-        return {};
+    std::vector<BSONObj> removeExistingIndexes(OperationContext* const opCtx,
+                                               const std::vector<BSONObj>& indexSpecsToBuild,
+                                               const bool removeIndexBuildsToo) const override {
+        return indexSpecsToBuild;
     }
 
     std::vector<BSONObj> removeExistingIndexesNoChecks(
diff --git a/src/mongo/db/catalog/multi_index_block.cpp b/src/mongo/db/catalog/multi_index_block.cpp
index 66b5c914a36..e2bf3d8fb25 100644
--- a/src/mongo/db/catalog/multi_index_block.cpp
+++ b/src/mongo/db/catalog/multi_index_block.cpp
@@ -61,10 +61,12 @@
 
 namespace mongo {
 
+MONGO_FAIL_POINT_DEFINE(hangAfterSettingUpIndexBuild);
 MONGO_FAIL_POINT_DEFINE(hangAfterStartingIndexBuild);
 MONGO_FAIL_POINT_DEFINE(hangAfterStartingIndexBuildUnlocked);
 MONGO_FAIL_POINT_DEFINE(hangBeforeIndexBuildOf);
 MONGO_FAIL_POINT_DEFINE(hangAfterIndexBuildOf);
+MONGO_FAIL_POINT_DEFINE(hangAndThenFailIndexBuild);
 MONGO_FAIL_POINT_DEFINE(leaveIndexBuildUnfinishedForShutdown);
 
 MultiIndexBlock::~MultiIndexBlock() {
@@ -282,8 +284,18 @@ StatusWith<std::vector<BSONObj>> MultiIndexBlock::init(OperationContext* opCtx,
         StatusWith<BSONObj> statusWithInfo =
             collection->getIndexCatalog()->prepareSpecForCreate(opCtx, info);
         Status status = statusWithInfo.getStatus();
-        if (!status.isOK())
+        if (!status.isOK()) {
+            // If we were given two identical indexes to build, we will run into an error trying to
+            // set up the same index a second time in this for-loop. This is the only way to
+            // encounter this error because callers filter out ready/in-progress indexes and start
+            // the build while holding a lock throughout.
+            if (status == ErrorCodes::IndexBuildAlreadyInProgress) {
+                invariant(indexSpecs.size() > 1);
+                return {ErrorCodes::OperationFailed,
+                        "Cannot build two identical indexes. Try again without duplicate indexes."};
+            }
             return status;
+        }
         info = statusWithInfo.getValue();
         indexInfoObjs.push_back(info);
 
@@ -387,6 +399,20 @@ Status MultiIndexBlock::insertAllDocumentsInCollection(OperationContext* opCtx,
         progress.set(CurOp::get(opCtx)->setProgress_inlock(curopMessage, numRecords));
     }
 
+    if (MONGO_FAIL_POINT(hangAfterSettingUpIndexBuild)) {
+        // Hang the build after the BackgroundOperation and curOP info is set up.
+        log() << "Hanging index build due to failpoint 'hangAfterSettingUpIndexBuild'";
+        MONGO_FAIL_POINT_PAUSE_WHILE_SET(hangAfterSettingUpIndexBuild);
+    }
+
+    if (MONGO_FAIL_POINT(hangAndThenFailIndexBuild)) {
+        // Hang the build after the BackgroundOperation and curOP info is set up.
+        log() << "Hanging index build due to failpoint 'hangAndThenFailIndexBuild'";
+        MONGO_FAIL_POINT_PAUSE_WHILE_SET(hangAndThenFailIndexBuild);
+        return {ErrorCodes::InternalError,
+                "Failed index build because of failpoint 'hangAndThenFailIndexBuild'"};
+    }
+
     Timer t;
 
     unsigned long long n = 0;