- developers - lists.mariadb.org

Re: [Maria-developers] Rev 4009: MDEV-5345 - Deadlock between mysql_change_user(), SHOW VARIABLES
by Sergei Golubchik 27 Jan '14

27 Jan '14

Hi, Sergey! Ok to push. A couple of comments below: On Jan 27, Sergey Vojtovich wrote: > revno: 4009 > revision-id: svoj(a)mariadb.org-20140127090430-g63lt751yxw9ddud > parent: sergii(a)pisem.net-20131217162654-dw2zlm3td1p12bxl > committer: Sergey Vojtovich <svoj(a)mariadb.org> > branch nick: 5.5-mdev5345 > timestamp: Mon 2014-01-27 13:04:30 +0400 > message: > MDEV-5345 - Deadlock between mysql_change_user(), SHOW VARIABLES and > INSTALL PLUGIN > > === modified file 'sql/sql_plugin.cc' > --- a/sql/sql_plugin.cc 2013-12-12 17:14:08 +0000 > +++ b/sql/sql_plugin.cc 2014-01-27 09:04:30 +0000 > @@ -1196,6 +1190,9 @@ static void plugin_deinitialize(struct s > if (ref_check && plugin->ref_count) > sql_print_error("Plugin '%s' has ref_count=%d after deinitialization.", > plugin->name.str, plugin->ref_count); > + > + mysql_del_sys_var_chain(plugin->system_vars); > + restore_pluginvar_names(plugin->system_vars); Can mysql_del_sys_var_chain() be called from inside restore_pluginvar_names() ? They seem always to be called together. > @@ -1342,7 +1335,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 options_only) > { I thought you'll rename a function :) But that's just fine too, whatever you prefer. Regards, Sergei

1 0

Re: [Maria-developers] MDEV-5345 - Deadlock between mysql_change_user(), SHOW VARIABLES
by Sergei Golubchik 27 Jan '14

27 Jan '14

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(a)mariadb.org-20131227101419-ahpdahcundclr6gv > parent: sergii(a)pisem.net-20131217162654-dw2zlm3td1p12bxl > committer: Sergey Vojtovich <svoj(a)mariadb.org> > 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

2 4

Re: [Maria-developers] Please review MDEV-5458 RQG hits 'sql/tztime.cc:799: my_time_t sec_since_epoch(int, int, int, int, int, int): Assertion `mon > 0 && mon < 13' failed.'
by Sergei Golubchik 27 Jan '14

27 Jan '14

Hi, Alexander! First, please cc: such requests to maria-developers@ instead. Thanks! On Jan 22, Alexander Barkov wrote: > > Please review a patch for a crashing bug MDEV-5458. > > Item_func_min_max::get_date() did not always honor the flags > given (such as NO_ZERO_IN_DATE and NO_ZERO_DATE) > in could erroneously return a not-NULL value instead of NULL. > > The patch moves the call for check_date_with_warn() > *AFTER* conversion from DATETIME to TIME, to make > sure that the returned value always matches the requested > flags. > > Previously it was done before conversion. The value before > conversion could be OK for the flags given, but the value > after conversion could be not OK. > > Also, I added a new function adjust_time_range_with_warn() > to avoid duplicate code. Reused it in three places. All ok. Just one comment: > === modified file 'sql/mysql_priv.h' > --- sql/mysql_priv.h 2013-12-02 11:09:34 +0000 > +++ sql/mysql_priv.h 2014-01-22 14:53:38 +0000 > @@ -2582,6 +2582,7 @@ check_date(const MYSQL_TIME *ltime, ulon > } > bool check_date_with_warn(const MYSQL_TIME *ltime, uint fuzzy_date, > timestamp_type ts_type); > +bool adjust_time_range_with_warn(MYSQL_TIME *ltime, uint dec); > int test_if_number(char *str,int *res,bool allow_wildcards); > void change_byte(uchar *,uint,char,char); > bool init_read_record(READ_RECORD *info, THD *thd, TABLE *reg_form, This is 5.3, right? Take care when merging to 5.5, there's no mysql_priv.h there, only sql_priv.h and this declaration should not end up there, but in sql_time.h Regards, Sergei

2 1

Re: [Maria-developers] Rev 4019: MDEV-5505: Assertion `! is_set()' fails on PREPARE SELECT with out of range in GROUP BY
by Sergei Golubchik 26 Jan '14

26 Jan '14

Hi, Sanja! On Jan 23, sanja(a)askmonty.org wrote: > === modified file 'sql/sql_select.cc' > --- a/sql/sql_select.cc 2014-01-21 13:27:36 +0000 > +++ b/sql/sql_select.cc 2014-01-23 12:50:54 +0000 > @@ -20463,7 +20463,8 @@ find_order_in_list(THD *thd, Item **ref_ > if (!order_item->fixed && > (order_item->fix_fields(thd, order->item) || > (order_item= *order->item)->check_cols(1) || > - thd->is_fatal_error)) > + thd->is_fatal_error || > + thd->is_error())) > return TRUE; /* Wrong field. */ Why not simply > - thd->is_fatal_error)) > + thd->is_error())) can there be a fatal error that is not an error? :) Regards, Sergei

1 0

Re: [Maria-developers] [Commits] Rev 3965: MDEV-5403 - Reduce usage of LOCK_open: tc_count in lp:maria/10.0
by Kristian Nielsen 24 Jan '14

24 Jan '14

Sergey Vojtovich <svoj(a)mariadb.org> writes: > +#ifdef __cplusplus > +template <typename T, int S> class Atomic_type_triat; > + > + > +template <typename T> class Atomic_type_triat<T, 4> > +template <typename T> class Atomic_type_triat<T, 8> > +template <typename T, typename S= Atomic_type_triat<T, sizeof(T)> > class Atomic > + T operator++() { return add(1) + 1; } [lots and lots of more template code snipped] I am vetoing this. We really, _really_ do not want to package up the atomic stuff in even more abstractions and API layers than we already have. Atomics and lock-free stuff is in itself plenty complicated. It is crucial that one can at least see easily in the code what is going on, without also having to spend effort to understand multiple layers of wrapping. As far as I can see, the only thing the templates do is hide stuff (like the data type used and the wrapping in locks on platforms without atomic support?), while what we need is to make those details clear to people reading the code, not to hide them! - Kristian.

3 5

[Maria-developers] benchmarking next steps
by Axel Schwenke 23 Jan '14

23 Jan '14

Hi, for those of you who didn't attend the Barcelona meeting last week, here are the plans for next steps in benchmarking: 1. top priority is automatic running of DBT3 for each new release of MariaDB-10.0 and MySQL-5.6. Probably also for MariaDB-5.5. I'll send out another mail to pinpoint DBT3 conditions (i.e. which table statistics to use etc.) 2. multithreaded replication slave is now in a condition that it can and should be benchmarked 3. the problem pointed out by Yoshinori in http://yoshinorimatsunobu.blogspot.de/2013/12/single-thread-performance-reg… must be checked for MariaDB. Monty announced that he will commit a patch this week that should help with that problem. 4. for the NUMA scalability problem, Kristian will try to find the definitive reason with profiling tools. Besides that ad-hoc testing of new patches from Svoj continues as before. 5. Galera benchmarking will be done by the Galera team for now. XL

5 8

[Maria-developers] MariaDB 10.1 tree
by Sergei Petrunia 22 Jan '14

22 Jan '14

Hi, Unless there are any objections, I am going to create a 10.1 tree on Launchpad. It will be based on current 10.0. BR Sergei -- Sergei Petrunia, Software Developer MariaDB | Skype: sergefp | Blog: http://s.petrunia.net/blog

2 3

[Maria-developers] packaging
by Leif Walsh 21 Jan '14

21 Jan '14

Hi all, How do you make debian and rpm packages for mariadb? Which are the "blessed" config files and how is the process automated (and how might I run it manually)? -- Cheers, Leif

2 2

Re: [Maria-developers] MDEV-5419 no audit events for warnings converted to errors in the strict mode
by Sergei Golubchik 21 Jan '14

21 Jan '14

Hi, Holyfoot! On Dec 15, holyfoot(a)askmonty.org wrote: > revno: 4005 > revision-id: holyfoot(a)askmonty.org-20131215112825-1a5p7fa6mwdoin26 > parent: sergii(a)pisem.net-20131213120038-hjea4r5zp7sjuq8l > committer: Alexey Botchkov <holyfoot(a)askmonty.org> > branch nick: mdev-5419 > timestamp: Sun 2013-12-15 15:28:25 +0400 > message: > MDEV-5419 no audit events for warnings converted to errors in the strict mode. > Plugins get error notifications only when my_message_sql() is called. > But errors are launched with THD::raise_condition() calls in other > places. These are push_warning(), implementations of SIGNAL and > RESIGNAL commands. > So it makes sence to notify plugins there in THD::raise_condition(). Looks ok. Please add test cases for MDEV-5419 and for MDEV-4846. Then ok to push. Regards, Sergei

1 0

Re: [Maria-developers] MDEV-5364 mysql_upgrade should be more verbose
by Sergei Golubchik 21 Jan '14

21 Jan '14

Hi, Holyfoot! On Dec 15, holyfoot(a)askmonty.org wrote: > At file:///home/hf/wmar/mdev-5364/ > > ------------------------------------------------------------ > revno: 4005 > revision-id: holyfoot(a)askmonty.org-20131215104419-ks2enrs2tds0z4ti > parent: sergii(a)pisem.net-20131213120038-hjea4r5zp7sjuq8l > committer: Alexey Botchkov <holyfoot(a)askmonty.org> > branch nick: mdev-5364 > timestamp: Sun 2013-12-15 14:44:19 +0400 > message: > MDEV-5364 mysql_upgrade should be more verbose. > If the check_version_match() fails during the mysql_upgrade > execution, it doesn't print the 'mysql' output. > So the printing of the output added to the check_version_match() > and also to the get_upgrade_info_file_name() functions. Looks ok, but I'd prefer to see a test case too. Can you please add it? Ok to push with a test case. Regards, Sergei

1 0