maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Sergei,

thanks for your feedback. Comments/questions inline.

On Fri, Nov 29, 2013 at 01:57:13PM +0100, Sergei Golubchik wrote:
> 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.
Right and I plan to do something about it later, when MDEV-5345 is fixed.
Anyway currently there should be no deadlock here, since it doesn't conflict
with write-lock order.

> 
> > "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 :(
It should be quite easy: for INSTALL PLUGIN we just move test_plugin_options()
to plugin_initialize() while plugin state is PLUGIN_IS_UNINITIALIZED. Same for
UNINSTALL PLUGIN.

Did I miss something important?

> 
> > - 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.
Right, I can't say if it may or may not be a problem yet.

> 
> 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.
Well, lock-free ref_count is not a problem. Lock-free state is challenging,
but looks doable. What about reap_plugins() which is called by plugin_unlock()?

Thanks,
Sergey