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