diff options
author | Ian Boros <ian.boros@mongodb.com> | 2020-05-08 14:05:12 -0400 |
---|---|---|
committer | Evergreen Agent <no-reply@evergreen.mongodb.com> | 2020-05-19 18:57:41 +0000 |
commit | 64906095f4a590a34e1f22042aea2836721f7bbe (patch) | |
tree | be6b8b712ad3dd33adbc95c1dcf5ffd58f33cc8e | |
parent | 25ee2b61ed8467f7c07b26bf52953d25caed95fd (diff) | |
download | mongo-64906095f4a590a34e1f22042aea2836721f7bbe.tar.gz |
SERVER-48052 Make updates create ExpressionContext with 'explain' field correctly initialized
-rw-r--r-- | jstests/core/find_and_modify_pipeline_update.js | 47 | ||||
-rw-r--r-- | jstests/core/update_pipeline_shell_helpers.js | 42 | ||||
-rw-r--r-- | src/mongo/db/commands/find_and_modify.cpp | 9 | ||||
-rw-r--r-- | src/mongo/db/commands/write_commands/write_commands.cpp | 2 | ||||
-rw-r--r-- | src/mongo/db/exec/update_stage.cpp | 8 | ||||
-rw-r--r-- | src/mongo/db/exec/upsert_stage.cpp | 2 | ||||
-rw-r--r-- | src/mongo/db/ops/parsed_update.cpp | 16 | ||||
-rw-r--r-- | src/mongo/db/ops/update.cpp | 2 | ||||
-rw-r--r-- | src/mongo/db/ops/update_request.h | 15 | ||||
-rw-r--r-- | src/mongo/db/pipeline/expression_context.cpp | 6 | ||||
-rw-r--r-- | src/mongo/db/pipeline/expression_context.h | 3 | ||||
-rw-r--r-- | src/mongo/db/query/get_executor.cpp | 5 |
12 files changed, 123 insertions, 34 deletions
diff --git a/jstests/core/find_and_modify_pipeline_update.js b/jstests/core/find_and_modify_pipeline_update.js index 781b4d0335d..0340d29bd66 100644 --- a/jstests/core/find_and_modify_pipeline_update.js +++ b/jstests/core/find_and_modify_pipeline_update.js @@ -6,6 +6,7 @@ "use strict"; load("jstests/libs/fixture_helpers.js"); // For isMongos. +load("jstests/libs/analyze_plan.js"); // For planHasStage(). const coll = db.find_and_modify_pipeline_update; coll.drop(); @@ -35,10 +36,52 @@ assert.eq(found, {_id: 2, y: 2}); found = coll.findAndModify({query: {_id: 3}, update: [{$set: {y: 3}}], fields: {x: 1}, new: true}); assert.eq(found, {_id: 3, x: 3}); -// We skip the following test for sharded fixtures as it will fail as the query for -// findAndModify must contain the shard key. +// +// Tests for explain using findAndModify with an _id equality query. +// +{ + let explain = + coll.explain("queryPlanner").findAndModify({query: {_id: 3}, update: [{$set: {y: 999}}]}); + assert(planHasStage(db, explain.queryPlanner.winningPlan, "IDHACK")); + assert(planHasStage(db, explain.queryPlanner.winningPlan, "UPDATE")); + + // Run explain with execution-level verbosity. + explain = + coll.explain("executionStats").findAndModify({query: {_id: 3}, update: [{$set: {y: 999}}]}); + assert.eq(explain.executionStats.nReturned, 1); + // UPDATE stage would modify one document. + const updateStage = getPlanStage(explain.executionStats.executionStages, "UPDATE"); + assert.eq(updateStage.nWouldModify, 1); + + // Check that no write was performed. + assert.eq(coll.find({y: 999}).count(), 0); +} + +// We skip the following tests for sharded fixtures as it will fail as the query when +// findAndModify doesn't contain the shard key. if (!FixtureHelpers.isMongos(db)) { + // + // Tests for explain with a query that requires a COLLSCAN. + // + let explain = + coll.explain("queryPlanner").findAndModify({query: {y: 3}, update: [{$set: {y: 999}}]}); + assert(planHasStage(db, explain.queryPlanner.winningPlan, "COLLSCAN")); + assert(planHasStage(db, explain.queryPlanner.winningPlan, "UPDATE")); + + // Run explain with execution-level verbosity. + explain = + coll.explain("executionStats").findAndModify({query: {y: 3}, update: [{$set: {y: 999}}]}); + assert.eq(explain.executionStats.nReturned, 1); + // UPDATE stage would modify one document. + const updateStage = getPlanStage(explain.executionStats.executionStages, "UPDATE"); + assert.eq(updateStage.nWouldModify, 1); + + // Check that no write was performed. + assert.eq(coll.find({y: 999}).count(), 0); + + // // Test that 'sort' works with pipeline-style update. + // assert(coll.drop()); assert.commandWorked( coll.insert([{_id: 0, x: 'b'}, {_id: 1, x: 'd'}, {_id: 2, x: 'a'}, {_id: 3, x: 'c'}])); diff --git a/jstests/core/update_pipeline_shell_helpers.js b/jstests/core/update_pipeline_shell_helpers.js index d8bb7d7eb3d..7a4d2419ad7 100644 --- a/jstests/core/update_pipeline_shell_helpers.js +++ b/jstests/core/update_pipeline_shell_helpers.js @@ -10,6 +10,8 @@ "use strict"; load("jstests/aggregation/extras/utils.js"); // For 'arrayEq'. +load("jstests/libs/analyze_plan.js"); // For planHasStage(). +load("jstests/libs/fixture_helpers.js"); // For isMongos(). // Make sure that the test collection is empty before starting the test. const testColl = db.update_pipeline_shell_helpers_test; @@ -82,6 +84,46 @@ const findOneAndUpdatePostImage = testColl.findOneAndUpdate( {_id: 1}, [{$set: {findOneAndUpdate: true}}], {returnNewDocument: true}); assert.docEq(findOneAndUpdatePostImage, expectedFindOneAndUpdatePostImage); +// +// Explain for updates that use an _id lookup query. +// +{ + let explain = testColl.explain("queryPlanner").update({_id: 2}, [{$set: {y: 999}}]); + assert(planHasStage(db, explain.queryPlanner.winningPlan, "IDHACK")); + assert(planHasStage(db, explain.queryPlanner.winningPlan, "UPDATE")); + + // Run explain with execution-level verbosity. + explain = testColl.explain("executionStats").update({_id: 2}, [{$set: {y: 999}}]); + assert.eq(explain.executionStats.totalDocsExamined, 1, explain); + // UPDATE stage would modify one document. + const updateStage = getPlanStage(explain.executionStats.executionStages, "UPDATE"); + assert.eq(updateStage.nWouldModify, 1); + + // Check that no write was performed. + assert.eq(testColl.find({y: 999}).count(), 0); +} + +// +// Explain for updates that use a query which requires a COLLSCAN. +// + +// We skip these tests under sharded fixtures, since sharded passthroughs require that FAM queries +// contain the shard key. +if (!FixtureHelpers.isMongos(db)) { + let explain = testColl.explain("queryPlanner").update({a: 2}, [{$set: {y: 999}}]); + assert(planHasStage(db, explain.queryPlanner.winningPlan, "COLLSCAN")); + assert(planHasStage(db, explain.queryPlanner.winningPlan, "UPDATE")); + + // Run explain with execution-level verbosity. + explain = testColl.explain("executionStats").update({a: 2}, [{$set: {y: 999}}]); + // UPDATE stage would modify one document. + const updateStage = getPlanStage(explain.executionStats.executionStages, "UPDATE"); + assert.eq(updateStage.nWouldModify, 1); + + // Check that no write was performed. + assert.eq(testColl.find({y: 999}).count(), 0); +} + // Shell helpers for replacement updates should reject pipeline-style updates. assert.throws(() => testColl.replaceOne({_id: 1}, [{$replaceWith: {}}])); assert.throws(() => testColl.findOneAndReplace({_id: 1}, [{$replaceWith: {}}])); diff --git a/src/mongo/db/commands/find_and_modify.cpp b/src/mongo/db/commands/find_and_modify.cpp index 2c86f93243d..40ab8129ec2 100644 --- a/src/mongo/db/commands/find_and_modify.cpp +++ b/src/mongo/db/commands/find_and_modify.cpp @@ -121,7 +121,7 @@ boost::optional<BSONObj> advanceExecutor(OperationContext* opCtx, void makeUpdateRequest(OperationContext* opCtx, const FindAndModifyRequest& args, - bool explain, + boost::optional<ExplainOptions::Verbosity> explain, UpdateRequest* requestOut) { requestOut->setQuery(args.getQuery()); requestOut->setProj(args.getFields()); @@ -291,8 +291,7 @@ public: } else { auto request = UpdateRequest(); request.setNamespaceString(nsString); - const bool isExplain = true; - makeUpdateRequest(opCtx, args, isExplain, &request); + makeUpdateRequest(opCtx, args, verbosity, &request); const ExtensionsCallbackReal extensionsCallback(opCtx, &request.getNamespaceString()); ParsedUpdate parsedUpdate(opCtx, &request, extensionsCallback); @@ -385,8 +384,8 @@ public: for (;;) { auto request = UpdateRequest(); request.setNamespaceString(nsString); - const bool isExplain = false; - makeUpdateRequest(opCtx, args, isExplain, &request); + const auto verbosity = boost::none; + makeUpdateRequest(opCtx, args, verbosity, &request); if (opCtx->getTxnNumber()) { request.setStmtId(stmtId); diff --git a/src/mongo/db/commands/write_commands/write_commands.cpp b/src/mongo/db/commands/write_commands/write_commands.cpp index 9228fa50fb5..6d1ea157c21 100644 --- a/src/mongo/db/commands/write_commands/write_commands.cpp +++ b/src/mongo/db/commands/write_commands/write_commands.cpp @@ -419,7 +419,7 @@ private: _batch.getRuntimeConstants().value_or(Variables::generateRuntimeConstants(opCtx))); updateRequest.setLetParameters(_batch.getLet()); updateRequest.setYieldPolicy(PlanExecutor::YIELD_AUTO); - updateRequest.setExplain(); + updateRequest.setExplain(verbosity); const ExtensionsCallbackReal extensionsCallback(opCtx, &updateRequest.getNamespaceString()); diff --git a/src/mongo/db/exec/update_stage.cpp b/src/mongo/db/exec/update_stage.cpp index dd7f405115e..8e8c6694865 100644 --- a/src/mongo/db/exec/update_stage.cpp +++ b/src/mongo/db/exec/update_stage.cpp @@ -250,7 +250,7 @@ BSONObj UpdateStage::transformAndUpdate(const Snapshotted<BSONObj>& oldObj, Reco RecordId newRecordId; CollectionUpdateArgs args; - if (!request->isExplain()) { + if (!request->explain()) { args.stmtId = request->getStmtId(); args.update = logObj; if (isUserInitiatedWrite) { @@ -272,7 +272,7 @@ BSONObj UpdateStage::transformAndUpdate(const Snapshotted<BSONObj>& oldObj, Reco } if (inPlace) { - if (!request->isExplain()) { + if (!request->explain()) { newObj = oldObj.value(); const RecordData oldRec(oldObj.value().objdata(), oldObj.value().objsize()); @@ -302,7 +302,7 @@ BSONObj UpdateStage::transformAndUpdate(const Snapshotted<BSONObj>& oldObj, Reco << BSONObjMaxUserSize, newObj.objsize() <= BSONObjMaxUserSize); - if (!request->isExplain()) { + if (!request->explain()) { if (_shouldCheckForShardKeyUpdate && checkUpdateChangesShardKeyFields(oldObj) && !args.preImageDoc) { args.preImageDoc = oldObj.value().getOwned(); @@ -336,7 +336,7 @@ BSONObj UpdateStage::transformAndUpdate(const Snapshotted<BSONObj>& oldObj, Reco // Only record doc modifications if they wrote (exclude no-ops). Explains get // recorded as if they wrote. - if (docWasModified || request->isExplain()) { + if (docWasModified || request->explain()) { _specificStats.nModified++; } diff --git a/src/mongo/db/exec/upsert_stage.cpp b/src/mongo/db/exec/upsert_stage.cpp index dc81610bb96..5860ccfb1cf 100644 --- a/src/mongo/db/exec/upsert_stage.cpp +++ b/src/mongo/db/exec/upsert_stage.cpp @@ -91,7 +91,7 @@ PlanStage::StageState UpsertStage::doWork(WorkingSetID* out) { _specificStats.objInserted = _produceNewDocumentForInsert(); // If this is an explain, skip performing the actual insert. - if (!_params.request->isExplain()) { + if (!_params.request->explain()) { _performInsert(_specificStats.objInserted); } diff --git a/src/mongo/db/ops/parsed_update.cpp b/src/mongo/db/ops/parsed_update.cpp index 18285ed37d5..34458e1f8dd 100644 --- a/src/mongo/db/ops/parsed_update.cpp +++ b/src/mongo/db/ops/parsed_update.cpp @@ -45,11 +45,15 @@ ParsedUpdate::ParsedUpdate(OperationContext* opCtx, const ExtensionsCallback& extensionsCallback) : _opCtx(opCtx), _request(request), - _expCtx(make_intrusive<ExpressionContext>(opCtx, - nullptr, - _request->getNamespaceString(), - _request->getRuntimeConstants(), - _request->getLetParameters())), + _expCtx(make_intrusive<ExpressionContext>( + opCtx, + nullptr, + _request->getNamespaceString(), + _request->getRuntimeConstants(), + _request->getLetParameters(), + true, // mayDbProfile. We pass 'true' here conservatively. In the future we may + // change this. + request->explain())), _driver(_expCtx), _canonicalQuery(), _extensionsCallback(extensionsCallback) {} @@ -124,7 +128,7 @@ Status ParsedUpdate::parseQueryToCQ() { auto qr = std::make_unique<QueryRequest>(_request->getNamespaceString()); qr->setFilter(_request->getQuery()); qr->setSort(_request->getSort()); - qr->setExplain(_request->isExplain()); + qr->setExplain(static_cast<bool>(_request->explain())); qr->setHint(_request->getHint()); // We get the collation off the ExpressionContext because it may contain a collection-default diff --git a/src/mongo/db/ops/update.cpp b/src/mongo/db/ops/update.cpp index 62e88d71e2e..c065f670167 100644 --- a/src/mongo/db/ops/update.cpp +++ b/src/mongo/db/ops/update.cpp @@ -57,7 +57,7 @@ UpdateResult update(OperationContext* opCtx, Database* db, const UpdateRequest& invariant(db); // Explain should never use this helper. - invariant(!request.isExplain()); + invariant(!request.explain()); const NamespaceString& nsString = request.getNamespaceString(); invariant(opCtx->lockState()->isCollectionLockedForMode(nsString, MODE_IX)); diff --git a/src/mongo/db/ops/update_request.h b/src/mongo/db/ops/update_request.h index 6404b5dcd70..d5bbbd4bdcc 100644 --- a/src/mongo/db/ops/update_request.h +++ b/src/mongo/db/ops/update_request.h @@ -196,12 +196,12 @@ public: return _fromOplogApplication; } - void setExplain(bool value = true) { - _isExplain = value; + void setExplain(boost::optional<ExplainOptions::Verbosity> verbosity) { + _explain = verbosity; } - bool isExplain() const { - return _isExplain; + boost::optional<ExplainOptions::Verbosity> explain() const { + return _explain; } void setReturnDocs(ReturnDocOption value) { @@ -277,7 +277,7 @@ public: builder << " multi: " << isMulti(); builder << " fromMigration: " << _fromMigration; builder << " fromOplogApplication: " << _fromOplogApplication; - builder << " isExplain: " << _isExplain; + builder << " isExplain: " << static_cast<bool>(_explain); return builder.str(); } @@ -314,8 +314,9 @@ private: // True if this update was triggered by the application of an oplog entry. bool _fromOplogApplication = false; - // Whether or not we are requesting an explained update. Explained updates are read-only. - bool _isExplain = false; + // Whether or not we are requesting an explained update, and if so, which type. Explained + // updates may involve executing stages, but they will not perform writes. + boost::optional<ExplainOptions::Verbosity> _explain; // Specifies which version of the documents to return, if any. // diff --git a/src/mongo/db/pipeline/expression_context.cpp b/src/mongo/db/pipeline/expression_context.cpp index 8625cf80903..f33d6a16e06 100644 --- a/src/mongo/db/pipeline/expression_context.cpp +++ b/src/mongo/db/pipeline/expression_context.cpp @@ -134,8 +134,10 @@ ExpressionContext::ExpressionContext(OperationContext* opCtx, const NamespaceString& nss, const boost::optional<RuntimeConstants>& runtimeConstants, const boost::optional<BSONObj>& letParameters, - bool mayDbProfile) - : ns(nss), + bool mayDbProfile, + boost::optional<ExplainOptions::Verbosity> explain) + : explain(explain), + ns(nss), opCtx(opCtx), mongoProcessInterface(std::make_shared<StubMongoProcessInterface>()), timeZoneDatabase(opCtx && opCtx->getServiceContext() diff --git a/src/mongo/db/pipeline/expression_context.h b/src/mongo/db/pipeline/expression_context.h index d3a491119d3..6cd1bba4f3b 100644 --- a/src/mongo/db/pipeline/expression_context.h +++ b/src/mongo/db/pipeline/expression_context.h @@ -142,7 +142,8 @@ public: const NamespaceString& ns, const boost::optional<RuntimeConstants>& runtimeConstants = boost::none, const boost::optional<BSONObj>& letParameters = boost::none, - bool mayDbProfile = true); + bool mayDbProfile = true, + boost::optional<ExplainOptions::Verbosity> explain = boost::none); /** * Used by a pipeline to check for interrupts so that killOp() works. Throws a UserAssertion if diff --git a/src/mongo/db/query/get_executor.cpp b/src/mongo/db/query/get_executor.cpp index b1bae642936..c27145180ac 100644 --- a/src/mongo/db/query/get_executor.cpp +++ b/src/mongo/db/query/get_executor.cpp @@ -915,7 +915,7 @@ StatusWith<unique_ptr<PlanExecutor, PlanExecutor::Deleter>> getExecutorUpdate( // the collection prior to calling this method. Explain, however, will never do // collection or database creation. if (!collection && request->isUpsert()) { - invariant(request->isExplain()); + invariant(request->explain()); } // If the parsed update does not have a user-specified collation, set it from the collection @@ -1002,9 +1002,6 @@ StatusWith<unique_ptr<PlanExecutor, PlanExecutor::Deleter>> getExecutorUpdate( // This is the regular path for when we have a CanonicalQuery. unique_ptr<CanonicalQuery> cq(parsedUpdate->releaseParsedQuery()); - // Transfer the explain verbosity level into the expression context. - cq->getExpCtx()->explain = verbosity; - std::unique_ptr<projection_ast::Projection> projection; if (!request->getProj().isEmpty()) { invariant(request->shouldReturnAnyDocs()); |