[SERVER-25540] investigate why StorageInterfaceImpl::_findOrDeleteOne needs to make owned copy of BSON document returned from PlanExecutor Created: 10/Aug/16  Updated: 16/Aug/16  Resolved: 16/Aug/16

Status: Closed
Project: Core Server
Component/s: Querying
Affects Version/s: None
Fix Version/s: None

Type: Bug Priority: Major - P3
Reporter: Benety Goh Assignee: David Storch
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: Text File backtrace.txt    
Issue Links:
Related
is related to SERVER-25538 StorageInterfaceImpl::_findOrDeleteOn... Closed
Backwards Compatibility: Fully Compatible
Operating System: ALL
Sprint: Query 2016-09-19
Participants:
Linked BF Score: 0

 Comments   
Comment by Benety Goh [ 16/Aug/16 ]

Thanks for looking into this. We definitely want the BSONObj to outlive the plan executor, so getOwned() works for us.

Comment by David Storch [ 16/Aug/16 ]

I spoke with benety.goh in person who confirmed that this is the desired fix. The returned BSONObj needs to outlive the PlanExecutor that generated it, and therefore it must always be owned. Closing as Works as Designed.

Comment by David Storch [ 15/Aug/16 ]

charlie.swanson, got it, things are starting to make sense now. The IXSCAN stage is totally allowed to return an unowned BSONObj. It will remain unowned if there is no subsequent parent stage, such as DELETE, which might transition the BSONObj to owned. So it doesn't look like there is a problem where the DELETE stage with the returnDeleted flag can somehow manage to return unowned BSON.

benety.goh, given this explanation, do you still believe that calling getOwned() as done in https://github.com/mongodb/mongo/commit/33f11f7786c4e742fe7a75333323612671667e32 is the correct fix?

Comment by Charlie Swanson [ 15/Aug/16 ]

After some investigation, it looks like jstests/replsets/initial_sync_update_missing_doc1.js is causing this path to be executed during StorageInterfaceImpl::_findOrDeleteOne.

This was determined after applying the following patch:

diff --git a/src/mongo/db/repl/storage_interface_impl.cpp b/src/mongo/db/repl/storage_interface_impl.cpp
index e8b32a1..78886b1 100644
--- a/src/mongo/db/repl/storage_interface_impl.cpp
+++ b/src/mongo/db/repl/storage_interface_impl.cpp
@@ -427,14 +427,19 @@ StatusWith<BSONObj> _findOrDeleteOne(OperationContext* txn,
         std::unique_ptr<PlanExecutor> planExecutor;
         if (!indexName) {
             // Use collection scan.
-            planExecutor = isFind
-                ? InternalPlanner::collectionScan(
-                      txn, nss.ns(), collection, PlanExecutor::YIELD_MANUAL, direction)
-                : InternalPlanner::deleteWithCollectionScan(txn,
-                                                            collection,
-                                                            makeDeleteStageParamsForDeleteOne(),
-                                                            PlanExecutor::YIELD_MANUAL,
-                                                            direction);
+            if (isFind) {
+                log() << "REGULAR COLL SCAN";
+                planExecutor = InternalPlanner::collectionScan(
+                    txn, nss.ns(), collection, PlanExecutor::YIELD_MANUAL, direction);
+            } else {
+                log() << "DELETE WITH COLL SCAN";
+                planExecutor =
+                    InternalPlanner::deleteWithCollectionScan(txn,
+                                                              collection,
+                                                              makeDeleteStageParamsForDeleteOne(),
+                                                              PlanExecutor::YIELD_MANUAL,
+                                                              direction);
+            }
         } else {
             // Use index scan.
             auto indexCatalog = collection->getIndexCatalog();
@@ -462,25 +467,30 @@ StatusWith<BSONObj> _findOrDeleteOne(OperationContext* txn,
                 isForward ? std::make_pair(minKey, maxKey) : std::make_pair(maxKey, minKey);
             bool endKeyInclusive = false;
 
-            planExecutor = isFind
-                ? InternalPlanner::indexScan(txn,
-                                             collection,
-                                             indexDescriptor,
-                                             bounds.first,
-                                             bounds.second,
-                                             endKeyInclusive,
-                                             PlanExecutor::YIELD_MANUAL,
-                                             direction,
-                                             InternalPlanner::IXSCAN_FETCH)
-                : InternalPlanner::deleteWithIndexScan(txn,
-                                                       collection,
-                                                       makeDeleteStageParamsForDeleteOne(),
-                                                       indexDescriptor,
-                                                       bounds.first,
-                                                       bounds.second,
-                                                       endKeyInclusive,
-                                                       PlanExecutor::YIELD_MANUAL,
-                                                       direction);
+            if (isFind) {
+                log() << "REGULAR INDEX SCAN";
+                planExecutor = InternalPlanner::indexScan(txn,
+                                                          collection,
+                                                          indexDescriptor,
+                                                          bounds.first,
+                                                          bounds.second,
+                                                          endKeyInclusive,
+                                                          PlanExecutor::YIELD_MANUAL,
+                                                          direction,
+                                                          InternalPlanner::IXSCAN_FETCH);
+            } else {
+                log() << "DELETE WITH INDEX SCAN";
+                planExecutor =
+                    InternalPlanner::deleteWithIndexScan(txn,
+                                                         collection,
+                                                         makeDeleteStageParamsForDeleteOne(),
+                                                         indexDescriptor,
+                                                         bounds.first,
+                                                         bounds.second,
+                                                         endKeyInclusive,
+                                                         PlanExecutor::YIELD_MANUAL,
+                                                         direction);
+            }
         }
 
         BSONObj doc;
@@ -490,6 +500,7 @@ StatusWith<BSONObj> _findOrDeleteOne(OperationContext* txn,
                     str::stream() << "Collection is empty, ns: " << nss.ns()};
         }
         invariant(PlanExecutor::ADVANCED == state);
+        invariant(doc.isOwned());
         return doc;
     }
     MONGO_WRITE_CONFLICT_RETRY_LOOP_END(txn, opStr, nss.ns());

Generated the attached backtrace while running jstests/replsets/initial_sync_update_missing_doc1.js.

Generated at Thu Feb 08 04:09:28 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.