Re: [Maria-developers] MDEV-5345 - Deadlock between mysql_change_user(), SHOW VARIABLES
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
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 <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 :) 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
Hi, Sergey! On Jan 22, Sergey Vojtovich wrote:
+ +# 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.
Generally, debug sync tests should be preferred, as they're deterministic and usually much faster. If this test is very fast already, you can keep it, if you'd like.
=== 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.
It cannot, if no other lock is ever taken under LOCK_plugin_mem_root. And that's exactly what I meant: mysql_mutex_lock(&LOCK_plugin_mem_root); alloc_root(); mysql_mutex_unlock(&LOCK_plugin_mem_root); with this usage pattern it's safe. But again, I don't see why this would be significantly better (or worse) than what you did. Your choice.
I'd suggest to remove plugin_mem_root at all. We can store plugin structures on dynamic arrays instead (instead of storing just pointers).
Hmm. Interesting. In a separate changeset then. Can you commit it and let me see?
@@ -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.
Up to you.
@@ -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()?
As you like. The point is - they shouldn't be duplicated in other function. Regards, Sergei
Hi Sergei, I will submit updated patch soon. Comments inline. On Sun, Jan 26, 2014 at 09:40:33PM +0100, Sergei Golubchik wrote: > Hi, Sergey! > > On Jan 22, Sergey Vojtovich wrote: > > > > + > > > > +# 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. > > Generally, debug sync tests should be preferred, as they're deterministic and > usually much faster. If this test is very fast already, you can keep it, > if you'd like. I attempted to create debug sync test, but it seems to be impossible: 1. mysql_change_user() resets debug_sync, thus we can't be certain if it acquired first lock or not 2. SET debug_sync='...' needs LOCK_open while performing find_sys_var() which adds a restriction that INSTALL PLUGIN must be executed last (not that big problem though). I kept original test case. Please let me know if you an have idea how to make debug_sync test. > > > > > === 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. > > It cannot, if no other lock is ever taken under LOCK_plugin_mem_root. > And that's exactly what I meant: > > mysql_mutex_lock(&LOCK_plugin_mem_root); > alloc_root(); > mysql_mutex_unlock(&LOCK_plugin_mem_root); > > with this usage pattern it's safe. > But again, I don't see why this would be significantly better (or worse) > than what you did. Your choice. You're right. > > > I'd suggest to remove plugin_mem_root at all. We can store plugin structures > > on dynamic arrays instead (instead of storing just pointers). > > Hmm. Interesting. In a separate changeset then. > Can you commit it and let me see? Yep, I will submit another patch some time soon. ...skip... Thanks, Sergey
Sergei, On Mon, Jan 27, 2014 at 12:32:19PM +0400, Sergey Vojtovich wrote: ...skip...
I'd suggest to remove plugin_mem_root at all. We can store plugin structures on dynamic arrays instead (instead of storing just pointers).
Hmm. Interesting. In a separate changeset then. Can you commit it and let me see? Yep, I will submit another patch some time soon. It came to be not as simple as I thought. Inserting new item to array may call realloc, which may invalidate all previously acquired plugin pointers.
Regards, Sergey
participants (2)
-
Sergei Golubchik -
Sergey Vojtovich