From 6246d9d3b89f58039ffd917b555313d5b8523005 Mon Sep 17 00:00:00 2001 From: Teemu Ollakka Date: Thu, 14 Jun 2018 11:58:47 +0300 Subject: [PATCH] Allow must_abort - cert_failed transition. If the BF abort happens during provider certify call which returns certification failure, the cert_failed state should take precedence. --- dbsim/dbms_simulator.cpp | 2 +- include/wsrep/client_context.hpp | 2 +- src/transaction_context.cpp | 6 ++- test/fake_client_context.hpp | 13 ++--- test/fake_provider.hpp | 28 +++++----- test/transaction_context_test.cpp | 77 ++++++++++++++++++++------- test/transaction_context_test_2pc.cpp | 4 +- 7 files changed, 86 insertions(+), 46 deletions(-) diff --git a/dbsim/dbms_simulator.cpp b/dbsim/dbms_simulator.cpp index a1481f8..edbafdb 100644 --- a/dbsim/dbms_simulator.cpp +++ b/dbsim/dbms_simulator.cpp @@ -412,7 +412,7 @@ public: wsrep::client_context::store_globals(); } private: - void debug_sync(wsrep::unique_lock&, const char*) override { } + void debug_sync(const char*) override { } void debug_suicide(const char*) override { } void on_error(enum wsrep::client_error) override { } diff --git a/include/wsrep/client_context.hpp b/include/wsrep/client_context.hpp index f2a1cef..6127235 100644 --- a/include/wsrep/client_context.hpp +++ b/include/wsrep/client_context.hpp @@ -522,7 +522,7 @@ namespace wsrep /*! * Enter debug synchronization point. */ - virtual void debug_sync(wsrep::unique_lock&, const char*) = 0; + virtual void debug_sync(const char*) = 0; /*! * diff --git a/src/transaction_context.cpp b/src/transaction_context.cpp index cb209be..c3bda95 100644 --- a/src/transaction_context.cpp +++ b/src/transaction_context.cpp @@ -646,7 +646,7 @@ void wsrep::transaction_context::state( { 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0}, /* oc */ { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, /* ct */ { 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0}, /* cf */ - { 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 1, 0}, /* ma */ + { 0, 0, 0, 0, 0, 0, 1, 0, 1, 0, 1, 0}, /* ma */ { 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0}, /* ab */ { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, /* ad */ { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}, /* mr */ @@ -778,6 +778,7 @@ int wsrep::transaction_context::certify_commit( state(lock, s_certifying); flags(flags() | wsrep::provider::flag::commit); + lock.unlock(); if (client_context_.prepare_data_for_replication(*this)) @@ -797,16 +798,17 @@ int wsrep::transaction_context::certify_commit( return 1; } + client_context_.debug_sync("wsrep_before_certification"); enum wsrep::provider::status cert_ret(provider_.certify(client_context_.id().get(), ws_handle_, flags(), ws_meta_)); + client_context_.debug_sync("wsrep_after_certification"); lock.lock(); assert(state() == s_certifying || state() == s_must_abort); - client_context_.debug_sync(lock, "wsrep_after_certification"); int ret(1); switch (cert_ret) diff --git a/test/fake_client_context.hpp b/test/fake_client_context.hpp index 747f51d..dfffaf4 100644 --- a/test/fake_client_context.hpp +++ b/test/fake_client_context.hpp @@ -110,20 +110,20 @@ namespace wsrep bool killed() const WSREP_OVERRIDE { return killed_before_certify_; } void abort() WSREP_OVERRIDE { ++aborts_; } void store_globals() WSREP_OVERRIDE { } - void debug_sync(wsrep::unique_lock& lock, - const char* sync_point) WSREP_OVERRIDE + void debug_sync(const char* sync_point) WSREP_OVERRIDE { - lock.unlock(); if (sync_point_enabled_ == sync_point) { switch (sync_point_action_) { - case spa_bf_abort: + case spa_bf_abort_unordered: + wsrep_test::bf_abort_unordered(*this); + break; + case spa_bf_abort_ordered: wsrep_test::bf_abort_ordered(*this); break; } } - lock.lock(); } void debug_suicide(const char*) WSREP_OVERRIDE { @@ -146,7 +146,8 @@ namespace wsrep std::string sync_point_enabled_; enum sync_point_action { - spa_bf_abort + spa_bf_abort_unordered, + spa_bf_abort_ordered } sync_point_action_; size_t bytes_generated_; private: diff --git a/test/fake_provider.hpp b/test/fake_provider.hpp index 2538804..39d09a6 100644 --- a/test/fake_provider.hpp +++ b/test/fake_provider.hpp @@ -22,10 +22,10 @@ namespace wsrep fake_provider(wsrep::server_context& server_context) : provider(server_context) - , certify_status_() - , commit_order_enter_status_() - , commit_order_leave_status_() - , release_status_() + , certify_result_() + , commit_order_enter_result_() + , commit_order_leave_result_() + , release_result_() , group_id_("1") , server_id_("1") , group_seqno_(0) @@ -57,11 +57,11 @@ namespace wsrep << "client: " << client_id.get() << " flags: " << std::hex << flags << std::dec - << " certify_status: " << certify_status_; + << " certify_status: " << certify_result_; - if (certify_status_) + if (certify_result_) { - return certify_status_; + return certify_result_; } ++fragments_; @@ -129,14 +129,14 @@ namespace wsrep enum wsrep::provider::status commit_order_enter(const wsrep::ws_handle&, const wsrep::ws_meta&) - { return commit_order_enter_status_; } + { return commit_order_enter_result_; } int commit_order_leave(const wsrep::ws_handle&, const wsrep::ws_meta&) - { return commit_order_leave_status_;} + { return commit_order_leave_result_;} int release(wsrep::ws_handle&) - { return release_status_; } + { return release_result_; } int replay(wsrep::ws_handle&, void*) { ::abort(); /* not impl */} @@ -171,10 +171,10 @@ namespace wsrep } // Parameters to control return value from the call - enum wsrep::provider::status certify_status_; - enum wsrep::provider::status commit_order_enter_status_; - enum wsrep::provider::status commit_order_leave_status_; - enum wsrep::provider::status release_status_; + enum wsrep::provider::status certify_result_; + enum wsrep::provider::status commit_order_enter_result_; + enum wsrep::provider::status commit_order_leave_result_; + enum wsrep::provider::status release_result_; size_t start_fragments() const { return start_fragments_; } size_t fragments() const { return fragments_; } diff --git a/test/transaction_context_test.cpp b/test/transaction_context_test.cpp index 59345ce..5794564 100644 --- a/test/transaction_context_test.cpp +++ b/test/transaction_context_test.cpp @@ -350,6 +350,42 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE( BOOST_REQUIRE(cc.replays() == 1); } +// +// Test a 1PC transaction which gets BF aborted simultaneously with +// certification failure. BF abort should not succeed as the +// transaction is going to roll back anyway. Certification failure +// should not generate seqno for write set meta. +// +BOOST_FIXTURE_TEST_CASE_TEMPLATE( + transaction_context_1pc_bf_before_unordered_cert_failure, T, + replicating_fixtures, T) +{ + wsrep::fake_server_context& sc(T::sc); + wsrep::fake_client_context& cc(T::cc); + const wsrep::transaction_context& tc(T::tc); + + cc.start_transaction(1); + BOOST_REQUIRE(tc.active()); + BOOST_REQUIRE(tc.id() == wsrep::transaction_id(1)); + BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_executing); + cc.sync_point_enabled_ = "wsrep_before_certification"; + cc.sync_point_action_ = wsrep::fake_client_context::spa_bf_abort_unordered; + sc.provider().certify_result_ = wsrep::provider::error_certification_failed; + BOOST_REQUIRE(cc.before_commit()); + BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_cert_failed); + BOOST_REQUIRE(tc.certified() == false); + BOOST_REQUIRE(tc.ordered() == false); + BOOST_REQUIRE(cc.before_rollback() == 0); + BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_aborting); + BOOST_REQUIRE(cc.after_rollback() == 0); + BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_aborted); + BOOST_REQUIRE(cc.after_statement() == wsrep::client_context::asr_error); + BOOST_REQUIRE(tc.active() == false); + BOOST_REQUIRE(tc.ordered() == false); + BOOST_REQUIRE(tc.certified() == false); + BOOST_REQUIRE(cc.current_error() == wsrep::e_deadlock_error); +} + // // Test a 1PC transaction which gets "warning error" from certify call // @@ -367,7 +403,7 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE( BOOST_REQUIRE(tc.id() == wsrep::transaction_id(1)); BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_executing); - sc.provider().certify_status_ = wsrep::provider::error_warning; + sc.provider().certify_result_ = wsrep::provider::error_warning; // Run before commit BOOST_REQUIRE(cc.before_commit()); @@ -375,7 +411,7 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE( BOOST_REQUIRE(tc.certified() == false); BOOST_REQUIRE(tc.ordered() == false); - sc.provider().certify_status_ = wsrep::provider::success; + sc.provider().certify_result_ = wsrep::provider::success; // Rollback sequence BOOST_REQUIRE(cc.before_rollback() == 0); @@ -408,7 +444,7 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE( BOOST_REQUIRE(tc.id() == wsrep::transaction_id(1)); BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_executing); - sc.provider().certify_status_ = wsrep::provider::error_transaction_missing; + sc.provider().certify_result_ = wsrep::provider::error_transaction_missing; // Run before commit BOOST_REQUIRE(cc.before_commit()); @@ -416,7 +452,7 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE( BOOST_REQUIRE(tc.certified() == false); BOOST_REQUIRE(tc.ordered() == false); - sc.provider().certify_status_ = wsrep::provider::success; + sc.provider().certify_result_ = wsrep::provider::success; // Rollback sequence BOOST_REQUIRE(cc.before_rollback() == 0); @@ -449,7 +485,7 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE( BOOST_REQUIRE(tc.id() == wsrep::transaction_id(1)); BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_executing); - sc.provider().certify_status_ = wsrep::provider::error_size_exceeded; + sc.provider().certify_result_ = wsrep::provider::error_size_exceeded; // Run before commit BOOST_REQUIRE(cc.before_commit()); @@ -457,7 +493,7 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE( BOOST_REQUIRE(tc.certified() == false); BOOST_REQUIRE(tc.ordered() == false); - sc.provider().certify_status_ = wsrep::provider::success; + sc.provider().certify_result_ = wsrep::provider::success; // Rollback sequence BOOST_REQUIRE(cc.before_rollback() == 0); @@ -490,7 +526,7 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE( BOOST_REQUIRE(tc.id() == wsrep::transaction_id(1)); BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_executing); - sc.provider().certify_status_ = wsrep::provider::error_connection_failed; + sc.provider().certify_result_ = wsrep::provider::error_connection_failed; // Run before commit BOOST_REQUIRE(cc.before_commit()); @@ -498,7 +534,7 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE( BOOST_REQUIRE(tc.certified() == false); BOOST_REQUIRE(tc.ordered() == false); - sc.provider().certify_status_ = wsrep::provider::success; + sc.provider().certify_result_ = wsrep::provider::success; // Rollback sequence BOOST_REQUIRE(cc.before_rollback() == 0); @@ -531,7 +567,7 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE( BOOST_REQUIRE(tc.id() == wsrep::transaction_id(1)); BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_executing); - sc.provider().certify_status_ = wsrep::provider::error_not_allowed; + sc.provider().certify_result_ = wsrep::provider::error_not_allowed; // Run before commit BOOST_REQUIRE(cc.before_commit()); @@ -539,7 +575,7 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE( BOOST_REQUIRE(tc.certified() == false); BOOST_REQUIRE(tc.ordered() == false); - sc.provider().certify_status_ = wsrep::provider::success; + sc.provider().certify_result_ = wsrep::provider::success; // Rollback sequence BOOST_REQUIRE(cc.before_rollback() == 0); @@ -572,7 +608,7 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE( BOOST_REQUIRE(tc.id() == wsrep::transaction_id(1)); BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_executing); - sc.provider().certify_status_ = wsrep::provider::error_fatal; + sc.provider().certify_result_ = wsrep::provider::error_fatal; // Run before commit BOOST_REQUIRE(cc.before_commit()); @@ -580,7 +616,7 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE( BOOST_REQUIRE(tc.certified() == false); BOOST_REQUIRE(tc.ordered() == false); - sc.provider().certify_status_ = wsrep::provider::success; + sc.provider().certify_result_ = wsrep::provider::success; // Rollback sequence BOOST_REQUIRE(cc.before_rollback() == 0); @@ -614,7 +650,7 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE( BOOST_REQUIRE(tc.id() == wsrep::transaction_id(1)); BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_executing); - sc.provider().certify_status_ = wsrep::provider::error_unknown; + sc.provider().certify_result_ = wsrep::provider::error_unknown; // Run before commit BOOST_REQUIRE(cc.before_commit()); @@ -622,7 +658,7 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE( BOOST_REQUIRE(tc.certified() == false); BOOST_REQUIRE(tc.ordered() == false); - sc.provider().certify_status_ = wsrep::provider::success; + sc.provider().certify_result_ = wsrep::provider::success; // Rollback sequence BOOST_REQUIRE(cc.before_rollback() == 0); @@ -643,7 +679,7 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE( // after certify call // BOOST_FIXTURE_TEST_CASE_TEMPLATE( - transaction_context_1pc_bf_abort_after_certify_regain_lock, T, + transaction_context_1pc_bf_abort_before_certify_regain_lock, T, replicating_fixtures, T) { // wsrep::fake_server_context& sc(T::sc); @@ -657,6 +693,7 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE( BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_executing); cc.sync_point_enabled_ = "wsrep_after_certification"; + cc.sync_point_action_ = wsrep::fake_client_context::spa_bf_abort_ordered; // Run before commit BOOST_REQUIRE(cc.before_commit()); BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_must_replay); @@ -988,9 +1025,9 @@ BOOST_FIXTURE_TEST_CASE(transaction_context_row_streaming_cert_fail_non_commit, BOOST_REQUIRE(cc.start_transaction(1) == 0); BOOST_REQUIRE(cc.after_row() == 0); BOOST_REQUIRE(tc.streaming_context_.fragments_certified() == 1); - sc.provider().certify_status_ = wsrep::provider::error_certification_failed; + sc.provider().certify_result_ = wsrep::provider::error_certification_failed; BOOST_REQUIRE(cc.after_row() == 1); - sc.provider().certify_status_ = wsrep::provider::success; + sc.provider().certify_result_ = wsrep::provider::success; BOOST_REQUIRE(cc.before_rollback() == 0); BOOST_REQUIRE(cc.after_rollback() == 0); BOOST_REQUIRE(cc.after_statement() == wsrep::client_context::asr_success); @@ -1008,10 +1045,10 @@ BOOST_FIXTURE_TEST_CASE(transaction_context_row_streaming_cert_fail_commit, BOOST_REQUIRE(cc.start_transaction(1) == 0); BOOST_REQUIRE(cc.after_row() == 0); BOOST_REQUIRE(tc.streaming_context_.fragments_certified() == 1); - sc.provider().certify_status_ = wsrep::provider::error_certification_failed; + sc.provider().certify_result_ = wsrep::provider::error_certification_failed; BOOST_REQUIRE(cc.before_commit() == 1); BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_cert_failed); - sc.provider().certify_status_ = wsrep::provider::success; + sc.provider().certify_result_ = wsrep::provider::success; BOOST_REQUIRE(cc.before_rollback() == 0); BOOST_REQUIRE(cc.after_rollback() == 0); BOOST_REQUIRE(cc.after_statement() == wsrep::client_context::asr_error); @@ -1141,7 +1178,7 @@ BOOST_FIXTURE_TEST_CASE(transaction_context_statement_streaming_cert_fail, BOOST_REQUIRE(cc.start_transaction(1) == 0); BOOST_REQUIRE(cc.after_row() == 0); BOOST_REQUIRE(tc.streaming_context_.fragments_certified() == 0); - sc.provider().certify_status_ = wsrep::provider::error_certification_failed; + sc.provider().certify_result_ = wsrep::provider::error_certification_failed; BOOST_REQUIRE(cc.after_statement() == wsrep::client_context::asr_error); BOOST_REQUIRE(cc.current_error() == wsrep::e_deadlock_error); BOOST_REQUIRE(sc.provider().fragments() == 0); diff --git a/test/transaction_context_test_2pc.cpp b/test/transaction_context_test_2pc.cpp index 735e86f..9072b0e 100644 --- a/test/transaction_context_test_2pc.cpp +++ b/test/transaction_context_test_2pc.cpp @@ -162,12 +162,12 @@ BOOST_FIXTURE_TEST_CASE( BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_executing); BOOST_REQUIRE(cc.before_prepare() == 0); BOOST_REQUIRE(cc.after_prepare() == 0); - sc.provider().commit_order_enter_status_ = wsrep::provider::error_bf_abort; + sc.provider().commit_order_enter_result_ = wsrep::provider::error_bf_abort; BOOST_REQUIRE(cc.before_commit()); BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_must_replay); BOOST_REQUIRE(tc.certified() == true); BOOST_REQUIRE(tc.ordered() == true); - sc.provider().commit_order_enter_status_ = wsrep::provider::success; + sc.provider().commit_order_enter_result_ = wsrep::provider::success; BOOST_REQUIRE(cc.before_rollback() == 0); BOOST_REQUIRE(tc.state() == wsrep::transaction_context::s_must_replay); BOOST_REQUIRE(cc.after_rollback() == 0);