[Maria-developers] MDEV-5345 - Deadlock between mysql_change_user(), SHOW VARIABLES and INSTALL PLUGIN
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. "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. - Store default storage engine as a string, not Sys_var_plugin. Thanks, Sergey
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
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
Hi, Sergey! On Dec 04, Sergey Vojtovich wrote:
"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.
Okay. Looks like it'll work.
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()?
reap_plugins() would need a lock, of course. No lock for intern_plugin_unlock() means that it might mark reap_needed=true, but before the plugin is reaped, another thread might lock it again and reap_plugins() will do nothing. Not a big deal. Still, there're race conditions around intern_plugin_lock() - when a plugin gets uninstalled and a new plugin is installed and intern_plugin_lock() doesn't notice it. I *think* we can solve all that, looks doable, indeed. But I'd rather prefer a simple solution now, if you can move test_plugin_options() as you want to. We can do lockless plugin reference counting later, when LOCK_plugin will be a performance bottleneck. Regards, Sergei
participants (2)
-
Sergei Golubchik -
Sergey Vojtovich