On Tue, 7 Sept 2021 at 21:29, Sergei Golubchik <s...@mariadb.org> wrote:
> 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 secur...@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
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp