SERVER-48307 Disable single-write-shard transaction commit optimization

6 files changed, 113 insertions, 62 deletions

diff --git a/jstests/noPassthrough/router_transactions_metrics.js b/jstests/noPassthrough/router_transactions_metrics.js
index 4933270e2f9..37f0cb5416e 100644
--- a/jstests/noPassthrough/router_transactions_metrics.js
+++ b/jstests/noPassthrough/router_transactions_metrics.js
@@ -1,6 +1,7 @@
 // Tests multi-statement transactions metrics in the serverStatus output from mongos in various
 // basic cases.
-// @tags: [uses_transactions, uses_multi_shard_transaction]
+// TODO (SERVER-48341): Remove requires_fcv_46 after backporting SERVER-48307 to 4.4.
+// @tags: [requires_fcv_46, uses_transactions, uses_multi_shard_transaction]
 (function() {
 "use strict";
 
@@ -373,6 +374,7 @@ jsTest.log("Failed single shard transaction.");
     verifyServerStatusValues(st, expectedStats);
 })();
 
+// TODO (SERVER-48340): Re-enable the single-write-shard transaction commit optimization.
 jsTest.log("Successful single write shard transaction.");
 (() => {
     startSingleWriteShardTransaction();
@@ -382,13 +384,14 @@ jsTest.log("Successful single write shard transaction.");
     expectedStats.currentOpen -= 1;
     expectedStats.currentInactive -= 1;
     expectedStats.totalCommitted += 1;
-    expectedStats.commitTypes.singleWriteShard.initiated += 1;
-    expectedStats.commitTypes.singleWriteShard.successful += 1;
+    expectedStats.commitTypes.twoPhaseCommit.initiated += 1;
+    expectedStats.commitTypes.twoPhaseCommit.successful += 1;
     expectedStats.totalParticipantsAtCommit += 2;
-    expectedStats.totalRequestsTargeted += 2;
+    expectedStats.totalRequestsTargeted += 1;
     verifyServerStatusValues(st, expectedStats);
 })();
 
+// TODO (SERVER-48340): Re-enable the single-write-shard transaction commit optimization.
 jsTest.log("Failed single write shard transaction.");
 (() => {
     startSingleWriteShardTransaction();
@@ -401,12 +404,10 @@ jsTest.log("Failed single write shard transaction.");
     expectedStats.currentInactive -= 1;
     expectedStats.totalAborted += 1;
     expectedStats.abortCause["NoSuchTransaction"] += 1;
-    expectedStats.commitTypes.singleWriteShard.initiated += 1;
+    expectedStats.commitTypes.twoPhaseCommit.initiated += 1;
     expectedStats.totalParticipantsAtCommit += 2;
-    // In a single write shard commit, all read shards are committed first, then the
-    // write shards, so if committing on a read shard fails, the write shards aren't targeted.
-    // The implicit abort after will target all shards.
-    expectedStats.totalRequestsTargeted += 1 + 2;
+    // There are no implicit aborts after two phase commit, so the coordinator is targeted once.
+    expectedStats.totalRequestsTargeted += 1;
     verifyServerStatusValues(st, expectedStats);
 })();
 
diff --git a/jstests/sharding/create_new_collections_prepared_transactions.js b/jstests/sharding/create_new_collections_prepared_transactions.js
index 902efdad413..7bab82ea151 100644
--- a/jstests/sharding/create_new_collections_prepared_transactions.js
+++ b/jstests/sharding/create_new_collections_prepared_transactions.js
@@ -1,8 +1,10 @@
 // Test that new collection creation fails in a cross-shard write transaction, but succeeds in a
 // single-shard write transaction.
 //
+// TODO (SERVER-48341): Remove requires_fcv_46 after backporting SERVER-48307 to 4.4.
 // @tags: [
 //   requires_find_command,
+//   requires_fcv_46,
 //   requires_sharding,
 //   uses_multi_shard_transaction,
 //   uses_transactions,
@@ -53,11 +55,13 @@ assert.commandFailedWithCode(session.commitTransaction_forTesting(),
                              ErrorCodes.OperationNotSupportedInTransaction);
 
 jsTest.log("Testing collection creation in a single-shard write transaction.");
+// TODO (SERVER-48340): Re-enable the single-write-shard transaction commit optimization.
 session.startTransaction({writeConcern: {w: "majority"}});
 assert.commandWorked(sessionDBShard0.createCollection(newCollName));
 doc2 = sessionDBShard2.getCollection(collName).findOne({_id: 4});
 assert.eq(doc2._id, 4);
-assert.commandWorked(session.commitTransaction_forTesting());
+assert.commandFailedWithCode(session.commitTransaction_forTesting(),
+                             ErrorCodes.OperationNotSupportedInTransaction);
 
 st.stop();
 })();
diff --git a/jstests/sharding/txn_commit_optimizations_for_read_only_shards.js b/jstests/sharding/txn_commit_optimizations_for_read_only_shards.js
index 7891b65f1b5..e9d26794d78 100644
--- a/jstests/sharding/txn_commit_optimizations_for_read_only_shards.js
+++ b/jstests/sharding/txn_commit_optimizations_for_read_only_shards.js
@@ -6,7 +6,8 @@
  * no failures, a participant having failed over, a participant being unable to satisfy the client's
  * writeConcern, and an invalid client writeConcern.
  *
- * @tags: [uses_transactions, uses_multi_shard_transaction]
+ * TODO (SERVER-48341): Remove requires_fcv_46 after backporting SERVER-48307 to 4.4.
+ * @tags: [requires_fcv_46, uses_transactions, uses_multi_shard_transaction]
  */
 
 (function() {
@@ -184,10 +185,12 @@ const transactionTypes = {
     writeReadSingleShardExpectSingleShardCommit: txnNumber => {
         return [writeShard0(txnNumber), readShard0(txnNumber)];
     },
-    readOneShardWriteOtherShardExpectSingleWriteShardCommit: txnNumber => {
+    // TODO (SERVER-48340): Re-enable the single-write-shard transaction commit optimization.
+    readOneShardWriteOtherShardExpectTwoPhaseCommit: txnNumber => {
         return [readShard0(txnNumber), writeShard1(txnNumber)];
     },
-    writeOneShardReadOtherShardExpectSingleWriteShardCommit: txnNumber => {
+    // TODO (SERVER-48340): Re-enable the single-write-shard transaction commit optimization.
+    writeOneShardReadOtherShardExpectTwoPhaseCommit: txnNumber => {
         return [writeShard0(txnNumber), readShard1(txnNumber)];
     },
     readOneShardWriteTwoOtherShardsExpectTwoPhaseCommit: txnNumber => {
diff --git a/jstests/sharding/txn_single_write_shard_failover.js b/jstests/sharding/txn_single_write_shard_failover.js
new file mode 100644
index 00000000000..f21ba4d5f81
--- /dev/null
+++ b/jstests/sharding/txn_single_write_shard_failover.js
@@ -0,0 +1,72 @@
+/**
+ * Runs a single-write-shard transaction which commits, but for which the client retries commit and
+ * a read-only shard fails over before the second commit attempt.
+ *
+ * The second commit attempt should still return a commit decision.
+ *
+ * This test was written to reproduce the bug in the original single-write-shard transaction commit
+ * optimization where if a read-only shard failed over before the client sent a second commit
+ * attempt, the second commit attempt would return NoSuchTransaction with TransientTransactionError,
+ * causing the client to retry the whole transaction at a higher transaction number and the
+ * transaction's write to be applied twice.
+ *
+ * TODO (SERVER-48341): Remove requires_fcv_46 after backporting SERVER-48307 to 4.4.
+ * requires_find_command because legacy queries cannot be run in a session.
+ * @tags: [requires_find_command, requires_fcv_46, uses_transactions, uses_multi_shard_transaction]
+ */
+
+(function() {
+'use strict';
+
+load("jstests/libs/fail_point_util.js");
+
+const db1Name = "db1";
+const coll1Name = "foo";
+const ns1 = db1Name + "." + coll1Name;
+
+const db2Name = "db2";
+const coll2Name = "bar";
+const ns2 = db2Name + "." + coll2Name;
+
+const st = new ShardingTest({
+    shards: {rs0: {nodes: 2}, rs1: {nodes: 1}},
+    config: 1,
+    other: {
+        mongosOptions: {verbose: 3},
+    }
+});
+
+jsTest.log("Create two databases on different primary shards.");
+// enableSharding creates the databases.
+assert.commandWorked(st.s.adminCommand({enableSharding: db1Name}));
+assert.commandWorked(st.s.adminCommand({enableSharding: db2Name}));
+assert.commandWorked(st.s.adminCommand({movePrimary: db1Name, to: st.shard0.shardName}));
+assert.commandWorked(st.s.adminCommand({movePrimary: db2Name, to: st.shard1.shardName}));
+
+jsTest.log("Insert data on both shards.");
+// This ensures all nodes refresh their routing caches.
+st.s.getDB(db1Name).getCollection(coll1Name).insert({_id: "dummy"});
+st.s.getDB(db2Name).getCollection(coll2Name).insert({_id: "dummy"});
+
+jsTest.log("Run a single-write-shard transaction and commit it.");
+const session = st.s.startSession();
+session.startTransaction();
+session.getDatabase(db1Name).getCollection(coll1Name).findOne({_id: "readOperationOnShard0"});
+session.getDatabase(db2Name).getCollection(coll2Name).insert({_id: "writeOperationOnShard1"});
+// Use adminCommand so we can pass writeConcern.
+assert.commandWorked(st.s.adminCommand({
+    commitTransaction: 1,
+    lsid: session.getSessionId(),
+    txnNumber: session.getTxnNumber_forTesting(),
+    autocommit: false,
+    writeConcern: {w: "majority"},
+}));
+
+jsTest.log("Induce a failover on the read shard.");
+assert.commandWorked(st.rs0.getPrimary().adminCommand({replSetStepDown: 60, force: true}));
+
+jsTest.log("Make second attempt to commit, should still return that the transaction committed");
+assert.commandWorked(session.commitTransaction_forTesting());
+
+st.stop();
+})();
diff --git a/src/mongo/s/transaction_router.cpp b/src/mongo/s/transaction_router.cpp
index 187ff3282e4..b810e79c752 100644
--- a/src/mongo/s/transaction_router.cpp
+++ b/src/mongo/s/transaction_router.cpp
@@ -1151,31 +1151,6 @@ BSONObj TransactionRouter::Router::_commitTransaction(
         return sendCommitDirectlyToShards(opCtx, readOnlyShards);
     }
 
-    if (writeShards.size() == 1) {
-        LOGV2_DEBUG(22894,
-                    3,
-                    "{sessionId}:{txnNumber} Committing single-write-shard transaction with "
-                    "{numReadOnlyShards} read-only shards, write shard: {writeShardId}",
-                    "Committing single-write-shard transaction",
-                    "sessionId"_attr = _sessionId().getId(),
-                    "txnNumber"_attr = o().txnNumber,
-                    "numReadOnlyShards"_attr = readOnlyShards.size(),
-                    "writeShardId"_attr = writeShards.front());
-        {
-            stdx::lock_guard<Client> lk(*opCtx->getClient());
-            o(lk).commitType = CommitType::kSingleWriteShard;
-            _onStartCommit(lk, opCtx);
-        }
-
-        const auto readOnlyShardsResponse = sendCommitDirectlyToShards(opCtx, readOnlyShards);
-
-        if (!getStatusFromCommandResult(readOnlyShardsResponse).isOK() ||
-            !getWriteConcernStatusFromCommandResult(readOnlyShardsResponse).isOK()) {
-            return readOnlyShardsResponse;
-        }
-        return sendCommitDirectlyToShards(opCtx, writeShards);
-    }
-
     {
         stdx::lock_guard<Client> lk(*opCtx->getClient());
         o(lk).commitType = CommitType::kTwoPhaseCommit;
diff --git a/src/mongo/s/transaction_router_test.cpp b/src/mongo/s/transaction_router_test.cpp
index d2fbabf43cf..d7ef7a1f019 100644
--- a/src/mongo/s/transaction_router_test.cpp
+++ b/src/mongo/s/transaction_router_test.cpp
@@ -1012,8 +1012,9 @@ TEST_F(TransactionRouterTestWithDefaultSession,
     ASSERT(expectedHostAndPorts == seenHostAndPorts);
 }
 
+// TODO (SERVER-48340): Re-enable the single-write-shard transaction commit optimization.
 TEST_F(TransactionRouterTestWithDefaultSession,
-       SendCommitDirectlyToReadOnlyShardsThenWriteShardForMultipleParticipantsOnlyOneDidAWrite) {
+       SendCoordinateCommitForMultipleParticipantsOnlyOneDidAWrite) {
     TxnNumber txnNum{3};
 
     auto txnRouter = TransactionRouter::get(operationContext());
@@ -1040,26 +1041,23 @@ TEST_F(TransactionRouterTestWithDefaultSession,
         ASSERT_EQ("admin", request.dbname);
 
         auto cmdName = request.cmdObj.firstElement().fieldNameStringData();
-        ASSERT_EQ(cmdName, "commitTransaction");
-
-        checkSessionDetails(request.cmdObj, getSessionId(), txnNum, true);
-
-        return BSON("ok" << 1);
-    });
+        ASSERT_EQ(cmdName, "coordinateCommitTransaction");
 
-    onCommand([&](const RemoteCommandRequest& request) {
-        ASSERT_EQ(hostAndPort2, request.target);
-        ASSERT_EQ("admin", request.dbname);
+        std::set<std::string> expectedParticipants = {shard1.toString(), shard2.toString()};
+        auto participantElements = request.cmdObj["participants"].Array();
+        ASSERT_EQ(expectedParticipants.size(), participantElements.size());
 
-        auto cmdName = request.cmdObj.firstElement().fieldNameStringData();
-        ASSERT_EQ(cmdName, "commitTransaction");
+        for (const auto& element : participantElements) {
+            auto shardId = element["shardId"].valuestr();
+            ASSERT_EQ(1ull, expectedParticipants.count(shardId));
+            expectedParticipants.erase(shardId);
+        }
 
-        checkSessionDetails(request.cmdObj, getSessionId(), txnNum, false);
+        checkSessionDetails(request.cmdObj, getSessionId(), txnNum, true);
 
         return BSON("ok" << 1);
     });
 
-
     future.default_timed_get();
 }
 
@@ -3028,8 +3026,7 @@ protected:
         startCapturingLogMessages();
         auto future = launchAsync(
             [&] { txnRouter().commitTransaction(operationContext(), kDummyRecoveryToken); });
-        expectCommitTransaction();
-        expectCommitTransaction();
+        expectCoordinateCommitTransaction();
         future.default_timed_get();
         stopCapturingLogMessages();
     }
@@ -3359,18 +3356,17 @@ TEST_F(TransactionRouterMetricsTest, SlowLoggingCommitType_SingleShard) {
     ASSERT_EQUALS(0, countTextFormatLogLinesContaining("coordinator:"));
 }
 
+// TODO (SERVER-48340): Re-enable the single-write-shard transaction commit optimization.
 TEST_F(TransactionRouterMetricsTest, SlowLoggingCommitType_SingleWriteShard) {
     beginSlowTxnWithDefaultTxnNumber();
     runSingleWriteShardCommit();
 
-    ASSERT_EQUALS(
-        1,
-        countBSONFormatLogLinesIsSubset(BSON(
-            "attr" << BSON("commitType"
-                           << "singleWriteShard"
-                           << "numParticipants" << 2 << "commitDurationMicros" << BSONUndefined))));
-
-    ASSERT_EQUALS(0, countTextFormatLogLinesContaining("coordinator:"));
+    ASSERT_EQUALS(1,
+                  countBSONFormatLogLinesIsSubset(
+                      BSON("attr" << BSON("commitType"
+                                          << "twoPhaseCommit"
+                                          << "numParticipants" << 2 << "commitDurationMicros"
+                                          << BSONUndefined << "coordinator" << BSONUndefined))));
 }
 
 TEST_F(TransactionRouterMetricsTest, SlowLoggingCommitType_ReadOnly) {