diff options
author | Pawel Terlecki <pawel.terlecki@mongodb.com> | 2020-02-26 03:22:52 -0500 |
---|---|---|
committer | Evergreen Agent <no-reply@evergreen.mongodb.com> | 2020-02-27 04:59:38 +0000 |
commit | 78020f2c19cb3318cac3d603be99cfd9a3fe12b1 (patch) | |
tree | cfa5f58a9da1341bd13bc8c6c073fbcf53a4ecc8 | |
parent | b2bb338e99292dc4a266f4915fe8671de4e5fa70 (diff) | |
download | mongo-78020f2c19cb3318cac3d603be99cfd9a3fe12b1.tar.gz |
SERVER-44367 Allow hinting the delete command
Add hinting to delete an findAndModify commands
based on how it was done for update.
-rw-r--r-- | jstests/core/delete_hint.js | 117 | ||||
-rw-r--r-- | jstests/core/find_and_modify_hint.js | 28 | ||||
-rw-r--r-- | jstests/core/update_hint.js | 21 | ||||
-rw-r--r-- | src/mongo/db/commands/find_and_modify.cpp | 1 | ||||
-rw-r--r-- | src/mongo/db/commands/write_commands/write_commands.cpp | 1 | ||||
-rw-r--r-- | src/mongo/db/ops/delete_request.h | 7 | ||||
-rw-r--r-- | src/mongo/db/ops/parsed_delete.cpp | 1 | ||||
-rw-r--r-- | src/mongo/db/ops/write_ops.idl | 4 | ||||
-rw-r--r-- | src/mongo/db/ops/write_ops_exec.cpp | 2 | ||||
-rw-r--r-- | src/mongo/db/query/find_and_modify_request.cpp | 6 | ||||
-rw-r--r-- | src/mongo/db/query/get_executor.cpp | 67 |
11 files changed, 198 insertions, 57 deletions
diff --git a/jstests/core/delete_hint.js b/jstests/core/delete_hint.js new file mode 100644 index 00000000000..a3f98e5e13f --- /dev/null +++ b/jstests/core/delete_hint.js @@ -0,0 +1,117 @@ +/** + * Tests passing hint to the delete command: + * - A bad argument to the hint option should raise an error. + * - The hint option should support both the name of the index, and the object spec of the + * index. + * + * @tags: [assumes_unsharded_collection, requires_non_retryable_writes] + */ + +(function() { +"use strict"; + +load("jstests/libs/analyze_plan.js"); + +function assertCommandUsesIndex(command, expectedHintKeyPattern) { + const out = assert.commandWorked(coll.runCommand({explain: command})); + const planStage = getPlanStage(out, "IXSCAN"); + assert.neq(null, planStage); + assert.eq(planStage.keyPattern, expectedHintKeyPattern, tojson(planStage)); +} + +const coll = db.jstests_delete_hint; + +function normalIndexTest() { + // Hint using a key pattern. + coll.drop(); + assert.commandWorked(coll.insert({x: 1, y: 1})); + assert.commandWorked(coll.createIndex({x: 1})); + assert.commandWorked(coll.createIndex({y: -1})); + + // Hint using index key pattern. + let deleteCmd = {delete: coll.getName(), deletes: [{q: {x: 1}, limit: 1, hint: {x: 1}}]}; + assertCommandUsesIndex(deleteCmd, {x: 1}); + + // Hint using an index name. + deleteCmd = {delete: coll.getName(), deletes: [{q: {x: 1}, limit: 1, hint: 'y_-1'}]}; + assertCommandUsesIndex(deleteCmd, {y: -1}); + + // Passing a hint should not use the idhack fast-path. + deleteCmd = {delete: coll.getName(), deletes: [{q: {_id: 1}, limit: 1, hint: 'y_-1'}]}; + assertCommandUsesIndex(deleteCmd, {y: -1}); +} + +function sparseIndexTest() { + // Create a sparse index with 2 documents. + coll.drop(); + assert.commandWorked(coll.insert([{x: 1}, {x: 1}, {x: 1, s: 0}, {x: 1, s: 0}])); + assert.commandWorked(coll.createIndex({x: 1})); + assert.commandWorked(coll.createIndex({s: 1}, {sparse: true})); + + // Hint should be respected, even on incomplete indexes. + let deleteCmd = {delete: coll.getName(), deletes: [{q: {_id: 1}, limit: 1, hint: {s: 1}}]}; + assertCommandUsesIndex(deleteCmd, {s: 1}); + + // Delete hinting a sparse index deletes only the document in the sparse index. + deleteCmd = {delete: coll.getName(), deletes: [{q: {}, limit: 0, hint: {s: 1}}]}; + let res = assert.commandWorked(coll.runCommand(deleteCmd)); + assert.eq(2, res.n); +} + +function shellHelpersTest() { + coll.drop(); + assert.commandWorked(coll.insert([{x: 1}, {x: 1, s: 0}, {x: 1, s: 0}])); + assert.commandWorked(coll.createIndex({x: 1})); + assert.commandWorked(coll.createIndex({s: 1}, {sparse: true})); + + // Test shell helpers using a hinted sparse index should only delete documents that exist in + // the sparse index. + let res = coll.deleteOne({x: 1}, {hint: {s: 1}}); + assert.eq(res.deletedCount, 1); + + // Test bulk writes. + let bulk = coll.initializeUnorderedBulkOp(); + bulk.find({x: 1}).hint({s: 1}).remove(); + res = bulk.execute(); + assert.eq(res.nRemoved, 2); + bulk = coll.initializeUnorderedBulkOp(); + bulk.find({x: 2}).hint({s: 1}).removeOne(); + res = bulk.execute(); + assert.eq(res.nRemoved, 0); + + assert.commandWorked(coll.insert([{x: 0}, {x: 1, s: 0}, {x: 1, s: 0}, {x: 1, s: 0}])); + res = coll.bulkWrite([{deleteOne: {filter: {x: 1}, hint: {s: 1}}}]); + assert.eq(res.deletedCount, 1); + + res = coll.bulkWrite([{ + deleteMany: { + filter: {x: 1}, + hint: {s: 1}, + } + }]); + assert.eq(res.deletedCount, 2); +} + +function failedHintTest() { + coll.drop(); + assert.commandWorked(coll.insert({x: 1})); + assert.commandWorked(coll.createIndex({x: 1})); + + // Command should fail with incorrectly formatted hints. + let deleteCmd = {delete: coll.getName(), deletes: [{q: {_id: 1}, limit: 1, hint: 1}]}; + assert.commandFailedWithCode(coll.runCommand(deleteCmd), ErrorCodes.FailedToParse); + deleteCmd = {delete: coll.getName(), deletes: [{q: {_id: 1}, limit: 1, hint: true}]}; + assert.commandFailedWithCode(coll.runCommand(deleteCmd), ErrorCodes.FailedToParse); + + // Command should fail with hints to non-existent indexes. + deleteCmd = {delete: coll.getName(), deletes: [{q: {_id: 1}, limit: 1, hint: {badHint: 1}}]}; + assert.commandFailedWithCode(coll.runCommand(deleteCmd), ErrorCodes.BadValue); +} + +normalIndexTest(); +sparseIndexTest(); +if (coll.getMongo().writeMode() === "commands") { + shellHelpersTest(); +} +failedHintTest(); +})(); diff --git a/jstests/core/find_and_modify_hint.js b/jstests/core/find_and_modify_hint.js index e6becb3e048..836fab7d4c8 100644 --- a/jstests/core/find_and_modify_hint.js +++ b/jstests/core/find_and_modify_hint.js @@ -42,14 +42,17 @@ const coll = db.jstests_find_and_modify_hint; famUpdateCmd = {findAndModify: coll.getName(), query: {_id: 1}, update: {$set: {y: 1}}, hint: 'y_-1'}; assertCommandUsesIndex(famUpdateCmd, {y: -1}); + + // Hint using an index name when removing documents. + const famRemoveCmd = + {findAndModify: coll.getName(), query: {_id: 1}, remove: true, hint: 'y_-1'}; + assertCommandUsesIndex(famRemoveCmd, {y: -1}); })(); (function sparseIndexTest() { // Create a sparse index which includes only 1 of the 3 documents in the collection. coll.drop(); - assert.commandWorked(coll.insert({_id: 0, x: 1})); - assert.commandWorked(coll.insert({_id: 1, x: 1})); - assert.commandWorked(coll.insert({_id: 2, x: 1, s: 0})); + assert.commandWorked(coll.insert([{_id: 0, x: 1}, {_id: 1, x: 1}, {_id: 2, x: 1, s: 0}])); assert.commandWorked(coll.createIndex({x: 1})); assert.commandWorked(coll.createIndex({s: 1}, {sparse: true})); @@ -78,13 +81,17 @@ const coll = db.jstests_find_and_modify_hint; res = assert.commandWorked(coll.runCommand(famUpdateCmd)); assert.eq(res.lastErrorObject.upserted, 3); // value of _id assert.docEq(res.value, {_id: 3, x: 2, y: 1}); + + // Make sure an indexed document gets deleted when index hint is provided. + assert.commandWorked(coll.insert({x: 1})); + const famRemoveCmd = {findAndModify: coll.getName(), query: {x: 1}, remove: true, hint: {s: 1}}; + res = assert.commandWorked(coll.runCommand(famRemoveCmd)); + assert.docEq(res.value, {_id: 2, x: 1, s: 0, y: 1}); })(); (function shellHelpersTest() { coll.drop(); - assert.commandWorked(coll.insert({_id: 0, x: 1})); - assert.commandWorked(coll.insert({_id: 1, x: 1})); - assert.commandWorked(coll.insert({_id: 2, x: 1, s: 0})); + assert.commandWorked(coll.insert([{_id: 0, x: 1}, {_id: 1, x: 1}, {_id: 2, x: 1, s: 0}])); assert.commandWorked(coll.createIndex({x: 1})); assert.commandWorked(coll.createIndex({s: 1}, {sparse: true})); @@ -106,6 +113,10 @@ const coll = db.jstests_find_and_modify_hint; newDoc = coll.findOneAndReplace( {x: 3}, {_id: 6, y: 2}, {hint: {s: 1}, upsert: true, returnNewDocument: true}); assert.docEq(newDoc, {_id: 6, y: 2}); + + // Make sure an indexed document gets deleted when index hint is provided. + newDoc = coll.findOneAndDelete({x: 2}, {hint: {s: 1}}); + assert.docEq(newDoc, {_id: 3, x: 2}); })(); (function failedHintTest() { @@ -129,10 +140,5 @@ const coll = db.jstests_find_and_modify_hint; hint: {badHint: 1} }; assert.commandFailedWithCode(coll.runCommand(famUpdateCmd), ErrorCodes.BadValue); - - // Cannot use hint with {remove: true}. - const famRemoveCmd = - {findAndModify: coll.getName(), query: {_id: 1}, remove: true, hint: {_id: 1}}; - assert.commandFailedWithCode(coll.runCommand(famRemoveCmd), ErrorCodes.FailedToParse); })(); })(); diff --git a/jstests/core/update_hint.js b/jstests/core/update_hint.js index 734ac92e5d2..287f63ef148 100644 --- a/jstests/core/update_hint.js +++ b/jstests/core/update_hint.js @@ -20,7 +20,6 @@ function assertCommandUsesIndex(command, expectedHintKeyPattern) { } const coll = db.jstests_update_hint; -let updateCmd = {update: coll.getName(), updates: [{q: {x: 1}, u: {$set: {y: 1}}, hint: {x: 1}}]}; function normalIndexTest() { // Hint using a key pattern. @@ -30,6 +29,10 @@ function normalIndexTest() { assert.commandWorked(coll.createIndex({y: -1})); // Hint using index key pattern. + let updateCmd = { + update: coll.getName(), + updates: [{q: {x: 1}, u: {$set: {y: 1}}, hint: {x: 1}}] + }; assertCommandUsesIndex(updateCmd, {x: 1}); // Hint using an index name. @@ -44,15 +47,15 @@ function normalIndexTest() { function sparseIndexTest() { // Create a sparse index with 2 documents. coll.drop(); - assert.commandWorked(coll.insert({x: 1})); - assert.commandWorked(coll.insert({x: 1})); - assert.commandWorked(coll.insert({x: 1, s: 0})); - assert.commandWorked(coll.insert({x: 1, s: 0})); + assert.commandWorked(coll.insert([{x: 1}, {x: 1}, {x: 1, s: 0}, {x: 1, s: 0}])); assert.commandWorked(coll.createIndex({x: 1})); assert.commandWorked(coll.createIndex({s: 1}, {sparse: true})); // Hint should be respected, even on incomplete indexes. - updateCmd = {update: coll.getName(), updates: [{q: {_id: 1}, u: {$set: {y: 1}}, hint: {s: 1}}]}; + let updateCmd = { + update: coll.getName(), + updates: [{q: {_id: 1}, u: {$set: {y: 1}}, hint: {s: 1}}] + }; assertCommandUsesIndex(updateCmd, {s: 1}); // Update hinting a sparse index updates only the document in the sparse index. @@ -76,9 +79,7 @@ function sparseIndexTest() { function shellHelpersTest() { coll.drop(); - assert.commandWorked(coll.insert({x: 1})); - assert.commandWorked(coll.insert({x: 1, s: 0})); - assert.commandWorked(coll.insert({x: 1, s: 0})); + assert.commandWorked(coll.insert([{x: 1}, {x: 1, s: 0}, {x: 1, s: 0}])); assert.commandWorked(coll.createIndex({x: 1})); assert.commandWorked(coll.createIndex({s: 1}, {sparse: true})); @@ -145,7 +146,7 @@ function failedHintTest() { assert.commandWorked(coll.createIndex({x: 1})); // Command should fail with incorrectly formatted hints. - updateCmd = {update: coll.getName(), updates: [{q: {_id: 1}, u: {$set: {y: 1}}, hint: 1}]}; + let updateCmd = {update: coll.getName(), updates: [{q: {_id: 1}, u: {$set: {y: 1}}, hint: 1}]}; assert.commandFailedWithCode(coll.runCommand(updateCmd), ErrorCodes.FailedToParse); updateCmd = {update: coll.getName(), updates: [{q: {_id: 1}, u: {$set: {y: 1}}, hint: true}]}; assert.commandFailedWithCode(coll.runCommand(updateCmd), ErrorCodes.FailedToParse); diff --git a/src/mongo/db/commands/find_and_modify.cpp b/src/mongo/db/commands/find_and_modify.cpp index 5d995f1c70e..35f9bb54a4d 100644 --- a/src/mongo/db/commands/find_and_modify.cpp +++ b/src/mongo/db/commands/find_and_modify.cpp @@ -146,6 +146,7 @@ void makeDeleteRequest(OperationContext* opCtx, requestOut->setRuntimeConstants( args.getRuntimeConstants().value_or(Variables::generateRuntimeConstants(opCtx))); requestOut->setSort(args.getSort()); + requestOut->setHint(args.getHint()); requestOut->setCollation(args.getCollation()); requestOut->setMulti(false); requestOut->setReturnDeleted(true); // Always return the old value. diff --git a/src/mongo/db/commands/write_commands/write_commands.cpp b/src/mongo/db/commands/write_commands/write_commands.cpp index 0b1184238ae..7930e04e482 100644 --- a/src/mongo/db/commands/write_commands/write_commands.cpp +++ b/src/mongo/db/commands/write_commands/write_commands.cpp @@ -445,6 +445,7 @@ private: deleteRequest.setCollation(write_ops::collationOf(_batch.getDeletes()[0])); deleteRequest.setMulti(_batch.getDeletes()[0].getMulti()); deleteRequest.setYieldPolicy(PlanExecutor::YIELD_AUTO); + deleteRequest.setHint(_batch.getDeletes()[0].getHint()); deleteRequest.setExplain(); ParsedDelete parsedDelete(opCtx, &deleteRequest); diff --git a/src/mongo/db/ops/delete_request.h b/src/mongo/db/ops/delete_request.h index a49e6eb37b9..4758b3486bc 100644 --- a/src/mongo/db/ops/delete_request.h +++ b/src/mongo/db/ops/delete_request.h @@ -116,6 +116,12 @@ public: bool isExplain() const { return _isExplain; } + void setHint(const BSONObj& hint) { + _hint = hint; + } + BSONObj getHint() const { + return _hint; + } bool shouldReturnDeleted() const { return _returnDeleted; } @@ -133,6 +139,7 @@ public: private: const NamespaceString& _nsString; + BSONObj _hint; BSONObj _query; BSONObj _proj; BSONObj _sort; diff --git a/src/mongo/db/ops/parsed_delete.cpp b/src/mongo/db/ops/parsed_delete.cpp index cc7039a10b9..c15e3e0d9b6 100644 --- a/src/mongo/db/ops/parsed_delete.cpp +++ b/src/mongo/db/ops/parsed_delete.cpp @@ -78,6 +78,7 @@ Status ParsedDelete::parseQueryToCQ() { qr->setSort(_request->getSort()); qr->setCollation(_request->getCollation()); qr->setExplain(_request->isExplain()); + qr->setHint(_request->getHint()); // Limit should only used for the findAndModify command when a sort is specified. If a sort // is requested, we want to use a top-k sort for efficiency reasons, so should pass the diff --git a/src/mongo/db/ops/write_ops.idl b/src/mongo/db/ops/write_ops.idl index 42f86b1f481..464b25ae9d4 100644 --- a/src/mongo/db/ops/write_ops.idl +++ b/src/mongo/db/ops/write_ops.idl @@ -150,6 +150,10 @@ structs: matching documents and 1 deletes a single document." type: multi_delete_bool cpp_name: multi + hint: + description: "Specifies the hint to use for the operation." + type: indexHint + default: mongo::BSONObj() collation: description: "Specifies the collation to use for the operation." type: object diff --git a/src/mongo/db/ops/write_ops_exec.cpp b/src/mongo/db/ops/write_ops_exec.cpp index f30b1bd979d..4c14cafac75 100644 --- a/src/mongo/db/ops/write_ops_exec.cpp +++ b/src/mongo/db/ops/write_ops_exec.cpp @@ -27,7 +27,6 @@ * it in the license file. */ -#define MONGO_LOG_DEFAULT_COMPONENT ::mongo::logger::LogComponent::kWrite #define MONGO_LOGV2_DEFAULT_COMPONENT ::mongo::logv2::LogComponent::kWrite #include "mongo/platform/basic.h" @@ -881,6 +880,7 @@ static SingleWriteResult performSingleDeleteOp(OperationContext* opCtx, request.setYieldPolicy(opCtx->inMultiDocumentTransaction() ? PlanExecutor::INTERRUPT_ONLY : PlanExecutor::YIELD_AUTO); request.setStmtId(stmtId); + request.setHint(op.getHint()); ParsedDelete parsedDelete(opCtx, &request); uassertStatusOK(parsedDelete.parseRequest()); diff --git a/src/mongo/db/query/find_and_modify_request.cpp b/src/mongo/db/query/find_and_modify_request.cpp index c86edcbde5c..703a3621a43 100644 --- a/src/mongo/db/query/find_and_modify_request.cpp +++ b/src/mongo/db/query/find_and_modify_request.cpp @@ -168,7 +168,6 @@ StatusWith<FindAndModifyRequest> FindAndModifyRequest::parseFromBSON(NamespaceSt bool isRemove = false; bool bypassDocumentValidation = false; bool arrayFiltersSet = false; - bool hintSet = false; std::vector<BSONObj> arrayFilters; boost::optional<RuntimeConstants> runtimeConstants; bool writeConcernOptionsSet = false; @@ -195,7 +194,6 @@ StatusWith<FindAndModifyRequest> FindAndModifyRequest::parseFromBSON(NamespaceSt sort = sortElement.embeddedObject(); } else if (field == kHintField) { hint = parseHint(cmdObj[kHintField]); - hintSet = true; } else if (field == kRemoveField) { isRemove = cmdObj[kRemoveField].trueValue(); } else if (field == kUpdateField) { @@ -287,10 +285,6 @@ StatusWith<FindAndModifyRequest> FindAndModifyRequest::parseFromBSON(NamespaceSt " 'remove' always returns the deleted document"}; } - if (hintSet) { - return {ErrorCodes::FailedToParse, "Cannot specify a hint with remove=true"}; - } - if (arrayFiltersSet) { return {ErrorCodes::FailedToParse, "Cannot specify arrayFilters and remove=true"}; } diff --git a/src/mongo/db/query/get_executor.cpp b/src/mongo/db/query/get_executor.cpp index cc85be2533f..829613fe814 100644 --- a/src/mongo/db/query/get_executor.cpp +++ b/src/mongo/db/query/get_executor.cpp @@ -767,37 +767,46 @@ StatusWith<unique_ptr<PlanExecutor, PlanExecutor::Deleter>> getExecutorDelete( } if (!parsedDelete->hasParsedQuery()) { - // This is the idhack fast-path for getting a PlanExecutor without doing the work to create - // a CanonicalQuery. - const BSONObj& unparsedQuery = request->getQuery(); - - const IndexDescriptor* descriptor = collection->getIndexCatalog()->findIdIndex(opCtx); - - // Construct delete request collator. - std::unique_ptr<CollatorInterface> collator; - if (!request->getCollation().isEmpty()) { - auto statusWithCollator = CollatorFactoryInterface::get(opCtx->getServiceContext()) - ->makeFromBSON(request->getCollation()); - if (!statusWithCollator.isOK()) { - return statusWithCollator.getStatus(); + + // Only consider using the idhack if no hint was provided. + if (request->getHint().isEmpty()) { + // This is the idhack fast-path for getting a PlanExecutor without doing the work to + // create a CanonicalQuery. + const BSONObj& unparsedQuery = request->getQuery(); + + const IndexDescriptor* descriptor = collection->getIndexCatalog()->findIdIndex(opCtx); + + // Construct delete request collator. + std::unique_ptr<CollatorInterface> collator; + if (!request->getCollation().isEmpty()) { + auto statusWithCollator = CollatorFactoryInterface::get(opCtx->getServiceContext()) + ->makeFromBSON(request->getCollation()); + if (!statusWithCollator.isOK()) { + return statusWithCollator.getStatus(); + } + collator = std::move(statusWithCollator.getValue()); } - collator = std::move(statusWithCollator.getValue()); - } - const bool hasCollectionDefaultCollation = request->getCollation().isEmpty() || - CollatorInterface::collatorsMatch(collator.get(), collection->getDefaultCollator()); + const bool hasCollectionDefaultCollation = request->getCollation().isEmpty() || + CollatorInterface::collatorsMatch(collator.get(), collection->getDefaultCollator()); - if (descriptor && CanonicalQuery::isSimpleIdQuery(unparsedQuery) && - request->getProj().isEmpty() && hasCollectionDefaultCollation) { - LOGV2_DEBUG(20928, - 2, - "Using idhack: {unparsedQuery}", - "unparsedQuery"_attr = redact(unparsedQuery)); - - auto idHackStage = std::make_unique<IDHackStage>( - opCtx, unparsedQuery["_id"].wrap(), ws.get(), descriptor); - unique_ptr<DeleteStage> root = std::make_unique<DeleteStage>( - opCtx, std::move(deleteStageParams), ws.get(), collection, idHackStage.release()); - return PlanExecutor::make(opCtx, std::move(ws), std::move(root), collection, policy); + if (descriptor && CanonicalQuery::isSimpleIdQuery(unparsedQuery) && + request->getProj().isEmpty() && hasCollectionDefaultCollation) { + LOGV2_DEBUG(20928, + 2, + "Using idhack: {unparsedQuery}", + "unparsedQuery"_attr = redact(unparsedQuery)); + + auto idHackStage = std::make_unique<IDHackStage>( + opCtx, unparsedQuery["_id"].wrap(), ws.get(), descriptor); + unique_ptr<DeleteStage> root = + std::make_unique<DeleteStage>(opCtx, + std::move(deleteStageParams), + ws.get(), + collection, + idHackStage.release()); + return PlanExecutor::make( + opCtx, std::move(ws), std::move(root), collection, policy); + } } // If we're here then we don't have a parsed query, but we're also not eligible for |