- developers - lists.mariadb.org

[Maria-developers] Unsubscribe
by TINKU BANSAL 11 Oct '20

11 Oct '20

Hi there, Please Unsubscribe me to this mailing list. Thanks

2 1

Re: [Maria-developers] 62c35fe93e1: MDEV-15703 Crash in EXECUTE IMMEDIATE 'CREATE OR REPLACE TABLE t1 (a INT DEFAULT ?)' USING DEFAULT
by Sergei Golubchik 10 Oct '20

10 Oct '20

Hi, Oleksandr! On Oct 09, Oleksandr Byelkin wrote: > revision-id: 62c35fe93e1 (mariadb-10.2.31-487-g62c35fe93e1) > parent(s): f584d567dbb > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2020-10-07 13:43:39 +0200 > message: > > MDEV-15703 Crash in EXECUTE IMMEDIATE 'CREATE OR REPLACE TABLE t1 (a INT DEFAULT ?)' USING DEFAULT > > part 2: > > - check that expressions are evaluable for > making empty row and assigning PS variable > > - correctly handling writing to a temporary tabe during multi-update > by setting associated field > > - Item::raise_error_not_evaluable() bakported from 10.4 and made vitrual > > - Item::is_evaluable_expression() bakported from 10.4 > > - Item::check_is_evaluable_expression_or_error() bakported from 10.4 I still don't quite understand all details of your patch. But here're more questions and comments below: > diff --git a/sql/field.cc b/sql/field.cc > index bdaaecc2026..8ce412a207d 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -11183,6 +11205,16 @@ bool Field::save_in_field_default_value(bool view_error_processing) > { > THD *thd= table->in_use; > > + if ( // table is not opened properly > + table == NULL || table->pos_in_table_list == NULL || > + table->pos_in_table_list->select_lex == NULL || > + // we are in subquery/derived/CTE > + !table->pos_in_table_list->top_table()->select_lex->is_top_level_select()) > + { > + DBUG_ASSERT(0); // shoud not happened > + my_error(ER_INVALID_DEFAULT_PARAM, MYF(0)); > + return false; > + } I'd still think it'd be better to do a series of asserts. a complex condition to check for something that cannot ever happen? > if (flags & NO_DEFAULT_VALUE_FLAG && > real_type() != MYSQL_TYPE_ENUM) > { > @@ -11221,13 +11253,30 @@ bool Field::save_in_field_default_value(bool view_error_processing) > > bool Field::save_in_field_ignore_value(bool view_error_processing) > { > - enum_sql_command com= table->in_use->lex->sql_command; > - // All insert-like commands > - if (com == SQLCOM_INSERT || com == SQLCOM_REPLACE || > - com == SQLCOM_INSERT_SELECT || com == SQLCOM_REPLACE_SELECT || > - com == SQLCOM_LOAD) > - return save_in_field_default_value(view_error_processing); > - return 0; // ignore > + if ( // table is not opened properly > + table == NULL || table->pos_in_table_list == NULL || > + table->pos_in_table_list->select_lex == NULL || > + // we are in subquery/derived/CTE > + !table->pos_in_table_list->top_table()->select_lex->is_top_level_select()) > + { > + DBUG_ASSERT(0); // shoud not happened > + my_error(ER_INVALID_DEFAULT_PARAM, MYF(0)); > + return false; > + } ditto > + switch (find_ignore_reaction(table->in_use)) > + { > + case IGNORE_MEANS_DEFAULT: > + return save_in_field_default_value(view_error_processing); > + case IGNORE_MEANS_FIELD_VALUE: > + return 0; // ignore > + default: > + ; // fall through to error > + } > + > + // unexpected command > + DBUG_ASSERT(0); // shoud not happened > + my_error(ER_INVALID_DEFAULT_PARAM, MYF(0)); > + return false; > } > > > diff --git a/sql/item.cc b/sql/item.cc > index 994d45a9dc3..401ee5d6be2 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -84,6 +85,17 @@ void item_init(void) > } > > > +void Item::raise_error_not_evaluable() > +{ > + String tmp; > + this->print(&tmp, QT_ORDINARY); > + // TODO-10.5: add an error message to errmsg-utf8.txt > + my_printf_error(ER_UNKNOWN_ERROR, > + "'%s' is not allowed in this context", MYF(0), tmp.ptr()); > + DBUG_ASSERT(0); // cannot be happen before 10.5 > +} > + > + better remove it, this dead code won't simplify merges. In 10.3 it'll be added with a merge normally. having check_is_evaluable_expression_or_error() makes sense though, it is used and copying it from 10.3 will help to merge indeed or, may be, just backport the patch from 10.3 to 10.2? > void Item::push_note_converted_to_negative_complement(THD *thd) > { > push_warning(thd, Sql_condition::WARN_LEVEL_NOTE, ER_UNKNOWN_ERROR, > @@ -8968,18 +9084,26 @@ bool Item_default_value::fix_fields(THD *thd, Item **items) > def_field->move_field_offset((my_ptrdiff_t) > (def_field->table->s->default_values - > def_field->table->record[0])); > - set_field(def_field); > - return FALSE; > + return def_field; > +} > > -error: > - context->process_error(thd); > - return TRUE; > +bool Item_default_value::associate_with_target_field(THD *thd, > + Item_field *field) > +{ > + associated= true; > + arg= field; iirc, `arg` used to tell the difference between DEFAULT and DEFAULT(x). how are you doing it now? > + return tie_field(thd); > } > > void Item_default_value::cleanup() > { > delete cached_field; // Free cached blob data > cached_field= 0; > + if (associated) > + { > + arg= NULL; > + associated= false; > + } > Item_field::cleanup(); > } > > @@ -9102,8 +9226,54 @@ void Item_ignore_value::print(String *str, enum_query_type query_type) > str->append(STRING_WITH_LEN("ignore")); > } > > + > +bool Item_ignore_value::associate_with_target_field(THD *thd, > + Item_field *field) > +{ > + bitmap_set_bit(field->field->table->read_set, field->field->field_index); > + return Item_default_value::associate_with_target_field(thd, field); > +} > + > + > int Item_ignore_value::save_in_field(Field *field_arg, bool no_conversions) > { > + if (arg) how is IGNORE with arg!=0 differs from IGNORE with arg==0? when happens what? > + { > + switch (find_ignore_reaction(field->table->in_use)) > + { > + case IGNORE_MEANS_DEFAULT: > + DBUG_ASSERT(0); // impossible now, but fully working code if needed > + /* > + save default value > + Note: (((Field*)(arg()->real_item()) is Item_field* (checked in > + tie_field(). > + */ > + if ((((Item_field*)(arg->real_item()))->field->flags & > + NO_DEFAULT_VALUE_FLAG)) > + { > + my_error(ER_NO_DEFAULT_FOR_FIELD, MYF(0), > + ((Item_field*)(arg->real_item()))->field->field_name); > + return true; > + } > + calculate(); > + return Item_field::save_in_field(field_arg, no_conversions); > + case IGNORE_MEANS_FIELD_VALUE: > + // checked on tie_field > + DBUG_ASSERT(arg->real_item()->type() == FIELD_ITEM); > + /* > + save original field (Item::save_in_field is not applicable because > + result_field for the referenced field set to this temporary table). > + */ > + arg->save_val(field_arg); > + return false; > + default: > + ; // fall through to error > + } > + DBUG_ASSERT(0); //impossible > + my_error(ER_INVALID_DEFAULT_PARAM, MYF(0)); > + return true; > + } > + > return field_arg->save_in_field_ignore_value(context->error_processor == > &view_error_processor); > } > diff --git a/sql/sp_head.cc b/sql/sp_head.cc > index 6a650183fb8..41a27a3aaaf 100644 > --- a/sql/sp_head.cc > +++ b/sql/sp_head.cc > @@ -418,6 +418,9 @@ sp_eval_expr(THD *thd, Field *result_field, Item **expr_item_ptr) > if (!(expr_item= sp_prepare_func_item(thd, expr_item_ptr))) > goto error; > > + if (expr_item->check_is_evaluable_expression_or_error()) > + goto error; for CALL(?) ? > + > /* > Set THD flags to emit warnings/errors in case of overflow/type errors > during saving the item into the field. > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index cc77b58cb3e..b52d6f96a84 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -8345,6 +8346,9 @@ fill_record(THD *thd, TABLE *table, Field **ptr, List<Item> &values, > field->field_name, table->s->table_name.str); > } > > + if(evaluable && value->check_is_evaluable_expression_or_error()) > + goto err; Is it a good idea to do this check constantly for every user and for every row in selects, insert/update, etc? Could it be done inside IGNORE/DEFAULT? For example, by checking if field->table->tmp_table == INTERNAL_TMP_TABLE. > + > if (use_value) > value->save_val(field); > else > diff --git a/sql/unireg.cc b/sql/unireg.cc > index 083960523c1..91f9073c7ef 100644 > --- a/sql/unireg.cc > +++ b/sql/unireg.cc > @@ -1002,6 +1002,12 @@ static bool make_empty_rec(THD *thd, uchar *buff, uint table_options, > > int res= !expr->fixed && // may be already fixed if ALTER TABLE > expr->fix_fields(thd, &expr); > + if (!res && expr->check_is_evaluable_expression_or_error()) is this for CREATE TABLE case? > + { > + error= 1; > + delete regfield; //To avoid memory leak > + goto err; > + } > if (!res) > res= expr->save_in_field(regfield, 1); > if (!res && (field->flags & BLOB_FLAG)) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 56f2b3d12b6: MDEV-23264 Unique blobs allow duplicate values upon UPDATE
by Sergei Golubchik 08 Oct '20

08 Oct '20

Hi, Sachin! I suspect is_null() could be wrong here. Please, add a test case where a table has a BEFORE UPDATE trigger and a long UNIQUE over a NOT NULL field. And see how cmp_binary_offset() is used in compare_record(), for example. On Oct 08, Sachin Setiya wrote: > revision-id: 56f2b3d12b6 (mariadb-10.4.11-385-g56f2b3d12b6) > parent(s): af834c218a5 > author: Sachin Setiya <sachin.setiya(a)mariadb.com> > committer: Sachin Setiya <sachin.setiya(a)mariadb.com> > timestamp: 2020-09-14 20:08:18 +0100 > message: > > MDEV-23264 Unique blobs allow duplicate values upon UPDATE > > Problem:- > We are able to insert duplicate value in table because cmp_binary_offset > is not able to differentiate between NULL and empty string. So > check_duplicate_long_entry_key is never called and we don't check for > duplicate. > Solution > Added a if condition with is_null() on field which can differentiate > between NULL and empty string. > > diff --git a/sql/handler.cc b/sql/handler.cc > index 40dea349272..71e6a904f23 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -6699,8 +6699,13 @@ static int check_duplicate_long_entries_update(TABLE *table, handler *h, uchar * > for (uint j= 0; j < key_parts; j++, keypart++) > { > field= keypart->field; > - /* Compare fields if they are different then check for duplicates*/ > - if(field->cmp_binary_offset(reclength)) > + /* > + Compare fields if they are different then check for duplicates > + cmp_binary_offset cannot differentiate between null and empty string > + So also check for that too > + */ > + if((field->is_null(0) != field->is_null(reclength)) || > + field->cmp_binary_offset(reclength)) > { > if((error= check_duplicate_long_entry_key(table, table->update_handler, > new_rec, i))) > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] Review of MDEV-4851
by Vicențiu Ciorbaru 07 Oct '20

07 Oct '20

Hi Daniel! The patch looks good, some comments about the test cases. About the commit message, we have --write-binlog not just --binlog. Also, check for typos in the last commit message and also, I would squash the two test cases commits into just one. > diff --git a/mysql-test/suite/rpl/t/rpl_mysql_upgrade.test b/mysql-test/suite/rpl/t/rpl_mysql_upgrade.test > index 1931e2eab2a..b836e889429 100644 > --- a/mysql-test/suite/rpl/t/rpl_mysql_upgrade.test > +++ b/mysql-test/suite/rpl/t/rpl_mysql_upgrade.test > @@ -14,8 +14,8 @@ call mtr.add_suppression("table or database name 'mysqltest-1'"); > > connection master; > --disable_warnings > -DROP DATABASE IF EXISTS `#mysql50#mysqltest-1`; > -CREATE DATABASE `#mysql50#mysqltest-1`; > +DROP DATABASE IF EXISTS `mysqltest-1`; > +CREATE DATABASE `mysqltest-1`; Since you are doing cleanup here anyway, let's get rid of the DROP statements a test case should not leave any dirty context behind and if we drop things just to be sure, we are masking potential problems. > --enable_warnings > sync_slave_with_master; > > @@ -34,13 +34,20 @@ if ($before_position == $after_position) > echo Master position is not changed; > } > > -#Some log events of the mysql_upgrade's will cause errors on slave. > +# Some log events of the mysql_upgrade previously caused errors on slave, > +# however with MDEV-4851 this should be ok, so we test it: > connection slave; > -STOP SLAVE SQL_THREAD; > -source include/wait_for_slave_sql_to_stop.inc; > +SET @old_general_log_state = @@global.general_log; > +SET @old_slow_log_state = @@global.slow_query_log; > +SET @old_log_output = @@global.log_output; > +SET GLOBAL general_log = 'ON'; > +SET GLOBAL slow_query_log = 'ON'; > +SET GLOBAL log_output = 'FILE'; > > connection master; > #With '--force' option, mysql_upgrade always executes all sql statements for upgrading. > +ALTER TABLE mysql.slow_log DROP COLUMN thread_id, DROP COLUMN rows_affected; > +DROP DATABASE IF EXISTS `mysqltest-1`; Why IF EXISTS? It *should* be there already. Can you add some syncs between master and slave and then show from the slave connection that the log table table is missing those columns and also that they get applied *after* mysql_upgrade is run? > --exec $MYSQL_UPGRADE --skip-verbose --write-binlog --force --user=root > $MYSQLTEST_VARDIR/log/mysql_upgrade.log 2>&1 > > let $datadir= `select @@datadir`; > @@ -55,8 +62,14 @@ if ($before_position != $after_position) > echo Master position has been changed; > } > > -DROP DATABASE `mysqltest-1`; Notice that the original code didn't use "IF EXISTS". > +sync_slave_with_master; > connection slave; > -DROP DATABASE `#mysql50#mysqltest-1`; > ---let $rpl_only_running_threads= 1 > +SET GLOBAL general_log = 'OFF'; > +SET GLOBAL slow_query_log = 'OFF'; > +truncate mysql.slow_log; > +truncate mysql.general_log; Why do you need to clear the tables? I tested without clearing and it still needs to pass. > +SET GLOBAL general_log = @old_general_log_state; > +SET GLOBAL slow_query_log = @old_slow_log_state; > +SET GLOBAL log_output = @old_log_output; > + > --source include/rpl_end.inc > diff --git a/mysql-test/t/log_tables.test b/mysql-test/t/log_tables.test > index f822ec8d758..0196e62f12f 100644 > --- a/mysql-test/t/log_tables.test > +++ b/mysql-test/t/log_tables.test

1 0

Re: [Maria-developers] Fwd: [Commits] 244f5bd: MDEV-23811: With large number of indexes optimizer chooses an inefficient plan
by Sergey Petrunia 06 Oct '20

06 Oct '20

Hello Igor, On Mon, Oct 05, 2020 at 10:13:17PM -0700, Igor Babaev wrote: > > I've accepted all your changes for my original fix. > This is a new patch for 10.2. > ... > diff --git a/sql/opt_range.cc b/sql/opt_range.cc > index e933d2a..9f1fb9a 100644 > --- a/sql/opt_range.cc > +++ b/sql/opt_range.cc > @@ -8899,11 +8902,10 @@ tree_or(RANGE_OPT_PARAM *param,SEL_TREE > *tree1,SEL_TREE *tree2) > { > SEL_ARG *key1= tree1->keys[key_no]; > SEL_ARG *key2= tree2->keys[key_no]; > - if (!must_be_ored) > - { > + if (!must_be_ored || no_merges1) > key1->incr_refs(); > + if (!must_be_ored || no_merges2) > key2->incr_refs(); > - } > if ((result->keys[key_no]= key_or(param, key1, key2))) > result->keys_map.set_bit(key_no); > } What is the purpose of this change? As far as I understand, the bug is fixed without it, too? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] Test failure in main.flush_logs_not_windows on x32
by Otto Kekäläinen 06 Oct '20

06 Oct '20

Hello! After uploading MariaDB 10.5.5 to Debian I came across that the test main.mysqld--help is failing on arch x32: ****************** main.flush_logs_not_windows w2 [ fail ] Test ended at 2020-09-28 19:25:55 CURRENT_TEST: main.flush_logs_not_windows mysqltest: At line 10: query 'flush logs' succeeded - should have failed with errno 1004... ****************** Full log: https://people.debian.org/~jrtc27/mariadb-10.5_10.5.5-1+jrtc27.1_x32-2020-0… Any tips on what to do about this one?

1 0

[Maria-developers] Test failure in main.mysqld--help on x32
by Otto Kekäläinen 05 Oct '20

05 Oct '20

Hello! After uploading MariaDB 10.5.5 to Debian I came across that the test main.mysqld--help is failing on arch x32: ****************** main.mysqld--help 'unix' w1 [ fail ] Test ended at 2020-09-28 19:24:57 CURRENT_TEST: main.mysqld--help --- /<<PKGBUILDDIR>>/mysql-test/main/mysqld--help.result 2020-08-07 12:57:35.000000000 +0000 +++ /<<PKGBUILDDIR>>/mysql-test/main/mysqld--help.reject 2020-09-28 19:24:57.628353544 +0000 @@ -1797,10 +1797,10 @@ sync-relay-log-info 10000 sysdate-is-now FALSE system-versioning-alter-history ERROR -table-cache 421 +table-cache 2000 table-definition-cache 400 -table-open-cache 421 -table-open-cache-instances 1 +table-open-cache 2000 +table-open-cache-instances 8 tc-heuristic-recover OFF tcp-keepalive-interval 0 tcp-keepalive-probes 0 main.flush_logs_not_windows w2 [ fail ] Test ended at 2020-09-28 19:25:55 CURRENT_TEST: main.flush_logs_not_windows mysqltest: At line 10: query 'flush logs' succeeded - should have failed with errno 1004... ****************** Any tips on what to do about this? On Zulip I was tipped about https://github.com/MariaDB/server/commit/deb365581b96527c5c96b3f5ed1235e105… but it does not apply on 10.5.

2 2

[Maria-developers] Test failure in main.index_intersect on hppa
by Otto Kekäläinen 05 Oct '20

05 Oct '20

Hello! I already asked this on Zulip but did not get anywhere, so re-posting here on the mailing list. The basic test suite run directly after the build fails with the following test on hppa on Debian: https://buildd.debian.org/status/fetch.php?pkg=mariadb-10.5&arch=hppa&ver=1… ****************** main.index_intersect w3 [ fail ] Test ended at 2020-09-27 16:41:38 CURRENT_TEST: main.index_intersect --- /<<PKGBUILDDIR>>/mysql-test/main/index_intersect.result 2020-08-07 12:57:35.000000000 +0000 +++ /<<PKGBUILDDIR>>/mysql-test/main/index_intersect.reject 2020-09-27 16:41:38.035115473 +0000 @@ -715,7 +715,7 @@ SELECT * FROM City WHERE Name BETWEEN 'G' AND 'J' AND Population > 500000 AND Country LIKE 'C%'; id select_type table type possible_keys key key_len ref rows Extra -1 SIMPLE City range Population,Country,Name Name 35 NULL # Using index condition; Using where +1 SIMPLE City index_merge Population,Country,Name Name,Country 35,3 NULL # Using sort_intersect(Name,Country); Using where EXPLAIN SELECT * FROM City WHERE ID BETWEEN 1 AND 500 AND Population > 700000 AND Country LIKE 'C%'; mysqltest: Result l ****************** Any suggestions what this 'range' -> 'index_merge' word change in the result might step from?

1 0

Re: [Maria-developers] 19bdb03e97d: MDEV-21211 LOCK_plugin optimization.
by Sergei Golubchik 02 Oct '20

02 Oct '20

Hi, Alexey! Thanks, this is great! Comments are below, there aren't many. The main concern - you only do RCU when installingplugins, but not when uninstalling. I'm not sure how it's supposed to work. On Oct 02, Alexey Botchkov wrote: > revision-id: 19bdb03e97d (mariadb-10.4.11-337-g19bdb03e97d) > parent(s): 0041dacc1b8 > author: Alexey Botchkov <holyfoot(a)mariadb.com> > committer: Alexey Botchkov <holyfoot(a)mariadb.com> > timestamp: 2020-08-11 00:01:53 +0400 > message: > > MDEV-21211 LOCK_plugin optimization. > > Use Apc_target calls to 'notify' threads about changes in plugins. > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index bf33148811e..1f38c55b922 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -1348,10 +1348,10 @@ bool do_command(THD *thd) > my_net_set_read_timeout(net, thd->variables.net_read_timeout); > > DBUG_ASSERT(packet_length); > - DBUG_ASSERT(!thd->apc_target.is_enabled()); > + thd->apc_target.enable(); > return_value= dispatch_command(command, thd, packet+1, > (uint) (packet_length-1), FALSE, FALSE); > - DBUG_ASSERT(!thd->apc_target.is_enabled()); > + thd->apc_target.disable(); is this because you don't want to post apc requests to threads that wait for the client? > > out: > thd->lex->restore_set_statement_var(); > diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc > index 981420f03ae..84438e3ff70 100644 > --- a/sql/sql_plugin.cc > +++ b/sql/sql_plugin.cc > @@ -217,17 +217,28 @@ static struct > #include "sql_plugin_services.ic" > > /* > - A mutex LOCK_plugin must be acquired before accessing the > - following variables/structures. > - We are always manipulating ref count, so a rwlock here is unneccessary. > + A mutex LOCK_plugin must be acquired when we change the > + new_plugins_state content and when we swap the global_plugins_state > + and the new_plugins_state. > */ > mysql_mutex_t LOCK_plugin; > +struct st_plugins_state > +{ > + DYNAMIC_ARRAY m_array; > + HASH m_hash[MYSQL_MAX_PLUGIN_TYPE_NUM]; > +#ifndef DBUG_OOF DBUG_OFF > + uint m_ref_counter; > +#endif > +}; > + > static DYNAMIC_ARRAY plugin_dl_array; > -static DYNAMIC_ARRAY plugin_array; > -static HASH plugin_hash[MYSQL_MAX_PLUGIN_TYPE_NUM]; > static MEM_ROOT plugin_mem_root; > static bool reap_needed= false; > volatile int global_plugin_version= 1; > +static st_plugins_state plugins_states[2]; > +st_plugins_state *global_plugins_state= plugins_states; it seems this can be static too, you don't use it outside of sql_plugin.cc (and making it global_plugins_state= &plugins_states[0], new_plugins_state= &plugins_state[1]; would be a bit more explicit) > +static st_plugins_state *new_plugins_state= plugins_states + 1; > + > > static bool initialized= 0; > ulong dlopen_count; > @@ -871,7 +882,8 @@ static void plugin_dl_del(struct st_plugin_dl *plugin_dl) > } > > > -static struct st_plugin_int *plugin_find_internal(const LEX_CSTRING *name, > +static struct st_plugin_int *plugin_find_internal(st_plugins_state *st, shouldn't `st` be const here too? > + const LEX_CSTRING *name, > int type) > { > uint i; > @@ -879,21 +891,19 @@ static struct st_plugin_int *plugin_find_internal(const LEX_CSTRING *name, > if (! initialized) > DBUG_RETURN(0); > > - mysql_mutex_assert_owner(&LOCK_plugin); may be you'd want here something like if (st == new_plugins_state) mysql_mutex_assert_owner(&LOCK_plugin); > - > if (type == MYSQL_ANY_PLUGIN) > { > for (i= 0; i < MYSQL_MAX_PLUGIN_TYPE_NUM; i++) > { > struct st_plugin_int *plugin= (st_plugin_int *) > - my_hash_search(&plugin_hash[i], (const uchar *)name->str, name->length); > + my_hash_search(&st->m_hash[i], (const uchar *)name->str, name->length); > if (plugin) > DBUG_RETURN(plugin); > } > } > else > DBUG_RETURN((st_plugin_int *) > - my_hash_search(&plugin_hash[type], (const uchar *)name->str, > + my_hash_search(&st->m_hash[type], (const uchar *)name->str, > name->length)); > DBUG_RETURN(0); > } > @@ -1210,13 +1209,6 @@ static void plugin_variables_deinit(struct st_plugin_int *plugin) > > static void plugin_deinitialize(struct st_plugin_int *plugin, bool ref_check) > { > - /* > - we don't want to hold the LOCK_plugin mutex as it may cause > - deinitialization to deadlock if plugins have worker threads > - with plugin locks > - */ > - mysql_mutex_assert_not_owner(&LOCK_plugin); does it mean you *do* keep LOCK_plugin here now? > - > if (plugin->plugin->status_vars) > { > /* > @@ -1280,24 +1273,25 @@ static void plugin_del(struct st_plugin_int *plugin) > DBUG_VOID_RETURN; > } > > + > static void reap_plugins(void) > { > uint count; > struct st_plugin_int *plugin, **reap, **list; > > - mysql_mutex_assert_owner(&LOCK_plugin); > - > if (!reap_needed) > return; > > + mysql_mutex_lock(&LOCK_plugin); > reap_needed= false; > - count= plugin_array.elements; > + count= global_plugins_state->m_array.elements; > reap= (struct st_plugin_int **)my_alloca(sizeof(plugin)*(count+1)); > *(reap++)= NULL; > > for (uint i=0; i < MYSQL_MAX_PLUGIN_TYPE_NUM; i++) > { > - HASH *hash= plugin_hash + plugin_type_initialization_order[i]; > + HASH *hash= global_plugins_state->m_hash + > + plugin_type_initialization_order[i]; do you need LOCK_plugin when changing plugin states? your comment at the LOCK_plugin declaration doesn't mention it. > for (uint j= 0; j < hash->records; j++) > { > plugin= (struct st_plugin_int *) my_hash_element(hash, j); > @@ -1542,6 +1529,38 @@ static void init_plugin_psi_keys(void) > } > #endif /* HAVE_PSI_INTERFACE */ > > + > +static bool copy_plugins_state(st_plugins_state *dest, st_plugins_state *src) > +{ > + uint i; > + mysql_mutex_assert_owner ? for documentation purposes. > + reset_dynamic(&dest->m_array); > + for (i= 0; i < MYSQL_MAX_PLUGIN_TYPE_NUM; i++) > + my_hash_reset(&dest->m_hash[i]); > + > + for (i= 0; i < src->m_array.elements; i++) > + { > + struct st_plugin_int *ptr; > + ptr= *dynamic_element(&src->m_array, i, struct st_plugin_int **); > + if (insert_dynamic(&dest->m_array, (uchar *) &ptr) || > + (ptr->state != PLUGIN_IS_FREED && > + my_hash_insert(&dest->m_hash[ptr->plugin->type], > + (uchar*) ptr))) > + goto err; > + } > + > + return false; > +err: > + return true; > +} > + > + > +static void set_new_global_plugins_state() > +{ mysql_mutex_assert_owner ? for documentation purposes. > + swap_variables(st_plugins_state *, global_plugins_state, new_plugins_state); > +} > + > + > /* > The logic is that we first load and initialize all compiled in plugins. > From there we load up the dynamic types (assuming we have not been told to > @@ -1815,7 +1834,9 @@ static void plugin_load(MEM_ROOT *tmp_root) > tables.init_one_table(&MYSQL_SCHEMA_NAME, &MYSQL_PLUGIN_NAME, 0, TL_READ); > tables.open_strategy= TABLE_LIST::OPEN_NORMAL; > > + set_new_global_plugins_state(); > result= open_and_lock_tables(new_thd, &tables, FALSE, MYSQL_LOCK_IGNORE_TIMEOUT); > + set_new_global_plugins_state(); why here, why twice? > > table= tables.table; > if (result) > @@ -1851,52 +1872,31 @@ static void plugin_load(MEM_ROOT *tmp_root) > if (!name.length || !dl.length) > continue; > > - /* > - Pre-acquire audit plugins for events that may potentially occur > - during [UN]INSTALL PLUGIN. > - > - When audit event is triggered, audit subsystem acquires interested > - plugins by walking through plugin list. Evidently plugin list > - iterator protects plugin list by acquiring LOCK_plugin, see > - plugin_foreach_with_mask(). > - > - On the other hand plugin_load is acquiring LOCK_plugin > - rather for a long time. > - > - When audit event is triggered during plugin_load plugin > - list iterator acquires the same lock (within the same thread) > - second time. > - > - This hack should be removed when LOCK_plugin is fixed so it > - protects only what it supposed to protect. > - > - See also mysql_install_plugin(), mysql_uninstall_plugin() and > - initialize_audit_plugin() > - */ > - if (mysql_audit_general_enabled()) > - mysql_audit_acquire_plugins(new_thd, event_class_mask); > - \o/ > /* > there're no other threads running yet, so we don't need a mutex. > but plugin_add() before is designed to work in multi-threaded > environment, and it uses mysql_mutex_assert_owner(), so we lock > the mutex here to satisfy the assert > */ > - mysql_mutex_lock(&LOCK_plugin); > plugin_add(tmp_root, false, &name, &dl, MYF(ME_ERROR_LOG)); > free_root(tmp_root, MYF(MY_MARK_BLOCKS_FREE)); > - mysql_mutex_unlock(&LOCK_plugin); > } > if (unlikely(error > 0)) > sql_print_error(ER_THD(new_thd, ER_GET_ERRNO), my_errno, > table->file->table_type()); > + set_new_global_plugins_state(); > end_read_record(&read_record_info); > table->mark_table_for_reopen(); > close_mysql_tables(new_thd); > -end: > +err_ret: > new_thd->db= null_clex_str; // Avoid free on thd->db > delete new_thd; > + set_new_global_plugins_state(); > DBUG_VOID_RETURN; > + > +end: > + set_new_global_plugins_state(); > + goto err_ret; eh, really? you swap twice again? > } > > > @@ -2058,13 +2055,8 @@ void plugin_shutdown(void) > plugin_deinitialize(plugins[i], false); > } > > - /* > - It's perfectly safe not to lock LOCK_plugin, as there're no > - concurrent threads anymore. But some functions called from here > - use mysql_mutex_assert_owner(), so we lock the mutex to satisfy it > - */ > - mysql_mutex_lock(&LOCK_plugin); > > + mysql_mutex_lock(&LOCK_plugin); the comment was helpful, I think > /* > We defer checking ref_counts until after all plugins are deinitialized > as some may have worker threads holding on to plugin references. > @@ -2176,6 +2173,44 @@ static bool finalize_install(THD *thd, TABLE *table, const LEX_CSTRING *name, > return 0; > } > > + > +/* > + Request object for ending the new_plugins_state change. > +*/ > + > +class Ping_all_threads_request : public Apc_target::Apc_call > +{ > +public: > + /* Overloaded virtual functions. */ > + void call_in_target_thread(); > +}; > + > + > +void Ping_all_threads_request::call_in_target_thread() > +{ > + /* Do nothing. We just make sure the thread got to this point. */ > +} > + > + > +static my_bool fix_plugins_state_callback(THD *thd, THD *caller_thd) > +{ > + bool timed_out; > + int timeout_sec= 30; > + > + Ping_all_threads_request ping_req; > + > + mysql_mutex_lock(&thd->LOCK_thd_kill); > + if (thd->get_command() == COM_SLEEP) here you also skip threads waiting on the client? > + { > + mysql_mutex_unlock(&thd->LOCK_thd_kill); > + return FALSE; > + } > + thd->apc_target.make_apc_call(caller_thd, &ping_req, > + timeout_sec, &timed_out); > + return FALSE; > +} > + > + > bool mysql_install_plugin(THD *thd, const LEX_CSTRING *name, > const LEX_CSTRING *dl_arg) > { > @@ -2252,12 +2272,19 @@ bool mysql_install_plugin(THD *thd, const LEX_CSTRING *name, > > if (unlikely(error != INSTALL_GOOD)) > { > + mysql_mutex_unlock(&LOCK_plugin); > reap_needed= true; > reap_plugins(); > + goto err; > } > -err: > global_plugin_version++; > + set_new_global_plugins_state(); > + thd->apc_target.disable(); > mysql_mutex_unlock(&LOCK_plugin); > + server_threads.iterate(fix_plugins_state_callback, thd); > + thd->apc_target.enable(); a comment be good here. to explain why you disable apc and why disable/unlock/iterate must happen in this particlar order. > + > +err: > if (argv) > free_defaults(argv); > DBUG_RETURN(error == INSTALL_FAIL_NOT_OK); > @@ -2414,10 +2416,12 @@ bool mysql_uninstall_plugin(THD *thd, const LEX_CSTRING *name, > error|= !MyFlags; > } > } > - reap_plugins(); > - > global_plugin_version++; > + thd->apc_target.disable(); > mysql_mutex_unlock(&LOCK_plugin); > + server_threads.iterate(fix_plugins_state_callback, thd); > + reap_plugins(); > + thd->apc_target.enable(); I don't quite understand this. You don't do copy-update here. I presume because plugins are only marked PLUGIN_IS_FREED but not actually removed from the array or hash? but you modify plugin state under a mutex, and read it without a mutex. how is that supposed to work? > DBUG_RETURN(error); > #ifdef WITH_WSREP > wsrep_error_label: Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 6f211b1f01b: MDEV-23518 Syntax error in ond SP results in misleading message on SHOW CREATE PROCEDURE
by Sergei Golubchik 02 Oct '20

02 Oct '20

Hi, Oleksandr! On Oct 01, Oleksandr Byelkin wrote: > revision-id: 6f211b1f01b (mariadb-10.4.11-369-g6f211b1f01b) > parent(s): 571764c04fc > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2020-08-28 16:05:38 +0200 > message: > > MDEV-23518 Syntax error in ond SP results in misleading message on SHOW CREATE PROCEDURE > > Add info to the error message how to get details about error which happened. > > diff --git a/mysql-test/main/errors.test b/mysql-test/main/errors.test > index f3aedc8e491..a4b95d48e5d 100644 > --- a/mysql-test/main/errors.test > +++ b/mysql-test/main/errors.test > @@ -246,3 +246,39 @@ SELECT UPDATEXML(-73 * -2465717823867977728,@@global.auto_increment_increment,nu > SELECT UPDATEXML(-73 * -2465717823867977728,@@global.long_query_time,null); > > -- echo # End of 10.3 tests > + > +--echo # > +--echo # MDEV-23518: Syntax error in ond SP results in misleading > +--echo # message on SHOW CREATE PROCEDURE > +--echo # > + > +DELIMITER $$; > + > +CREATE PROCEDURE P1 () > +BEGIN NOT ATOMIC > + IF (SELECT 2) THEN > + SELECT 4; > + END IF ; > +END; > +$$ > + > +DELIMITER ;$$ you could've had just CREATE PROCEDURE P1 () SELECT 1; one line, no need to change a delimiter. You overwrite the body below anyway :) > + > +select name,db,body from mysql.proc where name = "P1"; > +update mysql.proc set body_utf8="BEGIN NOT ATOMIC > +IF (SELECT 2) OR foo = 3 THEN > +SELECT 4; > +END IF ; > +END", body="BEGIN NOT ATOMIC > +IF (SELECT 2) OR foo = 3 THEN > +SELECT 4; > +END IF ; > +END"where name = "P1"; > + > +--error ER_SP_PROC_TABLE_CORRUPT > +show create procedure P1; > +show warnings; > + > +drop procedure P1; > + > diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt > index ddc52702e42..582c80e2ad8 100644 > --- a/sql/share/errmsg-utf8.txt > +++ b/sql/share/errmsg-utf8.txt > @@ -5714,8 +5714,7 @@ ER_SP_RECURSION_LIMIT > eng "Recursive limit %d (as set by the max_sp_recursion_depth variable) was exceeded for routine %.192s" > ger "Rekursionsgrenze %d (durch Variable max_sp_recursion_depth gegeben) wurde für Routine %.192s überschritten" > ER_SP_PROC_TABLE_CORRUPT > - eng "Failed to load routine %-.192s. The table mysql.proc is missing, corrupt, or contains bad data (internal code %d)" > - ger "Routine %-.192s konnte nicht geladen werden. Die Tabelle mysql.proc fehlt, ist beschädigt, oder enthält fehlerhaften Daten (interner Code: %d)" > + eng "Failed to load routine %-.192s. The table mysql.proc is missing, corrupt, etc. (internal code %d). For more details, run SHOW WARNINGS" Okay, so you just extended the error message. Perhaps "The table mysql.proc is missing, corrupt, etc." is now redundant and shoud go away too? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1