diff --git a/include/wsrep/client_state.hpp b/include/wsrep/client_state.hpp index 7d4eaf4..c871c73 100644 --- a/include/wsrep/client_state.hpp +++ b/include/wsrep/client_state.hpp @@ -202,14 +202,38 @@ namespace wsrep * received from DBMS client starts. * * This method will wait until the possible synchronous - * rollback for associated transaction has finished. + * rollback for associated transaction has finished unless + * wait_rollback_complete_and_acquire_ownership() has been + * called before. + * * The method has a side effect of changing the client * context state to executing. * + * The value set by keep_command_error has an effect on + * how before_command() behaves when it is entered after + * background rollback has been processed: + * + * - If keep_command_error is set true, the current error + * is set and success will be returned. + * - If keep_command_error is set false, the transaction is + * cleaned up and the return value will be non-zero to + * indicate error. + * + * @param keep_command_error Make client state to preserve error + * state in command hooks. + * This is needed if a current command is not supposed to + * return an error status to the client and the protocol must + * advance until the next client command to return error status. + * * @return Zero in case of success, non-zero in case of the * associated transaction was BF aborted. */ - int before_command(); + int before_command(bool keep_command_error); + + int before_command() + { + return before_command(false); + } /** * This method should be called before returning @@ -1013,6 +1037,7 @@ namespace wsrep , debug_log_level_(0) , current_error_(wsrep::e_success) , current_error_status_(wsrep::provider::success) + , keep_command_error_() { } private: @@ -1075,6 +1100,7 @@ namespace wsrep int debug_log_level_; enum wsrep::client_error current_error_; enum wsrep::provider::status current_error_status_; + bool keep_command_error_; /** * Marks external rollbacker thread for the client diff --git a/src/client_state.cpp b/src/client_state.cpp index b3e750b..5a405a9 100644 --- a/src/client_state.cpp +++ b/src/client_state.cpp @@ -34,6 +34,7 @@ void wsrep::client_state::open(wsrep::client_id id) { wsrep::unique_lock lock(mutex_); assert(state_ == s_none); + assert(keep_command_error_ == false); debug_log_state("open: enter"); owning_thread_id_ = wsrep::this_thread::get_id(); rollbacker_active_ = false; @@ -49,6 +50,7 @@ void wsrep::client_state::close() wsrep::unique_lock lock(mutex_); debug_log_state("close: enter"); state(lock, s_quitting); + keep_command_error_ = false; lock.unlock(); if (transaction_.active() && (mode_ != m_local || @@ -84,8 +86,7 @@ void wsrep::client_state::override_error(enum wsrep::client_error error, current_error_status_ = status; } - -int wsrep::client_state::before_command() +int wsrep::client_state::before_command(bool keep_command_error) { wsrep::unique_lock lock(mutex_); debug_log_state("before_command: enter"); @@ -96,6 +97,7 @@ int wsrep::client_state::before_command() assert(state_ == s_idle); do_wait_rollback_complete_and_acquire_ownership(lock); assert(state_ == s_exec); + client_service_.store_globals(); } else { @@ -104,6 +106,8 @@ int wsrep::client_state::before_command() assert(wsrep::this_thread::get_id() == owning_thread_id_); } + keep_command_error_ = keep_command_error; + // If the transaction is active, it must be either executing, // aborted as rolled back by rollbacker, or must_abort if the // client thread gained control via @@ -117,41 +121,43 @@ int wsrep::client_state::before_command() if (transaction_.active()) { - if (transaction_.state() == wsrep::transaction::s_must_abort) + if (transaction_.state() == wsrep::transaction::s_must_abort || + transaction_.state() == wsrep::transaction::s_aborted) { - override_error(wsrep::e_deadlock_error); - lock.unlock(); - client_service_.bf_rollback(); - (void)transaction_.after_statement(); - lock.lock(); - assert(transaction_.state() == - wsrep::transaction::s_aborted); - assert(transaction_.active() == false); - assert(current_error() != wsrep::e_success); - debug_log_state("before_command: error"); - return 1; - } - else if (transaction_.state() == wsrep::transaction::s_aborted) - { - // Transaction was rolled back either just before sending - // result to the client, or after client_state become idle. if (transaction_.is_xa()) { // Client will rollback explicitly, return error. debug_log_state("before_command: error"); return 1; } - else + + override_error(wsrep::e_deadlock_error); + if (transaction_.state() == wsrep::transaction::s_must_abort) { - // Clean up the transaction and return error. - override_error(wsrep::e_deadlock_error); lock.unlock(); - (void)transaction_.after_statement(); + client_service_.bf_rollback(); lock.lock(); - assert(transaction_.active() == false); - debug_log_state("before_command: error"); - return 1; + } + + if (keep_command_error_) + { + // Keep the error for the next command + debug_log_state("before_command: keep error"); + return 0; + } + + // Clean up the transaction and return error. + lock.unlock(); + (void)transaction_.after_statement(); + lock.lock(); + + assert(transaction_.active() == false); + assert(transaction_.state() == wsrep::transaction::s_aborted); + assert(current_error() != wsrep::e_success); + + debug_log_state("before_command: error"); + return 1; } } debug_log_state("before_command: success"); @@ -169,7 +175,14 @@ void wsrep::client_state::after_command_before_result() override_error(wsrep::e_deadlock_error); lock.unlock(); client_service_.bf_rollback(); - (void)transaction_.after_statement(); + // If keep current error is set, the result will be propagated + // back to client with some future command, so keep the transaction + // open here so that error handling can happen in before_command() + // hook. + if (not keep_command_error_) + { + (void)transaction_.after_statement(); + } lock.lock(); assert(transaction_.state() == wsrep::transaction::s_aborted); assert(current_error() != wsrep::e_success); @@ -193,11 +206,12 @@ void wsrep::client_state::after_command_after_result() assert(transaction_.state() == wsrep::transaction::s_aborted); override_error(wsrep::e_deadlock_error); } - else if (transaction_.active() == false) + else if (transaction_.active() == false && not keep_command_error_) { current_error_ = wsrep::e_success; current_error_status_ = wsrep::provider::success; } + keep_command_error_ = false; sync_wait_gtid_ = wsrep::gtid::undefined(); state(lock, s_idle); debug_log_state("after_command_after_result: leave"); diff --git a/test/transaction_test.cpp b/test/transaction_test.cpp index 7386c99..d56ef28 100644 --- a/test/transaction_test.cpp +++ b/test/transaction_test.cpp @@ -1071,6 +1071,176 @@ BOOST_FIXTURE_TEST_CASE( BOOST_REQUIRE(tc.active() == false); } +// +// Test before_command() with keep_command_error param +// Failure free case is not affected by keep_command_error +// +BOOST_FIXTURE_TEST_CASE_TEMPLATE(transaction_keep_error, T, + replicating_fixtures, T) +{ + wsrep::mock_client& cc(T::cc); + const wsrep::transaction& tc(T::tc); + + cc.start_transaction(wsrep::transaction_id(1)); + BOOST_REQUIRE(tc.active()); + cc.after_statement(); + cc.after_command_before_result(); + cc.after_command_after_result(); + + bool keep_command_error(true); + BOOST_REQUIRE(cc.before_command(keep_command_error) == 0); + BOOST_REQUIRE(tc.active()); + cc.after_command_before_result(); + cc.after_command_after_result(); + + keep_command_error = false; + BOOST_REQUIRE(cc.before_command(keep_command_error) == 0); + BOOST_REQUIRE(cc.before_statement() == 0); + BOOST_REQUIRE(cc.before_commit() == 0); + BOOST_REQUIRE(cc.ordered_commit() == 0); + BOOST_REQUIRE(cc.after_commit() == 0); + cc.after_statement(); + BOOST_REQUIRE(cc.current_error() == wsrep::e_success); +} + +// +// Test before_command() with keep_command_error param +// BF abort while idle +// +BOOST_FIXTURE_TEST_CASE(transaction_keep_error_bf_idle_sync_rm, + replicating_client_fixture_sync_rm) +{ + cc.start_transaction(wsrep::transaction_id(1)); + cc.after_statement(); + cc.after_command_before_result(); + cc.after_command_after_result(); + + BOOST_REQUIRE(cc.state() == wsrep::client_state::s_idle); + wsrep_test::bf_abort_unordered(cc); + cc.sync_rollback_complete(); + BOOST_REQUIRE(tc.state() == wsrep::transaction::s_aborted); + + bool keep_command_error(true); + BOOST_REQUIRE(cc.before_command(keep_command_error) == 0); + BOOST_REQUIRE(tc.active()); + BOOST_REQUIRE(cc.current_error() == wsrep::e_deadlock_error); + cc.after_command_before_result(); + cc.after_command_after_result(); + BOOST_REQUIRE(cc.current_error() == wsrep::e_deadlock_error); + + keep_command_error = false; + BOOST_REQUIRE(cc.before_command(keep_command_error) == 1); + BOOST_REQUIRE(cc.current_error() == wsrep::e_deadlock_error); + cc.after_command_before_result(); + cc.after_command_after_result(); + BOOST_REQUIRE(cc.current_error() == wsrep::e_success); +} + +// +// Test before_command() with keep_command_error param +// BF abort after ownership is acquired and before before_command() +// +BOOST_FIXTURE_TEST_CASE_TEMPLATE(transaction_keep_error_bf_after_ownership, T, + replicating_fixtures, T) +{ + wsrep::mock_client& cc(T::cc); + const wsrep::transaction& tc(T::tc); + + cc.start_transaction(wsrep::transaction_id(1)); + cc.after_statement(); + cc.after_command_before_result(); + cc.after_command_after_result(); + + cc.wait_rollback_complete_and_acquire_ownership(); + wsrep_test::bf_abort_unordered(cc); + BOOST_REQUIRE(tc.state() == wsrep::transaction::s_must_abort); + + bool keep_command_error(true); + BOOST_REQUIRE(cc.before_command(keep_command_error) == 0); + BOOST_REQUIRE(tc.active()); + BOOST_REQUIRE(cc.current_error() == wsrep::e_deadlock_error); + cc.after_command_before_result(); + cc.after_command_after_result(); + BOOST_REQUIRE(cc.current_error() == wsrep::e_deadlock_error); + + keep_command_error = false; + BOOST_REQUIRE(cc.before_command(keep_command_error) == 1); + BOOST_REQUIRE(cc.current_error() == wsrep::e_deadlock_error); + cc.after_command_before_result(); + cc.after_command_after_result(); + BOOST_REQUIRE(cc.current_error() == wsrep::e_success); +} + +// +// Test before_command() with keep_command_error param +// BF abort right after before_command() +// +BOOST_FIXTURE_TEST_CASE_TEMPLATE(transaction_keep_error_bf_after_before_command, T, + replicating_fixtures, T) +{ + wsrep::mock_client& cc(T::cc); + const wsrep::transaction& tc(T::tc); + + cc.start_transaction(wsrep::transaction_id(1)); + cc.after_statement(); + cc.after_command_before_result(); + cc.after_command_after_result(); + + bool keep_command_error(true); + BOOST_REQUIRE(cc.before_command(keep_command_error) == 0); + BOOST_REQUIRE(tc.active()); + + wsrep_test::bf_abort_unordered(cc); + BOOST_REQUIRE(tc.state() == wsrep::transaction::s_must_abort); + cc.after_command_before_result(); + cc.after_command_after_result(); + BOOST_REQUIRE(tc.state() == wsrep::transaction::s_aborted); + BOOST_REQUIRE(cc.current_error() == wsrep::e_deadlock_error); + + keep_command_error = false; + BOOST_REQUIRE(cc.before_command(keep_command_error) == 1); + BOOST_REQUIRE(cc.current_error() == wsrep::e_deadlock_error); + cc.after_command_before_result(); + cc.after_command_after_result(); + BOOST_REQUIRE(cc.current_error() == wsrep::e_success); +} + +// +// Test before_command() with keep_command_error param +// BF abort right after after_command_before_result() +// +BOOST_FIXTURE_TEST_CASE_TEMPLATE(transaction_keep_error_bf_after_after_command_before_result, T, + replicating_fixtures, T) +{ + wsrep::mock_client& cc(T::cc); + const wsrep::transaction& tc(T::tc); + + cc.start_transaction(wsrep::transaction_id(1)); + cc.after_statement(); + cc.after_command_before_result(); + cc.after_command_after_result(); + + bool keep_command_error(true); + BOOST_REQUIRE(cc.before_command(keep_command_error) == 0); + BOOST_REQUIRE(tc.active()); + + cc.after_command_before_result(); + + wsrep_test::bf_abort_unordered(cc); + BOOST_REQUIRE(tc.state() == wsrep::transaction::s_must_abort); + + cc.after_command_after_result(); + BOOST_REQUIRE(tc.state() == wsrep::transaction::s_aborted); + BOOST_REQUIRE(cc.current_error() == wsrep::e_deadlock_error); + + keep_command_error = false; + BOOST_REQUIRE(cc.before_command(keep_command_error) == 1); + BOOST_REQUIRE(cc.current_error() == wsrep::e_deadlock_error); + cc.after_command_before_result(); + cc.after_command_after_result(); + BOOST_REQUIRE(cc.current_error() == wsrep::e_success); +} + BOOST_FIXTURE_TEST_CASE(transaction_1pc_applying, applying_client_fixture) {