diff --git a/SConstruct b/SConstruct index 5029646a5..0418e84c4 100644 --- a/SConstruct +++ b/SConstruct @@ -236,7 +236,7 @@ env.Append(CPPFLAGS = ' -DHAVE_COMMON_H') # Common C/CXX flags # These should be kept minimal as they are appended after C/CXX specific flags -env.Append(CCFLAGS = ' -fPIC -Wall -Wextra -Wno-unused-parameter') +env.Append(CCFLAGS = ' -fPIC -Wall -Wextra -Wno-unused-parameter -Wno-implicit-fallthrough ') # C-specific flags env.Prepend(CFLAGS = '-std=c99 -fno-strict-aliasing -pipe ') diff --git a/galera/src/replicator_smm.hpp b/galera/src/replicator_smm.hpp index 38a3149e2..d8de2e7a4 100644 --- a/galera/src/replicator_smm.hpp +++ b/galera/src/replicator_smm.hpp @@ -349,8 +349,9 @@ namespace galera case OOOC: return true; case LOCAL_OOOC: - return trx_.is_local(); + if (trx_.is_local()) { return true; } // in case of remote trx fall through + // fall through case NO_OOOC: return (last_left + 1 == trx_.global_seqno()); } diff --git a/galera/src/wsdb.cpp b/galera/src/wsdb.cpp index 2c9cada17..f103ce59f 100644 --- a/galera/src/wsdb.cpp +++ b/galera/src/wsdb.cpp @@ -6,6 +6,7 @@ #include "trx_handle.hpp" #include "write_set.hpp" +#include "gu_thread.hpp" #include "gu_lock.hpp" #include "gu_throw.hpp" @@ -33,10 +34,11 @@ void galera::Wsdb::print(std::ostream& os) const galera::Wsdb::Wsdb() : trx_pool_ (TrxHandle::LOCAL_STORAGE_SIZE(), 512, "LocalTrxHandle"), - trx_map_ (), - trx_mutex_ (), - conn_map_ (), - conn_mutex_() + trx_map_ (), + conn_trx_map_(), + trx_mutex_ (), + conn_map_ (), + conn_mutex_ () {} @@ -54,6 +56,9 @@ galera::Wsdb::~Wsdb() assert(conn_map_.size() == 0); #else for_each(trx_map_.begin(), trx_map_.end(), Unref2nd()); + for_each(conn_trx_map_.begin(), + conn_trx_map_.end(), + Unref2nd()); #endif // !NDEBUG } @@ -63,9 +68,31 @@ galera::Wsdb::find_trx(wsrep_trx_id_t const trx_id) { gu::Lock lock(trx_mutex_); - TrxMap::iterator const i(trx_map_.find(trx_id)); + galera::TrxHandle* trx; + /* trx-id = 0 is safe-guard condition. + trx-id is generally assigned from thd->query-id and + query-id default is 0. If background thread try to assign + wsrep_next_trx_id before setting query-id we will hit + this assertion: */ + assert(trx_id != 0); - return (trx_map_.end() == i ? 0 : i->second); + if (trx_id != wsrep_trx_id_t(-1)) + { + /* trx_id is valid and unique. Search for this trx_id in the + trx_id -> trx map: */ + TrxMap::iterator const i(trx_map_.find(trx_id)); + trx = (trx_map_.end() == i ? NULL : i->second); + } + else + { + /* trx_id is default, so search for repsective connection id + in connection-transaction map: */ + gu_thread_t const id = gu_thread_self(); + ConnTrxMap::iterator const i(conn_trx_map_.find(id)); + trx = (conn_trx_map_.end() == i ? NULL : i->second); + } + + return (trx); } @@ -78,12 +105,28 @@ galera::Wsdb::create_trx(const TrxHandle::Params& params, gu::Lock lock(trx_mutex_); - std::pair i - (trx_map_.insert(std::make_pair(trx_id, trx))); - - if (gu_unlikely(i.second == false)) gu_throw_fatal; + galera::TrxHandle* trx_ref; + if (trx_id != wsrep_trx_id_t(-1)) + { + /* trx_id is valid, add it to trx-map as valid trx_id, which + is unique accross connections: */ + std::pair i + (trx_map_.insert(std::make_pair(trx_id, trx))); + if (gu_unlikely(i.second == false)) gu_throw_fatal; + trx_ref = i.first->second; + } + else + { + /* trx_id is default so add trx object to connection map + that is maintained based on gu_thread_id (actually it is + alias for connection_id): */ + std::pair i + (conn_trx_map_.insert(std::make_pair(gu_thread_self(), trx))); + if (gu_unlikely(i.second == false)) gu_throw_fatal; + trx_ref = i.first->second; + } - return i.first->second; + return (trx_ref); } @@ -153,11 +196,24 @@ galera::Wsdb::get_conn_query(const TrxHandle::Params& params, void galera::Wsdb::discard_trx(wsrep_trx_id_t trx_id) { gu::Lock lock(trx_mutex_); - TrxMap::iterator i; - if ((i = trx_map_.find(trx_id)) != trx_map_.end()) + if (trx_id != wsrep_trx_id_t(-1)) + { + TrxMap::iterator i; + if ((i = trx_map_.find(trx_id)) != trx_map_.end()) + { + i->second->unref(); + trx_map_.erase(i); + } + } + else { - i->second->unref(); - trx_map_.erase(i); + ConnTrxMap::iterator i; + gu_thread_t id = gu_thread_self(); + if ((i = conn_trx_map_.find(id)) != conn_trx_map_.end()) + { + i->second->unref(); + conn_trx_map_.erase(i); + } } } diff --git a/galera/src/wsdb.hpp b/galera/src/wsdb.hpp index 3b78f0312..072ecffcd 100644 --- a/galera/src/wsdb.hpp +++ b/galera/src/wsdb.hpp @@ -7,6 +7,7 @@ #include "trx_handle.hpp" #include "wsrep_api.h" #include "gu_unordered.hpp" +#include "gu_thread.hpp" namespace galera { @@ -55,6 +56,23 @@ namespace galera typedef gu::UnorderedMap TrxMap; + /* TrxMap structure doesn't take into consideration presence of + two trx objects with same trx_id (2^64 - 1 which is default trx_id) + belonging to two different connections. + This eventually causes same trx object to get shared among two + different unrelated connections which causes state inconsistency + leading to crash (RACE CONDITION). + This problem could be solved by taking into consideration conn-id, + but that would invite interface change. To avoid this we maintain a + separate map of such trx objects based on gu_thread_id: */ + class ConnTrxHash + { + public: + size_t operator()(const gu_thread_t& key) const { return key; } + }; + + typedef gu::UnorderedMap ConnTrxMap; + class ConnHash { public: @@ -117,6 +135,7 @@ namespace galera TrxHandle::LocalPool trx_pool_; TrxMap trx_map_; + ConnTrxMap conn_trx_map_; gu::Mutex trx_mutex_; ConnMap conn_map_; gu::Mutex conn_mutex_;