27 Jan
2014
27 Jan
'14
9:32 a.m.
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