diff --git a/sql/event_scheduler.cc b/sql/event_scheduler.cc index 837fad5a0936..e58e95a665c5 100644 --- a/sql/event_scheduler.cc +++ b/sql/event_scheduler.cc @@ -172,7 +172,6 @@ bool post_init_event_thread(THD *thd) { Global_THD_manager *thd_manager = Global_THD_manager::get_instance(); thd_manager->add_thd(thd); - thd_manager->inc_thread_running(); return false; } @@ -192,7 +191,6 @@ void deinit_event_thread(THD *thd) { DBUG_PRINT("exit", ("Event thread finishing")); thd->release_resources(); thd_manager->remove_thd(thd); - thd_manager->dec_thread_running(); delete thd; } diff --git a/sql/mysqld_thd_manager.cc b/sql/mysqld_thd_manager.cc index 62865f1b79b4..582fde033f70 100644 --- a/sql/mysqld_thd_manager.cc +++ b/sql/mysqld_thd_manager.cc @@ -364,6 +364,31 @@ THD_ptr Global_THD_manager::find_thd(Find_thd_with_id *func) { return THD_ptr{nullptr}; } +/** + This class implements callback for do_for_all_thd(). + It counts the total number of running threads from global thread list. +*/ +class Count_thread_running : public Do_THD_Impl { + public: + Count_thread_running() : m_count(0) {} + ~Count_thread_running() {} + virtual void operator()(THD *thd) { + if (thd->get_command() != COM_SLEEP) { + m_count++; + } + } + int get_count() { return m_count; } + + private: + int m_count; +}; + +void Global_THD_manager::count_num_thread_running() { + Count_thread_running count_thread_running; + do_for_all_thd(&count_thread_running); + atomic_num_thread_running = count_thread_running.get_count(); +} + void inc_thread_created() { Global_THD_manager::get_instance()->inc_thread_created(); } diff --git a/sql/mysqld_thd_manager.h b/sql/mysqld_thd_manager.h index 7beab1788ce4..d913df60181d 100644 --- a/sql/mysqld_thd_manager.h +++ b/sql/mysqld_thd_manager.h @@ -254,20 +254,15 @@ class Global_THD_manager { void remove_thd(THD *thd); /** - Retrieves thread running statistic variable. - @return int Returns the total number of threads currently running + Count thread running statistic variable. */ - int get_num_thread_running() const { return atomic_num_thread_running; } + void count_num_thread_running(); /** - Increments thread running statistic variable. - */ - void inc_thread_running() { atomic_num_thread_running++; } - - /** - Decrements thread running statistic variable. + Retrieves thread running statistic variable. + @return int Returns the total number of threads currently running */ - void dec_thread_running() { atomic_num_thread_running--; } + int get_num_thread_running() const { return atomic_num_thread_running; } /** Retrieves thread created statistic variable. diff --git a/storage/perfschema/pfs_variable.cc b/storage/perfschema/pfs_variable.cc index 64b8ad137c38..eafcf9b4d296 100644 --- a/storage/perfschema/pfs_variable.cc +++ b/storage/perfschema/pfs_variable.cc @@ -1188,6 +1188,16 @@ int PFS_status_variable_cache::do_materialize_global() { false, /* threads */ true, /* THDs */ &visitor); + /* + Because of the reason described in + PFS_status_variable_cache::do_materialize_all(THD *unsafe_thd), + PFS_status_variable_cache::do_materialize_session(THD *unsafe_thd) and + PFS_status_variable_cache::do_materialize_session(PFS_thread *pfs_thread), + count_num_thread_running() cannot put together with + get_num_thread_running(), so count_num_thread_running() is put here. + */ + Global_THD_manager::get_instance()->count_num_thread_running(); + /* Build the status variable cache using the SHOW_VAR array as a reference. Use the status totals collected from all threads. @@ -1233,6 +1243,22 @@ int PFS_status_variable_cache::do_materialize_all(THD *unsafe_thd) { init_show_var_array(OPT_SESSION, false); } + /* + count_num_thread_running() counts the total number of running threads + from global thread list, using LOCK_thd_list to protect sharded + global thread list. In lock_order_dependencies.txt, the lock order + is that LOCK_thd_list must be locked before LOCK_thd_data. In this + function, LOCK_thd_data is already locked in get_THD(), Then manifest() + will call get_num_thread_running(). If get_num_thread_running() counts + and returns the num, the lock order will be incorrect, which may + lead to dead lock. To prevent this situation, get_num_thread_running() + is split into two part, one is still called get_num_thread_running() + which returns the num, the other is called count_num_thread_running() + which counts the num and should be called before get_THD() and + get_num_thread_running(). So count_num_thread_running() is put here. + */ + Global_THD_manager::get_instance()->count_num_thread_running(); + /* Get and lock a validated THD from the thread manager. */ THD_ptr thd_ptr = get_THD(unsafe_thd); m_safe_thd = thd_ptr.get(); @@ -1280,6 +1306,22 @@ int PFS_status_variable_cache::do_materialize_session(THD *unsafe_thd) { init_show_var_array(OPT_SESSION, true); } + /* + count_num_thread_running() counts the total number of running threads + from global thread list, using LOCK_thd_list to protect sharded + global thread list. In lock_order_dependencies.txt, the lock order + is that LOCK_thd_list must be locked before LOCK_thd_data. In this + function, LOCK_thd_data is already locked in get_THD(), Then manifest() + will call get_num_thread_running(). If get_num_thread_running() counts + and returns the num, the lock order will be incorrect, which may + lead to dead lock. To prevent this situation, get_num_thread_running() + is split into two part, one is still called get_num_thread_running() + which returns the num, the other is called count_num_thread_running() + which counts the num and should be called before get_THD() and + get_num_thread_running(). So count_num_thread_running() is put here. + */ + Global_THD_manager::get_instance()->count_num_thread_running(); + /* Get and lock a validated THD from the thread manager. */ THD_ptr thd_ptr = get_THD(unsafe_thd); m_safe_thd = thd_ptr.get(); @@ -1321,6 +1363,22 @@ int PFS_status_variable_cache::do_materialize_session(PFS_thread *pfs_thread) { /* The SHOW_VAR array must be initialized externally. */ assert(m_initialized); + /* + count_num_thread_running() counts the total number of running threads + from global thread list, using LOCK_thd_list to protect sharded + global thread list. In lock_order_dependencies.txt, the lock order + is that LOCK_thd_list must be locked before LOCK_thd_data. In this + function, LOCK_thd_data is already locked in get_THD(), Then manifest() + will call get_num_thread_running(). If get_num_thread_running() counts + and returns the num, the lock order will be incorrect, which may + lead to dead lock. To prevent this situation, get_num_thread_running() + is split into two part, one is still called get_num_thread_running() + which returns the num, the other is called count_num_thread_running() + which counts the num and should be called before get_THD() and + get_num_thread_running(). So count_num_thread_running() is put here. + */ + Global_THD_manager::get_instance()->count_num_thread_running(); + /* Get and lock a validated THD from the thread manager. */ THD_ptr thd_ptr = get_THD(pfs_thread); m_safe_thd = thd_ptr.get(); @@ -1368,6 +1426,17 @@ int PFS_status_variable_cache::do_materialize_client(PFS_client *pfs_client) { */ m_sum_client_status(pfs_client, &status_totals); + /* + Because of the reason described in + PFS_status_variable_cache::do_materialize_all(THD *unsafe_thd), + PFS_status_variable_cache::do_materialize_session(THD *unsafe_thd) and + PFS_status_variable_cache::do_materialize_session(PFS_thread *pfs_thread), + count_num_thread_running() cannot put together with + get_num_thread_running(), so count_num_thread_running() is put here. + */ + Global_THD_manager::get_instance()->count_num_thread_running(); + + /* Build the status variable cache using the SHOW_VAR array as a reference and the status totals collected from threads associated with this client. diff --git a/unittest/gunit/thd_manager-t.cc b/unittest/gunit/thd_manager-t.cc index 8277f2902b94..8efc569a5ebc 100644 --- a/unittest/gunit/thd_manager-t.cc +++ b/unittest/gunit/thd_manager-t.cc @@ -84,14 +84,6 @@ TEST_F(ThreadManagerTest, AddRemoveTHDWithGuard) { EXPECT_EQ(0U, thd_manager->get_thd_count()); } -TEST_F(ThreadManagerTest, IncDecThreadRunning) { - EXPECT_EQ(0, thd_manager->get_num_thread_running()); - thd_manager->inc_thread_running(); - EXPECT_EQ(1, thd_manager->get_num_thread_running()); - thd_manager->dec_thread_running(); - EXPECT_EQ(0, thd_manager->get_num_thread_running()); -} - TEST_F(ThreadManagerTest, IncThreadCreated) { EXPECT_EQ(0U, thd_manager->get_num_thread_created()); thd_manager->inc_thread_created();