maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxxxx-20131227101419-ahpdahcundclr6gv
> parent: sergii@xxxxxxxxx-20131217162654-dw2zlm3td1p12bxl
> committer: Sergey Vojtovich <svoj@xxxxxxxxxxx>
> 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