maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxxxx