SERVER-31047 Rollback properly removes redundant index operations

3 files changed, 274 insertions, 83 deletions

diff --git a/src/mongo/db/repl/rs_rollback.cpp b/src/mongo/db/repl/rs_rollback.cpp
index b54eea9a7d8..f8ca535773d 100644
--- a/src/mongo/db/repl/rs_rollback.cpp
+++ b/src/mongo/db/repl/rs_rollback.cpp
@@ -122,19 +122,47 @@ void FixUpInfo::removeRedundantOperations() {
 }
 
 bool FixUpInfo::removeRedundantIndexCommands(UUID uuid, std::string indexName) {
+    LOG(2) << "Attempting to remove redundant index operations from the set of indexes to create "
+              "for collection "
+           << uuid << ", for index '" << indexName << "'";
+
+    // See if there are any indexes to create for this collection.
     auto indexes = indexesToCreate.find(uuid);
-    if (indexes != indexesToCreate.end()) {
-        invariant(indexesToCreate.count(uuid) == 1);
-        invariant((*indexes).second.count(indexName) == 1);
-        (*indexes).second.erase(indexName);
-        if ((*indexes).second.empty()) {
-            indexesToCreate.erase(uuid);
-        }
-        log() << "Index " << indexName
-              << " was previously dropped. The createIndexes command is canceled out.";
-        return true;
+
+    // There are no indexes to create for this collection UUID, so there are no index creation
+    // operations to remove.
+    if (indexes == indexesToCreate.end()) {
+        LOG(2)
+            << "Collection " << uuid
+            << " has no indexes to create. Not removing any index creation operations for index '"
+            << indexName << "'.";
+        return false;
+    }
+
+    // This is the set of all indexes to create for the given collection UUID. Keep a reference so
+    // we can modify the original object.
+    std::map<std::string, BSONObj>* indexesToCreateForColl = &(indexes->second);
+
+    // If this index was not previously added to the set of indexes that need to be created for this
+    // collection, then we do nothing.
+    if (indexesToCreateForColl->find(indexName) == indexesToCreateForColl->end()) {
+        LOG(2) << "Index '" << indexName << "' was not previously set to be created for collection "
+               << uuid << ". Not removing any index creation operations.";
+        return false;
     }
-    return false;
+
+    // This index was previously added to the set of indexes to create for this collection, so we
+    // remove it from that set.
+    LOG(2) << "Index '" << indexName << "' was previously set to be created for collection " << uuid
+           << ". Removing this redundant index creation operation.";
+    indexesToCreateForColl->erase(indexName);
+    // If there are now no remaining indexes to create for this collection, remove it from
+    // the set of collections that we need to create indexes for.
+    if (indexesToCreateForColl->empty()) {
+        indexesToCreate.erase(uuid);
+    }
+
+    return true;
 }
 
 void FixUpInfo::recordRollingBackDrop(const NamespaceString& nss, OpTime opTime, UUID uuid) {
diff --git a/src/mongo/db/repl/rs_rollback.h b/src/mongo/db/repl/rs_rollback.h
index 265e3104a62..fade3074331 100644
--- a/src/mongo/db/repl/rs_rollback.h
+++ b/src/mongo/db/repl/rs_rollback.h
@@ -312,10 +312,9 @@ struct FixUpInfo {
     void removeRedundantOperations();
 
     /**
-     * Removes any redundant index commands. An example is if we create
-     * an index with name "a_1" and then later proceed to drop that index.
-     * We return true if a redundant index command was found and false
-     * if it was not.
+     * Removes any redundant index commands. For example, if we create an index with name "a_1" and
+     * then later proceed to drop that index, we can ignore the first index creation. We return true
+     * if a redundant index command was removed and false if it was not.
      */
     bool removeRedundantIndexCommands(UUID uuid, std::string indexName);
 
diff --git a/src/mongo/db/repl/rs_rollback_test.cpp b/src/mongo/db/repl/rs_rollback_test.cpp
index ad413f2db21..dd849c9d1a8 100644
--- a/src/mongo/db/repl/rs_rollback_test.cpp
+++ b/src/mongo/db/repl/rs_rollback_test.cpp
@@ -182,16 +182,13 @@ OplogInterfaceMock::Operation makeCreateIndexOplogEntry(Collection* collection,
                                                         BSONObj key,
                                                         std::string indexName,
                                                         int time) {
-    auto indexSpec = BSON("createIndexes"
-                          << "t"
-                          << "ns"
-                          << collection->ns().ns()
-                          << "v"
-                          << static_cast<int>(kIndexVersion)
-                          << "key"
-                          << key
-                          << "name"
-                          << indexName);
+    auto indexSpec =
+        BSON("createIndexes" << collection->ns().coll() << "ns" << collection->ns().ns() << "v"
+                             << static_cast<int>(kIndexVersion)
+                             << "key"
+                             << key
+                             << "name"
+                             << indexName);
 
     return std::make_pair(BSON("ts" << Timestamp(Seconds(time), 0) << "h" << 1LL << "op"
                                     << "c"
@@ -234,6 +231,23 @@ OplogInterfaceMock::Operation makeRenameCollectionOplogEntry(const NamespaceStri
         RecordId(opTime.getTimestamp().getSecs()));
 }
 
+// Create an index on the given collection. Returns the number of indexes that exist on the
+// collection after the given index is created.
+int createIndexForColl(OperationContext* opCtx,
+                       Collection* coll,
+                       NamespaceString nss,
+                       BSONObj indexSpec) {
+    Lock::DBLock dbLock(opCtx, nss.db(), MODE_X);
+    MultiIndexBlock indexer(opCtx, coll);
+    ASSERT_OK(indexer.init(indexSpec).getStatus());
+    WriteUnitOfWork wunit(opCtx);
+    indexer.commit();
+    wunit.commit();
+    auto indexCatalog = coll->getIndexCatalog();
+    ASSERT(indexCatalog);
+    return indexCatalog->numIndexesReady(opCtx);
+}
+
 TEST_F(RSRollbackTest, InconsistentMinValid) {
     _replicationProcess->getConsistencyMarkers()->setAppliedThrough(
         _opCtx.get(), OpTime(Timestamp(Seconds(0), 0), 0));
@@ -523,26 +537,15 @@ TEST_F(RSRollbackTest, RollbackCreateIndexCommand) {
     createOplog(_opCtx.get());
     CollectionOptions options;
     options.uuid = UUID::gen();
-    auto collection = _createCollection(_opCtx.get(), "test.t", options);
-    auto indexSpec = BSON("ns"
-                          << "test.t"
-                          << "v"
-                          << static_cast<int>(kIndexVersion)
-                          << "key"
-                          << BSON("a" << 1)
-                          << "name"
-                          << "a_1");
-    {
-        Lock::DBLock dbLock(_opCtx.get(), "test", MODE_X);
-        MultiIndexBlock indexer(_opCtx.get(), collection);
-        ASSERT_OK(indexer.init(indexSpec).getStatus());
-        WriteUnitOfWork wunit(_opCtx.get());
-        indexer.commit();
-        wunit.commit();
-        auto indexCatalog = collection->getIndexCatalog();
-        ASSERT(indexCatalog);
-        ASSERT_EQUALS(2, indexCatalog->numIndexesReady(_opCtx.get()));
-    }
+    NamespaceString nss("test", "coll");
+    auto collection = _createCollection(_opCtx.get(), nss.toString(), options);
+    auto indexSpec = BSON("ns" << nss.toString() << "v" << static_cast<int>(kIndexVersion) << "key"
+                               << BSON("a" << 1)
+                               << "name"
+                               << "a_1");
+
+    int numIndexes = createIndexForColl(_opCtx.get(), collection, nss, indexSpec);
+    ASSERT_EQUALS(2, numIndexes);
 
     auto commonOperation =
         std::make_pair(BSON("ts" << Timestamp(Seconds(1), 0) << "h" << 1LL), RecordId(1));
@@ -564,12 +567,14 @@ TEST_F(RSRollbackTest, RollbackCreateIndexCommand) {
         _replicationProcess.get()));
     stopCapturingLogMessages();
     ASSERT_EQUALS(1,
-                  countLogLinesContaining(
-                      str::stream() << "Dropped index in rollback for collection: test.t, UUID: "
-                                    << options.uuid->toString()
-                                    << ", index: a_1"));
+                  countLogLinesContaining(str::stream()
+                                          << "Dropped index in rollback for collection: "
+                                          << nss.toString()
+                                          << ", UUID: "
+                                          << options.uuid->toString()
+                                          << ", index: a_1"));
     {
-        Lock::DBLock dbLock(_opCtx.get(), "test", MODE_S);
+        Lock::DBLock dbLock(_opCtx.get(), nss.db(), MODE_S);
         auto indexCatalog = collection->getIndexCatalog();
         ASSERT(indexCatalog);
         ASSERT_EQUALS(1, indexCatalog->numIndexesReady(_opCtx.get()));
@@ -733,27 +738,16 @@ TEST_F(RSRollbackTest, RollingBackDropAndCreateOfSameIndexNameWithDifferentSpecs
     createOplog(_opCtx.get());
     CollectionOptions options;
     options.uuid = UUID::gen();
-    auto collection = _createCollection(_opCtx.get(), "test.t", options);
+    NamespaceString nss("test", "coll");
+    auto collection = _createCollection(_opCtx.get(), nss.toString(), options);
 
-    auto indexSpec = BSON("ns"
-                          << "test.t"
-                          << "v"
-                          << static_cast<int>(kIndexVersion)
-                          << "key"
-                          << BSON("b" << 1)
-                          << "name"
-                          << "a_1");
-    {
-        Lock::DBLock dbLock(_opCtx.get(), "test", MODE_X);
-        MultiIndexBlock indexer(_opCtx.get(), collection);
-        ASSERT_OK(indexer.init(indexSpec).getStatus());
-        WriteUnitOfWork wunit(_opCtx.get());
-        indexer.commit();
-        wunit.commit();
-        auto indexCatalog = collection->getIndexCatalog();
-        ASSERT(indexCatalog);
-        ASSERT_EQUALS(2, indexCatalog->numIndexesReady(_opCtx.get()));
-    }
+    auto indexSpec = BSON("ns" << nss.toString() << "v" << static_cast<int>(kIndexVersion) << "key"
+                               << BSON("b" << 1)
+                               << "name"
+                               << "a_1");
+
+    int numIndexes = createIndexForColl(_opCtx.get(), collection, nss, indexSpec);
+    ASSERT_EQUALS(2, numIndexes);
 
     auto commonOperation =
         std::make_pair(BSON("ts" << Timestamp(Seconds(1), 0) << "h" << 1LL), RecordId(1));
@@ -776,22 +770,24 @@ TEST_F(RSRollbackTest, RollingBackDropAndCreateOfSameIndexNameWithDifferentSpecs
         _replicationProcess.get()));
     stopCapturingLogMessages();
     {
-        Lock::DBLock dbLock(_opCtx.get(), "test", MODE_S);
+        Lock::DBLock dbLock(_opCtx.get(), nss.db(), MODE_S);
         auto indexCatalog = collection->getIndexCatalog();
         ASSERT(indexCatalog);
         ASSERT_EQUALS(2, indexCatalog->numIndexesReady(_opCtx.get()));
-        ASSERT_EQUALS(
-            1,
-            countLogLinesContaining(str::stream()
-                                    << "Dropped index in rollback for collection: test.t, UUID: "
-                                    << options.uuid->toString()
-                                    << ", index: a_1"));
-        ASSERT_EQUALS(
-            1,
-            countLogLinesContaining(str::stream()
-                                    << "Created index in rollback for collection: test.t, UUID: "
-                                    << options.uuid->toString()
-                                    << ", index: a_1"));
+        ASSERT_EQUALS(1,
+                      countLogLinesContaining(str::stream()
+                                              << "Dropped index in rollback for collection: "
+                                              << nss.toString()
+                                              << ", UUID: "
+                                              << options.uuid->toString()
+                                              << ", index: a_1"));
+        ASSERT_EQUALS(1,
+                      countLogLinesContaining(str::stream()
+                                              << "Created index in rollback for collection: "
+                                              << nss.toString()
+                                              << ", UUID: "
+                                              << options.uuid->toString()
+                                              << ", index: a_1"));
         std::vector<IndexDescriptor*> indexes;
         indexCatalog->findIndexesByKeyPattern(_opCtx.get(), BSON("a" << 1), false, &indexes);
         ASSERT(indexes.size() == 1);
@@ -847,6 +843,174 @@ TEST_F(RSRollbackTest, RollbackCreateIndexCommandMissingIndexName) {
                       "Missing index name in createIndexes operation on rollback, document: "));
 }
 
+// Generators of standard index keys and names given an index 'id'.
+std::string idxKey(std::string id) {
+    return "key_" + id;
+};
+std::string idxName(std::string id) {
+    return "index_" + id;
+};
+
+// Create an index spec object given the namespace and the index 'id'.
+BSONObj idxSpec(NamespaceString nss, std::string id) {
+    return BSON("ns" << nss.toString() << "v" << static_cast<int>(kIndexVersion) << "key"
+                     << BSON(idxKey(id) << 1)
+                     << "name"
+                     << idxName(id));
+}
+
+// Returns the number of indexes that exist on the given collection.
+int numIndexesOnColl(OperationContext* opCtx, NamespaceString nss, Collection* coll) {
+    Lock::DBLock dbLock(opCtx, nss.db(), MODE_X);
+    auto indexCatalog = coll->getIndexCatalog();
+    ASSERT(indexCatalog);
+    return indexCatalog->numIndexesReady(opCtx);
+}
+
+TEST_F(RSRollbackTest, RollbackDropIndexOnCollectionWithTwoExistingIndexes) {
+    createOplog(_opCtx.get());
+    CollectionOptions options;
+    options.uuid = UUID::gen();
+    NamespaceString nss("test", "coll");
+    auto coll = _createCollection(_opCtx.get(), nss.toString(), options);
+
+    // Create the necessary indexes. Index 0 is created and dropped in the sequence of ops that will
+    // be rolled back, so we only create index 1.
+    int numIndexes = createIndexForColl(_opCtx.get(), coll, nss, idxSpec(nss, "1"));
+    ASSERT_EQUALS(2, numIndexes);
+
+    auto commonOp = std::make_pair(BSON("ts" << Timestamp(1, 0) << "h" << 1LL), RecordId(1));
+
+    // The ops that will be rolled back.
+    auto createIndex0Op = makeCreateIndexOplogEntry(coll, BSON(idxKey("0") << 1), idxName("0"), 2);
+    auto createIndex1Op = makeCreateIndexOplogEntry(coll, BSON(idxKey("1") << 1), idxName("1"), 3);
+    auto dropIndex0Op = makeDropIndexOplogEntry(coll, BSON(idxKey("0") << 1), idxName("0"), 4);
+
+    auto remoteOplog = {commonOp};
+    auto localOplog = {dropIndex0Op, createIndex1Op, createIndex0Op, commonOp};
+
+    // Set up the mock rollback source and then run rollback.
+    RollbackSourceMock rollbackSource(stdx::make_unique<OplogInterfaceMock>(remoteOplog));
+    ASSERT_OK(syncRollback(_opCtx.get(),
+                           OplogInterfaceMock(localOplog),
+                           rollbackSource,
+                           {},
+                           _coordinator,
+                           _replicationProcess.get()));
+
+    // Make sure the collection indexes are in the proper state post-rollback.
+    ASSERT_EQUALS(1, numIndexesOnColl(_opCtx.get(), nss, coll));
+}
+
+TEST_F(RSRollbackTest, RollbackTwoIndexDropsPrecededByTwoIndexCreationsOnSameCollection) {
+    createOplog(_opCtx.get());
+    CollectionOptions options;
+    options.uuid = UUID::gen();
+    NamespaceString nss("test", "coll");
+    auto coll = _createCollection(_opCtx.get(), nss.toString(), options);
+
+    auto commonOp = std::make_pair(BSON("ts" << Timestamp(1, 0) << "h" << 1LL), RecordId(1));
+
+    // The ops that will be rolled back.
+    auto createIndex0Op = makeCreateIndexOplogEntry(coll, BSON(idxKey("0") << 1), idxName("0"), 2);
+    auto createIndex1Op = makeCreateIndexOplogEntry(coll, BSON(idxKey("1") << 1), idxName("1"), 3);
+    auto dropIndex0Op = makeDropIndexOplogEntry(coll, BSON(idxKey("0") << 1), idxName("0"), 4);
+    auto dropIndex1Op = makeDropIndexOplogEntry(coll, BSON(idxKey("1") << 1), idxName("1"), 5);
+
+    auto remoteOplog = {commonOp};
+    auto localOplog = {dropIndex1Op, dropIndex0Op, createIndex1Op, createIndex0Op, commonOp};
+
+    // Set up the mock rollback source and then run rollback.
+    RollbackSourceMock rollbackSource(stdx::make_unique<OplogInterfaceMock>(remoteOplog));
+    ASSERT_OK(syncRollback(_opCtx.get(),
+                           OplogInterfaceMock(localOplog),
+                           rollbackSource,
+                           {},
+                           _coordinator,
+                           _replicationProcess.get()));
+
+    // Make sure the collection indexes are in the proper state post-rollback.
+    ASSERT_EQUALS(1, numIndexesOnColl(_opCtx.get(), nss, coll));
+}
+
+TEST_F(RSRollbackTest, RollbackMultipleCreateIndexesOnSameCollection) {
+    createOplog(_opCtx.get());
+    CollectionOptions options;
+    options.uuid = UUID::gen();
+    NamespaceString nss("test", "coll");
+    auto coll = _createCollection(_opCtx.get(), nss.toString(), options);
+
+    auto commonOp = std::make_pair(BSON("ts" << Timestamp(1, 0) << "h" << 1LL), RecordId(1));
+
+    // Create all of the necessary indexes.
+    createIndexForColl(_opCtx.get(), coll, nss, idxSpec(nss, "0"));
+    createIndexForColl(_opCtx.get(), coll, nss, idxSpec(nss, "1"));
+    createIndexForColl(_opCtx.get(), coll, nss, idxSpec(nss, "2"));
+    ASSERT_EQUALS(4, numIndexesOnColl(_opCtx.get(), nss, coll));
+
+    // The ops that will be rolled back.
+    auto createIndex0Op = makeCreateIndexOplogEntry(coll, BSON(idxKey("0") << 1), idxName("0"), 2);
+    auto createIndex1Op = makeCreateIndexOplogEntry(coll, BSON(idxKey("1") << 1), idxName("1"), 3);
+    auto createIndex2Op = makeCreateIndexOplogEntry(coll, BSON(idxKey("2") << 1), idxName("2"), 4);
+
+    auto remoteOplog = {commonOp};
+    auto localOplog = {createIndex2Op, createIndex1Op, createIndex0Op, commonOp};
+
+    // Set up the mock rollback source and then run rollback.
+    RollbackSourceMock rollbackSource(stdx::make_unique<OplogInterfaceMock>(remoteOplog));
+    ASSERT_OK(syncRollback(_opCtx.get(),
+                           OplogInterfaceMock(localOplog),
+                           rollbackSource,
+                           {},
+                           _coordinator,
+                           _replicationProcess.get()));
+
+    // Make sure the collection indexes are in the proper state post-rollback.
+    ASSERT_EQUALS(1, numIndexesOnColl(_opCtx.get(), nss, coll));
+}
+
+TEST_F(RSRollbackTest, RollbackCreateDropRecreateIndexOnCollection) {
+    createOplog(_opCtx.get());
+    CollectionOptions options;
+    options.uuid = UUID::gen();
+    NamespaceString nss("test", "coll");
+    auto coll = _createCollection(_opCtx.get(), nss.toString(), options);
+
+    // Create the necessary indexes. Index 0 is created, dropped, and created again in the
+    // sequence of ops, so we create that index.
+    auto indexSpec = BSON("ns" << nss.toString() << "v" << static_cast<int>(kIndexVersion) << "key"
+                               << BSON(idxKey("0") << 1)
+                               << "name"
+                               << idxName("0"));
+
+    int numIndexes = createIndexForColl(_opCtx.get(), coll, nss, indexSpec);
+    ASSERT_EQUALS(2, numIndexes);
+
+    auto commonOp = std::make_pair(BSON("ts" << Timestamp(1, 0) << "h" << 1LL), RecordId(1));
+
+    // The ops that will be rolled back.
+    auto createIndex0Op = makeCreateIndexOplogEntry(coll, BSON(idxKey("0") << 1), idxName("0"), 2);
+    auto dropIndex0Op = makeDropIndexOplogEntry(coll, BSON(idxKey("0") << 1), idxName("0"), 3);
+    auto createIndex0AgainOp =
+        makeCreateIndexOplogEntry(coll, BSON(idxKey("0") << 1), idxName("0"), 4);
+
+    auto remoteOplog = {commonOp};
+    auto localOplog = {createIndex0AgainOp, dropIndex0Op, createIndex0Op, commonOp};
+
+    // Set up the mock rollback source and then run rollback.
+    RollbackSourceMock rollbackSource(stdx::make_unique<OplogInterfaceMock>(remoteOplog));
+    ASSERT_OK(syncRollback(_opCtx.get(),
+                           OplogInterfaceMock(localOplog),
+                           rollbackSource,
+                           {},
+                           _coordinator,
+                           _replicationProcess.get()));
+
+    // Make sure the collection indexes are in the proper state post-rollback.
+    ASSERT_EQUALS(1, numIndexesOnColl(_opCtx.get(), nss, coll));
+}
+
+
 TEST_F(RSRollbackTest, RollbackUnknownCommand) {
     createOplog(_opCtx.get());
     auto commonOperation =