Hi Sergey, thanks for your review. Answers inline. On Wed, Jan 22, 2014 at 06:51:11PM +0100, Sergei Golubchik wrote:

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 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 :) You're right, I'll fix it.

...

+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? The only good reason is to save some time: we already have a test case, which is not that heavy and quite stable (it never succeeded with unpatched version). I can make debug sync test if you like.

...

+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.

Correct.

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 :)

It will make indirect relationship between locks again, like: LOCK_plugin -> LOCK_plugin_mem_root LOCK_system_variables_hash -> LOCK_plugin_mem_root It may (or may not) create a deadlock again. I'd suggest to remove plugin_mem_root at all. We can store plugin structures on dynamic arrays instead (instead of storing just pointers).

...

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?

I'd prefer to keep all initialization specific logics in one function, so caller won't have to bother much about it. So I'd vote for rename. Please let me know if you have other preference.

...

{ 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() ?

What about moving them to mysql_del_sys_var_chain()?

...

+ } + 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?

@@ -1627,14 +1638,11 @@ int plugin_init(int *argc, char **argv, if (!(flags & PLUGIN_INIT_SKIP_DYNAMIC_LOADING)) { if (opt_plugin_load) - plugin_load_list(&tmp_root, argc, argv, opt_plugin_load); + plugin_load_list(&tmp_root, opt_plugin_load); if (!(flags & PLUGIN_INIT_SKIP_PLUGIN_TABLE)) - plugin_load(&tmp_root, argc, argv); + plugin_load(&tmp_root); }

Skip built-in plugins which are already initialized. This is needed due to change in previous hunk: plugin_load_list()/plugin_load() do not initialize plugin options anymore...

- if (flags & PLUGIN_INIT_SKIP_INITIALIZATION) - goto end; -

...so we need to call plugin_initialize() indepently of flags.

/* Now we initialize all remaining plugins */

...

{ - 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

Thanks, Sergey