diff --git a/src/include/transaction/transaction_context.h b/src/include/transaction/transaction_context.h index 9f69fab5e5..d7d55efdeb 100644 --- a/src/include/transaction/transaction_context.h +++ b/src/include/transaction/transaction_context.h @@ -20,7 +20,8 @@ namespace transaction { * rollback the transaction by calling commit() or rollback(). * * AUTO_COMMIT is the default mode when a Connection is created. If an application calls - * begin[ReadOnly/Write]Transaction at any point, the mode switches to MANUAL. This creates + * begin[ReadOnly/Write]Transaction at any point and the begin succeeds, the mode switches to + * MANUAL; a refused begin leaves the connection in AUTO_COMMIT. This creates * an "active transaction" in the connection. When a connection is in MANUAL mode and the * active transaction is rolled back or committed, then the active transaction is removed (so * the connection no longer has an active transaction), and the mode automatically switches @@ -56,10 +57,14 @@ class LBUG_API TransactionContext { private: void beginTransactionInternal(TransactionType transactionType); + void beginManualTransaction(TransactionType transactionType); private: std::mutex mtx; main::ClientContext& clientContext; + // MANUAL implies activeTransaction != nullptr: beginManualTransaction sets the mode only after + // the begin succeeded and clearTransaction resets both. Only beginManualTransaction takes mtx; + // the invariant relies on a connection being used by one thread at a time. TransactionMode mode; Transaction* activeTransaction; }; diff --git a/src/transaction/transaction_context.cpp b/src/transaction/transaction_context.cpp index 7821e31162..8693aff2a4 100644 --- a/src/transaction/transaction_context.cpp +++ b/src/transaction/transaction_context.cpp @@ -17,15 +17,11 @@ TransactionContext::TransactionContext(main::ClientContext& clientContext) TransactionContext::~TransactionContext() = default; void TransactionContext::beginReadTransaction() { - std::unique_lock lck{mtx}; - mode = TransactionMode::MANUAL; - beginTransactionInternal(TransactionType::READ_ONLY); + beginManualTransaction(TransactionType::READ_ONLY); } void TransactionContext::beginWriteTransaction() { - std::unique_lock lck{mtx}; - mode = TransactionMode::MANUAL; - beginTransactionInternal(TransactionType::WRITE); + beginManualTransaction(TransactionType::WRITE); } void TransactionContext::beginAutoTransaction(bool readOnlyStatement) { @@ -40,9 +36,7 @@ void TransactionContext::beginAutoTransaction(bool readOnlyStatement) { } void TransactionContext::beginRecoveryTransaction() { - std::unique_lock lck{mtx}; - mode = TransactionMode::MANUAL; - beginTransactionInternal(TransactionType::RECOVERY); + beginManualTransaction(TransactionType::RECOVERY); } void TransactionContext::validateManualTransaction(bool readOnlyStatement) const { @@ -87,6 +81,13 @@ TransactionContext* TransactionContext::Get(const main::ClientContext& context) return context.transactionContext.get(); } +void TransactionContext::beginManualTransaction(TransactionType transactionType) { + std::unique_lock lck{mtx}; + beginTransactionInternal(transactionType); + // Only once the begin succeeded: a refused BEGIN leaves the connection in auto-commit mode. + mode = TransactionMode::MANUAL; +} + void TransactionContext::beginTransactionInternal(TransactionType transactionType) { DASSERT(!activeTransaction); activeTransaction = clientContext.getDatabase()->getTransactionManager()->beginTransaction( diff --git a/test/test_files/transaction/concurrency/ddl_serial_execution.test b/test/test_files/transaction/concurrency/ddl_serial_execution.test index 93c8c67c50..340df877eb 100644 --- a/test/test_files/transaction/concurrency/ddl_serial_execution.test +++ b/test/test_files/transaction/concurrency/ddl_serial_execution.test @@ -17,6 +17,38 @@ Cannot start a new write transaction in the system. Only one write transaction a 0|a|INT32|NULL|True 1|b|INT32|NULL|False +-CASE RefusedBeginKeepsConnectionUsable +-STATEMENT CREATE NODE TABLE t(a INT, PRIMARY KEY(a)); +---- ok +-STATEMENT BEGIN TRANSACTION; +---- ok +-STATEMENT CREATE (:t {a: 1}); +---- ok +-CREATE_CONNECTION conn2 +-STATEMENT [conn2] BEGIN TRANSACTION; +---- error +Cannot start a new write transaction in the system. Only one write transaction at a time is allowed in the system. +-STATEMENT [conn2] ROLLBACK; +---- error +No active transaction for ROLLBACK. +-STATEMENT [conn2] MATCH (n:t) RETURN count(*); +---- 1 +0 +-STATEMENT [conn2] CREATE (:t {a: 3}); +---- error +Cannot start a new write transaction in the system. Only one write transaction at a time is allowed in the system. +-STATEMENT COMMIT; +---- ok +-STATEMENT [conn2] BEGIN TRANSACTION; +---- ok +-STATEMENT [conn2] CREATE (:t {a: 2}); +---- ok +-STATEMENT [conn2] COMMIT; +---- ok +-STATEMENT MATCH (n:t) RETURN count(*); +---- 1 +2 + # Test CALL debug_enable_multi_writes=true -CASE EnableMultiWrites -STATEMENT CALL debug_enable_multi_writes=true; diff --git a/test/transaction/transaction_test.cpp b/test/transaction/transaction_test.cpp index 993a0ed6ff..84a79dfcbd 100644 --- a/test/transaction/transaction_test.cpp +++ b/test/transaction/transaction_test.cpp @@ -3,6 +3,7 @@ #include "api_test/api_test.h" #include "api_test/private_api_test.h" #include "common/exception/runtime.h" +#include "common/exception/transaction_manager.h" #include "graph_test/private_graph_test.h" #include "storage/buffer_manager/buffer_manager.h" #include "storage/buffer_manager/memory_manager.h" @@ -46,6 +47,51 @@ TEST_F(PrivateApiTest, TransactionModes) { ASSERT_EQ(TransactionMode::AUTO, getTransactionMode(*conn)); } +TEST_F(PrivateApiTest, RefusedBeginKeepsAutoMode) { + auto conn2 = std::make_unique(database.get()); + ASSERT_TRUE(conn->query("BEGIN TRANSACTION;")->isSuccess()); + ASSERT_FALSE(conn2->query("BEGIN TRANSACTION;")->isSuccess()); + ASSERT_EQ(TransactionMode::AUTO, getTransactionMode(*conn2)); + EXPECT_THROW(TransactionContext::Get(*getClientContext(*conn2))->beginRecoveryTransaction(), + TransactionManagerException); + ASSERT_EQ(TransactionMode::AUTO, getTransactionMode(*conn2)); + ASSERT_TRUE(conn2->query("RETURN 1;")->isSuccess()); + ASSERT_TRUE(conn->query("COMMIT;")->isSuccess()); +} + +TEST_F(PrivateApiTest, RefusedPreparedAndMultiStatementBeginKeepAutoMode) { + auto conn2 = std::make_unique(database.get()); + ASSERT_TRUE(conn->query("BEGIN TRANSACTION;")->isSuccess()); + auto prepared = conn2->prepare("BEGIN TRANSACTION;"); + ASSERT_TRUE(prepared->isSuccess()) << prepared->getErrorMessage(); + ASSERT_FALSE(conn2->execute(prepared.get())->isSuccess()); + ASSERT_EQ(TransactionMode::AUTO, getTransactionMode(*conn2)); + ASSERT_FALSE(conn2->query("BEGIN TRANSACTION; RETURN 1;")->isSuccess()); + ASSERT_EQ(TransactionMode::AUTO, getTransactionMode(*conn2)); + ASSERT_TRUE(conn2->query("RETURN 1;")->isSuccess()); + ASSERT_TRUE(conn->query("COMMIT;")->isSuccess()); +} + +TEST_F(PrivateApiTest, BeginOnPoisonedWALKeepsAutoMode) { + lbug::storage::WAL::Get(*getClientContext(*conn))->poison("test"); + auto result = conn->query("BEGIN TRANSACTION;"); + ASSERT_FALSE(result->isSuccess()); + EXPECT_NE(result->getErrorMessage().find("panic state"), std::string::npos) + << result->getErrorMessage(); + ASSERT_EQ(TransactionMode::AUTO, getTransactionMode(*conn)); + ASSERT_TRUE(conn->query("RETURN 1;")->isSuccess()); +} + +TEST_F(PrivateApiTest, ReadOnlyBeginBesideWriterSwitchesToManual) { + auto conn2 = std::make_unique(database.get()); + ASSERT_TRUE(conn->query("BEGIN TRANSACTION;")->isSuccess()); + ASSERT_TRUE(conn2->query("BEGIN TRANSACTION READ ONLY;")->isSuccess()); + ASSERT_EQ(TransactionMode::MANUAL, getTransactionMode(*conn2)); + ASSERT_TRUE(conn2->query("COMMIT;")->isSuccess()); + ASSERT_EQ(TransactionMode::AUTO, getTransactionMode(*conn2)); + ASSERT_TRUE(conn->query("COMMIT;")->isSuccess()); +} + TEST_F(PrivateApiTest, MultipleCallsFromSameTransaction) { conn->query("BEGIN TRANSACTION READ ONLY;"); auto activeTransactionID = getActiveTransactionID(*conn);