maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sergey!

On Nov 27, Sergey Vojtovich wrote:
> Hi Sergei,
> 
> I'm afraid I couldn't find easy fix for this deadlock. The question is if it
> is worth to spend time for more complex (and possibly less stable) solution.
> Details below.
> 
> Locking profile
> ---------------
> INSTALL PLUGIN: LOCK_plugin                    -> LOCK_system_variables_hash (w)
> SHOW VARIABLES: LOCK_system_variables_hash (r) -> LOCK_global_system_variables
>  change_user(): LOCK_global_system_variables   -> LOCK_plugin
> 
> INSTALL PLUGIN
> --------------
> Needs LOCK_plugin to register new plugin.
> Needs LOCK_system_variables_hash to register plugin variables.
> Doesn't seem to need both locks at the same time.
> Similar lock order is used in a few places.
> 
> SHOW VARIABLES
> --------------
> Needs LOCK_system_variables_hash to iterate system_variable_hash.
> Needs LOCK_global_system_variables to read variable value.
> Does seem to need both locks at the same time.
> This lock order is not used in other places.
> 
> change_user()
> -------------
> Needs LOCK_global_system_variables to read global_system_variable.table_plugin.
> Needs LOCK_plugin to my_plugin_lock(table_plugin).
> Does seem to need both locks at the same time.
> Similar lock order is used in a few places.

Observations:

THD::init() starts from locking LOCK_global_system_variables, then
invokes plugin_thdvar_init(), that invokes cleanup_variables(), that
read-locks LOCK_system_variables_hash.

This is directly against SHOW VARIABLES order. Suggestion: swap the
order of cleanup_variables() and LOCK_global_system_variables.

> "SHOW VARIABLES" looks correct. So I have only two ideas:
> - Fix INSTALL PLUGIN so it releases LOCK_plugin before registering variables.
>   Sounds like the best solution, but there are a few more things to fix, e.g.
>   UNINTALL PLUGIN.

Is it possible? On a quick look I wasn't able to answer that :(

> - Store default storage engine as a string, not Sys_var_plugin.

That'd mean, that neither global nor local @@default_storage_engine
setting locks the plugin. We'll need to do a hash lookup and lock the
plugin for every statement that uses the engine. Which may be or may not
be a problem.

Here's yet another idea: don't protect plugin->ref_cnt and plugin->state
with LOCK_plugin. Then intern_plugin_lock and intern_plugin_unlock could
be completely lock-free. One would need to increment ref_cnt before or
at the same time as checking the state.  It *seems* that the rest of the
code could support that with very few changes. But it'd need more
analysys.

Regards,
Sergei