- developers - lists.mariadb.org

Re: [Maria-developers] 1c0731e0544: MDEV-26695: Number of an invalid row is not calculated for table value constructor
by Sergei Golubchik 25 Apr '22

25 Apr '22

Hi, Rucha, what if you combine it with union? create table t (a char(1)) values ('a'),('b') union values ('foo'); please, add a test for that. On Apr 25, Rucha Deodhar wrote: > revision-id: 1c0731e0544 (mariadb-10.7.1-33-g1c0731e0544) > parent(s): 5e6fd4e8043 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2022-01-23 13:48:18 +0530 > message: > > MDEV-26695: Number of an invalid row is not calculated for table value > constructor > > Analysis: counter does not increment while sending rows for table value > constructor and so row_number assumes the default value (0 in this case). > Fix: Increment the counter to avoid counter using default value. > > diff --git a/sql/sql_tvc.cc b/sql/sql_tvc.cc > index bdaf6829fbd..7db0ab217ca 100644 > --- a/sql/sql_tvc.cc > +++ b/sql/sql_tvc.cc > @@ -422,7 +422,9 @@ bool table_value_constr::exec(SELECT_LEX *sl) > DBUG_ENTER("table_value_constr::exec"); > List_iterator_fast<List_item> li(lists_of_values); > List_item *elem; > + THD *cur_thd= sl->parent_lex->thd; > ha_rows send_records= 0; > + int rc=0; > > if (select_options & SELECT_DESCRIBE) > DBUG_RETURN(false); > @@ -438,10 +440,10 @@ bool table_value_constr::exec(SELECT_LEX *sl) > > while ((elem= li++)) > { > + cur_thd->get_stmt_da()->inc_current_row_for_warning(); > if (send_records >= sl->master_unit()->lim.get_select_limit()) > break; > - int rc= > - result->send_data_with_check(*elem, sl->master_unit(), send_records); > + rc= result->send_data_with_check(*elem, sl->master_unit(), send_records); you don't need to do that anymore (it was needed in your first patch, but not here) > if (!rc) > send_records++; > else if (rc > 0) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 585cd1f52e7: MDEV-26875: Wrong user in SET DEFAULT ROLE error
by Sergei Golubchik 25 Apr '22

25 Apr '22

Hi, Anel, On Apr 25, Anel Husakovic wrote: > revision-id: 585cd1f52e7 (mariadb-10.5.14-10-g585cd1f52e7) > parent(s): 52b32c60c26 > author: Anel Husakovic > committer: Anel Husakovic > timestamp: 2022-02-14 13:59:24 +0100 > message: > > MDEV-26875: Wrong user in SET DEFAULT ROLE error > > - Caused by 7c02e8717de5, where 957cb7b7ba35 introduced the bug. > > Reviewed by: > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index c4e66cf5d73..e83bc5635dc 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -3277,10 +3277,14 @@ static int check_user_can_set_role(THD *thd, const char *user, > check_role_is_granted_callback, > NULL) == -1)) > { > - /* Role is not granted but current user can see the role */ > - my_printf_error(ER_INVALID_ROLE, "User %`s@%`s has not been granted role %`s", > - MYF(0), thd->security_ctx->priv_user, > - thd->security_ctx->priv_host, rolename); > + /* If the SET ROLE is applied on the anonymous user, host is null */ > + if (!host) > + my_printf_error(ER_INVALID_ROLE, "User %`s@%`s has not been granted role %`s", > + MYF(0), thd->security_ctx->priv_user, thd->security_ctx->priv_host, rolename); I don't understand it. 1) what does it mean? why for anonymous user you print priv_user@priv_host ? 2) do you have any tests for that? There are none in the commit > + else > + /* Role is not granted but current user can see the role */ > + my_printf_error(ER_INVALID_ROLE, "User %`s@%`s has not been granted role %`s", > + MYF(0), user, host, rolename); > } > else > { Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 5f8ffc7271a: MDEV-20207: Assertion `! is_set()' failed in Diagnostics_area::set_eof_status upon HANDLER READ
by Sergei Golubchik 25 Apr '22

25 Apr '22

Hi, Rucha, The fix is ok to push. But as a general comment, better try to define variables in the smallest scope where they're used. In particular here it could've been { MY_BITMAP *old_map= dbug_tmp_use_all_columns(table, &table->write_set); - (void) item->save_in_field(key_part->field, 1); + int res= item->save_in_field(key_part->field, 1); because you don't need `res` outside of that scope. Makes the code easier to read. Of course, there can be exceptions. For example, if the variable is an object and its initialization is very expensive. But whether you move `int res` declaration inside or not - it's ok to push, no need to ask for another review. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org On Apr 25, Rucha Deodhar wrote: > revision-id: 5f8ffc7271a (mariadb-10.2.40-251-g5f8ffc7271a) > parent(s): 2cbf92522b5 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2022-01-27 01:18:19 +0530 > message: > > MDEV-20207: Assertion `! is_set()' failed in Diagnostics_area::set_eof_status > upon HANDLER READ > > Analysis: The error state is not stored while checking condition and key name. > Fix: Return true while checking condition and key name if error is reported > because geometry object can't be created from the data in the index value for > HANDLER READ. > > diff --git a/sql/sql_handler.cc b/sql/sql_handler.cc > index ec3756eceba..7c002a68d9e 100644 > --- a/sql/sql_handler.cc > +++ b/sql/sql_handler.cc > @@ -554,6 +554,7 @@ mysql_ha_fix_cond_and_key(SQL_HANDLER *handler, > { > THD *thd= handler->thd; > TABLE *table= handler->table; > + int res= 0; > if (cond) > { > /* This can only be true for temp tables */ > @@ -613,8 +614,10 @@ mysql_ha_fix_cond_and_key(SQL_HANDLER *handler, > if (!in_prepare) > { > MY_BITMAP *old_map= dbug_tmp_use_all_columns(table, &table->write_set); > - (void) item->save_in_field(key_part->field, 1); > + res= item->save_in_field(key_part->field, 1); > dbug_tmp_restore_column_map(&table->write_set, old_map); > + if (res) > + return 1; > } > key_len+= key_part->store_length; > keypart_map= (keypart_map << 1) | 1;

1 0

Re: [Maria-developers] 14cc679c95e: MDEV-27831 Let the SQL SERVICE user set the current user name.
by Sergei Golubchik 22 Apr '22

22 Apr '22

Hi, Alexey, On Mar 04, Alexey Botchkov wrote: > revision-id: 14cc679c95e (mariadb-10.7.2-8-g14cc679c95e) > parent(s): 33fd136c61b > author: Alexey Botchkov > committer: Alexey Botchkov > timestamp: 2022-02-15 11:35:18 +0400 > message: > > MDEV-27831 Let the SQL SERVICE user set the current user name. > > The 'user' argument added to the mysql_real_connect_local. I think this is wrong on many levels. A plugin name is already known inside the plugin, you shouldn't force the plugin to pass its own name as an argument in multiple places, the server should determine it automatically. You should not set current_user to an arbitrary string for audit plugin to see it. current_user is the name of the user account and it's used in many places as such. Try, for example, to create a view or a stored procedure. Who will be a definer? Setting only user() might be ok. Setting @@proxy_user or @@external_user is even better, if your audit plugin can show them. @@external_user would be the best, I think it's purely informational. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] 96b40a5e2f5: MDEV-27744 InnoDB: Failing assertion: !cursor->index->is_committed() in row0ins.cc (from row_ins_sec_index_entry_by_modify) | Assertion `0' failed in row_upd_sec_index_entry (debug) | Corruption
by Sergei Golubchik 22 Apr '22

22 Apr '22

Hi, Alexander, Some comments/questions here. I'll need to look at the patch again after I understand why you did it this way better. On Apr 18, Alexander Barkov wrote: > revision-id: 96b40a5e2f5 (mariadb-10.3.33-61-g96b40a5e2f5) > parent(s): 7355f7b1f5c > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-04-07 16:22:17 +0400 > message: > > MDEV-27744 InnoDB: Failing assertion: !cursor->index->is_committed() in row0ins.cc (from row_ins_sec_index_entry_by_modify) | Assertion `0' failed in row_upd_sec_index_entry (debug) | Corruption A detailed comment here is a must. What was the problem, how it was fixed. > diff --git a/sql/item_func.h b/sql/item_func.h > index 754b1cd1eb2..d844e45280a 100644 > --- a/sql/item_func.h > +++ b/sql/item_func.h > @@ -35,6 +35,36 @@ extern "C" /* Bug in BSDI include file */ > #include <cmath> > > > +template<class FUNC> > +FUNC* item_func_create_2_3_args(THD *thd, const LEX_CSTRING &name, > + List<Item> *item_list) > +{ > + int arg_count= item_list ? item_list->elements : 0; > + > + switch (arg_count) { > + case 2: > + { > + Item *param_1= item_list->pop(); > + Item *param_2= item_list->pop(); > + return new (get_thd_memroot(thd)) FUNC(thd, param_1, param_2); > + } > + case 3: > + { > + Item *param_1= item_list->pop(); > + Item *param_2= item_list->pop(); > + Item *param_3= item_list->pop(); > + return new (get_thd_memroot(thd)) FUNC(thd, param_1, param_2, param_3); A constructor that takes a List<Item> wouldn't need a switch. And wouldn't need a template either, your create_check_args_ge() isn't a template. > + break; > + } > + default: > + my_error(ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT, MYF(0), name.str); > + break; > + } > + > + return NULL; > +} > + > + > class Item_func :public Item_func_or_sum > { > void sync_with_sum_func_and_with_field(List<Item> &list); > @@ -56,8 +86,41 @@ class Item_func :public Item_func_or_sum > bool check_argument_types_can_return_text(uint start, uint end) const; > bool check_argument_types_can_return_date(uint start, uint end) const; > bool check_argument_types_can_return_time(uint start, uint end) const; > + > + void print_schema_name_if_needed(String *to) const > + { > + const LEX_CSTRING schema_name= schema_name_cstring(); > + if (schema_name.length) > + { > + to->append(schema_name); > + to->append('.'); > + } this should check that schema_name isn't the same as the schema in the path (implicit path, assumed by the current sql_mode). > + } > + void print_maybe_qualified_name(String *to) const > + { > + print_schema_name_if_needed(to); > + to->append(func_name()); > + } > + > public: > > + // Print an error message for a builtin-schema qualified function call > + static void wrong_param_count_error(const LEX_CSTRING &schema_name, > + const LEX_CSTRING &func_name); > + > + // Check that the number of arguments is greater or equal to "expected" > + static bool create_check_args_ge(const LEX_CSTRING &name, > + const List<Item> *item_list, > + uint expected) > + { > + if (!item_list || item_list->elements < expected) > + { > + my_error(ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT, MYF(0), name.str); what's the point of your wrong_param_count_error() if you aren't using it? > + return true; > + } > + return false; > + } > + > table_map not_null_tables_cache; > > enum Functype { UNKNOWN_FUNC,EQ_FUNC,EQUAL_FUNC,NE_FUNC,LT_FUNC,LE_FUNC, > diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc > index 75abd9906cd..e45fe6ef6d7 100644 > --- a/sql/item_strfunc.cc > +++ b/sql/item_strfunc.cc > @@ -3204,6 +3239,39 @@ static String *default_pad_str(String *pad_str, CHARSET_INFO *collation) > return pad_str; > } > > + > +Item_func_lpad* > +Item_func_lpad::create(THD *thd, const LEX_CSTRING &name, > + List<Item> *item_list) > +{ > + return item_func_create_2_3_args<Item_func_lpad>(thd, name, item_list); may be item_func_create_2_3_args should be in Item_func? Like create_check_args_ge() is? and may be create_check_args_ge() should be a template too? > +} > + > + > +Item_func_lpad_oracle* > +Item_func_lpad_oracle::create(THD *thd, const LEX_CSTRING &name, > + List<Item> *item_list) > +{ > + return item_func_create_2_3_args<Item_func_lpad_oracle>(thd, name, item_list); > +} > + > + > +Item_func_rpad* > +Item_func_rpad::create(THD *thd, const LEX_CSTRING &name, > + List<Item> *item_list) > +{ > + return item_func_create_2_3_args<Item_func_rpad>(thd, name, item_list); > +} > + > + > +Item_func_rpad_oracle* > +Item_func_rpad_oracle::create(THD *thd, const LEX_CSTRING &name, > + List<Item> *item_list) > +{ > + return item_func_create_2_3_args<Item_func_rpad_oracle>(thd, name, item_list); > +} > + > + > bool Item_func_pad::fix_length_and_dec() > { > if (arg_count == 3) > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > index 7f1b362d91d..6b0a351f76e 100644 > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -1542,7 +1541,18 @@ int Lex_input_stream::lex_one_token(YYSTYPE *yylval, THD *thd) > if (lex->parsing_options.lookup_keywords_after_qualifier) > next_state= MY_LEX_IDENT_OR_KEYWORD; > else > - next_state= MY_LEX_IDENT_START; // Next is ident (not keyword) > + { > + /* > + Next is: > + - A qualified func with a special syntax: > + mariadb_schema.REPLACE('a','b','c') > + mariadb_schema.SUSTRING('a',1,2) > + mariadb_schema.TRIM('a') > + - Or an identifier otherwise. No keyword lookup is done, > + all keywords are treated as identifiers. > + */ > + next_state= MY_LEX_IDENT_OR_QUALIFIED_SPECIAL_FUNC; I don't understand why you did it in the lexer and not in the parser, like - | REPLACE '(' expr ',' expr ',' expr ')' + | opt_schema REPLACE '(' expr ',' expr ',' expr ')' or (if the above won't work in LARL(1) parser) - | ident_cli '.' ident_cli '(' opt_expr_list ')' + | ident_cli '.' func_2nd_part with + func_2nd_part: ident_cli '(' opt_expr_list ')' + | REPLACE '(' expr ',' expr ',' expr ')' etc (not reviewing lexer changes anymore, until I understand the above) > + } > if (!ident_map[(uchar) yyPeek()]) // Probably ` or " > next_state= MY_LEX_START; > return((int) c); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 4

Re: [Maria-developers] 17548c8a8b6: MDEV-25477 Auto-create breaks replication when triggering event was not replicated
by Sergei Golubchik 20 Apr '22

20 Apr '22

Hi, Aleksey, Basically, it's fine, but I still didn't like that there's a new flag in THD to mark a statement that has to be logged. So I tried to change to log_current_statement. I know you tried that and you told me what tests fails, rpl.create_or_replace_*. I debugged that a bit, and I think the problem is that select_create inherits from select_insert, so CREATE ... SELECT runs your code that is only supposed to be run for INSERT ODKU or REPLACE. This change makes all tests pass (after removing vers_created_partitions): @@ -4225,7 +4225,8 @@ bool select_insert::prepare_eof() thd->clear_error(); else errcode= query_error_code(thd, killed_status == NOT_KILLED); - StatementBinlog stmt_binlog(thd, thd->binlog_need_stmt_format(trans_table)); + StatementBinlog stmt_binlog(thd, !can_rollback_data() && + thd->binlog_need_stmt_format(trans_table)); res= thd->binlog_query(THD::ROW_QUERY_TYPE, thd->query(), thd->query_length(), trans_table, FALSE, FALSE, errcode); On Apr 10, Aleksey Midenkov wrote: > revision-id: 17548c8a8b6 (mariadb-10.6.1-250-g17548c8a8b6) > parent(s): 6282e025a0e > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2022-03-07 23:49:35 +0300 > message: > > MDEV-25477 Auto-create breaks replication when triggering event was not replicated > > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 686e6e70766..4fed1bf7355 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -2914,6 +2914,13 @@ class THD: public THD_count, /* this must be first */ > int binlog_flush_pending_rows_event(bool stmt_end, bool is_transactional); > int binlog_remove_pending_rows_event(bool clear_maps, bool is_transactional); > > + bool binlog_need_stmt_format(bool is_transactional) const > + { > + return vers_created_partitions && !binlog_get_pending_rows_event(is_transactional); > + } > + > + bool binlog_for_noop_dml(bool transactional_table); > + > /** > Determine the binlog format of the current statement. > > @@ -3557,6 +3564,7 @@ class THD: public THD_count, /* this must be first */ > bool tablespace_op; /* This is TRUE in DISCARD/IMPORT TABLESPACE */ > /* True if we have to log the current statement */ > bool log_current_statement; > + uint vers_created_partitions; > /** > True if a slave error. Causes the slave to stop. Not the same > as the statement execution error (is_error()), since > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index 707b8a0d3bf..25551dfa197 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -4223,6 +4225,7 @@ bool select_insert::prepare_eof() > thd->clear_error(); > else > errcode= query_error_code(thd, killed_status == NOT_KILLED); > + StatementBinlog stmt_binlog(thd, thd->binlog_need_stmt_format(trans_table)); > res= thd->binlog_query(THD::ROW_QUERY_TYPE, > thd->query(), thd->query_length(), > trans_table, FALSE, FALSE, errcode); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 8

Re: [Maria-developers] 93493a0e9b5: MDEV-24176 Server crashes after insert in the table with virtual column generated using date_format() and if()
by Sergei Golubchik 17 Apr '22

17 Apr '22

Hi, Aleksey! Could you please explain in the commit comment what the bug was and what you are fixing? I wasn't able to understad that, so I had to debug those crashes myself. So, the reason is, objects allocated in the wrong arena, right? And it was done inconsistently, on different code paths the "re-fix" context was different. I agree that encapsulating all the context in one Vcol_expr_context is a good idea, it'll allow to have the same, correct, context everywhere. But always using expr_arena doesn't necessarily seem to be a correct choice. E.g. Arg_comparator creates Item_cache_temporal on every fix_field. If vcol expr is re-fixed all the time, expr_arena will grow continuously. Same for charset conversion, CASE, IN, etc. I suspect that if the vcol expr is always re-fixed, it has to use stmt arena. Why do you re-fix fields? I can hardly imagine that an Item_field can find itself in a different table. And if it's always in the same table, why to force it to find this table over and over? We've already discussed VCOL_TABLE_ALIAS, no need to go there again. On Jan 06, Aleksey Midenkov wrote: > revision-id: 93493a0e9b5 (mariadb-10.2.40-194-g93493a0e9b5) > parent(s): a565e63c54c > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2021-12-29 00:46:31 +0300 > message: > > MDEV-24176 Server crashes after insert in the table with virtual > column generated using date_format() and if() > > When table is reopened from cache vcol_info contains stale > expression. We refresh expression via TABLE::vcol_fix_exprs() but > first we must prepare a proper context (Vcol_expr_context) and do some > preparations which meet some requirements: > > 1. fields must not be fixed, we unfix them with cleanup() via > cleanup_processor. > > 2. Item_ident must have cached_table cleared. Again, this is stale > data which was already freed. cached_table interferes with > find_field_in_tables(). > > We cannot clear cached_table directly in Item_ident::cleanup() > method. Lots of spaghetti logic depends on cached_table existence even > after cleanup() done. > > 3. If Item_ident has table_name non-NULL we trigger expr update. That > is done via Item_ident::check_vcol_func_processor() and > VCOL_TABLE_ALIAS flag. > > (4-7) The below conditions are prepared by Vcol_expr_context and used in > both fix_session_expr_for_read() and TABLE::vcol_fix_exprs(): > > 4. Any fix_expr() must be done on expr_arena as there may be new items > created. It was a bug in fix_session_expr_for_read(). It was just not > reproduced because there were no second refix. Now refix is done for > more cases so it does reproduce. Tests affected: vcol.binlog > > 5. Also name resolution context must be narrowed to the single table > with all crutches in place. Tests affected: vcol.update > > 6. sql_mode must be clean and not fail expr update. > > sql_mode such as MODE_NO_BACKSLASH_ESCAPES, MODE_NO_ZERO_IN_DATE, etc > must not affect vcol expression update. If the table was created > successfully any further evaluation must not fail. Tests affected: > main.func_like > > 7. Warnings are disabled during expr update. It is assumed that these > warnings were printed before during initialization phase or later > during execution phase. Tests affected: vcol.wrong_arena > > Dictionary: refresh, update and refix are the synonyms for > vcol_fix_exprs() or fix_session_expr_for_read() calls. > > 8. cleanup_excluding_fields_processor() removed. It was just a quick > hack to exclude wrongly working Item_field from processing. Now it > works due to correct execution environment (Vcol_expr_context). > Related to MDEV-10355. > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 12

Re: [Maria-developers] 62d3496969d: MDEV-24176 Server crashes after insert in the table with virtual column generated using date_format() and if()
by Sergei Golubchik 12 Apr '22

12 Apr '22

Hi, Aleksey, This is a review of `git diff 5fbbdbc85ba3 62d3496969db` Basically, no new comments, see below: On Apr 07, Aleksey Midenkov wrote: > diff --git a/libmariadb b/libmariadb > index f6c3d9fd2af..735a7299dba 160000 > --- a/libmariadb > +++ b/libmariadb > @@ -1 +1 @@ > -Subproject commit f6c3d9fd2af5d17db64cc996574aa312efd70fcf > +Subproject commit 735a7299dbae19cc2b82b9697becaf90e9b43047 you've mistakenly checked in a submodule > diff --git a/sql/field.cc b/sql/field.cc > index f18fb25ebe3..04c492e6c69 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -2416,6 +2416,13 @@ int Field::set_default() > if (default_value) > { > Query_arena backup_arena; > + /* > + TODO: this imposes memory leak until table flush when save_in_field() > + does expr_arena allocation. F.ex. case from main.default: > + > + CREATE TABLE t1 (a INT DEFAULT CONCAT('1 ')); > + INSERT INTO t1 VALUES (DEFAULT); I wasn't able to repeat that. In your branch (didn't try in vanilla 10.3) and for the above test case I see no additional allocations in table->mem_root after the table is opened. > + */ > table->in_use->set_n_backup_active_arena(table->expr_arena, &backup_arena); > int rc= default_value->expr->save_in_field(this, 0); > table->in_use->restore_active_arena(table->expr_arena, &backup_arena); > diff --git a/sql/table.cc b/sql/table.cc > index 08d91678b25..d22da17c31a 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -2970,36 +2988,133 @@ static bool fix_vcol_expr(THD *thd, Virtual_column_info *vcol) ... > +bool Vcol_expr_context::init() > +{ > + /* > + As this is vcol expression we must narrow down name resolution to > + single table. > + */ > + if (init_lex_with_single_table(thd, table, &lex)) I've already asked about it in a separate email, so here I'm just marking a line that we haven't fully agreed on yet > + { > + my_error(ER_OUT_OF_RESOURCES, MYF(0)); > + table->map= old_map; > + return true; > + } > + > + lex.sql_command= old_lex->sql_command; > + thd->variables.sql_mode= 0; > + > + TABLE_LIST const *tl= table->pos_in_table_list; > + DBUG_ASSERT(table->pos_in_table_list); > + > + if (table->pos_in_table_list->security_ctx) > thd->security_ctx= tl->security_ctx; > - bool res= fix_session_vcol_expr(thd, vcol); > + > + inited= true; > + return false; > +} > + Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] e3e59c9e38d: MDEV-28266 Crash in Field_string::type_handler when calling procedures
by Sergei Golubchik 10 Apr '22

10 Apr '22

Hi, Oleksandr, On Apr 10, Oleksandr Byelkin wrote: > revision-id: e3e59c9e38d (mariadb-10.6.6-83-ge3e59c9e38d) > parent(s): 4e1ca388381 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2022-04-08 10:37:17 +0200 > message: > > MDEV-28266 Crash in Field_string::type_handler when calling procedures > > In case of only metadata request the cursor lack the call to redirect > Field::orig_table to the temporary table. > > diff --git a/sql/sql_cursor.cc b/sql/sql_cursor.cc > index 0add5845558..155346341f1 100644 > --- a/sql/sql_cursor.cc > +++ b/sql/sql_cursor.cc > @@ -183,6 +183,16 @@ int mysql_open_cursor(THD *thd, select_result *result, > { > Materialized_cursor *materialized_cursor= > result_materialize->materialized_cursor; > + if (result_materialize->view_structure_only()) > + { > + /* > + If it was view_structure_only then only send metadata call was made > + and there was no other call. So we have to "close" the cursor > + explicitly (which redirect orig_table in the fields, and untie > + it with closed tables) > + */ > + materialized_cursor->on_table_fill_finished(); > + } > > if ((rc= materialized_cursor->open(0))) > { > delete materialized_cursor; > goto end; > } normally on_table_fill_finished() is called from Select_materialize::send_eof() and Select_materialize::abort_result_set(). Note that directly after your added code there's materialized_cursor->open(), and there we have int Materialized_cursor::open(JOIN *join __attribute__((unused))) { ... if (!rc) { thd->server_status|= SERVER_STATUS_CURSOR_EXISTS; result->send_eof(); } else { result->abort_result_set(); } That is, one could expect that even without your added code, on_table_fill_finished() would be called. The problem is that result here is sp_cursor::Select_fetch_into_spvars, and sp_cursor::Select_fetch_into_spvars::send_eof() is simply { return FALSE; } So, as a fix I'd suggest to remove on_table_fill_finished() from send_eof/abort_result_set, and put it unconditionally after the if(), to make it work no matter what select_result was chosen. Like this: ====================================== --- a/sql/sql_cursor.cc +++ b/sql/sql_cursor.cc @@ -80,19 +80,7 @@ class Select_materialize: public select_unit Select_materialize(THD *thd_arg, select_result *result_arg): select_unit(thd_arg), result(result_arg), materialized_cursor(0) {} virtual bool send_result_set_metadata(List<Item> &list, uint flags); - bool send_eof() - { - if (materialized_cursor) - materialized_cursor->on_table_fill_finished(); - return false; - } - - void abort_result_set() - { - if (materialized_cursor) - materialized_cursor->on_table_fill_finished(); - } - + bool send_eof() { return false; } bool view_structure_only() const { return result->view_structure_only(); @@ -333,6 +321,8 @@ int Materialized_cursor::open(JOIN *join result->abort_result_set(); } + on_table_fill_finished(); + return rc; } ====================================== with the commit comment saying that "on_table_fill_finished() should always be done at the end of open(), even if result is not Select_materialize, but, for example, Select_fetch_into_spvars" Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] a10facb656b: MDEV-27065 Partitioning tables with custom data directories moves data back to default directory
by Sergei Golubchik 29 Mar '22

29 Mar '22

Hi, Nayuta, On Mar 29, Nayuta Yanagisawa wrote: > revision-id: a10facb656b (mariadb-10.2.43-48-ga10facb656b) > parent(s): 33ff18627ea > author: Nayuta Yanagisawa > committer: Nayuta Yanagisawa > timestamp: 2022-03-29 23:43:16 +0900 > message: > > MDEV-27065 Partitioning tables with custom data directories moves data back to default directory > > --- > .../suite/parts/r/partition_exch_qa_14.result | 22 ++++++++++++++++++++++ > sql/ha_partition.cc | 16 ++++++++++++++++ > 2 files changed, 38 insertions(+) > > diff --git a/mysql-test/suite/parts/r/partition_exch_qa_14.result b/mysql-test/suite/parts/r/partition_exch_qa_14.result > index 1420982436a..52839644f55 100644 > --- a/mysql-test/suite/parts/r/partition_exch_qa_14.result > +++ b/mysql-test/suite/parts/r/partition_exch_qa_14.result > @@ -9,6 +9,9 @@ PARTITION BY RANGE (a) > (PARTITION p0 VALUES LESS THAN (10) DATA DIRECTORY = 'MYSQLTEST_VARDIR/mysql-test-data-dir' INDEX DIRECTORY = 'MYSQLTEST_VARDIR/mysql-test-idx-dir', > PARTITION p1 VALUES LESS THAN (100) DATA DIRECTORY = 'MYSQLTEST_VARDIR/mysql-test-data-dir' INDEX DIRECTORY = 'MYSQLTEST_VARDIR/mysql-test-idx-dir', > PARTITION p2 VALUES LESS THAN (1000) DATA DIRECTORY = 'MYSQLTEST_VARDIR/mysql-test-data-dir' INDEX DIRECTORY = 'MYSQLTEST_VARDIR/mysql-test-idx-dir'); > +Warnings: > +Warning 1618 <DATA DIRECTORY> option ignored > +Warning 1618 <INDEX DIRECTORY> option ignored This is very confusing. There are four DATA DIRECTORY and four INDEX DIRECTORY in the statement. Which one is ignored? (yes, *I* know, the table level one) > @@ -148,6 +154,9 @@ PARTITION BY RANGE (a) > (PARTITION p0 VALUES LESS THAN (10) , > PARTITION p1 VALUES LESS THAN (100) , > PARTITION p2 VALUES LESS THAN (1000) ); > +Warnings: > +Warning 1618 <DATA DIRECTORY> option ignored > +Warning 1618 <INDEX DIRECTORY> option ignored and here DATA/INDEX DIRECTORY behaves not like other options. For a normal option table-level value would apply to all partitions. That is CREATE TABLE t1 (...) OPT="VAL (PARTITION p0 VALUES LESS THAN (10) , PARTITION p1 VALUES LESS THAN (100) , PARTITION p2 VALUES LESS THAN (1000) ); would be the same as CREATE TABLE t1 (...) (PARTITION p0 VALUES LESS THAN (10) OPT="VAL, PARTITION p1 VALUES LESS THAN (100) OPT="VAL, PARTITION p2 VALUES LESS THAN (1000) OPT="VAL); Is that correct? > CREATE TABLE tsp (a INT, b VARCHAR(55), PRIMARY KEY (a)) DATA DIRECTORY = 'MYSQLTEST_VARDIR/mysql-test-data-dir' INDEX DIRECTORY = 'MYSQLTEST_VARDIR/mysql-test-idx-dir' ENGINE = MYISAM > PARTITION BY RANGE (a) > SUBPARTITION BY HASH(a) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0