maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

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@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 :)
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?
Skip built-in plugins which are already initialized. This is needed due to
change in previous hunk:

> @@ -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);
>    }
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