SERVER-23551 $isolated writes should use WRITE_CONFLICT_RETRY_ONLY

7 files changed, 43 insertions, 26 deletions

diff --git a/jstests/noPassthrough/update_yield1.js b/jstests/noPassthrough/update_yield1.js
index 0135be037ce..29d1e99d46a 100644
--- a/jstests/noPassthrough/update_yield1.js
+++ b/jstests/noPassthrough/update_yield1.js
@@ -1,7 +1,19 @@
 // Ensure that multi-update operations yield regularly.
+// Unless they are $isolated and they shouldn't yield at all.
 (function() {
     'use strict';
 
+    function countUpdateYields(coll) {
+        var profileEntry = coll.getDB()
+                               .system.profile.find({ns: coll.getFullName()})
+                               .sort({$natural: -1})
+                               .limit(1)
+                               .next();
+        printjson(profileEntry);
+        assert.eq(profileEntry.op, 'update');
+        return profileEntry.numYield;
+    }
+
     var explain;
     var yieldCount;
 
@@ -11,7 +23,8 @@
     // Start a mongod that will yield every 50 work cycles.
     // clang-format off
     var mongod = MongoRunner.runMongod({
-        setParameter: `internalQueryExecYieldIterations=${worksPerYield}`
+        setParameter: `internalQueryExecYieldIterations=${worksPerYield}`,
+        profile: 2,
     });
     // clang-format on
     assert.neq(null, mongod, 'mongod was unable to start up');
@@ -25,15 +38,10 @@
 
     // A multi-update doing a collection scan should yield about nDocsToInsert / worksPerYield
     // times.
-    explain = coll.explain('executionStats').update({}, {$inc: {counter: 1}}, {multi: true});
-    assert.commandWorked(explain);
-    yieldCount = explain.executionStats.executionStages.saveState;
-    assert.gt(yieldCount, (nDocsToInsert / worksPerYield) - 2);
+    assert.writeOK(coll.update({}, {$inc: {counter: 1}}, {multi: true}));
+    assert.gt(countUpdateYields(coll), (nDocsToInsert / worksPerYield) - 2);
 
     // A multi-update shouldn't yield if it has $isolated.
-    explain = coll.explain('executionStats')
-                  .update({$isolated: true}, {$inc: {counter: 1}}, {multi: true});
-    assert.commandWorked(explain);
-    yieldCount = explain.executionStats.executionStages.saveState;
-    assert.eq(yieldCount, 0, 'yielded during $isolated multi-update');
+    assert.writeOK(coll.update({$isolated: true}, {$inc: {counter: 1}}, {multi: true}));
+    assert.eq(countUpdateYields(coll), 0, 'yielded during $isolated multi-update');
 })();
diff --git a/src/mongo/db/ops/parsed_delete.cpp b/src/mongo/db/ops/parsed_delete.cpp
index ba193ea9ac8..5b488412827 100644
--- a/src/mongo/db/ops/parsed_delete.cpp
+++ b/src/mongo/db/ops/parsed_delete.cpp
@@ -107,9 +107,14 @@ const DeleteRequest* ParsedDelete::getRequest() const {
     return _request;
 }
 
-bool ParsedDelete::canYield() const {
-    return !_request->isGod() && PlanExecutor::YIELD_AUTO == _request->getYieldPolicy() &&
-        !isIsolated();
+PlanExecutor::YieldPolicy ParsedDelete::yieldPolicy() const {
+    if (_request->isGod()) {
+        return PlanExecutor::YIELD_MANUAL;
+    }
+    if (_request->getYieldPolicy() == PlanExecutor::YIELD_AUTO && isIsolated()) {
+        return PlanExecutor::WRITE_CONFLICT_RETRY_ONLY;  // Don't yield locks.
+    }
+    return _request->getYieldPolicy();
 }
 
 bool ParsedDelete::isIsolated() const {
diff --git a/src/mongo/db/ops/parsed_delete.h b/src/mongo/db/ops/parsed_delete.h
index cdf22b4cac7..1a36b9b78ea 100644
--- a/src/mongo/db/ops/parsed_delete.h
+++ b/src/mongo/db/ops/parsed_delete.h
@@ -84,9 +84,9 @@ public:
     const DeleteRequest* getRequest() const;
 
     /**
-     * Is this delete allowed to yield?
+     * Get the YieldPolicy, adjusted for $isolated and GodMode.
      */
-    bool canYield() const;
+    PlanExecutor::YieldPolicy yieldPolicy() const;
 
     /**
      * Is this update supposed to be isolated?
diff --git a/src/mongo/db/ops/parsed_update.cpp b/src/mongo/db/ops/parsed_update.cpp
index 174538f33a8..f96bf92939c 100644
--- a/src/mongo/db/ops/parsed_update.cpp
+++ b/src/mongo/db/ops/parsed_update.cpp
@@ -123,9 +123,14 @@ Status ParsedUpdate::parseUpdate() {
     return _driver.parse(_request->getUpdates(), _request->isMulti());
 }
 
-bool ParsedUpdate::canYield() const {
-    return !_request->isGod() && PlanExecutor::YIELD_AUTO == _request->getYieldPolicy() &&
-        !isIsolated();
+PlanExecutor::YieldPolicy ParsedUpdate::yieldPolicy() const {
+    if (_request->isGod()) {
+        return PlanExecutor::YIELD_MANUAL;
+    }
+    if (_request->getYieldPolicy() == PlanExecutor::YIELD_AUTO && isIsolated()) {
+        return PlanExecutor::WRITE_CONFLICT_RETRY_ONLY;  // Don't yield locks.
+    }
+    return _request->getYieldPolicy();
 }
 
 bool ParsedUpdate::isIsolated() const {
diff --git a/src/mongo/db/ops/parsed_update.h b/src/mongo/db/ops/parsed_update.h
index 3c0cf015c94..71be604944f 100644
--- a/src/mongo/db/ops/parsed_update.h
+++ b/src/mongo/db/ops/parsed_update.h
@@ -30,6 +30,7 @@
 
 #include "mongo/base/disallow_copying.h"
 #include "mongo/base/status.h"
+#include "mongo/db/query/plan_executor.h"
 #include "mongo/db/ops/update_driver.h"
 
 namespace mongo {
@@ -89,9 +90,9 @@ public:
     UpdateDriver* getDriver();
 
     /**
-     * Is this update allowed to yield?
+     * Get the YieldPolicy, adjusted for $isolated and GodMode.
      */
-    bool canYield() const;
+    PlanExecutor::YieldPolicy yieldPolicy() const;
 
     /**
      * Is this update supposed to be isolated?
diff --git a/src/mongo/db/query/get_executor.cpp b/src/mongo/db/query/get_executor.cpp
index 933f60f34f8..ec3c113e2d8 100644
--- a/src/mongo/db/query/get_executor.cpp
+++ b/src/mongo/db/query/get_executor.cpp
@@ -676,8 +676,7 @@ StatusWith<unique_ptr<PlanExecutor>> getExecutorDelete(OperationContext* txn,
     deleteStageParams.opDebug = opDebug;
 
     unique_ptr<WorkingSet> ws = make_unique<WorkingSet>();
-    PlanExecutor::YieldPolicy policy =
-        parsedDelete->canYield() ? PlanExecutor::YIELD_AUTO : PlanExecutor::YIELD_MANUAL;
+    const PlanExecutor::YieldPolicy policy = parsedDelete->yieldPolicy();
 
     if (!parsedDelete->hasParsedQuery()) {
         // This is the idhack fast-path for getting a PlanExecutor without doing the work
@@ -816,8 +815,7 @@ StatusWith<unique_ptr<PlanExecutor>> getExecutorUpdate(OperationContext* txn,
         driver->refreshIndexKeys(lifecycle->getIndexKeys(txn));
     }
 
-    PlanExecutor::YieldPolicy policy =
-        parsedUpdate->canYield() ? PlanExecutor::YIELD_AUTO : PlanExecutor::YIELD_MANUAL;
+    const PlanExecutor::YieldPolicy policy = parsedUpdate->yieldPolicy();
 
     unique_ptr<WorkingSet> ws = make_unique<WorkingSet>();
     UpdateStageParams updateStageParams(request, driver, opDebug);
diff --git a/src/mongo/db/query/get_executor.h b/src/mongo/db/query/get_executor.h
index 34c8cf0f0e8..f96203b7fea 100644
--- a/src/mongo/db/query/get_executor.h
+++ b/src/mongo/db/query/get_executor.h
@@ -135,7 +135,7 @@ StatusWith<std::unique_ptr<PlanExecutor>> getExecutorCount(OperationContext* txn
  * and delete flags like 'isMulti'. The caller must hold the appropriate MODE_X or MODE_IX
  * locks, and must not release these locks until after the returned PlanExecutor is deleted.
  *
- * The returned PlanExecutor will yield if and only if parsedDelete->canYield().
+ * The returned PlanExecutor will used the YieldPolicy returned by parsedDelete->yieldPolicy().
  *
  * Does not take ownership of its arguments.
  *
@@ -155,7 +155,7 @@ StatusWith<std::unique_ptr<PlanExecutor>> getExecutorDelete(OperationContext* tx
  * to calling this function, and must not release these locks until after the returned
  * PlanExecutor is deleted.
  *
- * The returned PlanExecutor will yield if and only if parsedUpdate->canYield().
+ * The returned PlanExecutor will used the YieldPolicy returned by parsedUpdate->yieldPolicy().
  *
  * Does not take ownership of its arguments.
  *