- developers - lists.mariadb.org

Re: [Maria-developers] 4ba5b444b4b: MDEV-28437: Assertion `!eliminated' failed in Item_subselect::exec
by Sergei Golubchik 04 May '22

04 May '22

Hi, Sergei, On May 04, Sergei Petrunia wrote: > revision-id: 4ba5b444b4b (mariadb-10.2.43-97-g4ba5b444b4b) > parent(s): 70555454b4c > author: Sergei Petrunia > committer: Sergei Petrunia > timestamp: 2022-05-04 12:24:48 +0300 > message: > > MDEV-28437: Assertion `!eliminated' failed in Item_subselect::exec > > (This is the assert that was added in fix for MDEV-26047) > > Table elimination may remove an ON expression from an outer join. > However SELECT_LEX::update_used_tables() will still call > > item->walk(&Item::eval_not_null_tables) > > for eliminated expressions. If the subquery is constant and cheap > Item_cond_and will attempt to evaluate it, which will trigger an > assert. > The fix is not to call update_used_tables() or eval_not_null_tables() > for ON expressions that were eliminated. > > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > index 22ee8801e3a..8e718f2a942 100644 > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -4229,7 +4229,7 @@ void SELECT_LEX::update_used_tables() > } > } > while ((embedding= embedding->embedding)); > - if (tl->on_expr) > + if (tl->on_expr && !is_eliminated_table(join->eliminated_tables, tl)) > { > tl->on_expr->update_used_tables(); > tl->on_expr->walk(&Item::eval_not_null_tables, 0, NULL); > @@ -4253,8 +4253,11 @@ void SELECT_LEX::update_used_tables() > if (embedding->on_expr && > embedding->nested_join->join_list.head() == tl) > { can embedding be eliminated? if yes - does it make sense to do anything in tl's below that eliminated embedding? Perhaps it should be (in the do..while above), like do { bool maybe_null; if ((maybe_null= MY_TEST(embedding->outer_join))) { tl->table->maybe_null= maybe_null; break; } + if (is_eliminated_table(join->eliminated_tables, embedding)) + tl= embedding->embedding; } while ((embedding= embedding->embedding)); > + if (!is_eliminated_table(join->eliminated_tables, embedding)) > + { > embedding->on_expr->update_used_tables(); > embedding->on_expr->walk(&Item::eval_not_null_tables, 0, NULL); > + } > } > tl= embedding; > embedding= tl->embedding; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 85cc56875e9: MDEV-19398: Assertion `item1->type() == Item::FIELD_ITEM ...
by Sergei Golubchik 03 May '22

03 May '22

Hi, Sergei, The solutions looks fine Two comments below, about the implementation. ok to push after addressing them On May 03, Sergei Petrunia wrote: > revision-id: 85cc56875e9 (mariadb-10.2.43-75-g85cc56875e9) > parent(s): 3b6c04f44c4 > author: Sergei Petrunia > committer: Sergei Petrunia > timestamp: 2022-05-03 14:06:27 +0300 > message: > > MDEV-19398: Assertion `item1->type() == Item::FIELD_ITEM ... > > Window Functions code tries to minimize the number of times it > needs to sort the select's resultset by finding "compatible" > OVER (PARTITION BY ... ORDER BY ...) clauses. > > This employs compare_order_elements(). That function assumed that > the order expressions are Item_field-derived objects (that refer > to a temp.table). But this is not always the case: one can > construct queries order expressions are arbitrary item expressions. > > Add handling for such expressions: sort them according to the window > specification they appeared in. > This means we cannot detect that two compatible PARTITION BY clauses > that use expressions can share the sorting step. > But at least we won't crash. > can share the same sort > > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 989ca0c8803..457849a7569 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -8624,6 +8624,7 @@ bool st_select_lex::add_window_def(THD *thd, > fields_in_window_functions+= win_part_list_ptr->elements + > win_order_list_ptr->elements; > } > + win_def->win_spec_number= window_specs.elements; > return (win_def == NULL || window_specs.push_back(win_def)); > } > > @@ -8651,6 +8652,7 @@ bool st_select_lex::add_window_spec(THD *thd, > win_order_list_ptr->elements; > } > thd->lex->win_spec= win_spec; > + win_spec->win_spec_number= window_specs.elements; > return (win_spec == NULL || window_specs.push_back(win_spec)); > } > > diff --git a/sql/sql_window.cc b/sql/sql_window.cc > index 3ef751bc5b9..fbf7aca0197 100644 > --- a/sql/sql_window.cc > +++ b/sql/sql_window.cc > @@ -312,15 +312,44 @@ setup_windows(THD *thd, Ref_ptr_array ref_pointer_array, TABLE_LIST *tables, > #define CMP_GT 2 // Greater then > > static > -int compare_order_elements(ORDER *ord1, ORDER *ord2) > +int compare_order_elements(ORDER *ord1, int weight1, > + ORDER *ord2, int weight2) > { > if (*ord1->item == *ord2->item && ord1->direction == ord2->direction) > return CMP_EQ; > Item *item1= (*ord1->item)->real_item(); > Item *item2= (*ord2->item)->real_item(); > - DBUG_ASSERT(item1->type() == Item::FIELD_ITEM && > - item2->type() == Item::FIELD_ITEM); > - ptrdiff_t cmp= ((Item_field *) item1)->field - ((Item_field *) item2)->field; > + > + bool item1_field= (item1->type() == Item::FIELD_ITEM); > + bool item2_field= (item2->type() == Item::FIELD_ITEM); > + > + ptrdiff_t cmp; > + if (item1_field && item2_field) > + cmp= ((Item_field *) item1)->field - ((Item_field *) item2)->field; 1. why is this order stable? I'd rather sort by item->field->field_index 2. please, add an assert, that item1->field->table == item2->field->table > + else if (item1_field && !item2_field) > + return CMP_LT; > + else if (!item1_field && item2_field) > + return CMP_LT; > + else > + { > + /* > + Ok, item1_field==NULL and item2_field==NULL. > + We're not able to compare Item expressions. Order them according to > + their passed "weight" (which comes from Window_spec::win_spec_number): > + */ > + if (weight1 != weight2) > + cmp= weight1 - weight2; > + else > + { > + /* > + The weight is the same. That is, the elements come from the same > + window specification... This shouldn't happen. > + */ > + DBUG_ASSERT(0); > + cmp= item1 - item2; > + } > + } > + > if (cmp == 0) > { Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

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

26 Apr '22

Hi, Rucha, Looks good. please also check that row_number values match what's in a warning, and then ok to push! On Apr 26, Rucha Deodhar wrote: > On Tue, Apr 26, 2022 at 12:41 AM Sergei Golubchik <serg(a)mariadb.org> wrote: > > 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. > > > > > It gives 3 as row the number (in warning). Added a test for it. > Please find it here: 1c0731e0544db4d2f5c107473776bbd6d4d3e2f0 > > > 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) > > > > Removed. > > > > 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] 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