SERVER-23807 index build must X-lock collection

Index builds must lock the collection with an X lock, rather than an IX lock, to
prevent a race with concurrent updates.

3 files changed, 94 insertions, 2 deletions

diff --git a/jstests/noPassthroughWithMongod/indexbg_updates.js b/jstests/noPassthroughWithMongod/indexbg_updates.js
new file mode 100644
index 00000000000..603be6c40f5
--- /dev/null
+++ b/jstests/noPassthroughWithMongod/indexbg_updates.js
@@ -0,0 +1,81 @@
+// Perform concurrent updates with a background index build, testing that
+// indexes are updated appropriately. See SERVER-23590 and SERVER-22970.
+//
+// This test is a variation of index_multi.js.
+
+(function() {
+    "use strict";
+    Random.setRandomSeed();
+
+    var coll = db.getSiblingDB("indexbg_updates").coll;
+    coll.drop();
+
+    var numDocs = 10000;
+
+    var bulk = coll.initializeUnorderedBulkOp();
+    print("Populate the collection with random data");
+    for (var i = 0; i < numDocs; i++) {
+        var doc = {
+            "_id": i,
+            "field0": Random.rand()
+        };
+
+        bulk.insert(doc);
+    }
+    assert.writeOK(bulk.execute());
+
+    // Perform a bulk update on a single document, targeting the updates on the
+    // field being actively indexed in the background.
+    bulk = coll.initializeUnorderedBulkOp();
+    for (i = 0; i < numDocs; i++) {
+        var criteria = {
+            "_id": 1000
+        };
+        var mod = {};
+
+        if (Random.rand() < .8) {
+            mod["$set"] = {};
+            mod["$set"]["field0"] = Random.rand();
+        } else {
+            mod["$unset"] = {};
+            mod["$unset"]["field0"] = true;
+        }
+
+        bulk.find(criteria).update(mod);
+    }
+
+    // Build an index in the background on field0.
+    var backgroundIndexBuildShell = startParallelShell(
+        function() {
+            var coll = db.getSiblingDB("indexbg_updates").coll;
+            assert.commandWorked(coll.createIndex({"field0": 1}, {"background": true}));
+        },
+        null,  // port -- use default
+        false  // noconnect
+        );
+
+    print("Do some sets and unsets");
+    assert.writeOK(bulk.execute());
+
+    print("Start background index build");
+    backgroundIndexBuildShell();
+
+    var explain = coll.find().hint({"field0": 1}).explain();
+    assert("queryPlanner" in explain, tojson(explain));
+
+    // Validate the collection.
+    // In MongoDB 3.0, when a non-multi-key index has a mismatching number of keys and documents,
+    // validate does not produce an error. Therefore, we do a manual check of the number of keys.
+    var validationOutput = coll.validate(true);
+    printjson(validationOutput);
+
+    var indexes = Object.keys(validationOutput["keysPerIndex"]);
+    assert.eq(indexes.length, 2, "Expected only two indexes but found " + indexes);
+
+    for (var i = 0; i < indexes.length; i++) {
+        var keys = validationOutput["keysPerIndex"][indexes[i]];
+        assert.eq(keys, numDocs,
+                  "Regular index " + indexes[i] + " has a mismatching number of keys (" + keys +
+                  ") with documents in collection (" + numDocs + ")");
+    }
+}());
diff --git a/src/mongo/db/commands/create_indexes.cpp b/src/mongo/db/commands/create_indexes.cpp
index 238fdc7c4cb..e906eb9c2ae 100644
--- a/src/mongo/db/commands/create_indexes.cpp
+++ b/src/mongo/db/commands/create_indexes.cpp
@@ -233,7 +233,16 @@ public:
         }
 
         try {
-            Lock::CollectionLock colLock(txn->lockState(), ns.ns(), MODE_IX);
+            // We need an X lock and not an IX lock due to a race with concurrent updates. Consider
+            // the following scenario:
+            //
+            // 1. A document is about to be indexed by a background index builder.
+            // 2. An update concurrently reads the same document.
+            // 3. The update unindexes the old value of the document. However, this is a no-op
+            // because the document wasn't previously indexed, so no write conflict is thrown.
+            // 4. Both the update and the index builder insert their respective index keys into the
+            // index, resulting in more keys in the index than expected.
+            Lock::CollectionLock colLock(txn->lockState(), ns.ns(), MODE_X);
             uassertStatusOK(indexer.insertAllDocumentsInCollection());
         } catch (const DBException& e) {
             invariant(e.getCode() != ErrorCodes::WriteConflict);
diff --git a/src/mongo/db/index_builder.cpp b/src/mongo/db/index_builder.cpp
index ba2cbd6d6ef..0ed860c5dea 100644
--- a/src/mongo/db/index_builder.cpp
+++ b/src/mongo/db/index_builder.cpp
@@ -175,7 +175,9 @@ Status IndexBuilder::_build(OperationContext* txn,
                         dbLock->relockWithMode(MODE_IX);
                     }
 
-                    Lock::CollectionLock colLock(txn->lockState(), ns.ns(), MODE_IX);
+                    // We need an X lock and not an IX lock for background index building. See the
+                    // createIndexes command for a detailed example.
+                    Lock::CollectionLock colLock(txn->lockState(), ns.ns(), MODE_X);
                     status = indexer.insertAllDocumentsInCollection();
                 }