maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

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