summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--jstests/sharding/update_shard_key_conflicting_writes.js102
-rw-r--r--src/mongo/s/commands/cluster_find_and_modify_cmd.cpp2
-rw-r--r--src/mongo/s/commands/cluster_write_cmd.cpp10
-rw-r--r--src/mongo/s/commands/document_shard_key_update_util.cpp3
-rw-r--r--src/mongo/s/would_change_owning_shard_exception.cpp11
-rw-r--r--src/mongo/s/would_change_owning_shard_exception.h10
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>;
View Lorry Controller Status