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 _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
