April 2022 - developers - lists.mariadb.org

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