SERVER-47605 Single-phase index builds should only check constraint violations upon completion

7 files changed, 80 insertions, 105 deletions

diff --git a/src/mongo/db/catalog/index_build_block.cpp b/src/mongo/db/catalog/index_build_block.cpp
index 1f7a44abfb2..1bf41eb59ea 100644
--- a/src/mongo/db/catalog/index_build_block.cpp
+++ b/src/mongo/db/catalog/index_build_block.cpp
@@ -101,13 +101,8 @@ Status IndexBuildBlock::init(OperationContext* opCtx, Collection* collection) {
     _indexCatalogEntry =
         _indexCatalog->createIndexEntry(opCtx, std::move(descriptor), CreateIndexEntryFlags::kNone);
 
-    // Only track skipped records with two-phase index builds, which is indicated by a present build
-    // UUID.
-    const auto trackSkipped = (_buildUUID) ? IndexBuildInterceptor::TrackSkippedRecords::kTrack
-                                           : IndexBuildInterceptor::TrackSkippedRecords::kNoTrack;
     if (_method == IndexBuildMethod::kHybrid) {
-        _indexBuildInterceptor =
-            std::make_unique<IndexBuildInterceptor>(opCtx, _indexCatalogEntry, trackSkipped);
+        _indexBuildInterceptor = std::make_unique<IndexBuildInterceptor>(opCtx, _indexCatalogEntry);
         _indexCatalogEntry->setIndexBuildInterceptor(_indexBuildInterceptor.get());
     }
 
diff --git a/src/mongo/db/catalog/index_builds_manager.cpp b/src/mongo/db/catalog/index_builds_manager.cpp
index 61a147b481c..6111df352fc 100644
--- a/src/mongo/db/catalog/index_builds_manager.cpp
+++ b/src/mongo/db/catalog/index_builds_manager.cpp
@@ -98,7 +98,7 @@ Status IndexBuildsManager::setUpIndexBuild(OperationContext* opCtx,
     // secondaries. Secondaries can complete index builds in the middle of batches, which creates
     // the potential for finding duplicate key violations where there otherwise would be none at
     // consistent states.
-    // Two-phase builds will defer any unique key violations until commit-time.
+    // Index builds will otherwise defer any unique key constraint checks until commit-time.
     if (options.indexConstraints == IndexConstraints::kRelax &&
         options.protocol == IndexBuildProtocol::kSinglePhase) {
         builder->ignoreUniqueConstraint();
diff --git a/src/mongo/db/catalog/multi_index_block.cpp b/src/mongo/db/catalog/multi_index_block.cpp
index 59ef2232661..dd5ae289c9e 100644
--- a/src/mongo/db/catalog/multi_index_block.cpp
+++ b/src/mongo/db/catalog/multi_index_block.cpp
@@ -69,7 +69,6 @@ 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() {
@@ -258,18 +257,9 @@ StatusWith<std::vector<BSONObj>> MultiIndexBlock::init(OperationContext* opCtx,
             collection->getIndexCatalog()->prepareInsertDeleteOptions(
                 opCtx, descriptor, &index.options);
 
-            // Allow duplicates when explicitly allowed or when using hybrid builds, which will
-            // perform duplicate checking itself.
-            index.options.dupsAllowed =
-                index.options.dupsAllowed || index.block->getEntry()->isHybridBuilding();
-
-            // Two-phase index builds (with a build UUID) always relax constraints and check for
-            // violations at commit-time.
-            if (_buildUUID) {
-                index.options.getKeysMode = IndexAccessMethod::GetKeysMode::kRelaxConstraints;
-                index.options.dupsAllowed = true;
-            }
-
+            // Index builds always relax constraints and check for violations at commit-time.
+            index.options.getKeysMode = IndexAccessMethod::GetKeysMode::kRelaxConstraints;
+            index.options.dupsAllowed = true;
             index.options.fromIndexBuilder = true;
 
             LOGV2(20384,
@@ -383,15 +373,6 @@ Status MultiIndexBlock::insertAllDocumentsInCollection(OperationContext* opCtx,
         opCtx->recoveryUnit()->abandonSnapshot();
     }
 
-
-    if (MONGO_unlikely(hangAndThenFailIndexBuild.shouldFail())) {
-        // Hang the build after the BackgroundOperation and curOP info is set up.
-        LOGV2(20388, "Hanging index build due to failpoint 'hangAndThenFailIndexBuild'");
-        hangAndThenFailIndexBuild.pauseWhileSet();
-        return {ErrorCodes::InternalError,
-                "Failed index build because of failpoint 'hangAndThenFailIndexBuild'"};
-    }
-
     Timer t;
 
     unsigned long long n = 0;
@@ -587,15 +568,10 @@ Status MultiIndexBlock::drainBackgroundWrites(
         if (!interceptor)
             continue;
 
-        // Track duplicates for later constraint checking for two-phase builds (with a buildUUID),
-        // whenever key constraints are being enforced (i.e. single-phase builds on primaries), and
-        // never when _ignoreUnique is set explicitly.
-        auto trackDups = !_ignoreUnique &&
-                (_buildUUID ||
-                 IndexAccessMethod::GetKeysMode::kEnforceConstraints ==
-                     _indexes[i].options.getKeysMode)
-            ? IndexBuildInterceptor::TrackDuplicates::kTrack
-            : IndexBuildInterceptor::TrackDuplicates::kNoTrack;
+        // Track duplicates for later constraint checking for all index builds, except when
+        // _ignoreUnique is set explicitly.
+        auto trackDups = !_ignoreUnique ? IndexBuildInterceptor::TrackDuplicates::kTrack
+                                        : IndexBuildInterceptor::TrackDuplicates::kNoTrack;
         auto status = interceptor->drainWritesIntoIndex(
             opCtx, _indexes[i].options, trackDups, readSource, drainYieldPolicy);
         if (!status.isOK()) {
diff --git a/src/mongo/db/index/index_build_interceptor.cpp b/src/mongo/db/index/index_build_interceptor.cpp
index a5350dc5701..972ae9e2927 100644
--- a/src/mongo/db/index/index_build_interceptor.cpp
+++ b/src/mongo/db/index/index_build_interceptor.cpp
@@ -67,18 +67,13 @@ bool IndexBuildInterceptor::typeCanFastpathMultikeyUpdates(IndexType indexType)
     return (indexType == INDEX_BTREE);
 }
 
-IndexBuildInterceptor::IndexBuildInterceptor(OperationContext* opCtx,
-                                             IndexCatalogEntry* entry,
-                                             TrackSkippedRecords trackSkippedRecords)
+IndexBuildInterceptor::IndexBuildInterceptor(OperationContext* opCtx, IndexCatalogEntry* entry)
     : _indexCatalogEntry(entry),
       _sideWritesTable(
           opCtx->getServiceContext()->getStorageEngine()->makeTemporaryRecordStore(opCtx)),
+      _skippedRecordTracker(entry),
       _sideWritesCounter(std::make_shared<AtomicWord<long long>>()) {
 
-    if (TrackSkippedRecords::kTrack == trackSkippedRecords) {
-        _skippedRecordTracker = std::make_unique<SkippedRecordTracker>(_indexCatalogEntry);
-    }
-
     if (entry->descriptor()->unique()) {
         _duplicateKeyTracker = std::make_unique<DuplicateKeyTracker>(opCtx, entry);
     }
@@ -98,9 +93,7 @@ void IndexBuildInterceptor::deleteTemporaryTables(OperationContext* opCtx) {
     if (_duplicateKeyTracker) {
         _duplicateKeyTracker->deleteTemporaryTable(opCtx);
     }
-    if (_skippedRecordTracker) {
-        _skippedRecordTracker->deleteTemporaryTable(opCtx);
-    }
+    _skippedRecordTracker.deleteTemporaryTable(opCtx);
 }
 
 Status IndexBuildInterceptor::recordDuplicateKeys(OperationContext* opCtx,
@@ -487,10 +480,7 @@ Status IndexBuildInterceptor::sideWrite(OperationContext* opCtx,
 
 Status IndexBuildInterceptor::retrySkippedRecords(OperationContext* opCtx,
                                                   const Collection* collection) {
-    if (!_skippedRecordTracker) {
-        return Status::OK();
-    }
-    return _skippedRecordTracker->retrySkippedRecords(opCtx, collection);
+    return _skippedRecordTracker.retrySkippedRecords(opCtx, collection);
 }
 
 
diff --git a/src/mongo/db/index/index_build_interceptor.h b/src/mongo/db/index/index_build_interceptor.h
index 4afb108d15b..8458dc0620d 100644
--- a/src/mongo/db/index/index_build_interceptor.h
+++ b/src/mongo/db/index/index_build_interceptor.h
@@ -55,12 +55,6 @@ public:
     enum class Op { kInsert, kDelete };
 
     /**
-     * Indicates whether this interceptor will allow tracking of documents skipped due to key
-     * generation errors. When 'kTrack', a SkippedRecordTracker is created.
-     */
-    enum class TrackSkippedRecords { kNoTrack, kTrack };
-
-    /**
      * Indicates whether to record duplicate keys that have been inserted into the index. When set
      * to 'kNoTrack', inserted duplicate keys will be ignored. When set to 'kTrack', a subsequent
      * call to checkDuplicateKeyConstraints is required.
@@ -76,9 +70,7 @@ public:
      *
      * deleteTemporaryTable() must be called before destruction to delete the temporary tables.
      */
-    IndexBuildInterceptor(OperationContext* opCtx,
-                          IndexCatalogEntry* entry,
-                          TrackSkippedRecords trackSkippedRecords);
+    IndexBuildInterceptor(OperationContext* opCtx, IndexCatalogEntry* entry);
 
     /**
      * Deletes the temporary side writes and duplicate key constraint violations tables. Must be
@@ -138,11 +130,11 @@ public:
                                 DrainYieldPolicy drainYieldPolicy);
 
     SkippedRecordTracker* getSkippedRecordTracker() {
-        return _skippedRecordTracker.get();
+        return &_skippedRecordTracker;
     }
 
     const SkippedRecordTracker* getSkippedRecordTracker() const {
-        return _skippedRecordTracker.get();
+        return &_skippedRecordTracker;
     }
 
     /**
@@ -187,7 +179,7 @@ private:
     std::unique_ptr<TemporaryRecordStore> _sideWritesTable;
 
     // Records RecordIds that have been skipped due to indexing errors.
-    std::unique_ptr<SkippedRecordTracker> _skippedRecordTracker;
+    SkippedRecordTracker _skippedRecordTracker;
 
     std::unique_ptr<DuplicateKeyTracker> _duplicateKeyTracker;
 
diff --git a/src/mongo/db/index_builds_coordinator.cpp b/src/mongo/db/index_builds_coordinator.cpp
index da7001c1539..4418d21d51c 100644
--- a/src/mongo/db/index_builds_coordinator.cpp
+++ b/src/mongo/db/index_builds_coordinator.cpp
@@ -241,13 +241,12 @@ void unlockRSTL(OperationContext* opCtx) {
 void logFailure(Status status,
                 const NamespaceString& nss,
                 std::shared_ptr<ReplIndexBuildState> replState) {
-    LOGV2(
-        20649,
-        "Index build failed: {replState_buildUUID}: {nss} ( {replState_collectionUUID} ): {status}",
-        "replState_buildUUID"_attr = replState->buildUUID,
-        "nss"_attr = nss,
-        "replState_collectionUUID"_attr = replState->collectionUUID,
-        "status"_attr = status);
+    LOGV2(20649,
+          "Index build failed",
+          "buildUUID"_attr = replState->buildUUID,
+          "collection"_attr = nss,
+          "collectionUUID"_attr = replState->collectionUUID,
+          "status"_attr = status);
 }
 
 /**
@@ -1384,6 +1383,7 @@ void IndexBuildsCoordinator::createIndexes(OperationContext* opCtx,
     });
     uassertStatusOK(_indexBuildsManager.startBuildingIndex(opCtx, collection, buildUUID));
 
+    uassertStatusOK(_indexBuildsManager.retrySkippedRecords(opCtx, buildUUID, collection));
     uassertStatusOK(_indexBuildsManager.checkIndexConstraintViolations(opCtx, buildUUID));
 
     auto opObserver = opCtx->getServiceContext()->getOpObserver();
@@ -1522,7 +1522,7 @@ void IndexBuildsCoordinator::_unregisterIndexBuild(
 
     invariant(_allIndexBuilds.erase(replIndexBuildState->buildUUID));
 
-    LOGV2(4656004, "unregistering index build", "buildUUID"_attr = replIndexBuildState->buildUUID);
+    LOGV2(4656004, "Unregistering index build", "buildUUID"_attr = replIndexBuildState->buildUUID);
     _indexBuildsManager.unregisterIndexBuild(replIndexBuildState->buildUUID);
     _indexBuildsCondVar.notify_all();
 }
@@ -1967,6 +1967,16 @@ void IndexBuildsCoordinator::_runIndexBuildInner(OperationContext* opCtx,
         return;
     }
 
+    {
+        // If the index build has already been cleaned-up because it encountered an error at
+        // commit-time, there is no work to do. This is the most routine case, since index
+        // constraint checking happens at commit-time for index builds.
+        stdx::unique_lock<Latch> lk(replState->mutex);
+        if (replState->indexBuildState.isAborted()) {
+            uassertStatusOK(status);
+        }
+    }
+
     // We do not hold a collection lock here, but we are protected against the collection being
     // dropped while the index build is still registered for the collection -- until abortIndexBuild
     // is called. The collection can be renamed, but it is OK for the name to be stale just for
@@ -1980,31 +1990,20 @@ void IndexBuildsCoordinator::_runIndexBuildInner(OperationContext* opCtx,
     NamespaceString nss = collection->ns();
     logFailure(status, nss, replState);
 
-    {
-        // If the index build has already been cleaned-up because it encountered an error at
-        // commit-time, there is no work to do. This is the most routine case, since index
-        // constraint checking happens at commit-time for two phase index builds.
-        stdx::unique_lock<Latch> lk(replState->mutex);
-        if (replState->indexBuildState.isAborted()) {
-            uassertStatusOK(status);
-        }
-    }
-
     // If we received an external abort, the caller should have already set our state to kAborted.
     invariant(status.code() != ErrorCodes::IndexBuildAborted);
 
+    // Index builds only check index constraints when committing. If an error occurs at that point,
+    // then the build is cleaned up while still holding the appropriate locks. The only errors that
+    // we cannot anticipate are user interrupts and shutdown errors.
+    invariant(status.isA<ErrorCategory::Interruption>() ||
+                  status.isA<ErrorCategory::ShutdownError>(),
+              str::stream() << "Unnexpected error code during index build cleanup: " << status);
     if (IndexBuildProtocol::kSinglePhase == replState->protocol) {
         _cleanUpSinglePhaseAfterFailure(opCtx, collection, replState, indexBuildOptions, status);
     } else {
         invariant(IndexBuildProtocol::kTwoPhase == replState->protocol,
                   str::stream() << replState->buildUUID);
-        // Two-phase index builds only check index constraints when committing. If an error occurs
-        // at that point, then the build is cleaned up while still holding the appropriate locks.
-        // The only errors that we cannot anticipate are user interrupts and shutdown errors.
-        invariant(status.isA<ErrorCategory::Interruption>() ||
-                      status.isA<ErrorCategory::ShutdownError>(),
-                  str::stream() << "Unnexpected error code during two-phase index build cleanup: "
-                                << status);
         _cleanUpTwoPhaseAfterFailure(opCtx, collection, replState, indexBuildOptions, status);
     }
 
@@ -2152,7 +2151,9 @@ IndexBuildsCoordinator::CommitResult IndexBuildsCoordinator::_insertKeysFromSide
     }
 
     // If we are no longer primary after receiving a commit quorum, we must restart and wait for a
-    // new signal from a new primary because we cannot commit.
+    // new signal from a new primary because we cannot commit. Note that two-phase index builds can
+    // retry because a new signal should be received. Single-phase builds will be unable to commit
+    // and will self-abort.
     bool isMaster = replCoord->canAcceptWritesFor(opCtx, dbAndUUID);
     if (!isMaster && IndexBuildAction::kCommitQuorumSatisfied == action) {
         return CommitResult::kNoLongerPrimary;
@@ -2190,17 +2191,23 @@ IndexBuildsCoordinator::CommitResult IndexBuildsCoordinator::_insertKeysFromSide
         IndexBuildInterceptor::DrainYieldPolicy::kNoYield));
 
     try {
-        if (MONGO_unlikely(failIndexBuildOnCommit.shouldFail())) {
-            uasserted(4698903, "index build aborted due to failpoint");
+        failIndexBuildOnCommit.execute(
+            [](const BSONObj&) { uasserted(4698903, "index build aborted due to failpoint"); });
+
+        // If we are no longer primary and a single phase index build started as primary attempts to
+        // commit, trigger a self-abort.
+        if (!isMaster && IndexBuildAction::kSinglePhaseCommit == action &&
+            !indexBuildOptions.replSetAndNotPrimaryAtStart) {
+            uassertStatusOK(
+                {ErrorCodes::NotMaster,
+                 str::stream() << "Unable to commit index build because we are no longer primary: "
+                               << replState->buildUUID});
         }
 
-        // Retry indexing records that failed key generation while relaxing constraints (i.e. while
-        // a secondary node), but only if we are primary and committing the index build and during
-        // two-phase builds. Single-phase index builds are not resilient to state transitions and do
-        // not track skipped records. Secondaries rely on the primary's decision to commit as
-        // assurance that it has checked all key generation errors on its behalf.
-        if (IndexBuildProtocol::kTwoPhase == replState->protocol &&
-            replCoord->canAcceptWritesFor(opCtx, collection->ns())) {
+        // Retry indexing records that failed key generation, but only if we are primary.
+        // Secondaries rely on the primary's decision to commit as assurance that it has checked all
+        // key generation errors on its behalf.
+        if (isMaster) {
             uassertStatusOK(
                 _indexBuildsManager.retrySkippedRecords(opCtx, replState->buildUUID, collection));
         }
@@ -2217,19 +2224,30 @@ IndexBuildsCoordinator::CommitResult IndexBuildsCoordinator::_insertKeysFromSide
             uassertStatusOK(
                 _indexBuildsManager.checkIndexConstraintViolations(opCtx, replState->buildUUID));
         }
-    } catch (const ExceptionForCat<ErrorCategory::ShutdownError>&) {
+    } catch (const ExceptionForCat<ErrorCategory::ShutdownError>& e) {
+        logFailure(e.toStatus(), collection->ns(), replState);
         _completeAbortForShutdown(opCtx, replState, collection);
         throw;
     } catch (const DBException& e) {
+        auto status = e.toStatus();
+        logFailure(status, collection->ns(), replState);
+
         // It is illegal to abort the index build at this point. Note that Interruption exceptions
         // are allowed because we cannot control them as they bypass the routine abort machinery.
         invariant(e.code() != ErrorCodes::IndexBuildAborted);
 
-        // Index builds may not fail on secondaries at this point. If a primary replicated an
-        // abortIndexBuild oplog entry, then this index build would have been interrupted before
-        // committing with an IndexBuildAborted error code.
-        auto status = e.toStatus();
-        if (!isMaster) {
+        // Index build commit may not fail on secondaries because it implies diverenge with data on
+        // the primary. The only exception is single-phase builds started on primaries, which may
+        // fail after a state transition. In this case, we have not replicated anything to
+        // roll-back. With two-phase index builds, if a primary replicated an abortIndexBuild oplog
+        // entry, then this index build should have been interrupted before committing with an
+        // IndexBuildAborted error code.
+        const bool twoPhaseAndNotPrimary =
+            IndexBuildProtocol::kTwoPhase == replState->protocol && !isMaster;
+        const bool singlePhaseAndNotPrimaryAtStart =
+            IndexBuildProtocol::kSinglePhase == replState->protocol &&
+            indexBuildOptions.replSetAndNotPrimaryAtStart;
+        if (twoPhaseAndNotPrimary || singlePhaseAndNotPrimaryAtStart) {
             LOGV2_FATAL(4698902,
                         "Index build failed while not primary",
                         "buildUUID"_attr = replState->buildUUID,
diff --git a/src/mongo/dbtests/dbtests.cpp b/src/mongo/dbtests/dbtests.cpp
index cb440b75031..9f4d4b63984 100644
--- a/src/mongo/dbtests/dbtests.cpp
+++ b/src/mongo/dbtests/dbtests.cpp
@@ -131,6 +131,10 @@ Status createIndexFromSpec(OperationContext* opCtx, StringData ns, const BSONObj
     if (!status.isOK()) {
         return status;
     }
+    status = indexer.retrySkippedRecords(opCtx, coll);
+    if (!status.isOK()) {
+        return status;
+    }
     status = indexer.checkConstraints(opCtx);
     if (!status.isOK()) {
         return status;