SERVER-51420 Optimize delete to only read document once

6 files changed, 68 insertions, 13 deletions

diff --git a/src/mongo/db/catalog/collection.h b/src/mongo/db/catalog/collection.h
index 72d1497f362..3eab72b82ea 100644
--- a/src/mongo/db/catalog/collection.h
+++ b/src/mongo/db/catalog/collection.h
@@ -325,8 +325,21 @@ public:
                                                             const bool forward = true) const = 0;
 
     /**
-     * Deletes the document with the given RecordId from the collection.
+     * Deletes the document with the given RecordId from the collection. For a description of the
+     * parameters, see the overloaded function below.
+     */
+    virtual void deleteDocument(OperationContext* const opCtx,
+                                StmtId stmtId,
+                                RecordId loc,
+                                OpDebug* const opDebug,
+                                const bool fromMigrate = false,
+                                const bool noWarn = false,
+                                StoreDeletedDoc storeDeletedDoc = StoreDeletedDoc::Off) const = 0;
+
+    /**
+     * Deletes the document from the collection.
      *
+     * 'doc' the document to be deleted.
      * 'fromMigrate' indicates whether the delete was induced by a chunk migration, and
      * so should be ignored by the user as an internal maintenance operation and not a
      * real delete.
@@ -336,6 +349,7 @@ public:
      * will not be logged.
      */
     virtual void deleteDocument(OperationContext* const opCtx,
+                                Snapshotted<BSONObj> doc,
                                 StmtId stmtId,
                                 RecordId loc,
                                 OpDebug* const opDebug,
diff --git a/src/mongo/db/catalog/collection_impl.cpp b/src/mongo/db/catalog/collection_impl.cpp
index 1ede05ecd60..7dbb969842b 100644
--- a/src/mongo/db/catalog/collection_impl.cpp
+++ b/src/mongo/db/catalog/collection_impl.cpp
@@ -779,6 +779,18 @@ void CollectionImpl::deleteDocument(OperationContext* opCtx,
                                     bool fromMigrate,
                                     bool noWarn,
                                     Collection::StoreDeletedDoc storeDeletedDoc) const {
+    Snapshotted<BSONObj> doc = docFor(opCtx, loc);
+    deleteDocument(opCtx, doc, stmtId, loc, opDebug, fromMigrate, noWarn, storeDeletedDoc);
+}
+
+void CollectionImpl::deleteDocument(OperationContext* opCtx,
+                                    Snapshotted<BSONObj> doc,
+                                    StmtId stmtId,
+                                    RecordId loc,
+                                    OpDebug* opDebug,
+                                    bool fromMigrate,
+                                    bool noWarn,
+                                    Collection::StoreDeletedDoc storeDeletedDoc) const {
     if (isCapped()) {
         LOGV2(20291,
               "failing remove on a capped ns {ns}",
@@ -788,7 +800,6 @@ void CollectionImpl::deleteDocument(OperationContext* opCtx,
         return;
     }
 
-    Snapshotted<BSONObj> doc = docFor(opCtx, loc);
     getGlobalServiceContext()->getOpObserver()->aboutToDelete(opCtx, ns(), doc.value());
 
     boost::optional<BSONObj> deletedDoc;
diff --git a/src/mongo/db/catalog/collection_impl.h b/src/mongo/db/catalog/collection_impl.h
index 162a327456e..c6c10c1a755 100644
--- a/src/mongo/db/catalog/collection_impl.h
+++ b/src/mongo/db/catalog/collection_impl.h
@@ -121,8 +121,22 @@ public:
                                                     bool forward = true) const final;
 
     /**
-     * Deletes the document with the given RecordId from the collection.
-     *
+     * Deletes the document with the given RecordId from the collection. For a description of
+     * the parameters, see the overloaded function below.
+     */
+    void deleteDocument(
+        OperationContext* opCtx,
+        StmtId stmtId,
+        RecordId loc,
+        OpDebug* opDebug,
+        bool fromMigrate = false,
+        bool noWarn = false,
+        Collection::StoreDeletedDoc storeDeletedDoc = Collection::StoreDeletedDoc::Off) const final;
+
+    /**
+     * Deletes the document from the collection.
+
+     * 'doc' the document to be deleted.
      * 'stmtId' the statement id for this delete operation. Pass in kUninitializedStmtId if not
      * applicable.
      * 'fromMigrate' indicates whether the delete was induced by a chunk migration, and
@@ -137,6 +151,7 @@ public:
      */
     void deleteDocument(
         OperationContext* opCtx,
+        Snapshotted<BSONObj> doc,
         StmtId stmtId,
         RecordId loc,
         OpDebug* opDebug,
diff --git a/src/mongo/db/catalog/collection_mock.h b/src/mongo/db/catalog/collection_mock.h
index 062bc468be2..fd080fd64db 100644
--- a/src/mongo/db/catalog/collection_mock.h
+++ b/src/mongo/db/catalog/collection_mock.h
@@ -121,6 +121,18 @@ public:
         std::abort();
     }
 
+    void deleteDocument(
+        OperationContext* opCtx,
+        Snapshotted<BSONObj> doc,
+        StmtId stmtId,
+        RecordId loc,
+        OpDebug* opDebug,
+        bool fromMigrate = false,
+        bool noWarn = false,
+        Collection::StoreDeletedDoc storeDeletedDoc = Collection::StoreDeletedDoc::Off) const {
+        std::abort();
+    }
+
     Status insertDocuments(OperationContext* opCtx,
                            std::vector<InsertStatement>::const_iterator begin,
                            std::vector<InsertStatement>::const_iterator end,
diff --git a/src/mongo/db/exec/delete.cpp b/src/mongo/db/exec/delete.cpp
index faca86675bd..bef064c1cc6 100644
--- a/src/mongo/db/exec/delete.cpp
+++ b/src/mongo/db/exec/delete.cpp
@@ -167,16 +167,18 @@ PlanStage::StageState DeleteStage::doWork(WorkingSetID* out) {
         return PlanStage::NEED_TIME;
     }
 
-    // Ensure that the BSONObj underlying the WorkingSetMember is owned because saveState() is
-    // allowed to free the memory.
-    if (_params->returnDeleted) {
-        // Save a copy of the document that is about to get deleted, but keep it in the RID_AND_OBJ
-        // state in case we need to retry deleting it.
-        member->makeObjOwnedIfNeeded();
-    }
+    // Ensure that the BSONObj underlying the WSM is owned because saveState() is
+    // allowed to free the memory the BSONObj points to. The BSONObj will be needed
+    // later when it is passed to Collection::deleteDocument(). Note that the call to
+    // makeObjOwnedIfNeeded() will leave the WSM in the RID_AND_OBJ state in case we need to retry
+    // deleting it.
+    member->makeObjOwnedIfNeeded();
+
+    Snapshotted<Document> memberDoc = member->doc;
+    BSONObj bsonObjDoc = memberDoc.value().toBson();
 
     if (_params->removeSaver) {
-        uassertStatusOK(_params->removeSaver->goingToDelete(member->doc.value().toBson()));
+        uassertStatusOK(_params->removeSaver->goingToDelete(bsonObjDoc));
     }
 
     // TODO: Do we want to buffer docs and delete them in a group rather than saving/restoring state
@@ -193,6 +195,7 @@ PlanStage::StageState DeleteStage::doWork(WorkingSetID* out) {
         try {
             WriteUnitOfWork wunit(opCtx());
             collection()->deleteDocument(opCtx(),
+                                         Snapshotted(memberDoc.snapshotId(), bsonObjDoc),
                                          _params->stmtId,
                                          recordId,
                                          _params->opDebug,
diff --git a/src/mongo/db/storage/ephemeral_for_test/ephemeral_for_test_record_store.cpp b/src/mongo/db/storage/ephemeral_for_test/ephemeral_for_test_record_store.cpp
index 5bf74f9fb04..1e85c6ed572 100644
--- a/src/mongo/db/storage/ephemeral_for_test/ephemeral_for_test_record_store.cpp
+++ b/src/mongo/db/storage/ephemeral_for_test/ephemeral_for_test_record_store.cpp
@@ -130,7 +130,7 @@ bool RecordStore::findRecord(OperationContext* opCtx, const RecordId& loc, Recor
     if (it == workingCopy->end()) {
         return false;
     }
-    *rd = RecordData(it->second.c_str(), it->second.length());
+    *rd = RecordData(it->second.c_str(), it->second.length()).getOwned();
     return true;
 }