SERVER-46123 Remove ScopedStopNewDatabaseIndexBuilds/ScopedStopNewCollectionIndexBuilds and add the ability to abort index builds on the database without having to wait

3 files changed, 39 insertions, 369 deletions

diff --git a/src/mongo/db/index_builds_coordinator.cpp b/src/mongo/db/index_builds_coordinator.cpp
index 3b524dea8c7..fafdc30c716 100644
--- a/src/mongo/db/index_builds_coordinator.cpp
+++ b/src/mongo/db/index_builds_coordinator.cpp
@@ -340,8 +340,6 @@ IndexBuildsCoordinator* IndexBuildsCoordinator::get(OperationContext* operationC
 
 IndexBuildsCoordinator::~IndexBuildsCoordinator() {
     invariant(_databaseIndexBuilds.empty());
-    invariant(_disallowedDbs.empty());
-    invariant(_disallowedCollections.empty());
     invariant(_collectionIndexBuilds.empty());
 }
 
@@ -549,25 +547,43 @@ std::vector<UUID> IndexBuildsCoordinator::abortCollectionIndexBuildsNoWait(
     return _abortCollectionIndexBuilds(lk, collectionUUID, reason, shouldWait);
 }
 
-void IndexBuildsCoordinator::abortDatabaseIndexBuilds(StringData db, const std::string& reason) {
-    stdx::unique_lock<Latch> lk(_mutex);
-
-    // Ensure the caller correctly stopped any new index builds on the database.
-    auto it = _disallowedDbs.find(db);
-    invariant(it != _disallowedDbs.end());
-
+void IndexBuildsCoordinator::_abortDatabaseIndexBuilds(stdx::unique_lock<Latch>& lk,
+                                                       const StringData& db,
+                                                       const std::string& reason,
+                                                       bool shouldWait) {
     auto dbIndexBuilds = _databaseIndexBuilds[db];
     if (!dbIndexBuilds) {
         return;
     }
 
+    LOGV2(4612302,
+          "About to abort all index builders running for collections in the given database",
+          "database"_attr = db);
+
     dbIndexBuilds->runOperationOnAllBuilds(lk, &_indexBuildsManager, abortIndexBuild, reason);
 
+    if (!shouldWait) {
+        return;
+    }
+
     // 'dbIndexBuilds' is a shared ptr, so it can be safely waited upon without destructing before
     // waitUntilNoIndexBuildsRemain() returns, which would cause a use-after-free memory error.
     dbIndexBuilds->waitUntilNoIndexBuildsRemain(lk);
 }
 
+void IndexBuildsCoordinator::abortDatabaseIndexBuilds(StringData db, const std::string& reason) {
+    stdx::unique_lock<Latch> lk(_mutex);
+    const bool shouldWait = true;
+    _abortDatabaseIndexBuilds(lk, db, reason, shouldWait);
+}
+
+void IndexBuildsCoordinator::abortDatabaseIndexBuildsNoWait(StringData db,
+                                                            const std::string& reason) {
+    stdx::unique_lock<Latch> lk(_mutex);
+    const bool shouldWait = false;
+    _abortDatabaseIndexBuilds(lk, db, reason, shouldWait);
+}
+
 namespace {
 NamespaceString getNsFromUUID(OperationContext* opCtx, const UUID& uuid) {
     auto& catalog = CollectionCatalog::get(opCtx);
@@ -1133,8 +1149,6 @@ void IndexBuildsCoordinator::sleepIndexBuilds_forTestOnly(bool sleep) {
 
 void IndexBuildsCoordinator::verifyNoIndexBuilds_forTestOnly() {
     invariant(_databaseIndexBuilds.empty());
-    invariant(_disallowedDbs.empty());
-    invariant(_disallowedCollections.empty());
     invariant(_collectionIndexBuilds.empty());
 }
 
@@ -1170,16 +1184,6 @@ void IndexBuildsCoordinator::updateCurOpOpDescription(OperationContext* opCtx,
 
 Status IndexBuildsCoordinator::_registerIndexBuild(
     WithLock lk, std::shared_ptr<ReplIndexBuildState> replIndexBuildState) {
-
-    auto itns = _disallowedCollections.find(replIndexBuildState->collectionUUID);
-    auto itdb = _disallowedDbs.find(replIndexBuildState->dbName);
-    if (itns != _disallowedCollections.end() || itdb != _disallowedDbs.end()) {
-        return Status(ErrorCodes::CannotCreateIndex,
-                      str::stream() << "Collection ( " << replIndexBuildState->collectionUUID
-                                    << " ) is in the process of being dropped. New index builds "
-                                       "are not currently allowed.");
-    }
-
     // Check whether any indexes are already being built with the same index name(s). (Duplicate
     // specs will be discovered by the index builder.)
     auto collIndexBuildsIt = _collectionIndexBuilds.find(replIndexBuildState->collectionUUID);
@@ -2159,50 +2163,6 @@ StatusWith<std::pair<long long, long long>> IndexBuildsCoordinator::_runIndexReb
     return status;
 }
 
-void IndexBuildsCoordinator::_stopIndexBuildsOnDatabase(StringData dbName) {
-    stdx::unique_lock<Latch> lk(_mutex);
-
-    auto it = _disallowedDbs.find(dbName);
-    if (it != _disallowedDbs.end()) {
-        ++(it->second);
-        return;
-    }
-    _disallowedDbs[dbName] = 1;
-}
-
-void IndexBuildsCoordinator::_stopIndexBuildsOnCollection(const UUID& collectionUUID) {
-    stdx::unique_lock<Latch> lk(_mutex);
-
-    auto it = _disallowedCollections.find(collectionUUID);
-    if (it != _disallowedCollections.end()) {
-        ++(it->second);
-        return;
-    }
-    _disallowedCollections[collectionUUID] = 1;
-}
-
-void IndexBuildsCoordinator::_allowIndexBuildsOnDatabase(StringData dbName) {
-    stdx::unique_lock<Latch> lk(_mutex);
-
-    auto it = _disallowedDbs.find(dbName);
-    invariant(it != _disallowedDbs.end());
-    invariant(it->second);
-    if (--(it->second) == 0) {
-        _disallowedDbs.erase(it);
-    }
-}
-
-void IndexBuildsCoordinator::_allowIndexBuildsOnCollection(const UUID& collectionUUID) {
-    stdx::unique_lock<Latch> lk(_mutex);
-
-    auto it = _disallowedCollections.find(collectionUUID);
-    invariant(it != _disallowedCollections.end());
-    invariant(it->second > 0);
-    if (--(it->second) == 0) {
-        _disallowedCollections.erase(it);
-    }
-}
-
 StatusWith<std::shared_ptr<ReplIndexBuildState>> IndexBuildsCoordinator::_getIndexBuild(
     const UUID& buildUUID) const {
     stdx::unique_lock<Latch> lk(_mutex);
@@ -2224,26 +2184,6 @@ std::vector<std::shared_ptr<ReplIndexBuildState>> IndexBuildsCoordinator::_getIn
     return indexBuilds;
 }
 
-ScopedStopNewDatabaseIndexBuilds::ScopedStopNewDatabaseIndexBuilds(
-    IndexBuildsCoordinator* indexBuildsCoordinator, StringData dbName)
-    : _indexBuildsCoordinatorPtr(indexBuildsCoordinator), _dbName(dbName.toString()) {
-    _indexBuildsCoordinatorPtr->_stopIndexBuildsOnDatabase(_dbName);
-}
-
-ScopedStopNewDatabaseIndexBuilds::~ScopedStopNewDatabaseIndexBuilds() {
-    _indexBuildsCoordinatorPtr->_allowIndexBuildsOnDatabase(_dbName);
-}
-
-ScopedStopNewCollectionIndexBuilds::ScopedStopNewCollectionIndexBuilds(
-    IndexBuildsCoordinator* indexBuildsCoordinator, const UUID& collectionUUID)
-    : _indexBuildsCoordinatorPtr(indexBuildsCoordinator), _collectionUUID(collectionUUID) {
-    _indexBuildsCoordinatorPtr->_stopIndexBuildsOnCollection(_collectionUUID);
-}
-
-ScopedStopNewCollectionIndexBuilds::~ScopedStopNewCollectionIndexBuilds() {
-    _indexBuildsCoordinatorPtr->_allowIndexBuildsOnCollection(_collectionUUID);
-}
-
 int IndexBuildsCoordinator::getNumIndexesTotal(OperationContext* opCtx, Collection* collection) {
     invariant(collection);
     const auto& nss = collection->ns();
diff --git a/src/mongo/db/index_builds_coordinator.h b/src/mongo/db/index_builds_coordinator.h
index 1be72d0554c..2e4c42d1ef8 100644
--- a/src/mongo/db/index_builds_coordinator.h
+++ b/src/mongo/db/index_builds_coordinator.h
@@ -197,16 +197,6 @@ public:
      * must continue to operate on the collection by UUID to protect against rename collection. The
      * provided 'reason' will be used in the error message that the index builders return to their
      * callers.
-     *
-     * First create a ScopedStopNewCollectionIndexBuilds to block further index builds on the
-     * collection before calling this and for the duration of the drop collection operation.
-     *
-     * {
-     *     ScopedStopNewCollectionIndexBuilds scopedStop(collectionUUID);
-     *     indexBuildsCoord->abortCollectionIndexBuilds(collectionUUID, "...");
-     *     AutoGetCollection autoColl(..., collectionUUID, ...);
-     *     autoColl->dropCollection(...);
-     * }
      */
     void abortCollectionIndexBuilds(const UUID& collectionUUID, const std::string& reason);
 
@@ -223,21 +213,16 @@ public:
      * Signals all of the index builds on the specified 'db' to abort and then waits until the index
      * builds are no longer running. The provided 'reason' will be used in the error message that
      * the index builders return to their callers.
-     *
-     * First create a ScopedStopNewDatabaseIndexBuilds to block further index builds on the
-     * specified
-     * database before calling this and for the duration of the drop database operation.
-     *
-     * {
-     *     ScopedStopNewDatabaseIndexBuilds scopedStop(dbName);
-     *     indexBuildsCoord->abortDatabaseIndexBuilds(dbName, "...");
-     *     AutoGetDb autoDb(...);
-     *     autoDb->dropDatabase(...);
-     * }
      */
     void abortDatabaseIndexBuilds(StringData db, const std::string& reason);
 
     /**
+     * Signals all of the index builds on the specified database to abort. The provided 'reason'
+     * will be used in the error message that the index builders return to their callers.
+     */
+    void abortDatabaseIndexBuildsNoWait(StringData db, const std::string& reason);
+
+    /**
      * Aborts an index build by index build UUID. Returns when the index build thread exits.
      */
     void abortIndexBuildByBuildUUID(OperationContext* opCtx,
@@ -437,28 +422,6 @@ public:
 
 
 private:
-    // Friend classes in order to be the only allowed callers of
-    //_stopIndexBuildsOnCollection/Database and _allowIndexBuildsOnCollection/Database.
-    friend class ScopedStopNewDatabaseIndexBuilds;
-    friend class ScopedStopNewCollectionIndexBuilds;
-
-    /**
-     * Prevents new index builds being registered on the provided collection or database.
-     *
-     * It is safe to call this on the same collection/database concurrently in different threads. It
-     * will still behave correctly.
-     */
-    void _stopIndexBuildsOnDatabase(StringData dbName);
-    void _stopIndexBuildsOnCollection(const UUID& collectionUUID);
-
-    /**
-     * Allows new index builds to again be registered on the provided collection or database. Should
-     * only be called after calling stopIndexBuildsOnCollection or stopIndexBuildsOnDatabase on the
-     * same collection or database, respectively.
-     */
-    void _allowIndexBuildsOnDatabase(StringData dbName);
-    void _allowIndexBuildsOnCollection(const UUID& collectionUUID);
-
     /**
      * Registers an index build so that the rest of the system can discover it.
      *
@@ -673,16 +636,17 @@ protected:
                                                   const std::string& reason,
                                                   bool shouldWait);
 
+    /**
+     * Helper for 'abortDatabaseIndexBuilds' and 'abortDatabaseIndexBuildsNoWait'.
+     */
+    void _abortDatabaseIndexBuilds(stdx::unique_lock<Latch>& lk,
+                                   const StringData& db,
+                                   const std::string& reason,
+                                   bool shouldWait);
+
     // Protects the below state.
     mutable Mutex _mutex = MONGO_MAKE_LATCH("IndexBuildsCoordinator::_mutex");
 
-    // New index builds are not allowed on a collection or database if the collection or database is
-    // in either of these maps. These are used when concurrent operations need to abort index builds
-    // on a collection or database and must wait for the index builds to drain, without further
-    // index builds being allowed to begin.
-    StringMap<int> _disallowedDbs;
-    stdx::unordered_map<UUID, int, UUID::Hash> _disallowedCollections;
-
     // Maps database name to database information. Tracks and accesses index builds on a database
     // level. Can be used to abort and wait upon the completion of all index builds for a database.
     //
@@ -709,68 +673,6 @@ protected:
     bool _sleepForTest = false;
 };
 
-/**
- * For this object's lifetime no new index builds will be allowed on the specified database. An
- * error will be returned by the IndexBuildsCoordinator to any caller attempting to register a new
- * index build on the blocked collection or database.
- *
- * This should be used by operations like drop database, where the active index builds must be
- * signaled to abort, but it takes time for them to wrap up, during which time no further index
- * builds should be scheduled.
- */
-class ScopedStopNewDatabaseIndexBuilds {
-    ScopedStopNewDatabaseIndexBuilds(const ScopedStopNewDatabaseIndexBuilds&) = delete;
-    ScopedStopNewDatabaseIndexBuilds& operator=(const ScopedStopNewDatabaseIndexBuilds&) = delete;
-
-public:
-    /**
-     * Takes either the full collection namespace or a database name and will block further index
-     * builds on that collection or database.
-     */
-    ScopedStopNewDatabaseIndexBuilds(IndexBuildsCoordinator* indexBuildsCoordinator,
-                                     StringData dbName);
-
-    /**
-     * Allows new index builds on the collection or database that were previously disallowed.
-     */
-    ~ScopedStopNewDatabaseIndexBuilds();
-
-private:
-    IndexBuildsCoordinator* _indexBuildsCoordinatorPtr;
-    std::string _dbName;
-};
-
-/**
- * For this object's lifetime no new index builds will be allowed on the specified collection. An
- * error will be returned by the IndexBuildsCoordinator to any caller attempting to register a new
- * index build on the blocked collection.
- *
- * This should be used by operations like drop collection, where the active index builds must be
- * signaled to abort, but it takes time for them to wrap up, during which time no further index
- * builds should be scheduled.
- */
-class ScopedStopNewCollectionIndexBuilds {
-    ScopedStopNewCollectionIndexBuilds(const ScopedStopNewCollectionIndexBuilds&) = delete;
-    ScopedStopNewCollectionIndexBuilds& operator=(const ScopedStopNewCollectionIndexBuilds&) =
-        delete;
-
-public:
-    /**
-     * Blocks further index builds on the specified collection.
-     */
-    ScopedStopNewCollectionIndexBuilds(IndexBuildsCoordinator* indexBuildsCoordinator,
-                                       const UUID& collectionUUID);
-
-    /**
-     * Allows new index builds on the collection that were previously disallowed.
-     */
-    ~ScopedStopNewCollectionIndexBuilds();
-
-private:
-    IndexBuildsCoordinator* _indexBuildsCoordinatorPtr;
-    UUID _collectionUUID;
-};
-
 // These fail points are used to control index build progress. Declared here to be shared
 // temporarily between createIndexes command and IndexBuildsCoordinator.
 extern FailPoint hangAfterIndexBuildFirstDrain;
diff --git a/src/mongo/db/index_builds_coordinator_mongod_test.cpp b/src/mongo/db/index_builds_coordinator_mongod_test.cpp
index 8a9939579b4..8709041d43f 100644
--- a/src/mongo/db/index_builds_coordinator_mongod_test.cpp
+++ b/src/mongo/db/index_builds_coordinator_mongod_test.cpp
@@ -258,178 +258,6 @@ TEST_F(IndexBuildsCoordinatorMongodTest, Registration) {
     ASSERT_NOT_EQUALS(_testFooNss, _othertestFooNss);
 }
 
-// Exercises the stopIndexBuildsOnCollection/Database() and allowIndexBuildsOnCollection/Database()
-// functions, checking that they correctly disallow and allow index builds when
-// ScopedStopNewCollectionIndexBuilds and ScopedStopNewDatabaseIndexBuilds are present on a
-// collection or database name.
-TEST_F(IndexBuildsCoordinatorMongodTest, DisallowNewBuildsOnNamespace) {
-    {
-        _indexBuildsCoord->sleepIndexBuilds_forTestOnly(true);
-
-        // Create a scoped object to block new index builds ONLY on _testFooNss.
-        ScopedStopNewCollectionIndexBuilds scopedStop(_indexBuildsCoord.get(), _testFooUUID);
-
-        // Registering an index build on _testFooNss should fail.
-        ASSERT_EQ(ErrorCodes::CannotCreateIndex,
-                  _indexBuildsCoord
-                      ->startIndexBuild(operationContext(),
-                                        _testFooNss.db().toString(),
-                                        _testFooUUID,
-                                        makeSpecs(_testFooNss, {"a", "b"}),
-                                        UUID::gen(),
-                                        IndexBuildProtocol::kTwoPhase,
-                                        _indexBuildOptions)
-                      .getStatus());
-
-        // Registering index builds on other collections and databases should still succeed.
-        auto testBarFuture =
-            assertGet(_indexBuildsCoord->startIndexBuild(operationContext(),
-                                                         _testBarNss.db().toString(),
-                                                         _testBarUUID,
-                                                         makeSpecs(_testBarNss, {"c", "d"}),
-                                                         UUID::gen(),
-                                                         IndexBuildProtocol::kTwoPhase,
-                                                         _indexBuildOptions));
-        auto othertestFooFuture =
-            assertGet(_indexBuildsCoord->startIndexBuild(operationContext(),
-                                                         _othertestFooNss.db().toString(),
-                                                         _othertestFooUUID,
-                                                         makeSpecs(_othertestFooNss, {"e", "f"}),
-                                                         UUID::gen(),
-                                                         IndexBuildProtocol::kTwoPhase,
-                                                         _indexBuildOptions));
-
-        _indexBuildsCoord->sleepIndexBuilds_forTestOnly(false);
-
-        auto indexCatalogStats = unittest::assertGet(testBarFuture.getNoThrow());
-        ASSERT_EQ(1, indexCatalogStats.numIndexesBefore);
-        ASSERT_EQ(3, indexCatalogStats.numIndexesAfter);
-        indexCatalogStats = unittest::assertGet(othertestFooFuture.getNoThrow());
-        ASSERT_EQ(1, indexCatalogStats.numIndexesBefore);
-        ASSERT_EQ(3, indexCatalogStats.numIndexesAfter);
-    }
-
-    {
-        // Check that the scoped object correctly cleared.
-        auto testFooFuture =
-            assertGet(_indexBuildsCoord->startIndexBuild(operationContext(),
-                                                         _testFooNss.db().toString(),
-                                                         _testFooUUID,
-                                                         makeSpecs(_testFooNss, {"a", "b"}),
-                                                         UUID::gen(),
-                                                         IndexBuildProtocol::kTwoPhase,
-                                                         _indexBuildOptions));
-        auto indexCatalogStats = unittest::assertGet(testFooFuture.getNoThrow());
-        ASSERT_EQ(1, indexCatalogStats.numIndexesBefore);
-        ASSERT_EQ(3, indexCatalogStats.numIndexesAfter);
-    }
-
-    {
-        _indexBuildsCoord->sleepIndexBuilds_forTestOnly(true);
-
-        // Create a scoped object to block new index builds on the 'test' database.
-        ScopedStopNewDatabaseIndexBuilds scopedStop(_indexBuildsCoord.get(), _testFooNss.db());
-
-        // Registering an index build on any collection in the 'test' database should fail.
-        ASSERT_EQ(ErrorCodes::CannotCreateIndex,
-                  _indexBuildsCoord
-                      ->startIndexBuild(operationContext(),
-                                        _testFooNss.db().toString(),
-                                        _testFooUUID,
-                                        makeSpecs(_testFooNss, {"c", "d"}),
-                                        UUID::gen(),
-                                        IndexBuildProtocol::kTwoPhase,
-                                        _indexBuildOptions)
-                      .getStatus());
-        ASSERT_EQ(ErrorCodes::CannotCreateIndex,
-                  _indexBuildsCoord
-                      ->startIndexBuild(operationContext(),
-                                        _testBarNss.db().toString(),
-                                        _testBarUUID,
-                                        makeSpecs(_testBarNss, {"a", "b"}),
-                                        UUID::gen(),
-                                        IndexBuildProtocol::kTwoPhase,
-                                        _indexBuildOptions)
-                      .getStatus());
-
-        // Registering index builds on another database should still succeed.
-        auto othertestFooFuture =
-            assertGet(_indexBuildsCoord->startIndexBuild(operationContext(),
-                                                         _othertestFooNss.db().toString(),
-                                                         _othertestFooUUID,
-                                                         makeSpecs(_othertestFooNss, {"g", "h"}),
-                                                         UUID::gen(),
-                                                         IndexBuildProtocol::kTwoPhase,
-                                                         _indexBuildOptions));
-
-        _indexBuildsCoord->sleepIndexBuilds_forTestOnly(false);
-
-        auto indexCatalogStats = unittest::assertGet(othertestFooFuture.getNoThrow());
-        ASSERT_EQ(3, indexCatalogStats.numIndexesBefore);
-        ASSERT_EQ(5, indexCatalogStats.numIndexesAfter);
-    }
-
-    {
-        // Check that the scoped object correctly cleared.
-        auto testFooFuture =
-            assertGet(_indexBuildsCoord->startIndexBuild(operationContext(),
-                                                         _testFooNss.db().toString(),
-                                                         _testFooUUID,
-                                                         makeSpecs(_testFooNss, {"c", "d"}),
-                                                         UUID::gen(),
-                                                         IndexBuildProtocol::kTwoPhase,
-                                                         _indexBuildOptions));
-        auto indexCatalogStats = unittest::assertGet(testFooFuture.getNoThrow());
-        ASSERT_EQ(3, indexCatalogStats.numIndexesBefore);
-        ASSERT_EQ(5, indexCatalogStats.numIndexesAfter);
-    }
-
-    {
-        // Test concurrency of multiple scoped objects to block an index builds.
-
-        ScopedStopNewCollectionIndexBuilds scopedStop(_indexBuildsCoord.get(), _testFooUUID);
-        {
-            ScopedStopNewCollectionIndexBuilds scopedStop(_indexBuildsCoord.get(), _testFooUUID);
-
-            ASSERT_EQ(ErrorCodes::CannotCreateIndex,
-                      _indexBuildsCoord
-                          ->startIndexBuild(operationContext(),
-                                            _testFooNss.db().toString(),
-                                            _testFooUUID,
-                                            makeSpecs(_testFooNss, {"e", "f"}),
-                                            UUID::gen(),
-                                            IndexBuildProtocol::kTwoPhase,
-                                            _indexBuildOptions)
-                          .getStatus());
-        }
-        ASSERT_EQ(ErrorCodes::CannotCreateIndex,
-                  _indexBuildsCoord
-                      ->startIndexBuild(operationContext(),
-                                        _testFooNss.db().toString(),
-                                        _testFooUUID,
-                                        makeSpecs(_testFooNss, {"e", "f"}),
-                                        UUID::gen(),
-                                        IndexBuildProtocol::kTwoPhase,
-                                        _indexBuildOptions)
-                      .getStatus());
-    }
-
-    {
-        // Check that the scoped object correctly cleared.
-        auto testFooFuture =
-            assertGet(_indexBuildsCoord->startIndexBuild(operationContext(),
-                                                         _testFooNss.db().toString(),
-                                                         _testFooUUID,
-                                                         makeSpecs(_testFooNss, {"e", "f"}),
-                                                         UUID::gen(),
-                                                         IndexBuildProtocol::kTwoPhase,
-                                                         _indexBuildOptions));
-        auto indexCatalogStats = unittest::assertGet(testFooFuture.getNoThrow());
-        ASSERT_EQ(5, indexCatalogStats.numIndexesBefore);
-        ASSERT_EQ(7, indexCatalogStats.numIndexesAfter);
-    }
-}
-
 TEST_F(IndexBuildsCoordinatorMongodTest, SetCommitQuorumWithBadArguments) {
     _indexBuildsCoord->sleepIndexBuilds_forTestOnly(true);