- developers - lists.mariadb.org

Re: [Maria-developers] [Commits] a0ea3a58224: MDEV-14857: problem with 10.2.11 server crashing when executing stored procedure
by Sergey Petrunia 31 Jan '18

31 Jan '18

Hi Sanja, Please find my review below. Lots of typos, but the two important comments are at the bottom, for the changes in sql_select.cc and sql_trigger.cc On Sat, Jan 27, 2018 at 11:39:10AM +0100, Oleksandr Byelkin wrote: > revision-id: a0ea3a5822486fa1702d6615f1be30684c3d412f (mariadb-10.1.30-26-ga0ea3a58224) > parent(s): 524221e7a34d42214e82dd868348b453f2ef1591 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2018-01-27 11:38:38 +0100 > message: > > MDEV-14857: problem with 10.2.11 server crashing when executing stored procedure > > Counter for select numbering made stored with the statement (before was global) > So now it does have always accurate value which does not depend on > interruption of statement prepare by errors like lack of table in > a view definition. > > --- > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 4556487bdfe..2dc2fb61e9d 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -1027,6 +1027,21 @@ class Statement: public ilink, public Query_arena > > LEX_STRING name; /* name for named prepared statements */ > LEX *lex; // parse tree descriptor > + /* > + LEX whith represent curent statement (conventional, SP or PS) s/whith/which/ s/curent/current/ s/represent/represents/ > + > + For example during view parsing THD::lex will point to the views LEX and > + THD::stmt_lex will point on LEX of the statement where the view will be s/point on/point to/ > + included > + > + Now used to have alwais correct select numbering inside statemet s/Now used/Currently it is used/ s/alwais/always/ s/statemet/statement/ > + (LEX::current_select_number) without storing and restoring a global > + counter which was THD::select_number. > + > + TODO: make some unified statement representation (now SP has different) > + to store such data like LEX::current_select_number. > + */ > + LEX *stmt_lex; > /* > Points to the query associated with this statement. It's const, but > we need to declare it char * because all table handlers are written > diff --git a/sql/sql_lex.h b/sql/sql_lex.h > index b57fba08b47..a2da6f672ca 100644 > --- a/sql/sql_lex.h > +++ b/sql/sql_lex.h > @@ -727,7 +727,13 @@ class st_select_lex: public st_select_lex_node > Item *prep_having;/* saved HAVING clause for prepared statement processing */ > /* Saved values of the WHERE and HAVING clauses*/ > Item::cond_result cond_value, having_value; > - /* point on lex in which it was created, used in view subquery detection */ > + /* > + Point on lex in which it was created, used in view subquery detection. "Points to the LEX...". Also please shift the whole comment 1 position to the left. > + > + TODO: make also st_select_lex::parent_stmt_lex (see THD::stmt_lex) > + and use st_select_lex::parent_lex & st_select_lex::parent_stmt_lex > + instead of global (from THD) references where it is possible. > + */ > LEX *parent_lex; > enum olap_type olap; > /* FROM clause - points to the beginning of the TABLE_LIST::next_local list. */ > @@ -2452,7 +2458,7 @@ struct LEX: public Query_tables_list > /** SELECT of CREATE VIEW statement */ > LEX_STRING create_view_select; > > - uint number_of_selects; // valid only for view > + uint current_select_number; // valid for statmet LEX (not view) s/statmet/statement/ > > /** Start of 'ON table', in trigger statements. */ > const char* raw_trg_on_table_name_begin; > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 6084c59a257..8d3116cd8b3 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -6903,7 +6903,12 @@ void THD::reset_for_next_command(bool do_clear_error) > clear_error(1); > > thd->free_list= 0; > - thd->select_number= 1; > + /* > + The same thd->stmt_lex made in lex_start, but during bootstrup this "We also assign thd->stmt_lex in lex_start()" s/bootstrup/bootstrap > + code executed first code is executed > + */ > + thd->stmt_lex= &main_lex; thd->stmt_lex->current_select_number= 1; > + DBUG_PRINT("info", ("Lex %p stmt_lex: %p", thd->lex, thd->stmt_lex)); > /* > Those two lines below are theoretically unneeded as > THD::cleanup_after_query() should take care of this already. > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index b9fe8f3162a..7a6a028ee9c 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -2462,6 +2462,17 @@ void JOIN::save_explain_data(Explain_query *output, bool can_overwrite, > bool need_tmp_table, bool need_order, > bool distinct) > { > + /* > + If there is SELECT in this statemet with the same number it must be the > + same SELECT > + */ > + DBUG_ASSERT(select_lex->select_number == UINT_MAX || > + select_lex->select_number == INT_MAX || > + !output || Is it really possible that output==NULL? If yes, that's probably a bug. > + !output->get_select(select_lex->select_number) || > + output->get_select(select_lex->select_number)->select_lex == > + select_lex); > + > if (select_lex->select_number != UINT_MAX && > select_lex->select_number != INT_MAX /* this is not a UNION's "fake select */ && > have_query_plan != JOIN::QEP_NOT_PRESENT_YET && > @@ -24601,6 +24612,11 @@ int JOIN::save_explain_data_intern(Explain_query *output, bool need_tmp_table, > { > explain= new (output->mem_root) Explain_select(output->mem_root, > thd->lex->analyze_stmt); > + if (!explain) > + DBUG_RETURN(1); // EoM > +#ifndef DBUG_OFF > + explain->select_lex= select_lex; > +#endif > join->select_lex->set_explain_type(true); > > explain->select_id= join->select_lex->select_number; > diff --git a/sql/sql_trigger.cc b/sql/sql_trigger.cc > index 0b4978b2862..878cd5a3b57 100644 > --- a/sql/sql_trigger.cc > +++ b/sql/sql_trigger.cc > @@ -1378,7 +1378,7 @@ bool Table_triggers_list::check_n_load(THD *thd, const char *db, > ulonglong save_sql_mode= thd->variables.sql_mode; > LEX_STRING *on_table_name; > > - thd->lex= &lex; > + thd->lex= thd->stmt_lex= &lex; > The same comment as before: we change thd->stmt_lex here. The function has code below that restores the value of thd->lex: // QQ: anything else ? lex_end(&lex); thd->lex= old_lex; But the value of thd->stmt_lex is not restored? > save_db.str= thd->db; > save_db.length= thd->db_length; BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] Wording suggestion?
by Sergei Golubchik 30 Jan '18

30 Jan '18

Hi, Russell! On Jan 30, Russell J.T. Dyer wrote: > How about concurrent_assign ? I think "concurrent" already has a clear meaning, at least in MariaDB - operations are done "concurrently" if they're executed truly in parallel, usually in different threads. For example, both MyISAM and InnoDB allow concurrent SELECTs, but only InnoDB allows concurrent UPDATEs. I considered simultaneous_assign, as a slightly shorter alternative to simultaneous_assignment. But it looked somewhat unnatural to me. Still, this is the case when I happily yield to the opinion of a documentation writer :) If you think that simultaneous_assign looks just fine, I'll use it. > > It's not only for update. There is a patch to fix SELECT ... INTO, > > and I suppose we'll have "simultaneous assignment" behavior > > everywhere eventually. > > > > So, I'm leaning towards either simultaneous_assignment or > > simultaneous_set as mode names. The first is more exact, but it's > > rather long (sql_mode names range from 3 to 26 characters, and this > > one is 23). The second (with 16) fits right in the middle of the > > range, length-wise. But there's no SET keyword in SELECT ... INTO. > > On the other hand, there's no ASSIGNMENT keyword anywhere either :) > > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] Mdev-15085 Invisible Column Non-constant Default value results...
by Sergei Golubchik 30 Jan '18

30 Jan '18

Hi, Sachin! ok to push! On Jan 30, Sachin Setiya wrote: > commit a2e6d3a2fe6842bd3c35dffb1cd6f6d8a3117107 > Author: Sachin Setiya <sachin.setiya(a)mariadb.com> > Date: Tue Jan 30 14:04:05 2018 +0530 > > Mdev-15085 Invisible Column Non-constant Default value results... > > Problem:- If we create table field with dynamic default value then that > field always gets NULL value. > > Analyze:- This is because in fill_record we simple continue at Invisible > column because we though that share->default_values(default value is > always copied into table->record[0] before insert) will have a default > value for them(which is true for constant defaults , but not for dynamic > defaults). > > Solution:- We simple set all_fields_have_value to null , and this will > make call to update_default_fields (in the case of dynamic default), And > default expr will be evaluted and value will be set in field. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] Wording suggestion?
by Sergei Golubchik 30 Jan '18

30 Jan '18

Hi, Russell! On Jan 29, Russell J.T. Dyer wrote: > Is this mode related only to UPDATE and this feature? If so, maybe it > could be something like one of these: > > update_static > update_stasis > update_isolation > > These would focus on the fact that it's related just to UPDATE and that the > values held in a state temporarily, or in isolation of each other. Sorry, I should've said earlier. It's not only for update. There is a patch to fix SELECT ... INTO, and I suppose we'll have "simultaneous assignment" behavior everywhere eventually. So, I'm leaning towards either simultaneous_assignment or simultaneous_set as mode names. The first is more exact, but it's rather long (sql_mode names range from 3 to 26 characters, and this one is 23). The second (with 16) fits right in the middle of the range, length-wise. But there's no SET keyword in SELECT ... INTO. On the other hand, there's no ASSIGNMENT keyword anywhere either :) Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 2

[Maria-developers] Wording suggestion?
by Sergei Golubchik 30 Jan '18

30 Jan '18

Hi, Ian, Russell, As you know, UPDATE in MariaDB is executed left-to-right, say, in the case of UPDATE t1 SET a=b, b=a both columns will have the same value, old value of `b`. We're now introducing a new sql_mode to have standard compatible behavior, where all assignments are executed kind of "simultaneously" (and the statement above would swap the values). What could the name for this new sql_mode be? Regards, Sergei

2 2

Re: [Maria-developers] [Commits] Mdev-15085 Rebased to latest 10.3
by Sergei Golubchik 30 Jan '18

30 Jan '18

Hi, Sachin! On Jan 29, Sachin Setiya wrote: > commit 905806689979ae91c89115f18b107ebacbc489e2 > Author: Sachin Setiya <sachin.setiya(a)maridb.com> > Date: Fri Jan 26 22:58:34 2018 +0530 > > Mdev-15085 Invisible Column Non-constant Default value results... > > Problem:- If we create table field with dynamic default value then that > field always gets NULL value. > > Analyze:- This is because in fill_record we simple continue at Invisible > column because we though that share->default_values(default value is > always copied into table->record[0] before insert) will have a default > value for them(which is true for constant defaults , but not for dynamic > defaults). > > Solution:- In case of invisible field we will assign default value to 'value' > variable, but we do this only for columns which have non dynamic default value. > We do not have to worry for Auto_increment field , storage engine will take > of it. > > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index 45efa63..17ff7eb 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -8382,7 +8382,12 @@ fill_record(THD *thd, TABLE *table, Field **ptr, > List<Item> &values, > > if (field->invisible) > { > - continue; > + if(field->flags & AUTO_INCREMENT_FLAG || > + !field->default_value) > + continue; > + else > + value= new (thd->mem_root) Item_default_value(thd, > + &thd->lex->select_lex.context); No-no, you cannot create items in fill_record(). It would create a new item for every inserted row. I think the only thing you need to do here (and it doesn't depend on AUTO_INCREMENT_FLAG or field->default_value), you need to set all_fields_have_values to false. Like if (field->invisible) { all_fields_have_values= false; continue; } Indeed, hidden columns did not get a value, so all_fields_have_values should be false. I'd expect the rest will work automatically :) > } > else > value=v++; Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 1

Re: [Maria-developers] MDEV-14849 CREATE + ALTER with user-invisible columns produce ...
by Sergei Golubchik 29 Jan '18

29 Jan '18

Hi, Sachin! On Jan 29, Sachin Setiya wrote: > commit 5bd97f22094f3b6a658d503f98022ccd9076bb19 > Author: Sachin Setiya <sachin.setiya(a)maridb.com> > Date: Mon Jan 29 12:31:07 2018 +0530 > > MDEV-14849 CREATE + ALTER with user-invisible columns produce ... > > Problem:- > create or replace table t1 (pk int auto_increment primary key invisible, i int); > alter table t1 modify pk int invisible; > This last alter makes a invisible column which is not null and does not > have default value. > > Analysis:- > This is caused because our error check for NOT_NULL_FLAG and > NO_DEFAULT_VALUE_FLAG flag misses this sql_field , but this is not the fault > of error check :).Actually this field come via mysql_prepare_alter_table > and it does not have NO_DEFAULT_VALUE_FLAG flag turned on. (If it was create > table NO_DEFAULT_VALUE_FLAG would have turned on Column_definition::check) > and this would have generated error. > > Solution:- > I have moved the error check to kind last of mysql_prepare_create_table > because upto this point we have applied NO_DEFAULT_VALUE_FLAG to required > column. ok to push! Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] MDEV-14785 SYSTEM_INVISIBLE behaviour not consistent ---Initial Patch
by Sergei Golubchik 29 Jan '18

29 Jan '18

Hi, Sachin! On Jan 29, Sachin Setiya wrote: > commit 5e2f5aa78a5f53b000a2b9c6989c9ed7076e2527 > Author: Sachin Setiya <sachin.setiya(a)maridb.com> Note the typo in your email ^^^^^ > Date: Mon Jan 29 17:09:32 2018 +0530 > > MDEV-14785 SYSTEM_INVISIBLE behaviour not consistent > > TODO Commit message > > diff --git a/sql/item.h b/sql/item.h > index d178d9f..0072dfd 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -2907,11 +2907,17 @@ class Item_field :public Item_ident > bool check_vcol_func_processor(void *arg) > { > context= 0; > - if (field && (field->unireg_check == Field::NEXT_NUMBER)) > + if (field) > { > - // Auto increment fields are unsupported > - return mark_unsupported_function(field_name.str, arg, VCOL_FIELD_REF | VCOL_AUTO_INC); > + if (field->unireg_check == Field::NEXT_NUMBER) > + // Auto increment fields are unsupported > + return mark_unsupported_function(field_name.str, arg, > + VCOL_FIELD_REF | VCOL_AUTO_INC); > + if (field->invisible > INVISIBLE_USER) > + return mark_unsupported_function(field_name.str, arg, > + VCOL_FIELD_REF | VCOL_SYSTEM_INVISIBLE); Why would you do that, instead of hiding system invisible fields in CREATE TABLE? From the user point of view they aren't fields, they don't exist in the table, and the error should be "no such field", not "Function or expression 'on SYSTEM_INVISIBLE column' cannot be used" > } > + > return mark_unsupported_function(field_name.str, arg, VCOL_FIELD_REF); > } > bool set_fields_as_dependent_processor(void *arg) > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index 17ff7eb..89ba2ed 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -8128,6 +8128,11 @@ fill_record(THD *thd, TABLE *table_arg, List<Item> &fields, List<Item> &values, > value=v++; > DBUG_ASSERT(value); > Field *rfield= field->field; > + if (rfield->invisible == INVISIBLE_SYSTEM) > + { > + my_error(ER_BAD_FIELD_ERROR, MYF(0), rfield->field_name.str, thd->where); The error is correct. But do you need to do it in fill_record()? I'd thought you can do it when the statement is prepared, once, not in fill_record() which is called for every row, many times per statement. > + goto err; > + } > TABLE* table= rfield->table; > if (table->next_number_field && > rfield->field_index == table->next_number_field->field_index) > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index d2fd2ce..e4971d7 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -16639,6 +16639,7 @@ Field *create_tmp_field_from_field(THD *thd, Field *org_field, > new_field->next_equal_field= NULL; > new_field->option_list= NULL; > new_field->option_struct= NULL; > + new_field->invisible= org_field->invisible; What is it for? It'd mean that temporary tables can have invisible fields. What bugs does this change fix? > } > return new_field; > } > diff --git a/sql/sql_update.cc b/sql/sql_update.cc > index 7b4938f..7bdedc7 100644 > --- a/sql/sql_update.cc > +++ b/sql/sql_update.cc > @@ -1497,6 +1497,9 @@ int mysql_multi_update_prepare(THD *thd) > TABLE_LIST *table_list= lex->query_tables; > TABLE_LIST *tl; > List<Item> *fields= &lex->select_lex.item_list; > + List_iterator<Item> it(*fields); > + Item *item; > + Item_field *field; > table_map tables_for_update; > bool update_view= 0; > /* > @@ -1564,7 +1567,21 @@ int mysql_multi_update_prepare(THD *thd) > { > DBUG_RETURN(TRUE); > } > - > + //Check if we got any INVISIBLE_SYSTEM field > + while ((item= it++)) > + { > + if (Item::FIELD_ITEM == item->type()) > + { > + field= (Item_field *)item; > + if (field->field->invisible == INVISIBLE_SYSTEM) > + { > + my_error(ER_BAD_FIELD_ERROR,MYF(0), > + field->field->field_name.str, > + field->field->table->s->table_name.str); > + DBUG_RETURN(TRUE); > + } > + } > + } Dunno. What if I do UPDATE ... SET normal_field = row_start you'll have an invisible field, but it's not updated, so should be ok, right? May be something like if (field->field->invisible == INVISIBLE_SYSTEM && bitmap_is_set(field->table->write_set, field->field_index)) { ... my_error } And why here? I'd think this belongs into check_fields() function. > thd->table_map_for_update= tables_for_update= get_table_map(fields); Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] MDEV-13790 UNHEX() of a somewhat complicated CONCAT() returns NULL
by Alexander Barkov 29 Jan '18

29 Jan '18

Hello Sergei, Please review a patch for MDEV-13790. It removes the attempt to search the best suitable buffer and just returns the result in "str" passed to val_str(). I'm quite sure this optimization gives nothing. In can be useful only under very rare conditions, but generates so many bugs. Moreover, in 10.3 (or 10.4) we'll work on reentrant items. So all Item_xxx::tmp_buffer will be gone anyway, together with this optimization. I think an optimization proposal. Why not to introduce a new method: class Item { public: .. virtual append_to_string(String *str); .. } So for example Field_varchar could write directly to "str" passed to Item_func_concat::val_str(). This will make sure that the first allocation is done on the target "str", and "str" will always be the best buffer instead of any Item_xxx::tmp_value. Thanks!

2 3

Re: [Maria-developers] 980a5c31880: MDEV-14572: Assertion `! is_set()' failed in Diagnostics_area::set_eof_status upon EXPLAIN UPDATE in PS
by Sergei Golubchik 28 Jan '18

28 Jan '18

Hi, Oleksandr! On Dec 22, Oleksandr Byelkin wrote: > revision-id: 980a5c318803d6aa7355e019869597976dcf19bf (mariadb-10.0.33-37-g980a5c31880) > parent(s): 042f763268c0f209e7c12e0a6a72bb5d204dfe29 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2017-12-22 14:00:10 +0100 > message: > > MDEV-14572: Assertion `! is_set()' failed in Diagnostics_area::set_eof_status upon EXPLAIN UPDATE in PS > > Restore EXPAIN flag in SELECT_LEX before execution multi-update by flag in LEX > (the same but in other way made before INSERT/DELETE/SELECT) Please explain what was the reason for the bug, why the status was set twice, and how your change fixes it. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0