SERVER-28546 confirm WiredTiger unindex operations remove the correct document

Previously, unindex operations on WiredTiger would not confirm the record id matches.
This could result in erroroneous entries being removed from the index, for the case where
an index with a unique constraint also had a partial filter expression.

(cherry picked from commit 052345f6c592b1b5e626e05ed8c59e9b20b6d7ee)

4 files changed, 61 insertions, 17 deletions

diff --git a/jstests/core/index_partial_write_ops.js b/jstests/core/index_partial_write_ops.js
index b962347a26d..22c4d0dd519 100644
--- a/jstests/core/index_partial_write_ops.js
+++ b/jstests/core/index_partial_write_ops.js
@@ -19,7 +19,7 @@
     coll.drop();
 
     // Create partial index.
-    assert.commandWorked(coll.ensureIndex({x: 1}, {partialFilterExpression: {a: 1}}));
+    assert.commandWorked(coll.ensureIndex({x: 1}, {unique: true, partialFilterExpression: {a: 1}}));
 
     assert.writeOK(coll.insert({_id: 1, x: 5, a: 2, b: 1}));  // Not in index.
     assert.writeOK(coll.insert({_id: 2, x: 6, a: 1, b: 1}));  // In index.
@@ -59,4 +59,20 @@
     // Delete that does affect partial index.
     assert.writeOK(coll.remove({x: 6}));
     assert.eq(0, getNumKeys("x_1"));
+
+    // Documents with duplicate keys that straddle the index.
+    assert.writeOK(coll.insert({_id: 3, x: 1, a: 1}));  // In index.
+    assert.writeOK(coll.insert({_id: 4, x: 1, a: 0}));  // Not in index.
+    assert.writeErrorWithCode(
+        coll.insert({_id: 5, x: 1, a: 1}),
+        ErrorCodes.DuplicateKey);  // Duplicate key constraint prevents insertion.
+
+    // Only _id 3 is in the index.
+    assert.eq(1, getNumKeys("x_1"));
+
+    // Remove _id 4, _id 3 should remain in index.
+    assert.writeOK(coll.remove({_id: 4}));
+
+    // _id 3 is still in the index.
+    assert.eq(1, getNumKeys("x_1"));
 })();
diff --git a/src/mongo/db/catalog/index_catalog.cpp b/src/mongo/db/catalog/index_catalog.cpp
index bd2191dbaf4..7ae43d645ea 100644
--- a/src/mongo/db/catalog/index_catalog.cpp
+++ b/src/mongo/db/catalog/index_catalog.cpp
@@ -1169,9 +1169,12 @@ Status IndexCatalog::_unindexRecord(OperationContext* txn,
     options.logIfError = logIfError;
     options.dupsAllowed = isDupsAllowed(index->descriptor());
 
-    // For unindex operations, dupsAllowed=false really means that it is safe to delete anything
-    // that matches the key, without checking the RecordID, since dups are impossible. We need
-    // to disable this behavior for in-progress indexes. See SERVER-17487 for more details.
+    // On WiredTiger, we do blind unindexing of records for efficiency.  However, when duplicates
+    // are allowed in unique indexes, WiredTiger does not do blind unindexing, and instead confirms
+    // that the recordid matches the element we are removing.
+    // We need to disable blind-deletes for in-progress indexes, in order to force recordid-matching
+    // for unindex operations, since initial sync can build an index over a collection with
+    // duplicates. See SERVER-17487 for more details.
     options.dupsAllowed = options.dupsAllowed || !index->isReady(txn);
 
     int64_t removed;
diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_index.cpp b/src/mongo/db/storage/wiredtiger/wiredtiger_index.cpp
index 63993365b95..4394317934a 100644
--- a/src/mongo/db/storage/wiredtiger/wiredtiger_index.cpp
+++ b/src/mongo/db/storage/wiredtiger/wiredtiger_index.cpp
@@ -37,22 +37,22 @@
 #include <set>
 
 #include "mongo/base/checked_cast.h"
-#include "mongo/db/json.h"
 #include "mongo/db/catalog/index_catalog_entry.h"
 #include "mongo/db/concurrency/write_conflict_exception.h"
 #include "mongo/db/index/index_descriptor.h"
+#include "mongo/db/json.h"
 #include "mongo/db/service_context.h"
 #include "mongo/db/storage/key_string.h"
+#include "mongo/db/storage/storage_options.h"
 #include "mongo/db/storage/wiredtiger/wiredtiger_customization_hooks.h"
 #include "mongo/db/storage/wiredtiger/wiredtiger_global_options.h"
 #include "mongo/db/storage/wiredtiger/wiredtiger_record_store.h"
 #include "mongo/db/storage/wiredtiger/wiredtiger_session_cache.h"
 #include "mongo/db/storage/wiredtiger/wiredtiger_util.h"
-#include "mongo/db/storage/storage_options.h"
 #include "mongo/stdx/memory.h"
 #include "mongo/util/assert_util.h"
-#include "mongo/util/hex.h"
 #include "mongo/util/fail_point.h"
+#include "mongo/util/hex.h"
 #include "mongo/util/log.h"
 #include "mongo/util/mongoutils/str.h"
 
@@ -993,7 +993,7 @@ public:
 WiredTigerIndexUnique::WiredTigerIndexUnique(OperationContext* ctx,
                                              const std::string& uri,
                                              const IndexDescriptor* desc)
-    : WiredTigerIndex(ctx, uri, desc) {}
+    : WiredTigerIndex(ctx, uri, desc), _partial(desc->isPartial()) {}
 
 std::unique_ptr<SortedDataInterface::Cursor> WiredTigerIndexUnique::newCursor(OperationContext* txn,
                                                                               bool forward) const {
@@ -1077,8 +1077,37 @@ void WiredTigerIndexUnique::_unindex(WT_CURSOR* c,
     WiredTigerItem keyItem(data.getBuffer(), data.getSize());
     c->set_key(c, keyItem.Get());
 
+    auto triggerWriteConflictAtPoint = [&keyItem](WT_CURSOR* point) {
+        // WT_NOTFOUND may occur during a background index build. Insert a dummy value and
+        // delete it again to trigger a write conflict in case this is being concurrently
+        // indexed by the background indexer.
+        point->set_key(point, keyItem.Get());
+        point->set_value(point, emptyItem.Get());
+        invariantWTOK(WT_OP_CHECK(point->insert(point)));
+        point->set_key(point, keyItem.Get());
+        invariantWTOK(WT_OP_CHECK(point->remove(point)));
+    };
+
     if (!dupsAllowed) {
-        // nice and clear
+        if (_partial) {
+            // Check that the record id matches.  We may be called to unindex records that are not
+            // present in the index due to the partial filter expression.
+            int ret = WT_OP_CHECK(c->search(c));
+            if (ret == WT_NOTFOUND) {
+                triggerWriteConflictAtPoint(c);
+                return;
+            }
+            WT_ITEM value;
+            invariantWTOK(c->get_value(c, &value));
+            BufReader br(value.data, value.size);
+            fassert(40416, br.remaining());
+            if (KeyString::decodeRecordId(&br) != id) {
+                return;
+            }
+            // Ensure there aren't any other values in here.
+            KeyString::TypeBits::fromBuffer(&br);
+            fassert(40417, !br.remaining());
+        }
         int ret = WT_OP_CHECK(c->remove(c));
         if (ret == WT_NOTFOUND) {
             return;
@@ -1091,14 +1120,7 @@ void WiredTigerIndexUnique::_unindex(WT_CURSOR* c,
 
     int ret = WT_OP_CHECK(c->search(c));
     if (ret == WT_NOTFOUND) {
-        // WT_NOTFOUND is only expected during a background index build. Insert a dummy value and
-        // delete it again to trigger a write conflict in case this is being concurrently indexed by
-        // the background indexer.
-        c->set_key(c, keyItem.Get());
-        c->set_value(c, emptyItem.Get());
-        invariantWTOK(WT_OP_CHECK(c->insert(c)));
-        c->set_key(c, keyItem.Get());
-        invariantWTOK(WT_OP_CHECK(c->remove(c)));
+        triggerWriteConflictAtPoint(c);
         return;
     }
     invariantWTOK(ret);
diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_index.h b/src/mongo/db/storage/wiredtiger/wiredtiger_index.h
index 4ea5741e7a5..66284492e69 100644
--- a/src/mongo/db/storage/wiredtiger/wiredtiger_index.h
+++ b/src/mongo/db/storage/wiredtiger/wiredtiger_index.h
@@ -164,6 +164,9 @@ public:
     Status _insert(WT_CURSOR* c, const BSONObj& key, const RecordId& id, bool dupsAllowed) override;
 
     void _unindex(WT_CURSOR* c, const BSONObj& key, const RecordId& id, bool dupsAllowed) override;
+
+private:
+    bool _partial;
 };
 
 class WiredTigerIndexStandard : public WiredTigerIndex {