- developers - lists.mariadb.org

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

Re: [Maria-developers] [Commits] ee8755e3c51: MDEV-15012: ASAN: numerous test failures in PS
by Sergei Golubchik 26 Jan '18

26 Jan '18

Hi, Oleksandr! Nice one! Ok to push! On Jan 24, Oleksandr Byelkin wrote: > revision-id: ee8755e3c51a1da8fcf108ad0257a7e62fc94347 (mariadb-5.5.59-32-gee8755e3c51) > parent(s): 76577e1e2602f3c30859a176808c433a263e1b0a > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2018-01-24 14:42:52 +0100 > message: > > MDEV-15012: ASAN: numerous test failures in PS > > First roll back changes, then free Items which can lead to memory freeing. > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] Patch for MDEV-13417 - UPDATE produces wrong values if an updated column is later used as an update source
by jerome brauge 24 Jan '18

24 Jan '18

Hello Sergei, Here is a new version of the patch following your comments. I let you decide if SET_CONSISTENCY must also be part of TRADITIONAL and ANSI. Regards, Jérôme. > -----Message d'origine----- > De : Sergei Golubchik [mailto:serg@mariadb.org] > Envoyé : jeudi 11 janvier 2018 23:17 > À : jerome brauge > Cc : bar(a)mariadb.org > Objet : Re: Patch for MDEV-13417 - UPDATE produces wrong values if an > updated column is later used as an update source > > Hi, Jérôme! > > Happy New Year! > > Sorry for the delay. > > But I've finally got around to reviewing your MDEV-13417 > > The patch was pretty good and with good tests too! > > I had only a few minor comments, see below. > If you'd like, I can apply your patch and do these changes myself. > > See the review below. > > On Dec 28, jerome brauge wrote: > > Hello Sergei, > > Do you have time to review my patch for this mdev ? > > This is very important for us because I don't see any workaround > > (without unique key and cursor). > > > diff --git a/sql/sql_class.h b/sql/sql_class.h index f30b442..a5e2104 > > 100644 > > --- a/sql/sql_class.h > > +++ b/sql/sql_class.h > > @@ -139,6 +139,7 @@ enum enum_binlog_row_image { > > #define MODE_HIGH_NOT_PRECEDENCE (1ULL << 29) > > #define MODE_NO_ENGINE_SUBSTITUTION (1ULL << 30) > > #define MODE_PAD_CHAR_TO_FULL_LENGTH (1ULL << 31) > > +#define MODE_SET_CONSISTENCY (1ULL << 32) > > I don't particularly like "SET_CONSISTENCY" name, but cannot come up with a > good alternative. It doesn't matter, renaming the mode is a trivial change > that doesn't affect anything else. > So, unless you have a strong preference for some specific mode name - just > ignore this naming issue completely. > And I'll ask our documentation guys if they could suggest something. > > > > > /* Bits for different old style modes */ > > #define OLD_MODE_NO_DUP_KEY_WARNINGS_WITH_IGNORE (1 << 0) > > diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 60247c8..debb81a > > 100644 > > --- a/sql/sql_base.cc > > +++ b/sql/sql_base.cc > > @@ -7952,15 +7954,41 @@ fill_record(THD *thd, TABLE *table_arg, > List<Item> &fields, List<Item> &values, > > ER_THD(thd, > ER_WARNING_NON_DEFAULT_VALUE_FOR_VIRTUAL_COLUMN), > > rfield->field_name.str, table->s->table_name.str); > > } > > - if (rfield->stored_in_db() && > > - (value->save_in_field(rfield, 0)) < 0 && !ignore_errors) > > + if (rfield->stored_in_db()) > > { > > - my_message(ER_UNKNOWN_ERROR, ER_THD(thd, > ER_UNKNOWN_ERROR), MYF(0)); > > - goto err; > > + if (value->save_in_field(rfield, 0) < 0 && !ignore_errors) > > + { > > + my_message(ER_UNKNOWN_ERROR, ER_THD(thd, > ER_UNKNOWN_ERROR), MYF(0)); > > + goto err; > > + } > > + /* > > + ** In sql MODE_SET_CONSISTENCY, > > + ** move field pointer on value stored in record[1] > > + ** which contains row before update (see MDEV-13417) > > + */ > > + if (update && thd->variables.sql_mode & MODE_SET_CONSISTENCY) > > + rfield->move_field_offset((my_ptrdiff_t) (table->record[1] - > > + table->record[0])); > > Cool! A simple and nice solution :) > > > } > > rfield->set_explicit_default(value); > > } > > > > + if (update && thd->variables.sql_mode & MODE_SET_CONSISTENCY) { > > + // restore fields pointers on record[0] > > + f.rewind(); > > + while ((fld= f++)) > > + { > > + rfield= fld->field_for_view_update()->field; > > + if (rfield->stored_in_db()) > > + { > > + table= rfield->table; > > + rfield->move_field_offset((my_ptrdiff_t) (table->record[0] - > > + table->record[1])); > > + } > > + } > > + } > > + > > if (!update && table_arg->default_field && > > table_arg->update_default_fields(0, ignore_errors)) > > goto err; > > diff --git a/sql/sql_update.cc b/sql/sql_update.cc index > > 568dd40..5796b96 100644 > > --- a/sql/sql_update.cc > > +++ b/sql/sql_update.cc > > @@ -148,7 +148,25 @@ bool compare_record(const TABLE *table) > > we make temporary copy of Item_field, to avoid influence of changing > > result_field on Item_ref which refer on this field > > */ > > thd->change_item_tree(it.ref(), new (thd->mem_root) Item_field(thd, > field)); > > } > > + } > > + > > + if (thd->variables.sql_mode & MODE_SET_CONSISTENCY) { > > + // Make sure that a column is updated only once > > + List_iterator_fast<Item> it(items); > > + List<Item> ul; > > + ul.empty(); > > + while ((item= it++) && !ul.add_unique(item, &cmp_items)) ; > > Dunno, it's O(N²) and item->eq() is not trivial either. > perhaps it'd be faster, say, mark fields some way, like > > while ((item= it++)) > { > Field *f= item->field_for_view_update()->field; > if (f->has_explicit_value()) > { > my_error(ER_UPDATED_COLUMN_ONLY_ONCE, MYF(0), > f->table_name, f->field_name.str); > return TRUE; > } > f->set_has_explicit_value(); > } > > note that this assumes the change in the error message, %s -> %`s.%`s > > > + > > + if (item) > > + { > > + // same field appear more than once in a single update statement. > > + my_error(ER_UPDATED_COLUMN_ONLY_ONCE, MYF(0), item- > >full_name()); > > + return TRUE; > > + } > > + } > > return FALSE; > > } > > diff --git a/mysql-test/r/set_consistency.result > > b/mysql-test/r/set_consistency.result > > new file mode 100644 > > index 0000000..747b6b3 > > --- /dev/null > > +++ b/mysql-test/r/set_consistency.result > > @@ -0,0 +1,185 @@ > > +SET > > > +sql_mode='STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_A > UTO_CREA > > +TE_USER,NO_ENGINE_SUBSTITUTION,SET_CONSISTENCY'; > > shouldn't SET_CONSISTENCY be part of TRADITIONAL, ORACLE, and ANSI? > > > +# > > +# MDEV-13417 UPDATE produces wrong values if an UPDATEd column is > > +later used as an UPDATE source # MDEV-13418 Compatibility: The order > > +of evaluation of SELECT..INTO assignments # CREATE TABLE t1 (c1 > > +INTEGER, c2 INTEGER, c3 INTEGER) ENGINE=InnoDb; INSERT INTO > > +t1(c1,c2,c3) VALUES (1,1,1); CREATE TABLE t2 (c1 INTEGER, c2 > > +INTEGER, c3 INTEGER) ENGINE=InnoDb; INSERT INTO t2(c1,c2,c3) VALUES > > +(1,1,1); > > + > > +Check that a column is only updated once. > > + > > please, comment out these messages, just like the MDEV numbers above > > > +UPDATE t1 > > +SET c1 = 1, > > +c1 = 2; > > +ERROR HY000: The column test.t1.c1 cannot be changed more than once > > +in a single UPDATE statement > > Regards, > Sergei > Chief Architect MariaDB > and security(a)mariadb.org

2 1