From 95626d0eca8325488d354221b1ec5b1019743fa1 Mon Sep 17 00:00:00 2001 From: Ali Mir Date: Wed, 1 Apr 2020 16:58:26 -0400 Subject: SERVER-43889 Distinguish between retryable write and transaction when failing a command (cherry picked from commit eb284b042c71edf0eac445d3ceb79f7fdeabc5d1) --- jstests/sharding/retryable_writes.js | 25 ++++++++++++ src/mongo/db/session.cpp | 42 ++++++++++++++----- src/mongo/db/session.h | 7 ++-- src/mongo/db/session_test.cpp | 78 ++++++++++++++++++++++++++++++++++++ 4 files changed, 139 insertions(+), 13 deletions(-) diff --git a/jstests/sharding/retryable_writes.js b/jstests/sharding/retryable_writes.js index cfcef0d0eed..8133a8c9011 100644 --- a/jstests/sharding/retryable_writes.js +++ b/jstests/sharding/retryable_writes.js @@ -508,6 +508,30 @@ assert.commandFailed(localDB.runCommand(cmd)); } + function runRetryableWriteErrorTest(mainConn) { + // Test TransactionTooOld error message on retryable writes + + const lsid = UUID(); + const testDb = mainConn.getDB('TestDB'); + assert.commandWorked(testDb.runCommand({ + insert: 'user', + documents: [{x: 1}], + ordered: true, + lsid: {id: lsid}, + txnNumber: NumberLong(2) + })); + const writeResult = testDb.runCommand({ + update: 'user', + updates: [{q: {x: 1}, u: {$inc: {x: 1}}}], + ordered: true, + lsid: {id: lsid}, + txnNumber: NumberLong(1) + }); + assert.commandFailedWithCode(writeResult, ErrorCodes.TransactionTooOld); + assert(writeResult.errmsg.includes("Retryable write with txnNumber 1 is prohibited"), + writeResult); + } + // Tests for replica set var replTest = new ReplSetTest({nodes: 2}); replTest.startSet({verbose: 5}); @@ -519,6 +543,7 @@ runFailpointTests(priConn, priConn); runMultiTests(priConn); runInvalidTests(priConn); + runRetryableWriteErrorTest(priConn); replTest.stopSet(); diff --git a/src/mongo/db/session.cpp b/src/mongo/db/session.cpp index c94ea1bfba0..edcc5be1294 100644 --- a/src/mongo/db/session.cpp +++ b/src/mongo/db/session.cpp @@ -504,7 +504,7 @@ void Session::_beginOrContinueTxn(WithLock wl, // Check if the given transaction number is valid for this session. The transaction number must // be >= the active transaction number. - _checkTxnValid(wl, txnNumber); + _checkTxnValid(wl, txnNumber, autocommit); // // Continue an active transaction. @@ -610,14 +610,33 @@ void Session::_beginOrContinueTxn(WithLock wl, invariant(_transactionOperations.empty()); } -void Session::_checkTxnValid(WithLock, TxnNumber txnNumber) const { - uassert(ErrorCodes::TransactionTooOld, - str::stream() << "Cannot start transaction " << txnNumber << " on session " - << getSessionId() - << " because a newer transaction " - << _activeTxnNumber - << " has already started.", - txnNumber >= _activeTxnNumber); +void Session::_checkTxnValid(WithLock, + TxnNumber txnNumber, + boost::optional autocommit) const { + if (txnNumber < _activeTxnNumber) { + const std::string currOperation = + _txnState == MultiDocumentTransactionState::kNone ? "retryable write" : "transaction"; + if (!autocommit) { + uasserted(ErrorCodes::TransactionTooOld, + str::stream() << "Retryable write with txnNumber " << txnNumber + << " is prohibited on session " + << _sessionId + << " because a newer " + << currOperation + << " with txnNumber " + << _activeTxnNumber + << " has already started on this session."); + } else { + uasserted(ErrorCodes::TransactionTooOld, + str::stream() << "Cannot start transaction " << txnNumber << " on session " + << _sessionId + << " because a newer " + << currOperation + << " with txnNumber " + << _activeTxnNumber + << " has already started on this session."); + } + } } Session::TxnResources::TxnResources(OperationContext* opCtx, bool keepTicket) { @@ -994,7 +1013,10 @@ void Session::_abortTransaction(WithLock wl) { void Session::_beginOrContinueTxnOnMigration(WithLock wl, TxnNumber txnNumber) { _checkValid(wl); - _checkTxnValid(wl, txnNumber); + // The value for 'autocommit' is only used to + // generate the uassert error message. In this case, the exception will never be + // propagated to the user. + _checkTxnValid(wl, txnNumber, boost::optional()); // Check for continuing an existing transaction if (txnNumber == _activeTxnNumber) diff --git a/src/mongo/db/session.h b/src/mongo/db/session.h index 01d94721537..df9bceeae2e 100644 --- a/src/mongo/db/session.h +++ b/src/mongo/db/session.h @@ -421,9 +421,10 @@ private: // Checks if there is a conflicting operation on the current Session void _checkValid(WithLock) const; - // Checks that a new txnNumber is higher than the activeTxnNumber so - // we don't start a txn that is too old. - void _checkTxnValid(WithLock, TxnNumber txnNumber) const; + // Checks that a new txnNumber is higher than the activeTxnNumber so we don't start a + // transaction or retryable write that is older + // than the current one. + void _checkTxnValid(WithLock, TxnNumber txnNumber, boost::optional autocommit) const; void _setActiveTxn(WithLock, TxnNumber txnNumber); diff --git a/src/mongo/db/session_test.cpp b/src/mongo/db/session_test.cpp index e16f0a74663..fcd5ad7b9dc 100644 --- a/src/mongo/db/session_test.cpp +++ b/src/mongo/db/session_test.cpp @@ -921,6 +921,84 @@ TEST_F(SessionTest, CannotSpecifyStartTransactionOnInProgressTxn) { ErrorCodes::ConflictingOperationInProgress); } +TEST_F(SessionTest, OlderTransactionFailsOnSessionWithNewerTransaction) { + // Will start the transaction. + const auto sessionId = makeLogicalSessionIdForTest(); + Session session(sessionId); + session.refreshFromStorageIfNeeded(opCtx()); + + const bool autocommit = false; + const bool startTransaction = true; + const TxnNumber txnNum = 20; + session.beginOrContinueTxn(opCtx(), txnNum, autocommit, startTransaction, "testDB", "insert"); + + StringBuilder sb; + sb << "Cannot start transaction 19 on session " << sessionId + << " because a newer transaction with txnNumber 20 has already started on this session."; + ASSERT_THROWS_WHAT(session.beginOrContinueTxn( + opCtx(), txnNum - 1, autocommit, startTransaction, "testDB", "insert"), + AssertionException, + sb.str()); + ASSERT(session.getLastWriteOpTime(txnNum).isNull()); +} + +TEST_F(SessionTest, OldRetryableWriteFailsOnSessionWithNewerTransaction) { + // Will start the transaction. + const auto sessionId = makeLogicalSessionIdForTest(); + Session session(sessionId); + session.refreshFromStorageIfNeeded(opCtx()); + + const bool autocommit = false; + const bool startTransaction = true; + const TxnNumber txnNum = 20; + session.beginOrContinueTxn(opCtx(), txnNum, autocommit, startTransaction, "testDB", "insert"); + + StringBuilder sb; + sb << "Retryable write with txnNumber 19 is prohibited on session " << sessionId + << " because a newer transaction with txnNumber 20 has already started on this session."; + ASSERT_THROWS_WHAT(session.beginOrContinueTxn( + opCtx(), txnNum - 1, boost::none, boost::none, "testDB", "insert"), + AssertionException, + sb.str()); + ASSERT(session.getLastWriteOpTime(txnNum).isNull()); +} + +TEST_F(SessionTest, OlderRetryableWriteFailsOnSessionWithNewerRetryableWrite) { + const auto sessionId = makeLogicalSessionIdForTest(); + Session session(sessionId); + session.refreshFromStorageIfNeeded(opCtx()); + const TxnNumber txnNum = 22; + + StringBuilder sb; + sb << "Retryable write with txnNumber 21 is prohibited on session " << sessionId + << " because a newer retryable write with txnNumber 22 has already started on this session."; + session.beginOrContinueTxn(opCtx(), txnNum, boost::none, boost::none, "testDB", "insert"); + ASSERT_THROWS_WHAT(session.beginOrContinueTxn( + opCtx(), txnNum - 1, boost::none, boost::none, "testDB", "insert"), + AssertionException, + sb.str()); + ASSERT(session.getLastWriteOpTime(txnNum).isNull()); +} + +TEST_F(SessionTest, OldTransactionFailsOnSessionWithNewerRetryableWrite) { + const auto sessionId = makeLogicalSessionIdForTest(); + Session session(sessionId); + session.refreshFromStorageIfNeeded(opCtx()); + + const TxnNumber txnNum = 22; + const auto autocommit = false; + + StringBuilder sb; + sb << "Cannot start transaction 21 on session " << sessionId + << " because a newer retryable write with txnNumber 22 has already started on this session."; + session.beginOrContinueTxn(opCtx(), txnNum, boost::none, boost::none, "testDB", "insert"); + ASSERT_THROWS_WHAT(session.beginOrContinueTxn( + opCtx(), txnNum - 1, autocommit, boost::none, "testDB", "insert"), + AssertionException, + sb.str()); + ASSERT(session.getLastWriteOpTime(txnNum).isNull()); +} + TEST_F(SessionTest, AutocommitRequiredOnEveryTxnOp) { const auto sessionId = makeLogicalSessionIdForTest(); Session session(sessionId); -- cgit v1.2.1