SERVER-28309 remove RangePreserver class

RangePreserver was an old way to ensure that necessary chunk
ranges are not deleted during query execution. This is now
handled by ScopedCollectionMetadata.

12 files changed, 29 insertions, 175 deletions

diff --git a/jstests/noPassthrough/aggregation_zero_batchsize.js b/jstests/noPassthrough/aggregation_zero_batchsize.js
index c53edf21355..7660985af74 100644
--- a/jstests/noPassthrough/aggregation_zero_batchsize.js
+++ b/jstests/noPassthrough/aggregation_zero_batchsize.js
@@ -59,8 +59,7 @@
         cursor: {batchSize: 0}
     }));
     cursor = new DBCommandCursor(conn, res);
-    // SERVER-28309 We should only report 1 open cursor per aggregation.
-    assertNumOpenCursors(2);
+    assertNumOpenCursors(1);
 
     assert.throws(() => cursor.itcount(), [], "expected getMore to fail");
     assertNumOpenCursors(0);
@@ -76,8 +75,7 @@
         cursor: {batchSize: 0}
     }));
     cursor = new DBCommandCursor(conn, res);
-    // SERVER-28309 We should only report 1 open cursor per aggregation.
-    assertNumOpenCursors(2);
+    assertNumOpenCursors(1);
 
     // Add a document validation rule to the $out collection so that insertion will fail.
     assert.commandWorked(testDB.runCommand(
diff --git a/src/mongo/db/catalog/cursor_manager.cpp b/src/mongo/db/catalog/cursor_manager.cpp
index e1c662b194b..a1c442b78e5 100644
--- a/src/mongo/db/catalog/cursor_manager.cpp
+++ b/src/mongo/db/catalog/cursor_manager.cpp
@@ -360,15 +360,11 @@ void CursorManager::invalidateDocument(OperationContext* opCtx,
 
     for (ExecSet::iterator it = _nonCachedExecutors.begin(); it != _nonCachedExecutors.end();
          ++it) {
-        PlanExecutor* exec = *it;
-        exec->invalidate(opCtx, dl, type);
+        (*it)->invalidate(opCtx, dl, type);
     }
 
     for (CursorMap::const_iterator i = _cursors.begin(); i != _cursors.end(); ++i) {
-        PlanExecutor* exec = i->second->getExecutor();
-        if (exec) {
-            exec->invalidate(opCtx, dl, type);
-        }
+        i->second->getExecutor()->invalidate(opCtx, dl, type);
     }
 }
 
@@ -415,9 +411,8 @@ StatusWith<ClientCursorPin> CursorManager::pinCursor(OperationContext* opCtx, Cu
 
     ClientCursor* cursor = it->second;
     uassert(12051, str::stream() << "cursor id " << id << " is already in use", !cursor->_isPinned);
-    if (cursor->_killed) {
+    if (cursor->getExecutor()->isMarkedAsKilled()) {
         // This cursor was killed while it was idle.
-        invariant(cursor->getExecutor());  // We should never unpin RangePreserver cursors.
         Status error{ErrorCodes::QueryPlanKilled,
                      str::stream() << "cursor killed because: "
                                    << cursor->getExecutor()->getKillReason()};
@@ -485,24 +480,9 @@ ClientCursorPin CursorManager::registerCursor(OperationContext* opCtx,
     cursorParams.exec->unsetRegistered();
 
     CursorId cursorId = _allocateCursorId_inlock();
+    invariant(cursorId);
     std::unique_ptr<ClientCursor, ClientCursor::Deleter> clientCursor(
         new ClientCursor(std::move(cursorParams), this, cursorId));
-    return _registerCursor_inlock(opCtx, std::move(clientCursor));
-}
-
-ClientCursorPin CursorManager::registerRangePreserverCursor(OperationContext* opCtx,
-                                                            const Collection* collection) {
-    stdx::lock_guard<SimpleMutex> lk(_mutex);
-    CursorId cursorId = _allocateCursorId_inlock();
-    std::unique_ptr<ClientCursor, ClientCursor::Deleter> clientCursor(
-        new ClientCursor(collection, this, cursorId));
-    return _registerCursor_inlock(opCtx, std::move(clientCursor));
-}
-
-ClientCursorPin CursorManager::_registerCursor_inlock(
-    OperationContext* opCtx, std::unique_ptr<ClientCursor, ClientCursor::Deleter> clientCursor) {
-    CursorId cursorId = clientCursor->cursorid();
-    invariant(cursorId);
 
     // Transfer ownership of the cursor to '_cursors'.
     ClientCursor* unownedCursor = clientCursor.release();
diff --git a/src/mongo/db/catalog/cursor_manager.h b/src/mongo/db/catalog/cursor_manager.h
index 7dd65313d4c..9a5ae951490 100644
--- a/src/mongo/db/catalog/cursor_manager.h
+++ b/src/mongo/db/catalog/cursor_manager.h
@@ -129,13 +129,6 @@ public:
     ClientCursorPin registerCursor(OperationContext* opCtx, ClientCursorParams&& cursorParams);
 
     /**
-     * Constructs and pins a special ClientCursor used to track sharding state for the given
-     * collection. See range_preserver.h for more details.
-     */
-    ClientCursorPin registerRangePreserverCursor(OperationContext* opCtx,
-                                                 const Collection* collection);
-
-    /**
      * Pins and returns the cursor with the given id.
      *
      * Returns ErrorCodes::CursorNotFound if the cursor does not exist or
@@ -195,8 +188,6 @@ private:
 
     CursorId _allocateCursorId_inlock();
     void _deregisterCursor_inlock(ClientCursor* cc);
-    ClientCursorPin _registerCursor_inlock(
-        OperationContext* opCtx, std::unique_ptr<ClientCursor, ClientCursor::Deleter> clientCursor);
 
     void deregisterCursor(ClientCursor* cc);
 
diff --git a/src/mongo/db/clientcursor.cpp b/src/mongo/db/clientcursor.cpp
index 48730e12b8c..3b1c5c97307 100644
--- a/src/mongo/db/clientcursor.cpp
+++ b/src/mongo/db/clientcursor.cpp
@@ -92,18 +92,9 @@ ClientCursor::ClientCursor(ClientCursorParams&& params,
     init();
 }
 
-ClientCursor::ClientCursor(const Collection* collection,
-                           CursorManager* cursorManager,
-                           CursorId cursorId)
-    : _cursorid(cursorId),
-      _nss(collection->ns()),
-      _cursorManager(cursorManager),
-      _queryOptions(QueryOption_NoCursorTimeout) {
-    init();
-}
-
 void ClientCursor::init() {
     invariant(_cursorManager);
+    invariant(_exec);
 
     cursorStatsOpen.increment();
 
@@ -126,10 +117,7 @@ ClientCursor::~ClientCursor() {
 }
 
 void ClientCursor::markAsKilled(const std::string& reason) {
-    if (_exec) {
-        _exec->markAsKilled(reason);
-    }
-    _killed = true;
+    _exec->markAsKilled(reason);
 }
 
 void ClientCursor::dispose(OperationContext* opCtx) {
@@ -137,10 +125,7 @@ void ClientCursor::dispose(OperationContext* opCtx) {
         return;
     }
 
-    if (_exec) {
-        _exec->dispose(opCtx, _cursorManager);
-    }
-
+    _exec->dispose(opCtx, _cursorManager);
     _disposed = true;
 }
 
@@ -238,7 +223,7 @@ void ClientCursorPin::release() {
 
     invariant(_cursor->_isPinned);
 
-    if (_cursor->_killed) {
+    if (_cursor->getExecutor()->isMarkedAsKilled()) {
         // The ClientCursor was killed while we had it.  Therefore, it is our responsibility to
         // call dispose() and delete it.
         deleteUnderlying();
@@ -262,7 +247,7 @@ void ClientCursorPin::deleteUnderlying() {
     //   we can't simply unpin with the cursor manager lock here, since we need to guarantee
     //   exclusive ownership of the cursor when we are deleting it).
 
-    if (!_cursor->_killed) {
+    if (!_cursor->getExecutor()->isMarkedAsKilled()) {
         _cursor->_cursorManager->deregisterCursor(_cursor);
     }
 
diff --git a/src/mongo/db/clientcursor.h b/src/mongo/db/clientcursor.h
index e23c0101775..d1d299f0d0d 100644
--- a/src/mongo/db/clientcursor.h
+++ b/src/mongo/db/clientcursor.h
@@ -112,6 +112,10 @@ public:
         return _isReadCommitted;
     }
 
+    /**
+     * Returns a pointer to the underlying query plan executor. All cursors manage a PlanExecutor,
+     * so this method never returns a null pointer.
+     */
     PlanExecutor* getExecutor() const {
         return _exec.get();
     }
@@ -234,11 +238,6 @@ private:
     ClientCursor(ClientCursorParams&& params, CursorManager* cursorManager, CursorId cursorId);
 
     /**
-     * Constructs a special ClientCursor used to track sharding state for the given collection.
-     */
-    ClientCursor(const Collection* collection, CursorManager* cursorManager, CursorId cursorId);
-
-    /**
      * Destroys a ClientCursor. This is private, since only the CursorManager or the ClientCursorPin
      * is allowed to destroy a cursor.
      *
@@ -285,9 +284,6 @@ private:
     // an error to use a ClientCursor once it has been disposed.
     bool _disposed = false;
 
-    // TODO SERVER-28309 Remove this field and instead use _exec->markedAsKilled().
-    bool _killed = false;
-
     // Tracks the number of results returned by this cursor so far.
     long long _pos = 0;
 
@@ -312,7 +308,7 @@ private:
     // Unused maxTime budget for this cursor.
     Microseconds _leftoverMaxTimeMicros = Microseconds::max();
 
-    // The underlying query execution machinery.
+    // The underlying query execution machinery. Must be non-null.
     std::unique_ptr<PlanExecutor, PlanExecutor::Deleter> _exec;
 };
 
diff --git a/src/mongo/db/commands/count_cmd.cpp b/src/mongo/db/commands/count_cmd.cpp
index e0c782779e5..8ca762e2022 100644
--- a/src/mongo/db/commands/count_cmd.cpp
+++ b/src/mongo/db/commands/count_cmd.cpp
@@ -40,7 +40,6 @@
 #include "mongo/db/query/get_executor.h"
 #include "mongo/db/query/plan_summary_stats.h"
 #include "mongo/db/query/view_response_formatter.h"
-#include "mongo/db/range_preserver.h"
 #include "mongo/db/repl/replication_coordinator_global.h"
 #include "mongo/db/server_options.h"
 #include "mongo/db/views/resolved_view.h"
@@ -148,7 +147,8 @@ public:
 
         // Prevent chunks from being cleaned up during yields - this allows us to only check the
         // version on initial entry into count.
-        RangePreserver preserver(opCtx, collection);
+        auto rangePreserver =
+            CollectionShardingState::get(opCtx, request.getValue().getNs())->getMetadata();
 
         auto statusWithPlanExecutor = getExecutorCount(opCtx,
                                                        collection,
@@ -216,7 +216,8 @@ public:
 
         // Prevent chunks from being cleaned up during yields - this allows us to only check the
         // version on initial entry into count.
-        RangePreserver preserver(opCtx, collection);
+        auto rangePreserver =
+            CollectionShardingState::get(opCtx, request.getValue().getNs())->getMetadata();
 
         auto statusWithPlanExecutor = getExecutorCount(opCtx,
                                                        collection,
diff --git a/src/mongo/db/commands/geo_near_cmd.cpp b/src/mongo/db/commands/geo_near_cmd.cpp
index 43b57c06b68..65cf9a857dc 100644
--- a/src/mongo/db/commands/geo_near_cmd.cpp
+++ b/src/mongo/db/commands/geo_near_cmd.cpp
@@ -52,7 +52,6 @@
 #include "mongo/db/query/find_common.h"
 #include "mongo/db/query/get_executor.h"
 #include "mongo/db/query/plan_summary_stats.h"
-#include "mongo/db/range_preserver.h"
 #include "mongo/db/server_options.h"
 #include "mongo/platform/unordered_map.h"
 #include "mongo/util/log.h"
@@ -234,7 +233,7 @@ public:
 
         // Prevent chunks from being cleaned up during yields - this allows us to only check the
         // version on initial entry into geoNear.
-        RangePreserver preserver(opCtx, collection);
+        auto rangePreserver = CollectionShardingState::get(opCtx, nss)->getMetadata();
 
         auto statusWithPlanExecutor =
             getExecutor(opCtx, collection, std::move(cq), PlanExecutor::YIELD_AUTO, 0);
diff --git a/src/mongo/db/commands/mr.cpp b/src/mongo/db/commands/mr.cpp
index 667a05ea69b..7b64dadbbdb 100644
--- a/src/mongo/db/commands/mr.cpp
+++ b/src/mongo/db/commands/mr.cpp
@@ -58,7 +58,6 @@
 #include "mongo/db/query/get_executor.h"
 #include "mongo/db/query/plan_summary_stats.h"
 #include "mongo/db/query/query_planner.h"
-#include "mongo/db/range_preserver.h"
 #include "mongo/db/repl/replication_coordinator_global.h"
 #include "mongo/db/s/collection_metadata.h"
 #include "mongo/db/s/collection_sharding_state.h"
@@ -1406,35 +1405,17 @@ public:
         uassert(16149, "cannot run map reduce without the js engine", getGlobalScriptEngine());
 
         // Prevent sharding state from changing during the MR.
-        unique_ptr<RangePreserver> rangePreserver;
         ScopedCollectionMetadata collMetadata;
         {
-            AutoGetCollectionForReadCommand ctx(opCtx, config.nss);
-
-            Collection* collection = ctx.getCollection();
+            // Get metadata before we check our version, to make sure it doesn't increment in the
+            // meantime.
+            AutoGetCollectionForReadCommand autoColl(opCtx, config.nss);
+            auto collection = autoColl.getCollection();
             if (collection) {
-                rangePreserver.reset(new RangePreserver(opCtx, collection));
-            }
-
-            // Get metadata before we check our version, to make sure it doesn't increment
-            // in the meantime.  Need to do this in the same lock scope as the block.
-            if (ShardingState::get(opCtx)->needCollectionMetadata(opCtx, config.nss.ns())) {
                 collMetadata = CollectionShardingState::get(opCtx, config.nss)->getMetadata();
             }
         }
 
-        // Ensure that the RangePreserver is freed under the lock. This is necessary since the
-        // RangePreserver's destructor unpins a ClientCursor, and access to the CursorManager must
-        // be done under the lock.
-        ON_BLOCK_EXIT([opCtx, &config, &rangePreserver] {
-            if (rangePreserver) {
-                // Be sure not to use AutoGetCollectionForReadCommand here, since that has
-                // side-effects other than lock acquisition.
-                AutoGetCollection ctx(opCtx, config.nss, MODE_IS);
-                rangePreserver.reset();
-            }
-        });
-
         bool shouldHaveData = false;
 
         BSONObjBuilder countsBuilder;
diff --git a/src/mongo/db/pipeline/document_source_cursor.cpp b/src/mongo/db/pipeline/document_source_cursor.cpp
index aaa362575a4..9704d826e53 100644
--- a/src/mongo/db/pipeline/document_source_cursor.cpp
+++ b/src/mongo/db/pipeline/document_source_cursor.cpp
@@ -237,7 +237,6 @@ void DocumentSourceCursor::cleanupExecutor() {
     auto cursorManager = collection ? collection->getCursorManager() : nullptr;
     _exec->dispose(opCtx, cursorManager);
     _exec.reset();
-    _rangePreserver.release();
 }
 
 DocumentSourceCursor::~DocumentSourceCursor() {
@@ -250,7 +249,6 @@ DocumentSourceCursor::DocumentSourceCursor(
     const intrusive_ptr<ExpressionContext>& pCtx)
     : DocumentSource(pCtx),
       _docsAddedToBatches(0),
-      _rangePreserver(pExpCtx->opCtx, collection),
       _exec(std::move(exec)),
       _outputSorts(_exec->getOutputSorts()) {
 
diff --git a/src/mongo/db/pipeline/document_source_cursor.h b/src/mongo/db/pipeline/document_source_cursor.h
index 11489897b72..85e79b3cb2a 100644
--- a/src/mongo/db/pipeline/document_source_cursor.h
+++ b/src/mongo/db/pipeline/document_source_cursor.h
@@ -33,13 +33,11 @@
 #include "mongo/db/namespace_string.h"
 #include "mongo/db/pipeline/document_source.h"
 #include "mongo/db/pipeline/document_source_limit.h"
+#include "mongo/db/query/plan_executor.h"
 #include "mongo/db/query/plan_summary_stats.h"
-#include "mongo/db/range_preserver.h"
 
 namespace mongo {
 
-class PlanExecutor;
-
 /**
  * Constructs and returns Documents from the BSONObj objects produced by a supplied PlanExecutor.
  */
@@ -175,8 +173,8 @@ private:
     boost::intrusive_ptr<DocumentSourceLimit> _limit;
     long long _docsAddedToBatches;  // for _limit enforcement
 
-    // Both '_rangePreserver' and '_exec' must be destroyed while holding the collection lock.
-    RangePreserver _rangePreserver;
+    // The underlying query plan which feeds this pipeline. Must be destroyed while holding the
+    // collection lock.
     std::unique_ptr<PlanExecutor, PlanExecutor::Deleter> _exec;
 
     BSONObjSet _outputSorts;
diff --git a/src/mongo/db/query/plan_executor.h b/src/mongo/db/query/plan_executor.h
index 4e9a39b426c..edefc56465a 100644
--- a/src/mongo/db/query/plan_executor.h
+++ b/src/mongo/db/query/plan_executor.h
@@ -402,7 +402,7 @@ public:
         _registered = false;
     }
 
-    bool isMarkedAsKilled() {
+    bool isMarkedAsKilled() const {
         return static_cast<bool>(_killReason);
     };
 
diff --git a/src/mongo/db/range_preserver.h b/src/mongo/db/range_preserver.h
deleted file mode 100644
index 2a893c61b87..00000000000
--- a/src/mongo/db/range_preserver.h
+++ /dev/null
@@ -1,73 +0,0 @@
-/**
- *    Copyright (C) 2013 10gen Inc.
- *
- *    This program is free software: you can redistribute it and/or  modify
- *    it under the terms of the GNU Affero General Public License, version 3,
- *    as published by the Free Software Foundation.
- *
- *    This program is distributed in the hope that it will be useful,
- *    but WITHOUT ANY WARRANTY; without even the implied warranty of
- *    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *    GNU Affero General Public License for more details.
- *
- *    You should have received a copy of the GNU Affero General Public License
- *    along with this program.  If not, see <http://www.gnu.org/licenses/>.
- *
- *    As a special exception, the copyright holders give permission to link the
- *    code of portions of this program with the OpenSSL library under certain
- *    conditions as described in each individual source file and distribute
- *    linked combinations including the program with the OpenSSL library. You
- *    must comply with the GNU Affero General Public License in all respects for
- *    all of the code used other than as permitted herein. If you modify file(s)
- *    with this exception, you may extend this exception to your version of the
- *    file(s), but you are not obligated to do so. If you do not wish to do so,
- *    delete this exception statement from your version. If you delete this
- *    exception statement from all source files in the program, then also delete
- *    it in the license file.
- */
-
-#include <boost/optional.hpp>
-
-#include "mongo/db/catalog/collection.h"
-#include "mongo/db/clientcursor.h"
-
-namespace mongo {
-
-/**
- * A RangePreserver prevents the RangeDeleter from removing any new data ranges in a collection.
- * Previously queued ranges may still be deleted but the documents in those ranges will be
- * filtered by CollectionMetadata::belongsToMe.
- */
-class RangePreserver {
-public:
-    /**
-     * Sharding uses the set of active cursor IDs as the current state. We pin a dummy
-     * ClientCursor, which creates an additional cursor ID. The cursor ID lasts as long as this
-     * object does. The ClientCursorPin guarantees that the underlying ClientCursor is not deleted
-     * until this object goes out of scope.
-     */
-    RangePreserver(OperationContext* opCtx, const Collection* collection) {
-        // Empty collections don't have any data we need to preserve
-        if (collection) {
-            // Pin keeps the CC from being deleted while it's in scope.  We delete it ourselves.
-            _pin.emplace(
-                collection->getCursorManager()->registerRangePreserverCursor(opCtx, collection));
-        }
-    }
-
-    void release() {
-        if (_pin) {
-            _pin->deleteUnderlying();
-            _pin.reset();
-        }
-    }
-
-    ~RangePreserver() {
-        release();
-    }
-
-private:
-    boost::optional<ClientCursorPin> _pin;
-};
-
-}  // namespace mongo