Re: [Maria-developers] 9e1c15355a9: resolve the possible deadlock issue through intern_plugin_lock
Hi, Nikita! Looks good so far but I noticed too late that it's just one patch on top of others, not a complete thing. See few comments below. Shall I review everything? Is it feature complete except that it skips inactive threads? On Sep 07, Nikita Malyavin wrote:
revision-id: 9e1c15355a9 (mariadb-10.6.3-44-g9e1c15355a9) parent(s): d7c36600144 author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2021-09-07 18:37:51 +0300 message:
resolve the possible deadlock issue through intern_plugin_lock
diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc index be043bf229a..ecd885ed296 100644 --- a/sql/sql_plugin.cc +++ b/sql/sql_plugin.cc @@ -1053,6 +1053,15 @@ plugin_ref plugin_lock(THD *thd, plugin_ref ptr) }
this needs a function comment. the function is quite specialized and very different from normal plugin lock functions. it requires a THD (normally it's optional), it does not return a locked plugin like the others (because, exactly, it's stored in the THD). because it's not what people usually expect from a plugin_lock* function, the comment should describe all these differences.
+void plugin_lock_by_var_if_different(THD *thd, sys_var *var, sys_var *var_prev) +{ + sys_var_pluginvar *pv= var->cast_pluginvar(); + sys_var_pluginvar *pv_prev= var_prev ? var_prev->cast_pluginvar() : NULL; + if (pv && (!pv_prev || pv->plugin != pv_prev->plugin)) + intern_plugin_lock(thd->lex, plugin_int_to_ref(pv->plugin)); +} + + /* Notes on lifetime:
diff --git a/storage/perfschema/pfs_variable.cc b/storage/perfschema/pfs_variable.cc index 984b68e699b..85656532c72 100644 --- a/storage/perfschema/pfs_variable.cc +++ b/storage/perfschema/pfs_variable.cc @@ -100,6 +102,15 @@ bool PFS_system_variable_cache::init_show_var_array(enum_var_type scope, bool st
mysql_prlock_unlock(&LOCK_system_variables_hash);
+ for (uint i= 0; i < m_show_var_array.elements(); i++)
why separately, not in the loop above?
+ { + sys_var *var= (sys_var *)m_show_var_array.at(i).value; + if (!var) continue;
can var be NULL?
+ sys_var *prev= i == 0 ? NULL : (sys_var *)m_show_var_array.at(i-1).value;
hmm, normally this is done with sys_var *prev= NULL; before the loop and prev= var; in the loop, after plugin_lock_by_var_if_different().
+ + plugin_lock_by_var_if_different(m_current_thd, var, prev); + } + /* Increase cache size if necessary. */ m_cache.reserve(m_show_var_array.elements());
@@ -316,7 +327,27 @@ class PFS_system_variable_cache_apc: public Apc_target::Apc_call
void call_in_target_thread() override { - (m_pfs->*m_func)(m_param); + call(m_pfs, m_func, m_param); + } +public: + static void call(PFS_system_variable_cache *pfs, Request func, uint param) + { + THD *safe_thd= pfs->safe_thd(); + + if (pfs->query_scope() == OPT_SESSION)
what's the point doing apc call if the query_scope() is not OPT_SESSION? It seems like here you need a DBUG_ASSERT, and the if() should be much earlier up the stack.
+ { + mysql_mutex_lock(&LOCK_global_system_variables); + if (!safe_thd->variables.dynamic_variables_ptr || + global_system_variables.dynamic_variables_head > + safe_thd->variables.dynamic_variables_head) + { + mysql_prlock_rdlock(&LOCK_system_variables_hash); + sync_dynamic_session_variables(safe_thd, false); + mysql_prlock_unlock(&LOCK_system_variables_hash); + } + mysql_mutex_unlock(&LOCK_global_system_variables); + } + (pfs->*func)(param); } };
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
On Tue, 7 Sept 2021 at 21:29, Sergei Golubchik
Hi, Nikita!
Looks good so far but I noticed too late that it's just one patch on top of others, not a complete thing.
See few comments below.
Shall I review everything?
Hello, Sergei!
I didn't suppose it as the ready to push code, just wanted your comments. But here we go, so yes, there are five commits at this moment on bb-10.6-nikita regarding this MDEV: 9e1c1535 resolve the possible deadlock issue through intern_plugin_lock d7c36600 do not lock LOCK_thd_data c1ae08b0 MDEV-16440 merge 5.7 P_S sysvars instrumentation and tables 1faf3a05 Better grain of LOCK_global_system_variables in Sys_var_multi_source d1a925b9 fix ASAN use-after-poison in fix_dl_name 1faf3a05 and d1a925b9 are supposed to be separate commits, the rest should be squashed. I just wanted 9e1c1535 to be convenient to look at, so they're split for now. Is it feature complete except that it skips inactive threads? I didn't yet went through some test results, like perfschema.show_{misc,coverage} [they were quite different]. But both global_variables, session_variables and variables_by_thread seem to work fine.
diff --git a/storage/perfschema/pfs_variable.cc b/storage/perfschema/pfs_variable.cc
index 984b68e699b..85656532c72 100644 --- a/storage/perfschema/pfs_variable.cc +++ b/storage/perfschema/pfs_variable.cc @@ -100,6 +102,15 @@ bool PFS_system_variable_cache::init_show_var_array(enum_var_type scope, bool st
mysql_prlock_unlock(&LOCK_system_variables_hash);
+ for (uint i= 0; i < m_show_var_array.elements(); i++)
why separately, not in the loop above?
I wanted to do ti outside of LOCK_system_variables_hash, only that's why.
+ { + sys_var *var= (sys_var *)m_show_var_array.at(i).value; + if (!var) continue;
can var be NULL?
I have seen this protection elsewhere in PS code, and blindly relied on that it's important:) See for example the loop in PFS_system_variable_cache::do_materialize_global. They only continue until the value is NULL. Do you have a clue why?
+ sys_var *prev= i == 0 ? NULL : (sys_var *)m_show_var_array.at (i-1).value;
hmm, normally this is done with
sys_var *prev= NULL;
before the loop and
prev= var;
in the loop, after plugin_lock_by_var_if_different().
Right, that'll be better. I'll rewrite.
+ plugin_lock_by_var_if_different(m_current_thd, var, prev); + } + /* Increase cache size if necessary. */ m_cache.reserve(m_show_var_array.elements());
@@ -316,7 +327,27 @@ class PFS_system_variable_cache_apc: public Apc_target::Apc_call
void call_in_target_thread() override { - (m_pfs->*m_func)(m_param); + call(m_pfs, m_func, m_param); + } +public: + static void call(PFS_system_variable_cache *pfs, Request func, uint
+ param)
+ { + THD *safe_thd= pfs->safe_thd(); + + if (pfs->query_scope() == OPT_SESSION)
what's the point doing apc call if the query_scope() is not OPT_SESSION? It seems like here you need a DBUG_ASSERT, and the if() should be much earlier up the stack.
I just used the same mechanism for global variables as well. I understand now it's an overkill. Will simplify!
+ { + mysql_mutex_lock(&LOCK_global_system_variables); + if (!safe_thd->variables.dynamic_variables_ptr || + global_system_variables.dynamic_variables_head > + safe_thd->variables.dynamic_variables_head) + { + mysql_prlock_rdlock(&LOCK_system_variables_hash); + sync_dynamic_session_variables(safe_thd, false); + mysql_prlock_unlock(&LOCK_system_variables_hash); + } + mysql_mutex_unlock(&LOCK_global_system_variables); + } + (pfs->*func)(param); } };
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
I will also add the comment for plugin_lock_by_var_if_different and fix the test results (and run through them) soon. -- Yours truly, Nikita Malyavin
Sergei, I have pushed the updates in the separate commit:
diff --git a/storage/perfschema/pfs_variable.cc
b/storage/perfschema/pfs_variable.cc
index 984b68e699b..85656532c72 100644 --- a/storage/perfschema/pfs_variable.cc +++ b/storage/perfschema/pfs_variable.cc @@ -100,6 +102,15 @@ bool PFS_system_variable_cache::init_show_var_array(enum_var_type scope, bool st
mysql_prlock_unlock(&LOCK_system_variables_hash);
+ for (uint i= 0; i < m_show_var_array.elements(); i++)
why separately, not in the loop above?
I wanted to do ti outside of LOCK_system_variables_hash, only that's why.
+ { + sys_var *var= (sys_var *)m_show_var_array.at(i).value; + if (!var) continue;
can var be NULL?
I have seen this protection elsewhere in PS code, and blindly relied on that it's important:)
See for example the loop in PFS_system_variable_cache::do_materialize_global. They only continue until the value is NULL. Do you have a clue why?
+ sys_var *prev= i == 0 ? NULL : (sys_var *)m_show_var_array.at (i-1).value;
hmm, normally this is done with
sys_var *prev= NULL;
before the loop and
prev= var;
in the loop, after plugin_lock_by_var_if_different().
Right, that'll be better. I'll rewrite.
Since it is already a separate loop, I have extracted it to the plugin_lock_by_sys_var_array function altogether. The function's meaning is now much cleaner itself. And besides there's much less conditions computed in the loop, so it's faster.
@@ -316,7 +327,27 @@ class PFS_system_variable_cache_apc: public
Apc_target::Apc_call
void call_in_target_thread() override { - (m_pfs->*m_func)(m_param); + call(m_pfs, m_func, m_param); + } +public: + static void call(PFS_system_variable_cache *pfs, Request func, uint
param)
+ { + THD *safe_thd= pfs->safe_thd(); + + if (pfs->query_scope() == OPT_SESSION)
what's the point doing apc call if the query_scope() is not OPT_SESSION? It seems like here you need a DBUG_ASSERT, and the if() should be much earlier up the stack.
I just used the same mechanism for global variables as well. I understand now it's an overkill. Will simplify!
I realized i didn't touch global variables traverse, so moving this check to an assertion was enough -- Yours truly, Nikita Malyavin
participants (2)
-
Nikita Malyavin -
Sergei Golubchik