diff options
-rw-r--r-- | jstests/noPassthrough/mirror_reads.js | 4 | ||||
-rw-r--r-- | src/mongo/db/commands/SConscript | 30 | ||||
-rw-r--r-- | src/mongo/db/commands/command_mirroring_test.cpp | 22 | ||||
-rw-r--r-- | src/mongo/db/commands/write_commands/write_commands.cpp | 62 |
4 files changed, 68 insertions, 50 deletions
diff --git a/jstests/noPassthrough/mirror_reads.js b/jstests/noPassthrough/mirror_reads.js index 3f897912dab..1a31a775c55 100644 --- a/jstests/noPassthrough/mirror_reads.js +++ b/jstests/noPassthrough/mirror_reads.js @@ -147,6 +147,10 @@ function verifyMirrorReads(rst, cmd) { jsTestLog("Verifying mirrored reads for 'findAndModify' commands"); verifyMirrorReads(rst, {findAndModify: kCollName, query: {}, update: {'$inc': {x: 1}}}); + jsTestLog("Verifying mirrored reads for 'update' commands"); + verifyMirrorReads( + rst, {update: kCollName, updates: [{q: {_id: 1}, u: {'$inc': {x: 1}}}], ordered: false}); + rst.stopSet(); } diff --git a/src/mongo/db/commands/SConscript b/src/mongo/db/commands/SConscript index 3673aa5db9b..1d4d971be89 100644 --- a/src/mongo/db/commands/SConscript +++ b/src/mongo/db/commands/SConscript @@ -572,22 +572,20 @@ env.CppUnitTest( ], ) -# TODO SERVER-46533 Reenable this test when update can participate in read mirroring again -if False: - env.CppUnitTest( - target="command_mirroring_test", - source=[ - 'command_mirroring_test.cpp', - ], - LIBDEPS=[ - '$BUILD_DIR/mongo/base', - '$BUILD_DIR/mongo/db/auth/authorization_manager_global', - '$BUILD_DIR/mongo/db/auth/authmocks', - '$BUILD_DIR/mongo/db/commands/standalone', - '$BUILD_DIR/mongo/db/service_context', - '$BUILD_DIR/mongo/unittest/unittest', - ], - ) +env.CppUnitTest( + target="command_mirroring_test", + source=[ + 'command_mirroring_test.cpp', + ], + LIBDEPS=[ + '$BUILD_DIR/mongo/base', + '$BUILD_DIR/mongo/db/auth/authorization_manager_global', + '$BUILD_DIR/mongo/db/auth/authmocks', + '$BUILD_DIR/mongo/db/commands/standalone', + '$BUILD_DIR/mongo/db/service_context', + '$BUILD_DIR/mongo/unittest/unittest', + ], +) env.CppUnitTest( target="db_commands_test", diff --git a/src/mongo/db/commands/command_mirroring_test.cpp b/src/mongo/db/commands/command_mirroring_test.cpp index e9677a77b9d..3f7f2753a5b 100644 --- a/src/mongo/db/commands/command_mirroring_test.cpp +++ b/src/mongo/db/commands/command_mirroring_test.cpp @@ -54,14 +54,13 @@ public: client.reset(nullptr); } - virtual BSONObj makeCommand(std::string, std::vector<BSONObj>) = 0; + virtual OpMsgRequest makeCommand(std::string, std::vector<BSONObj>) = 0; const LogicalSessionId& getLogicalSessionId() const { return _lsid; } - BSONObj getMirroredCommand(BSONObj& bson) { - auto request = OpMsgRequest::fromDBAndBody(kDB, bson); + BSONObj getMirroredCommand(OpMsgRequest& request) { auto cmd = globalCommandRegistry()->findCommand(request.getCommandName()); ASSERT(cmd); @@ -84,18 +83,23 @@ private: class UpdateCommandTest : public CommandMirroringTest { public: - BSONObj makeCommand(std::string coll, std::vector<BSONObj> updates) override { + OpMsgRequest makeCommand(std::string coll, std::vector<BSONObj> updates) override { BSONObjBuilder bob; bob << "update" << coll; - BSONArrayBuilder bab; + bob << "lsid" << getLogicalSessionId().toBSON(); + + auto request = OpMsgRequest::fromDBAndBody(kDB, bob.obj()); + + // Directly add `updates` to `OpMsg::sequences` to emulate `OpMsg::parse()` behavior. + OpMsg::DocumentSequence seq; + seq.name = "updates"; for (auto update : updates) { - bab << update; + seq.objs.emplace_back(std::move(update)); } - bob << "updates" << bab.arr(); - bob << "lsid" << getLogicalSessionId().toBSON(); + request.sequences.emplace_back(std::move(seq)); - return bob.obj(); + return request; } }; diff --git a/src/mongo/db/commands/write_commands/write_commands.cpp b/src/mongo/db/commands/write_commands/write_commands.cpp index 93d0b8650e0..8241c5e3de6 100644 --- a/src/mongo/db/commands/write_commands/write_commands.cpp +++ b/src/mongo/db/commands/write_commands/write_commands.cpp @@ -334,40 +334,47 @@ private: UpdateMetrics* updateMetrics) : InvocationBase(cmd, request), _batch(UpdateOp::parse(request)), - _updateMetrics{updateMetrics} { + _updateMetrics{updateMetrics}, + _commandObj(request.body) { invariant(_updateMetrics); + invariant(_commandObj.isOwned()); + + // Extend the lifetime of `updates` to allow asynchronous mirroring. + if (auto seq = request.getSequence("updates"_sd); seq && !seq->objs.empty()) { + // Current design ignores contents of `updates` array except for the first entry. + // Assuming identical collation for all elements in `updates`, future design could + // use the disjunction primitive (i.e, `$or`) to compile all queries into a single + // filter. Such a design also requires a sound way of combining hints. + invariant(seq->objs.front().isOwned()); + _updateOpObj = seq->objs.front(); + } } bool supportsReadMirroring() const override { - // TODO SERVER-46533 Remove this and uncomment below when update can be safely mirrored. - // Disable readMIrrroing for update - return false; - - // This would translate to a find command with no filter! - // Based on the documentation for `update`, this vector should never be empty. - // return !_batch.getUpdates().empty(); + return true; } void appendMirrorableRequest(BSONObjBuilder* bob) const override { - auto extractQueryDetails = [](const write_ops::Update& query, - BSONObjBuilder* bob) -> void { - auto updates = query.getUpdates(); - // `supportsReadMirroring()` is responsible for this validation. - invariant(!updates.empty()); - - // Current design ignores contents of `updates` array except for the first entry. - // Assuming identical collation for all elements in `updates`, future design could - // use the disjunction primitive (i.e, `$or`) to compile all queries into a single - // filter. Such a design also requires a sound way of combining hints. - bob->append("filter", updates.front().getQ()); - if (!updates.front().getHint().isEmpty()) - bob->append("hint", updates.front().getHint()); - if (updates.front().getCollation()) - bob->append("collation", *updates.front().getCollation()); + auto extractQueryDetails = [](const BSONObj& update, BSONObjBuilder* bob) -> void { + // "filter", "hint", and "collation" fields are optional. + if (update.isEmpty()) + return; + + // The constructor verifies the following. + invariant(update.isOwned()); + + if (update.hasField("q")) + bob->append("filter", update["q"].Obj()); + if (update.hasField("hint") && !update["hint"].Obj().isEmpty()) + bob->append("hint", update["hint"].Obj()); + if (update.hasField("collation") && !update["collation"].Obj().isEmpty()) + bob->append("collation", update["collation"].Obj()); }; - bob->append("find", _batch.getNamespace().coll()); - extractQueryDetails(_batch, bob); + invariant(!_commandObj.isEmpty()); + + bob->append("find", _commandObj["update"].String()); + extractQueryDetails(_updateOpObj, bob); bob->append("batchSize", 1); bob->append("singleBatch", true); } @@ -452,6 +459,11 @@ private: // Update related command execution metrics. UpdateMetrics* const _updateMetrics; + + BSONObj _commandObj; + + // Holds a shared pointer to the first entry in `updates` array. + BSONObj _updateOpObj; }; std::unique_ptr<CommandInvocation> parse(OperationContext*, const OpMsgRequest& request) { |