SERVER-44778 Two-phase index builds aborted due to rollback should leave indexes unfinished in the catalog

5 files changed, 61 insertions, 43 deletions

diff --git a/src/mongo/db/catalog/database_holder_impl.cpp b/src/mongo/db/catalog/database_holder_impl.cpp
index 7f75cbffe48..6ee95d92a81 100644
--- a/src/mongo/db/catalog/database_holder_impl.cpp
+++ b/src/mongo/db/catalog/database_holder_impl.cpp
@@ -39,6 +39,7 @@
 #include "mongo/db/catalog/collection_impl.h"
 #include "mongo/db/catalog/database_impl.h"
 #include "mongo/db/concurrency/write_conflict_exception.h"
+#include "mongo/db/index_builds_coordinator.h"
 #include "mongo/db/operation_context.h"
 #include "mongo/db/service_context.h"
 #include "mongo/db/stats/top.h"
@@ -243,18 +244,9 @@ void DatabaseHolderImpl::closeAll(OperationContext* opCtx) {
 
     std::set<std::string> dbs;
     for (DBs::const_iterator i = _dbs.begin(); i != _dbs.end(); ++i) {
-        for (auto collIt = i->second->begin(opCtx); collIt != i->second->end(opCtx); ++collIt) {
-            auto coll = *collIt;
-            if (!coll) {
-                continue;
-            }
-
-            // It is the caller's responsibility to ensure that no index builds are active in the
-            // database.
-            invariant(!coll->getIndexCatalog()->haveAnyIndexesInProgress(),
-                      str::stream()
-                          << "An index is building on collection '" << coll->ns() << "'.");
-        }
+        // It is the caller's responsibility to ensure that no index builds are active in the
+        // database.
+        IndexBuildsCoordinator::get(opCtx)->assertNoBgOpInProgForDb(i->first);
         dbs.insert(i->first);
     }
 
diff --git a/src/mongo/db/catalog/index_builds_manager.cpp b/src/mongo/db/catalog/index_builds_manager.cpp
index 3c222bd16cf..bbb3cf32c0f 100644
--- a/src/mongo/db/catalog/index_builds_manager.cpp
+++ b/src/mongo/db/catalog/index_builds_manager.cpp
@@ -272,9 +272,9 @@ bool IndexBuildsManager::abortIndexBuild(const UUID& buildUUID, const std::strin
     return true;
 }
 
-bool IndexBuildsManager::interruptIndexBuild(OperationContext* opCtx,
-                                             const UUID& buildUUID,
-                                             const std::string& reason) {
+bool IndexBuildsManager::abortIndexBuildWithoutCleanup(OperationContext* opCtx,
+                                                       const UUID& buildUUID,
+                                                       const std::string& reason) {
     stdx::unique_lock<Latch> lk(_mutex);
 
     auto builderIt = _builders.find(buildUUID);
@@ -282,7 +282,7 @@ bool IndexBuildsManager::interruptIndexBuild(OperationContext* opCtx,
         return false;
     }
 
-    log() << "Index build interrupted: " << buildUUID << ": " << reason;
+    log() << "Index build aborted without cleanup: " << buildUUID << ": " << reason;
     std::shared_ptr<MultiIndexBlock> builder = builderIt->second;
 
     lk.unlock();
diff --git a/src/mongo/db/catalog/index_builds_manager.h b/src/mongo/db/catalog/index_builds_manager.h
index c16b5b57af4..7cbc05a982c 100644
--- a/src/mongo/db/catalog/index_builds_manager.h
+++ b/src/mongo/db/catalog/index_builds_manager.h
@@ -164,15 +164,15 @@ public:
     bool abortIndexBuild(const UUID& buildUUID, const std::string& reason);
 
     /**
-     * Signals the index build to be interrupted and returns without waiting for it to stop. Does
-     * nothing if the index build has already been cleared away.
+     * Signals the index build to be aborted without being cleaned up and returns without waiting
+     * for it to stop. Does nothing if the index build has already been cleared away.
      *
      * Returns true if a build existed to be signaled, as opposed to having already finished and
      * been cleared away, or not having yet started..
      */
-    bool interruptIndexBuild(OperationContext* opCtx,
-                             const UUID& buildUUID,
-                             const std::string& reason);
+    bool abortIndexBuildWithoutCleanup(OperationContext* opCtx,
+                                       const UUID& buildUUID,
+                                       const std::string& reason);
 
     /**
      * Cleans up the index build state and unregisters it from the manager.
diff --git a/src/mongo/db/commands/create_indexes.cpp b/src/mongo/db/commands/create_indexes.cpp
index e85ec4911fd..4e6167326b4 100644
--- a/src/mongo/db/commands/create_indexes.cpp
+++ b/src/mongo/db/commands/create_indexes.cpp
@@ -398,20 +398,6 @@ Collection* getOrCreateCollection(OperationContext* opCtx,
 }
 
 /**
- * Returns true if index specs include any unique indexes. Due to uniqueness constraints set up at
- * the start of the index build, we are not able to support failing over a two phase index build on
- * a unique index to a new primary on stepdown.
- */
-bool containsUniqueIndexes(const std::vector<BSONObj>& specs) {
-    for (const auto& spec : specs) {
-        if (spec["unique"].trueValue()) {
-            return true;
-        }
-    }
-    return false;
-}
-
-/**
  * Creates indexes using the given specs for the mobile storage engine.
  * TODO(SERVER-42513): Remove this function.
  */
@@ -759,8 +745,7 @@ bool runCreateIndexesWithCoordinator(OperationContext* opCtx,
             // If this node is no longer a primary, the index build will continue to run in the
             // background and will complete when this node receives a commitIndexBuild oplog entry
             // from the new primary.
-            // TODO(SERVER-44654): re-enable failover support for unique indexes.
-            if (indexBuildsCoord->supportsTwoPhaseIndexBuild() && !containsUniqueIndexes(specs) &&
+            if (indexBuildsCoord->supportsTwoPhaseIndexBuild() &&
                 ErrorCodes::InterruptedDueToReplStateChange == interruptionEx.code()) {
                 log() << "Index build continuing in background: " << buildUUID;
                 throw;
@@ -785,8 +770,7 @@ bool runCreateIndexesWithCoordinator(OperationContext* opCtx,
 
             // The index build will continue to run in the background and will complete when this
             // node receives a commitIndexBuild oplog entry from the new primary.
-            // TODO(SERVER-44654): re-enable failover support for unique indexes.
-            if (indexBuildsCoord->supportsTwoPhaseIndexBuild() && !containsUniqueIndexes(specs)) {
+            if (indexBuildsCoord->supportsTwoPhaseIndexBuild()) {
                 log() << "Index build continuing in background: " << buildUUID;
                 throw;
             }
diff --git a/src/mongo/db/index_builds_coordinator.cpp b/src/mongo/db/index_builds_coordinator.cpp
index 50d5b1a7213..ca37985492b 100644
--- a/src/mongo/db/index_builds_coordinator.cpp
+++ b/src/mongo/db/index_builds_coordinator.cpp
@@ -522,11 +522,40 @@ void IndexBuildsCoordinator::abortIndexBuildByBuildUUID(OperationContext* opCtx,
     }
 }
 
+/**
+ * Returns true if index specs include any unique indexes. Due to uniqueness constraints set up at
+ * the start of the index build, we are not able to support failing over a two phase index build on
+ * a unique index to a new primary on stepdown.
+ */
+namespace {
+// TODO(SERVER-44654): remove when unique indexes support failover
+bool containsUniqueIndexes(const std::vector<BSONObj>& specs) {
+    for (const auto& spec : specs) {
+        if (spec["unique"].trueValue()) {
+            return true;
+        }
+    }
+    return false;
+}
+}  // namespace
+
 void IndexBuildsCoordinator::onStepUp(OperationContext* opCtx) {
     log() << "IndexBuildsCoordinator::onStepUp - this node is stepping up to primary";
 
     auto indexBuilds = _getIndexBuilds();
-    auto onIndexBuild = [](std::shared_ptr<ReplIndexBuildState> replState) {
+    auto onIndexBuild = [this, opCtx](std::shared_ptr<ReplIndexBuildState> replState) {
+        // TODO(SERVER-44654): re-enable failover support for unique indexes.
+        if (containsUniqueIndexes(replState->indexSpecs)) {
+            // We abort unique index builds on step-up on the new primary, as opposed to on
+            // step-down on the old primary. This is because the old primary cannot generate any new
+            // oplog entries, and consequently does not have a timestamp to delete the index from
+            // the durable catalog. This abort will replicate to the old primary, now secondary, to
+            // abort the build.
+            abortIndexBuildByBuildUUID(
+                opCtx, replState->buildUUID, "unique indexes do not support failover");
+            return;
+        }
+
         stdx::unique_lock<Latch> lk(replState->mutex);
         if (!replState->aborted) {
             // Leave commit timestamp as null. We will be writing a commitIndexBuild oplog entry now
@@ -1178,7 +1207,8 @@ void IndexBuildsCoordinator::_cleanUpSinglePhaseAfterFailure(
     const Status& status) {
     if (status == ErrorCodes::InterruptedAtShutdown) {
         // Leave it as-if kill -9 happened. Startup recovery will rebuild the index.
-        _indexBuildsManager.interruptIndexBuild(opCtx, replState->buildUUID, "shutting down");
+        _indexBuildsManager.abortIndexBuildWithoutCleanup(
+            opCtx, replState->buildUUID, "shutting down");
         _indexBuildsManager.tearDownIndexBuild(
             opCtx, collection, replState->buildUUID, MultiIndexBlock::kNoopOnCleanUpFn);
         return;
@@ -1228,7 +1258,8 @@ void IndexBuildsCoordinator::_cleanUpTwoPhaseAfterFailure(
 
     if (status == ErrorCodes::InterruptedAtShutdown) {
         // Leave it as-if kill -9 happened. Startup recovery will restart the index build.
-        _indexBuildsManager.interruptIndexBuild(opCtx, replState->buildUUID, "shutting down");
+        _indexBuildsManager.abortIndexBuildWithoutCleanup(
+            opCtx, replState->buildUUID, "shutting down");
         _indexBuildsManager.tearDownIndexBuild(
             opCtx, collection, replState->buildUUID, MultiIndexBlock::kNoopOnCleanUpFn);
         return;
@@ -1255,11 +1286,22 @@ void IndexBuildsCoordinator::_cleanUpTwoPhaseAfterFailure(
             invariant(replState->aborted, replState->buildUUID.toString());
             Timestamp abortIndexBuildTimestamp = replState->abortTimestamp;
 
+            // If we were aborted and no abort timestamp is set, then we should leave the index
+            // build unfinished. This can happen during rollback because we are not primary and
+            // cannot generate an optime to timestamp the index build abort. We rely on the
+            // rollback process to correct this state.
+            if (abortIndexBuildTimestamp.isNull()) {
+                _indexBuildsManager.abortIndexBuildWithoutCleanup(
+                    opCtx, replState->buildUUID, "no longer primary");
+                _indexBuildsManager.tearDownIndexBuild(
+                    opCtx, collection, replState->buildUUID, MultiIndexBlock::kNoopOnCleanUpFn);
+                return;
+            }
+
             // Unlock the RSTL to avoid deadlocks with state transitions. See SERVER-42824.
             unlockRSTLForIndexCleanup(opCtx);
             Lock::CollectionLock collLock(opCtx, nss, MODE_X);
 
-            // TimestampBlock is a no-op if the abort timestamp is unset.
             TimestampBlock tsBlock(opCtx, abortIndexBuildTimestamp);
             _indexBuildsManager.tearDownIndexBuild(
                 opCtx, collection, replState->buildUUID, MultiIndexBlock::kNoopOnCleanUpFn);