maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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