- developers - lists.mariadb.org

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

Re: [Maria-developers] 88f8aa20bba: MDEV-23486 RBR can bypass secure_timestamp=YES
by Sergei Golubchik 26 Mar '22

26 Mar '22

Hi, Aleksey! On Jan 09, Aleksey Midenkov wrote: > revision-id: 88f8aa20bba (mariadb-10.3.31-79-g88f8aa20bba) > parent(s): d5451867af1 > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2021-12-23 04:15:39 +0300 > message: > > MDEV-23486 RBR can bypass secure_timestamp=YES > > Pass to a slave whether master table is system-versioned via > TM_SYSTEM_VERSIONED flag. That deprecates m_vers_from_plain > detection. If secure_timestamp == YES or master did not supply row_end > data treat the row as current data and generate timestamps for it. I don't understand why does it matter whether the master is system versioned or not. secure_timestamp=YES means the table has to store actual timestamps when rows were changed. It doesn't trust the master to set timestamps. So it should not matter whether master table is versioned or not, if you don't trust the master, you cannot trust its TM_SYSTEM_VERSIONED flag either. Even if the table was system versioned on the master, the master could've had secure_timestamp=NO and a completely fake history. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] MDEV-24560 SIGSEGV in st_join_table::cleanup
by Sergey Petrunia 25 Mar '22

25 Mar '22

Hi Oleg, > commit 4f41909c3ee1d7e42c56947c9a9156fb78c22d8a > Author: Oleg Smirnov <olernov(a)gmail.com> > Date: Fri Mar 11 21:18:34 2022 +0700 > > MDEV-24560 SIGSEGV in st_join_table::cleanup > > If JOIN::create_postjoin_aggr_table encounters errors during execution > then free_tmp_table() is then called twice for JOIN_TAB::aggr. > The solution is to initialize JOIN_TAB::aggr and JOIN_TAB::table only > on successful completion of JOIN::create_postjoin_aggr_table > > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index c044656e916..8c07014d981 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -4037,10 +4037,6 @@ JOIN::create_postjoin_aggr_table(JOIN_TAB *tab, List<Item> *table_fields, > !tables_list); > if (tab > join_tab) > (tab - 1)->next_select= sub_select_postjoin_aggr; > - if (!(tab->aggr= new (thd->mem_root) AGGR_OP(tab))) > - goto err; > - tab->table= table; > - table->reginfo.join_tab= tab; > > /* if group or order on first table, sort first */ > if ((group_list && simple_group) || > @@ -4089,7 +4085,10 @@ JOIN::create_postjoin_aggr_table(JOIN_TAB *tab, List<Item> *table_fields, > order= NULL; > } > } > - > + if (!(tab->aggr= new (thd->mem_root) AGGR_OP(tab))) > + goto err; > + tab->table= table; > + table->reginfo.join_tab= tab; > DBUG_RETURN(false); I see a potential issue in the patch: The moved code does this assignment: > tab->table= table; Let's look inside this if-branch: >> if ((group_list && simple_group) || >> (implicit_grouping && select_lex->have_window_funcs())) The code inside if() { ... } part seems to be fine: it does not touch the contents of 'tab', in particular tab->table. Let's look at the coed inside the else {... } part: It has this condition: >> if (!group_list && !table->distinct && order && simple_order && >> tab == join_tab + const_tables) >> { Note that tab==join_tab+const_tables. Then, a few lines below: >> add_sorting_to_table(join_tab + const_tables, order)) This is essentially add_sorting_to_table(tab, order). Inside JOIN::add_sorting_to_table one can find this: TABLE *table= tab->table; if ((tab == join_tab + const_tables) && table->pos_in_table_list && table->pos_in_table_list->is_sjm_scan_table()) that is, we access tab->table and tab->table->pos_in_table_list. Which are not assigned yet (because your patch moves the assignment to happen later). I've tried to construct an example to demonstrate this issue, but was not successful. I've made this change: diff --git a/sql/sql_select.cc b/sql/sql_select.cc index c044656e916..5a9a7d7e73f 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -4080,6 +4080,8 @@ JOIN::create_postjoin_aggr_table(JOIN_TAB *tab, List<Item> *table_fields, tab == join_tab + const_tables) { DBUG_PRINT("info",("Sorting for order")); + fprintf(stderr,"AAABBBCCC:%s\n", thd->query()); + DBUG_ASSERT(0); THD_STAGE_INFO(thd, stage_sorting_for_order); if (ordered_index_usage != ordered_index_order_by && and ran the entire testsuite with it. There were no failures. I'm not sure if this is deadcode or we just don't have enough test coverage. I suggest to assume the latter and change the patch to avoid this issue. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 0