SERVER-43908 Modify IndexConsistency hash-map keys to avoid hitting an invariant on duplicate index keys in KeyString form for different indexes

3 files changed, 149 insertions, 9 deletions

diff --git a/src/mongo/db/catalog/index_consistency.cpp b/src/mongo/db/catalog/index_consistency.cpp
index 7467df99d07..f60a5ee7777 100644
--- a/src/mongo/db/catalog/index_consistency.cpp
+++ b/src/mongo/db/catalog/index_consistency.cpp
@@ -50,6 +50,23 @@ namespace mongo {
 namespace {
 // The number of items we can scan before we must yield.
 static const int kScanLimit = 1000;
+
+/**
+ * Returns a key for the '_extraIndexEntries' and '_missingIndexEntries' maps. The key is a pair
+ * of index name and the index key represented in KeyString form.
+ * Using the index name is required as the index keys are passed in as KeyStrings which do not
+ * contain field names.
+ *
+ * If we had the following document: { a: 1, b: 1 } with two indexes on keys "a" and "b", then
+ * the KeyStrings for the index keys of the document would be identical as the field name in the
+ * KeyString is not present. The BSON representation of this would look like: { : 1 } for both.
+ * To distinguish these as different index keys, return a pair of index name and index key.
+ */
+std::pair<std::string, std::string> _generateKeyForMap(const IndexInfo& indexInfo,
+                                                       const KeyString& ks) {
+    return std::make_pair(indexInfo.indexName, std::string(ks.getBuffer(), ks.getSize()));
+}
+
 }  // namespace
 
 IndexConsistency::IndexConsistency(OperationContext* opCtx,
@@ -341,10 +358,10 @@ void IndexConsistency::_addDocKey_inlock(const KeyString& ks,
             idKey = data["_id"];
         }
 
-        std::string key = std::string(ks.getBuffer(), ks.getSize());
         BSONObj info = _generateInfo(indexNumber, recordId, indexKey, idKey);
 
-        // Cannot have duplicate KeyStrings during the document scan phase.
+        // Cannot have duplicate KeyStrings during the document scan phase for the same index.
+        IndexKey key = _generateKeyForMap(_indexesInfo.at(indexNumber), ks);
         invariant(_missingIndexEntries.count(key) == 0);
         _missingIndexEntries.insert(std::make_pair(key, info));
     }
@@ -381,7 +398,7 @@ void IndexConsistency::_addIndexKey_inlock(const KeyString& ks,
             return;
         }
 
-        std::string key = std::string(ks.getBuffer(), ks.getSize());
+        IndexKey key = _generateKeyForMap(_indexesInfo.at(indexNumber), ks);
         BSONObj info = _generateInfo(indexNumber, recordId, indexKey, boost::none);
 
         if (_missingIndexEntries.count(key) == 0) {
diff --git a/src/mongo/db/catalog/index_consistency.h b/src/mongo/db/catalog/index_consistency.h
index 2d7cbdcd552..5da15ea1221 100644
--- a/src/mongo/db/catalog/index_consistency.h
+++ b/src/mongo/db/catalog/index_consistency.h
@@ -75,6 +75,7 @@ struct IndexInfo {
 
 class IndexConsistency final {
     using ValidateResultsMap = std::map<std::string, ValidateResults>;
+    using IndexKey = std::pair<std::string, std::string>;
 
 public:
     IndexConsistency(OperationContext* opCtx,
@@ -212,15 +213,15 @@ private:
 
     // Populated during the second phase of validation, this map contains the index entries that
     // were pointing at an invalid document key.
-    // The map contains a KeyString pointing at a set of BSON objects as there may be multiple
-    // extra index entries for the same KeyString.
-    std::map<std::string, BSONObjSet> _extraIndexEntries;
+    // The map contains a IndexKey pointing at a set of BSON objects as there may be multiple
+    // extra index entries for the same IndexKey.
+    std::map<IndexKey, BSONObjSet> _extraIndexEntries;
 
     // Populated during the second phase of validation, this map contains the index entries that
     // were missing while the document key was in place.
-    // The map contains a KeyString pointing to a BSON object as there can only be one missing index
-    // entry for a given KeyString.
-    std::map<std::string, BSONObj> _missingIndexEntries;
+    // The map contains a IndexKey pointing to a BSON object as there can only be one missing index
+    // entry for a given IndexKey for each index.
+    std::map<IndexKey, BSONObj> _missingIndexEntries;
 
     /**
      * During the first phase of validation, given the document's key KeyString, increment the
diff --git a/src/mongo/dbtests/validate_tests.cpp b/src/mongo/dbtests/validate_tests.cpp
index 8c2dd38457a..86a8a255d47 100644
--- a/src/mongo/dbtests/validate_tests.cpp
+++ b/src/mongo/dbtests/validate_tests.cpp
@@ -1229,6 +1229,126 @@ public:
     }
 };
 
+class ValidateDuplicateDocumentIndexKeySet : public ValidateBase {
+public:
+    ValidateDuplicateDocumentIndexKeySet() : ValidateBase(/*full=*/false, /*background=*/false) {}
+
+    void run() {
+        // Create a new collection.
+        lockDb(MODE_X);
+        Collection* coll;
+        {
+            WriteUnitOfWork wunit(&_opCtx);
+            ASSERT_OK(_db->dropCollection(&_opCtx, _ns));
+            coll = _db->createCollection(&_opCtx, _ns);
+            wunit.commit();
+        }
+
+        // Create two identical indexes only differing by key pattern and name.
+        {
+            const auto indexName = "a";
+            const auto indexKey = BSON("a" << 1);
+            auto status =
+                dbtests::createIndexFromSpec(&_opCtx,
+                                             coll->ns().ns(),
+                                             BSON("name" << indexName << "key" << indexKey << "v"
+                                                         << static_cast<int>(kIndexVersion)
+                                                         << "background"
+                                                         << false
+                                                         << "ns"
+                                                         << _ns));
+            ASSERT_OK(status);
+        }
+
+        {
+            const auto indexName = "b";
+            const auto indexKey = BSON("b" << 1);
+            auto status =
+                dbtests::createIndexFromSpec(&_opCtx,
+                                             coll->ns().ns(),
+                                             BSON("name" << indexName << "key" << indexKey << "v"
+                                                         << static_cast<int>(kIndexVersion)
+                                                         << "background"
+                                                         << false
+                                                         << "ns"
+                                                         << _ns));
+            ASSERT_OK(status);
+        }
+
+        // Insert a document.
+        OpDebug* const nullOpDebug = nullptr;
+        RecordId rid = RecordId::min();
+        lockDb(MODE_X);
+        {
+            WriteUnitOfWork wunit(&_opCtx);
+            ASSERT_OK(
+                coll->insertDocument(&_opCtx,
+                                     InsertStatement(BSON("_id" << 1 << "a" << 1 << "b" << 1)),
+                                     nullOpDebug,
+                                     true));
+            rid = coll->getCursor(&_opCtx)->next()->id;
+            wunit.commit();
+        }
+        releaseDb();
+        ASSERT_TRUE(checkValid());
+
+        // Remove the index entry for index "a".
+        {
+            lockDb(MODE_X);
+
+            IndexCatalog* indexCatalog = coll->getIndexCatalog();
+            const std::string indexName = "a";
+            auto descriptor = indexCatalog->findIndexByName(&_opCtx, indexName);
+            auto iam =
+                const_cast<IndexAccessMethod*>(indexCatalog->getEntry(descriptor)->accessMethod());
+
+            WriteUnitOfWork wunit(&_opCtx);
+            int64_t numDeleted;
+            const BSONObj actualKey = BSON("a" << 1);
+            InsertDeleteOptions options;
+            options.logIfError = true;
+            options.dupsAllowed = true;
+
+            ASSERT_OK(iam->remove(&_opCtx, actualKey, RecordId(1), options, &numDeleted));
+
+            ASSERT_EQUALS(numDeleted, 1);
+            wunit.commit();
+
+            releaseDb();
+        }
+
+        // Remove the index entry for index "b".
+        {
+            lockDb(MODE_X);
+
+            IndexCatalog* indexCatalog = coll->getIndexCatalog();
+            const std::string indexName = "b";
+            auto descriptor = indexCatalog->findIndexByName(&_opCtx, indexName);
+            auto iam =
+                const_cast<IndexAccessMethod*>(indexCatalog->getEntry(descriptor)->accessMethod());
+
+            WriteUnitOfWork wunit(&_opCtx);
+            int64_t numDeleted;
+            const BSONObj actualKey = BSON("b" << 1);
+            InsertDeleteOptions options;
+            options.logIfError = true;
+            options.dupsAllowed = true;
+
+            ASSERT_OK(iam->remove(&_opCtx, actualKey, RecordId(1), options, &numDeleted));
+            ASSERT_EQUALS(numDeleted, 1);
+            wunit.commit();
+
+            releaseDb();
+        }
+
+        {
+            // Now we have two missing index entries with the keys { : 1 } since the KeyStrings
+            // aren't hydrated with their field names.
+            ASSERT_TRUE(!checkValid());
+        }
+    }
+};
+
 class ValidateTests : public Suite {
 public:
     ValidateTests() : Suite("validate_tests") {}
@@ -1269,6 +1389,8 @@ public:
         add<ValidateMissingAndExtraIndexEntryResults<false, false>>();
         add<ValidateMissingIndexEntryResults<false, false>>();
         add<ValidateExtraIndexEntryResults<false, false>>();
+
+        add<ValidateDuplicateDocumentIndexKeySet>();
     }
 } validateTests;
 }  // namespace ValidateTests