SERVER-22002 Do not retry findAndModify on MMAPv1

6 files changed, 29 insertions, 134 deletions

diff --git a/jstests/concurrency/fsm_workloads/findAndModify_remove_queue.js b/jstests/concurrency/fsm_workloads/findAndModify_remove_queue.js
index e1fd1f747d4..9df0e8c8636 100644
--- a/jstests/concurrency/fsm_workloads/findAndModify_remove_queue.js
+++ b/jstests/concurrency/fsm_workloads/findAndModify_remove_queue.js
@@ -10,6 +10,8 @@
  *
  * This workload was designed to reproduce SERVER-18304.
  */
+load('jstests/concurrency/fsm_workload_helpers/server_types.js');  // for isMongod and isMMAPv1
+
 var $config = (function() {
 
     var data =  {
@@ -65,10 +67,15 @@ var $config = (function() {
             assertAlways.commandWorked(res);
 
             var doc = res.value;
-            assertWhenOwnColl.neq(
-                doc, null, 'findAndModify should have found and removed a matching document');
+            if (isMongod(db) && !isMMAPv1(db)) {
+                // MMAPv1 does not automatically retry if there was a conflict, so it is expected
+                // that it may return null in the case of a conflict. All other storage engines
+                // should automatically retry the operation, and thus should never return null.
+                assertWhenOwnColl.neq(
+                    doc, null, 'findAndModify should have found and removed a matching document');
+            }
             if (doc !== null) {
-                this.saveDocId.call(this, db, collName, doc._id);
+                this.saveDocId(db, collName, doc._id);
             }
         }
 
@@ -106,10 +113,16 @@ var $config = (function() {
         var ownedDB = db.getSiblingDB(db.getName() + this.uniqueDBName);
 
         if (this.opName === 'removed') {
-            // Each findAndModify should remove exactly one document, and this.numDocs ==
-            // this.iterations * this.threadCount, so there should not be any documents remaining.
-            assertWhenOwnColl.eq(
-                db[collName].find().itcount(), 0, 'Expected all documents to have been removed');
+            if (isMongod(db) && !isMMAPv1(db)) {
+                // On storage engines other than MMAPv1, each findAndModify should remove exactly
+                // one document. This is not true on MMAPv1 since it will not automatically retry a
+                // findAndModify when there is a conflict, indicating there were no matches instead.
+                // Since this.numDocs == this.iterations * this.threadCount, there should not be any
+                // documents remaining.
+                assertWhenOwnColl.eq(db[collName].find().itcount(),
+                                     0,
+                                     'Expected all documents to have been removed');
+            }
         }
 
         assertWhenOwnColl(function() {
diff --git a/jstests/concurrency/fsm_workloads/findAndModify_update_queue.js b/jstests/concurrency/fsm_workloads/findAndModify_update_queue.js
index 920be7feb66..0ca53015b68 100644
--- a/jstests/concurrency/fsm_workloads/findAndModify_update_queue.js
+++ b/jstests/concurrency/fsm_workloads/findAndModify_update_queue.js
@@ -13,6 +13,7 @@
  */
 load('jstests/concurrency/fsm_libs/extend_workload.js'); // for extendWorkload
 load('jstests/concurrency/fsm_workloads/findAndModify_remove_queue.js'); // for $config
+load('jstests/concurrency/fsm_workload_helpers/server_types.js');  // for isMongod and isMMAPv1
 
 var $config = extendWorkload($config, function($config, $super) {
 
@@ -43,10 +44,15 @@ var $config = extendWorkload($config, function($config, $super) {
             assertAlways.commandWorked(res);
 
             var doc = res.value;
-            assertWhenOwnColl.neq(
-                doc, null, 'findAndModify should have found and updated a matching document');
+            if (isMongod(db) && !isMMAPv1(db)) {
+                // MMAPv1 does not automatically retry if there was a conflict, so it is expected
+                // that it may return null in the case of a conflict. All other storage engines
+                // should automatically retry the operation, and thus should never return null.
+                assertWhenOwnColl.neq(
+                    doc, null, 'findAndModify should have found and updated a matching document');
+            }
             if (doc !== null) {
-                this.saveDocId.call(this, db, collName, doc._id);
+                this.saveDocId(db, collName, doc._id);
             }
         }
 
diff --git a/src/mongo/db/exec/delete.cpp b/src/mongo/db/exec/delete.cpp
index 366ab76f108..3e5ece8d4d7 100644
--- a/src/mongo/db/exec/delete.cpp
+++ b/src/mongo/db/exec/delete.cpp
@@ -146,16 +146,6 @@ PlanStage::StageState DeleteStage::doWork(WorkingSetID* out) {
 
     if (!member->hasLoc()) {
         // We expect to be here because of an invalidation causing a force-fetch.
-
-        // When we're doing a findAndModify with a sort, the sort will have a limit of 1, so will
-        // not produce any more results even if there is another matching document. Throw a WCE here
-        // so that these operations get another chance to find a matching document. The
-        // findAndModify command should automatically retry if it gets a WCE.
-        // TODO: this is not necessary if there was no sort specified.
-        if (_params.returnDeleted) {
-            throw WriteConflictException();
-        }
-
         ++_specificStats.nInvalidateSkips;
         return PlanStage::NEED_TIME;
     }
diff --git a/src/mongo/db/exec/update.cpp b/src/mongo/db/exec/update.cpp
index 504f3689481..7569e93b2cc 100644
--- a/src/mongo/db/exec/update.cpp
+++ b/src/mongo/db/exec/update.cpp
@@ -827,17 +827,6 @@ PlanStage::StageState UpdateStage::doWork(WorkingSetID* out) {
 
         if (!member->hasLoc()) {
             // We expect to be here because of an invalidation causing a force-fetch.
-
-            // When we're doing a findAndModify with a sort, the sort will have a limit of 1, so
-            // will not produce any more results even if there is another matching document.
-            // Throw a WCE here so that these operations get another chance to find a matching
-            // document. The findAndModify command should automatically retry if it gets a WCE. The
-            // findAndModify command should automatically retry if it gets a WCE.
-            // TODO: this is not necessary if there was no sort specified.
-            if (_params.request->shouldReturnAnyDocs()) {
-                throw WriteConflictException();
-            }
-
             ++_specificStats.nInvalidateSkips;
             return PlanStage::NEED_TIME;
         }
diff --git a/src/mongo/dbtests/query_stage_delete.cpp b/src/mongo/dbtests/query_stage_delete.cpp
index 047cbefdb7f..4b6dacc27d1 100644
--- a/src/mongo/dbtests/query_stage_delete.cpp
+++ b/src/mongo/dbtests/query_stage_delete.cpp
@@ -302,50 +302,6 @@ public:
     }
 };
 
-/**
- * Test that a delete stage which has been asked to return the deleted document will throw an
- * exception when a child returns a WorkingSetMember in the OWNED_OBJ state. A WorkingSetMember in
- * the OWNED_OBJ state implies there was a conflict during execution.
- *
- * The delete stage is only asked to return documents during a findAndModify, and should throw a
- * WriteConflictException if there was a conflict. The findAndModify command will automatically
- * retry any WriteConflictExceptions.
- */
-class QueryStageDeleteShouldRetryConflictsForFindAndModify : public QueryStageDeleteBase {
-public:
-    void run() {
-        // Various variables we'll need.
-        OldClientWriteContext ctx(&_txn, nss.ns());
-        Collection* coll = ctx.getCollection();
-        const BSONObj query = BSONObj();
-        const auto ws = make_unique<WorkingSet>();
-        const unique_ptr<CanonicalQuery> cq(canonicalize(query));
-
-        // Configure a QueuedDataStage to pass an OWNED_OBJ to the delete stage.
-        auto qds = make_unique<QueuedDataStage>(&_txn, ws.get());
-        {
-            WorkingSetID id = ws->allocate();
-            WorkingSetMember* member = ws->get(id);
-            member->obj = Snapshotted<BSONObj>(SnapshotId(), fromjson("{x: 1}"));
-            member->transitionToOwnedObj();
-            qds->pushBack(id);
-        }
-
-        // Configure the delete.
-        DeleteStageParams deleteParams;
-        deleteParams.isMulti = false;
-        deleteParams.returnDeleted = true;  // Emulate a findAndModify.
-        deleteParams.canonicalQuery = cq.get();
-
-        const auto deleteStage =
-            make_unique<DeleteStage>(&_txn, deleteParams, ws.get(), coll, qds.release());
-
-        // Call work, passing the set up member to the delete stage.
-        WorkingSetID id = WorkingSet::INVALID_ID;
-        ASSERT_THROWS(deleteStage->work(&id), WriteConflictException);
-    }
-};
-
 
 class All : public Suite {
 public:
@@ -356,7 +312,6 @@ public:
         add<QueryStageDeleteInvalidateUpcomingObject>();
         add<QueryStageDeleteReturnOldDoc>();
         add<QueryStageDeleteSkipOwnedObjects>();
-        add<QueryStageDeleteShouldRetryConflictsForFindAndModify>();
     }
 };
 
diff --git a/src/mongo/dbtests/query_stage_update.cpp b/src/mongo/dbtests/query_stage_update.cpp
index f4721c7b9f3..10526f041ac 100644
--- a/src/mongo/dbtests/query_stage_update.cpp
+++ b/src/mongo/dbtests/query_stage_update.cpp
@@ -588,63 +588,6 @@ public:
     }
 };
 
-/**
- * Test that an update stage which has been asked to return the updated document will throw an
- * exception when a child returns a WorkingSetMember in the OWNED_OBJ state. A WorkingSetMember in
- * the OWNED_OBJ state implies there was a conflict during execution.
- *
- * The update stage is only asked to return the updated document during a findAndModify, and should
- * throw a WriteConflictException if there was a conflict. The findAndModify command will
- * automatically retry any WriteConflictExceptions.
- */
-class QueryStageUpdateShouldRetryConflictsForFindAndModify : public QueryStageUpdateBase {
-public:
-    void run() {
-        // Various variables we'll need.
-        OldClientWriteContext ctx(&_txn, nss.ns());
-        OpDebug* opDebug = &CurOp::get(_txn)->debug();
-        Collection* coll = ctx.getCollection();
-        UpdateLifecycleImpl updateLifecycle(false, nss);
-        UpdateRequest request(nss);
-        UpdateDriver driver((UpdateDriver::Options()));
-        const BSONObj query = BSONObj();
-        const auto ws = make_unique<WorkingSet>();
-        const unique_ptr<CanonicalQuery> cq(canonicalize(query));
-
-        // Populate the request.
-        request.setQuery(query);
-        request.setUpdates(fromjson("{$set: {x: 0}}"));
-        request.setSort(BSONObj());
-        request.setMulti(false);
-        // Emulate a findAndModify.
-        request.setReturnDocs(UpdateRequest::ReturnDocOption::RETURN_OLD);
-        request.setLifecycle(&updateLifecycle);
-
-        ASSERT_OK(driver.parse(request.getUpdates(), request.isMulti()));
-
-        // Configure a QueuedDataStage to pass an OWNED_OBJ to the update stage.
-        auto qds = make_unique<QueuedDataStage>(&_txn, ws.get());
-        {
-            WorkingSetID id = ws->allocate();
-            WorkingSetMember* member = ws->get(id);
-            member->obj = Snapshotted<BSONObj>(SnapshotId(), fromjson("{x: 1}"));
-            member->transitionToOwnedObj();
-            qds->pushBack(id);
-        }
-
-        // Configure the update.
-        UpdateStageParams updateParams(&request, &driver, opDebug);
-        updateParams.canonicalQuery = cq.get();
-
-        const auto updateStage =
-            make_unique<UpdateStage>(&_txn, updateParams, ws.get(), coll, qds.release());
-
-        // Call work, passing the set up member to the update stage.
-        WorkingSetID id = WorkingSet::INVALID_ID;
-        ASSERT_THROWS(updateStage->work(&id), WriteConflictException);
-    }
-};
-
 class All : public Suite {
 public:
     All() : Suite("query_stage_update") {}
@@ -656,7 +599,6 @@ public:
         add<QueryStageUpdateReturnOldDoc>();
         add<QueryStageUpdateReturnNewDoc>();
         add<QueryStageUpdateSkipOwnedObjects>();
-        add<QueryStageUpdateShouldRetryConflictsForFindAndModify>();
     }
 };