diff options
author | jannaerin <golden.janna@gmail.com> | 2019-04-18 11:40:56 -0400 |
---|---|---|
committer | jannaerin <golden.janna@gmail.com> | 2019-04-18 15:02:16 -0400 |
commit | 724615ae608b123bb2e82dc0731f8e61fab17a82 (patch) | |
tree | 88a80f902bb4dca2b6095f83db7a8f63bb5593e9 | |
parent | 5ad20825e2c02b14f1aeb5bfb4dcc0b4dce927b2 (diff) | |
download | mongo-724615ae608b123bb2e82dc0731f8e61fab17a82.tar.gz |
SERVER-40655 Test that a retryable write that changes the shard key behaves correctly with conflicting writes
-rw-r--r-- | jstests/sharding/update_shard_key_conflicting_writes.js | 102 | ||||
-rw-r--r-- | src/mongo/s/commands/cluster_find_and_modify_cmd.cpp | 2 | ||||
-rw-r--r-- | src/mongo/s/commands/cluster_write_cmd.cpp | 10 | ||||
-rw-r--r-- | src/mongo/s/commands/document_shard_key_update_util.cpp | 3 | ||||
-rw-r--r-- | src/mongo/s/would_change_owning_shard_exception.cpp | 11 | ||||
-rw-r--r-- | src/mongo/s/would_change_owning_shard_exception.h | 10 |
6 files changed, 119 insertions, 19 deletions
diff --git a/jstests/sharding/update_shard_key_conflicting_writes.js b/jstests/sharding/update_shard_key_conflicting_writes.js index f0d6942d1c0..3fc3fb9f416 100644 --- a/jstests/sharding/update_shard_key_conflicting_writes.js +++ b/jstests/sharding/update_shard_key_conflicting_writes.js @@ -341,6 +341,108 @@ assert.eq(0, db.foo.find({"x": -500}).itcount()); })(); + /** + * Test scenarios where a user sends an update as a retryable write that changes the shard key + * and there is a concurrent update/delete that mutates the same document which completes after + * the change to the shard key throws WouldChangeOwningShard the first time, but before mongos + * starts a transaction to change the shard key. + * + * The scenario looks like: + * 1. user sends db.foo.update({shardKey: x}, {shardKey: new x}) + * 2. shard throws WCOS for this update + * 3. user sends db.foo.update({shardKey: x}, {otherFieldInDoc: y}) on a different thread, this + * write completes successfully + * 4. mongos starts a transaction and resends the update on line 1 + * 5. mongos deletes the old doc, inserts a doc with the updated shard key, and commits the txn + */ + + // Assert that if the concurrent update modifies the document so that the update which changes + // the shard key no longer matches the doc, it does not modify the doc. + (() => { + let codeToRunInParallelShell = `{ + let session = db.getMongo().startSession({retryWrites : true}); + let sessionDB = session.getDatabase("db"); + let res = sessionDB.foo.update({"x": -150, "a" : 15}, {$set: {"x": 1000}}); + assert.commandWorked(res); + assert.eq(0, res.nMatched); + assert.eq(0, res.nModified); + }`; + let awaitShell = setFailPointAndSendUpdateToShardKeyInParallelShell( + "hangAfterThrowWouldChangeOwningShardRetryableWrite", + "alwaysOn", + st.s, + codeToRunInParallelShell); + // Send update that changes "a" so that the original update will no longer match this doc. + // Turn off the failpoint so the server stops hanging. + assert.commandWorked(sessionDB2.foo.update({"x": -150}, {$set: {"a": 3000}})); + assert.commandWorked(st.s.adminCommand({ + configureFailPoint: "hangAfterThrowWouldChangeOwningShardRetryableWrite", + mode: "off", + })); + awaitShell(); + assert.eq(1, db.foo.find({"x": -150, "a": 3000}).itcount()); + assert.eq(0, db.foo.find({"a": 15}).itcount()); + assert.eq(0, db.foo.find({"x": 1000}).itcount()); + })(); + + // Assert that if the concurrent update modifies the document and the update which changes the + // shard key still matches the doc, the final document reflects both updates. + (() => { + let codeToRunInParallelShell = `{ + let session = db.getMongo().startSession({retryWrites : true}); + let sessionDB = session.getDatabase("db"); + let res = sessionDB.foo.update({"x": 150}, {$set: {"x": -1000}}); + assert.commandWorked(res); + assert.eq(1, res.nMatched); + assert.eq(1, res.nModified); + }`; + let awaitShell = setFailPointAndSendUpdateToShardKeyInParallelShell( + "hangAfterThrowWouldChangeOwningShardRetryableWrite", + "alwaysOn", + st.s, + codeToRunInParallelShell); + // Send update that changes "a". The original update will still match this doc because it + // queries only on the shard key. Turn off the failpoint so the server stops hanging. + assert.commandWorked(sessionDB2.foo.update({"x": 150}, {$set: {"a": -200}})); + assert.commandWorked(st.s.adminCommand({ + configureFailPoint: "hangAfterThrowWouldChangeOwningShardRetryableWrite", + mode: "off", + })); + awaitShell(); + assert.eq(1, db.foo.find({"x": -1000, "a": -200}).itcount()); + assert.eq(0, db.foo.find({"a": 20}).itcount()); + assert.eq(0, db.foo.find({"x": 150}).itcount()); + })(); + + // Assert that if a concurrent delete removes the same document that the original update + // attempts to modify the shard key for, we don't match any docs. + (() => { + let codeToRunInParallelShell = `{ + let session = db.getMongo().startSession({retryWrites : true}); + let sessionDB = session.getDatabase("db"); + let res = sessionDB.foo.update({"x": -150}, {$set: {"x": 1000}}); + assert.commandWorked(res); + assert.eq(0, res.nMatched); + assert.eq(0, res.nModified); + }`; + let awaitShell = setFailPointAndSendUpdateToShardKeyInParallelShell( + "hangAfterThrowWouldChangeOwningShardRetryableWrite", + "alwaysOn", + st.s, + codeToRunInParallelShell); + // Remove this doc so that the original update will no longer match any doc. + // Turn off the failpoint so the server stops hanging. + assert.commandWorked(sessionDB2.foo.remove({"x": -150})); + assert.commandWorked(st.s.adminCommand({ + configureFailPoint: "hangAfterThrowWouldChangeOwningShardRetryableWrite", + mode: "off", + })); + awaitShell(); + assert.eq(0, db.foo.find({"x": -150}).itcount()); + assert.eq(0, db.foo.find({"a": 3000}).itcount()); + assert.eq(0, db.foo.find({"x": 1000}).itcount()); + })(); + st.stop(); }()); diff --git a/src/mongo/s/commands/cluster_find_and_modify_cmd.cpp b/src/mongo/s/commands/cluster_find_and_modify_cmd.cpp index 1b11eb0d0aa..1cc1c01089e 100644 --- a/src/mongo/s/commands/cluster_find_and_modify_cmd.cpp +++ b/src/mongo/s/commands/cluster_find_and_modify_cmd.cpp @@ -110,7 +110,7 @@ void updateShardKeyValueOnWouldChangeOwningShardError(OperationContext* opCtx, if (matchedDoc) { result->append("value", cmdObj.getBoolField("new") - ? wouldChangeOwningShardExtraInfo.getPostImage().get() + ? wouldChangeOwningShardExtraInfo.getPostImage() : wouldChangeOwningShardExtraInfo.getPreImage()); } else { result->appendNull("value"); diff --git a/src/mongo/s/commands/cluster_write_cmd.cpp b/src/mongo/s/commands/cluster_write_cmd.cpp index a8b0d6db2c3..cfc69ba09cb 100644 --- a/src/mongo/s/commands/cluster_write_cmd.cpp +++ b/src/mongo/s/commands/cluster_write_cmd.cpp @@ -61,6 +61,8 @@ namespace mongo { namespace { +MONGO_FAIL_POINT_DEFINE(hangAfterThrowWouldChangeOwningShardRetryableWrite); + void batchErrorToLastError(const BatchedCommandRequest& request, const BatchedCommandResponse& response, LastError* error) { @@ -226,6 +228,11 @@ bool handleWouldChangeOwningShardError(OperationContext* opCtx, return false; if (isRetryableWrite) { + if (MONGO_FAIL_POINT(hangAfterThrowWouldChangeOwningShardRetryableWrite)) { + log() << "Hit hangAfterThrowWouldChangeOwningShardRetryableWrite failpoint"; + MONGO_FAIL_POINT_PAUSE_WHILE_SET_OR_INTERRUPTED( + opCtx, hangAfterThrowWouldChangeOwningShardRetryableWrite); + } RouterOperationContextSession routerSession(opCtx); try { // Start transaction and re-run the original update command @@ -234,7 +241,8 @@ bool handleWouldChangeOwningShardError(OperationContext* opCtx, auto txnRouterForShardKeyChange = documentShardKeyUpdateUtil::startTransactionForShardKeyUpdate(opCtx); - + // Clear the error details from the response object before sending the write again + response->unsetErrDetails(); ClusterWriter::write(opCtx, request, &stats, response); wouldChangeOwningShardErrorInfo = getWouldChangeOwningShardErrorInfo(opCtx, request, response, !isRetryableWrite); diff --git a/src/mongo/s/commands/document_shard_key_update_util.cpp b/src/mongo/s/commands/document_shard_key_update_util.cpp index 0222da03d00..706c54a1c18 100644 --- a/src/mongo/s/commands/document_shard_key_update_util.cpp +++ b/src/mongo/s/commands/document_shard_key_update_util.cpp @@ -122,8 +122,7 @@ bool updateShardKeyForDocument(OperationContext* opCtx, const WouldChangeOwningShardInfo& documentKeyChangeInfo, int stmtId) { auto updatePreImage = documentKeyChangeInfo.getPreImage().getOwned(); - invariant(documentKeyChangeInfo.getPostImage()); - auto updatePostImage = documentKeyChangeInfo.getPostImage()->getOwned(); + auto updatePostImage = documentKeyChangeInfo.getPostImage().getOwned(); auto deleteCmdObj = constructShardKeyDeleteCmdObj(nss, updatePreImage, stmtId); auto insertCmdObj = constructShardKeyInsertCmdObj(nss, updatePostImage, stmtId); diff --git a/src/mongo/s/would_change_owning_shard_exception.cpp b/src/mongo/s/would_change_owning_shard_exception.cpp index 9563c586431..e2d4eed46de 100644 --- a/src/mongo/s/would_change_owning_shard_exception.cpp +++ b/src/mongo/s/would_change_owning_shard_exception.cpp @@ -46,8 +46,7 @@ constexpr StringData kPostImage = "postImage"_sd; void WouldChangeOwningShardInfo::serialize(BSONObjBuilder* bob) const { bob->append(kPreImage, _preImage); - if (_postImage) - bob->append(kPostImage, _postImage.get()); + bob->append(kPostImage, _postImage); } std::shared_ptr<const ErrorExtraInfo> WouldChangeOwningShardInfo::parse(const BSONObj& obj) { @@ -55,12 +54,8 @@ std::shared_ptr<const ErrorExtraInfo> WouldChangeOwningShardInfo::parse(const BS } WouldChangeOwningShardInfo WouldChangeOwningShardInfo::parseFromCommandError(const BSONObj& obj) { - boost::optional<BSONObj> originalUpdate = boost::none; - boost::optional<BSONObj> postImage = boost::none; - if (obj[kPostImage]) - postImage = obj[kPostImage].Obj().getOwned(); - - return WouldChangeOwningShardInfo(obj[kPreImage].Obj().getOwned(), postImage); + return WouldChangeOwningShardInfo(obj[kPreImage].Obj().getOwned(), + obj[kPostImage].Obj().getOwned()); } } // namespace mongo diff --git a/src/mongo/s/would_change_owning_shard_exception.h b/src/mongo/s/would_change_owning_shard_exception.h index db48f95792e..c50e9a53d48 100644 --- a/src/mongo/s/would_change_owning_shard_exception.h +++ b/src/mongo/s/would_change_owning_shard_exception.h @@ -46,12 +46,8 @@ class WouldChangeOwningShardInfo final : public ErrorExtraInfo { public: static constexpr auto code = ErrorCodes::WouldChangeOwningShard; - explicit WouldChangeOwningShardInfo(const BSONObj& preImage, - const boost::optional<BSONObj>& postImage) - : _preImage(preImage.getOwned()) { - if (postImage) - _postImage = postImage->getOwned(); - } + explicit WouldChangeOwningShardInfo(const BSONObj& preImage, const BSONObj& postImage) + : _preImage(preImage.getOwned()), _postImage(postImage.getOwned()) {} const auto& getPreImage() const { return _preImage; @@ -76,7 +72,7 @@ private: BSONObj _preImage; // The post image returned by the update stage - boost::optional<BSONObj> _postImage; + BSONObj _postImage; }; using WouldChangeOwningShardException = ExceptionFor<ErrorCodes::WouldChangeOwningShard>; |