Hi, Sergey! Looks ok, see a couple of question (and minor style-related comments) below. On Dec 27, Sergey Vojtovich wrote:
At lp:maria/5.5
------------------------------------------------------------ revno: 4009 revision-id: svoj@mariadb.org-20131227101419-ahpdahcundclr6gv parent: sergii@pisem.net-20131217162654-dw2zlm3td1p12bxl committer: Sergey Vojtovich <svoj@mariadb.org> branch nick: 5.5-mdev5345 timestamp: Fri 2013-12-27 14:14:19 +0400 message: MDEV-5345 - Deadlock between mysql_change_user(), SHOW VARIABLES and INSTALL PLUGIN
There was mixed lock order between LOCK_plugin, LOCK_global_system_variables and LOCK_system_variables_hash. This patch ensures that write-lock on LOCK_system_variables_hash doesn't intersect with LOCK_plugin.
Fixed by moving initialization/deinitialization of plugin options from plugin_add()/plugin_del() to plugin_initialize()/plugin_deinitalize(). So that plugin options are handled without protection of LOCK_plugin. === added file 'mysql-test/r/plugin_vars.result' --- a/mysql-test/r/plugin_vars.result 1970-01-01 00:00:00 +0000 +++ b/mysql-test/r/plugin_vars.result 2013-12-27 10:14:19 +0000 @@ -0,0 +1,22 @@ +# +# MDEV-5345 - Deadlock between mysql_change_user(), SHOW VARIABLES and +# INSTALL PLUGIN +# +CREATE PROCEDURE p_install(x INT) +BEGIN +DECLARE CONTINUE HANDLER FOR 1126 BEGIN END; +WHILE x DO +SET x= x - 1; +INSTALL PLUGIN no_such_plugin SONAME 'no_such_object'; +END WHILE; +END| +CREATE PROCEDURE p_show_vars(x INT) +WHILE x DO +SET x= x - 1; +SHOW VARIABLES; +END WHILE| +CALL p_install(100);; +CALL p_show_vars(100);;
Note two semicolons at the end. The mysqltest syntax is either builtin_command arguments; or --builtin_command arguments I mean, it either ends with a semicolon or starts with dash-dash. Not a big deal, of course :)
+USE test; +DROP PROCEDURE p_install; +DROP PROCEDURE p_show_vars;
=== added file 'mysql-test/t/plugin_vars.test' --- a/mysql-test/t/plugin_vars.test 1970-01-01 00:00:00 +0000 +++ b/mysql-test/t/plugin_vars.test 2013-12-27 10:14:19 +0000 @@ -0,0 +1,56 @@ +--echo # +--echo # MDEV-5345 - Deadlock between mysql_change_user(), SHOW VARIABLES and +--echo # INSTALL PLUGIN +--echo # + +# Prepare test +delimiter |; +CREATE PROCEDURE p_install(x INT) +BEGIN + DECLARE CONTINUE HANDLER FOR 1126 BEGIN END; + WHILE x DO + SET x= x - 1; + INSTALL PLUGIN no_such_plugin SONAME 'no_such_object'; + END WHILE; +END| + +CREATE PROCEDURE p_show_vars(x INT) +WHILE x DO + SET x= x - 1; + SHOW VARIABLES; +END WHILE| +delimiter ;| + +connect(con1, localhost, root,,); +connect(con2, localhost, root,,); + +# Start test +connection con1; +--send CALL p_install(100); + +connection con2; +--send CALL p_show_vars(100);
Why did you prefer this test over a deterministic one with debug sync points?
+connection default; + +disable_result_log; +let $i= 100; +while ($i) +{ + change_user; + dec $i; +} + +# Cleanup +connection con1; +reap; +connection con2; +reap; +connection default; +enable_result_log; + +disconnect con1; +disconnect con2; +USE test; +DROP PROCEDURE p_install; +DROP PROCEDURE p_show_vars;
=== modified file 'sql/sql_plugin.cc' --- a/sql/sql_plugin.cc 2013-12-12 17:14:08 +0000 +++ b/sql/sql_plugin.cc 2013-12-27 10:14:19 +0000 @@ -201,7 +202,7 @@ static bool initialized= 0; write-lock on LOCK_system_variables_hash is required before modifying the following variables/structures */ -static MEM_ROOT plugin_mem_root; +static MEM_ROOT plugin_vars_mem_root;
ok, so you couldn't consistently use one lock for plugin_mem_root, and had to create a second memroot. I see. another option would be to use one memroot and a dedicated lock for it. but I don't think it matters much what solution we use here :)
static uint global_variables_dynamic_size= 0; static HASH bookmark_hash;
@@ -1342,7 +1337,8 @@ void plugin_unlock_list(THD *thd, plugin }
-static int plugin_initialize(struct st_plugin_int *plugin) +static int plugin_initialize(MEM_ROOT *tmp_root, struct st_plugin_int *plugin, + int *argc, char **argv, bool initialize)
this looks pretty weird. plugin_initialize() function that doesn't initialize if its initialize argument is false? perhaps it's better to rename the function then? Or create another one that tests plugin options and invokes plugin_initialize() if necessary?
{ int ret= 1; DBUG_ENTER("plugin_initialize"); @@ -1413,6 +1421,14 @@ static int plugin_initialize(struct st_p ret= 0;
err: + if (ret) + { + mysql_rwlock_wrlock(&LOCK_system_variables_hash); + mysql_del_sys_var_chain(plugin->system_vars); + mysql_rwlock_unlock(&LOCK_system_variables_hash); + restore_pluginvar_names(plugin->system_vars);
may be, you'd move these four lines in a separate function? like, cleanup_plugin_sysvars() ?
+ } + mysql_mutex_lock(&LOCK_plugin); plugin->state= state;
@@ -1646,9 +1654,10 @@ int plugin_init(int *argc, char **argv, for (i= 0; i < plugin_array.elements; i++) { plugin_ptr= *dynamic_element(&plugin_array, i, struct st_plugin_int **); - if (plugin_ptr->state == PLUGIN_IS_UNINITIALIZED) + if (plugin_ptr->plugin_dl && plugin_ptr->state == PLUGIN_IS_UNINITIALIZED)
Why?
{ - if (plugin_initialize(plugin_ptr)) + if (plugin_initialize(&tmp_root, plugin_ptr, argc, argv, + !(flags & PLUGIN_INIT_SKIP_INITIALIZATION))) { plugin_ptr->state= PLUGIN_IS_DYING; *(reap++)= plugin_ptr;
Regards, Sergei