maria-developers team mailing list archive

[Nikita Malyavin <nikitamalyavin@xxxxxxxxx>]

On Tue, 7 Sept 2021 at 21:29, Sergei Golubchik <serg@xxxxxxxxxxx> 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 security@xxxxxxxxxxx
>

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