- developers - lists.mariadb.org

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

Re: [Maria-developers] 3be1e491fb5: MDEV-27743 Remove Lex::charset
by Sergei Golubchik 22 Mar '22

22 Mar '22

Hi, Alexander, All comments from the previous review were addressed and there were no other changes. So, ok to push. On Mar 22, Alexander Barkov wrote: > revision-id: 3be1e491fb5 (mariadb-10.6.1-383-g3be1e491fb5) > parent(s): c69defd3b61 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-03-21 21:33:30 +0400 > message: > > MDEV-27743 Remove Lex::charset > > This patch also fixes: > > MDEV-27690 Crash on `CHARACTER SET csname COLLATE DEFAULT` in column definition > MDEV-27853 Wrong data type on column `COLLATE DEFAULT` and table `COLLATE some_non_default_collation` > MDEV-28067 Multiple conflicting column COLLATE clauses are not rejected > MDEV-28118 Wrong collation of `CAST(.. AS CHAR COLLATE DEFAULT)` > MDEV-28119 Wrong column collation on MODIFY + CONVERT > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 026e52bad84: MDEV-27743 Remove Lex::charset
by Sergei Golubchik 21 Mar '22

21 Mar '22

Hi, Alexander, Please, see below On Mar 18, Alexander Barkov wrote: > revision-id: 026e52bad84 (mariadb-10.6.1-369-g026e52bad84) > parent(s): 369498d6b8c > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-03-18 12:26:03 +0400 > message: > > MDEV-27743 Remove Lex::charset > > This patch also fixes: > > MDEV-27690 Crash on `CHARACTER SET csname COLLATE DEFAULT` in column definition > MDEV-27853 Wrong data type on column `COLLATE DEFAULT` and table `COLLATE some_non_default_collation` > MDEV-28067 Multiple conflicting column COLLATE clauses are not rejected > MDEV-28118 Wrong collation of `CAST(.. AS CHAR COLLATE DEFAULT)` > MDEV-28119 Wrong column collation on MODIFY + CONVERT > > diff --git a/mysql-test/main/ctype_collate_column.test b/mysql-test/main/ctype_collate_column.test > new file mode 100644 > index 00000000000..6a7cf2cc84c > --- /dev/null > +++ b/mysql-test/main/ctype_collate_column.test > @@ -0,0 +1,637 @@ ... > + > +DELIMITER $$; > +CREATE PROCEDURE p1(dt TEXT, cs TEXT, cl0 TEXT, cl1 TEXT, cl2 TEXT, tcs TEXT) > +BEGIN > + DECLARE errstate TEXT DEFAULT NULL; > + DECLARE errno INT DEFAULT NULL; > + DECLARE errmsg TEXT DEFAULT NULL; > + DECLARE query TEXT; > + DECLARE result TEXT; > + DECLARE CONTINUE HANDLER FOR SQLEXCEPTION > + BEGIN > + GET DIAGNOSTICS CONDITION 1 errstate = RETURNED_SQLSTATE, > + errno = MYSQL_ERRNO, errmsg = MESSAGE_TEXT; > + END; > + SET query= CONCAT('CREATE TABLE t1 (a ', dt, ' ', cs, ' ', cl0, > + ' NOT NULL ',cl1, > + ' DEFAULT '''' ', cl2, > + ') ', tcs, ' ENGINE=Memory'); > + EXECUTE IMMEDIATE query; > + IF errmsg IS NOT NULL > + THEN > + SET result=CONCAT('ERROR: ', > + COALESCE(errstate,'<NULL>'), ' ', > + COALESCE(errno,'<NULL>'), ' ', > + COALESCE(errmsg,'<NULL>')); > + INSERT INTO results (dt,cs,cl0,cl1,cl2,tcs,query,result) > + VALUES (dt,cs,cl0,cl1,cl2,tcs,query,result); > + ELSE > + FOR row IN (SELECT CONCAT(COLUMN_TYPE, > + ' CHARACTER SET ', CHARACTER_SET_NAME, > + ' COLLATE ', COLLATION_NAME) AS result > + FROM INFORMATION_SCHEMA.COLUMNS > + WHERE TABLE_SCHEMA='test' AND TABLE_NAME='t1') > + DO > + INSERT INTO results (dt,cs,cl0,cl1,cl2,tcs,query,result) > + VALUES (dt,cs,cl0,cl1,cl2,tcs,query,row.result); > + END FOR; > + DROP TABLE t1; > + END IF; > +END; > +$$ > +DELIMITER ;$$ > + > + > +DELIMITER $$; > +CREATE PROCEDURE p3(dt TEXT) > +BEGIN > + FOR row IN ( > + SELECT cs, cl0, cl1.cl1 AS cl1, cl2.cl1 AS cl2, tcs > + FROM cs, cl0, cl1, cl1 AS cl2, tcs > + ORDER BY cs, cl0, cl1, cl2, tcs > + ) > + DO > + CALL p1('char(10)', row.cs, row.cl0, row.cl1, row.cl2, row.tcs); minor detail: here ^^^ you want to use dt, not 'char(10)' > + END FOR; > +END; > +$$ > +DELIMITER ;$$ > + > + > +--disable_column_names > +CALL p3('char(10)'); > +--enable_column_names > + > +SELECT * FROM results_statistics; isn't it redundant? you select all rows from result anyway. If something will change, the result of that select will already cause the the to fail. Why do you need second mismatch in the result/reject. > + > +--vertical_results > +SELECT query, result, '' AS `` FROM results > +ORDER BY dt, cs, cl0, cl1, cl2, tcs; > +--horizontal_results > + > +DROP PROCEDURE p1; > +DROP PROCEDURE p3; > + > +DROP TABLE cs, cl0, cl1, tcs; ... > +# CREATE TABLE t1 > +# ( > +# a CHAR(10) BINARY NOT NULL DEFAULT '' > +# ) CHARACTER SET cs COLLATE cs_bin; > + > +DELETE FROM results > +WHERE cs='' > + AND cl0='BINARY' > + AND cl1='' > + AND cl2='' > + AND tcs LIKE 'CHARACTER SET%' > + AND result NOT LIKE 'ERROR%' > + AND result RLIKE CONCAT('COLLATE ', tcs_character_set_name, '_bin'); > +SELECT ROW_COUNT(); ... > +DROP FUNCTION is_collate_clause_with_explicit_default_collation; > +DROP FUNCTION is_collate_clause_with_explicit_collation; > +DROP FUNCTION is_conflicting_charset_explicit_collate_explicit; > +DROP FUNCTION is_conflicting_collate_explicit2; > +DROP FUNCTION is_conflicting_collate_default_collate_explicit; > +DROP FUNCTION collate_cs_default_collation; I think the main result is `select * from results` (with order by, etc). It's great, that you added that. Everything else is redundant, I suggest to remove other selects and deletes. > + > + > +--echo # > +--echo # End of 10.9 tests > +--echo # > diff --git a/sql/lex_charset.h b/sql/lex_charset.h > new file mode 100644 > index 00000000000..d95f0bc1920 > --- /dev/null > +++ b/sql/lex_charset.h > @@ -0,0 +1,202 @@ ... > +#define LEX_CHARSET_COLLATION_TYPE_BITS 2 > + static_assert(((1<<LEX_CHARSET_COLLATION_TYPE_BITS)-1) >= > + TYPE_COLLATE_CONTEXTUALLY_TYPED, > + "Lex_charset_collation_st::Type bits check"); > + > +protected: > + CHARSET_INFO *m_ci; > + Type m_type; > +public: > + static CHARSET_INFO *find_bin_collation(CHARSET_INFO *cs); > + static CHARSET_INFO *find_default_collation(CHARSET_INFO *cs); > +public: ... > + void set_contextually_typed_binary_style() > + { > + m_ci= &my_collation_contextually_typed_binary; > + m_type= TYPE_COLLATE_CONTEXTUALLY_TYPED; > + } > + bool is_contextually_typed_collate_default() const > + { > + return m_ci == &my_collation_contextually_typed_default && > + m_type == TYPE_COLLATE_CONTEXTUALLY_TYPED; what does it mean when only one of these two expressions is true? Say, the first true, the second false? Or the first false, the second true? > + } > + bool is_contextually_typed_binary_style() const > + { > + return m_ci == &my_collation_contextually_typed_binary && > + m_type == TYPE_COLLATE_CONTEXTUALLY_TYPED; > + } ... > diff --git a/strings/ctype-bin.c b/strings/ctype-bin.c > index bb746ad90b0..a5c9129ebea 100644 > --- a/strings/ctype-bin.c > +++ b/strings/ctype-bin.c > @@ -630,3 +630,69 @@ struct charset_info_st my_charset_bin = > &my_charset_handler, > &my_collation_binary_handler > }; > + > + > +struct charset_info_st my_collation_contextually_typed_binary= > +{ > + 0,0,0, /* number */ I thought you will never need to resove the &my_collation_contextually_typed_binary pointer, that is, I thought the values in the my_collation_contextually_typed_binary struct should not matter as they'll never be used. Was that wrong? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2