- developers - lists.mariadb.org

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

Re: [Maria-developers] [Commits] 16298d9e82e: MDEV-7533: COLUMN_JSON() doesn't escape control characters in string values
by Sergei Golubchik 22 Jan '18

22 Jan '18

Hi, Oleksandr! On Jan 22, Oleksandr Byelkin wrote: > revision-id: 16298d9e82eb507c5e0ca6d097e6a7016582ccbc (mariadb-10.0.33-47-g16298d9e82e) > parent(s): 5e87f49a9927bf3fd0bd9af789bbdd329e4b4416 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2018-01-22 16:53:10 +0100 > message: > > MDEV-7533: COLUMN_JSON() doesn't escape control characters in string values > > escape all charecters less or equal 0x1F (control symbols) > (shorter sequence are not used to make code simple, long encoding is > always legal according to the rfc4627) ok to push Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] MDEV-14603 signal 11 with short stacktrace
by Alexander Barkov 22 Jan '18

22 Jan '18

Hi Sergei, can you please review a patch for MDEV-14603? Thanks!

2 5

Re: [Maria-developers] [JIRA] (MDEV-14185) CREATE TEMPORARY TABLE AS SELECT causes error 1290 with read_only and InnoDB
by Sergei Golubchik 22 Jan '18

22 Jan '18

Hi, Vesa! On Jan 16, Vesa Pentti (JIRA) wrote: > > Vesa Pentti updated MDEV-14185: > ------------------------------- > Status: In Review (was: In Progress) > Assignee: Sergei Golubchik (was: Vesa Pentti) > > commit d4366c4a2c9b41901b11ca180d3ddbf79e992490 Please send commit emails next time. This time I had to go and download the patch from github and paste it in an email. > diff --git a/sql/handler.cc b/sql/handler.cc > index 174034a7eb55..85b73719f0e8 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -4387,8 +4390,11 @@ handler::ha_create_partitioning_metadata(const char *name, const char *old_name, > partitions. > */ > DBUG_ASSERT(m_lock_type == F_UNLCK || > - (!old_name && strcmp(name, table_share->path.str))); > - mark_trx_read_write(); > + (!old_name && table_share && strcmp(name, table_share->path.str))); > + if (should_mark_rw) > + { > + mark_trx_read_write(); > + } Just remove mark_trx_read_write() from ha_create_partitioning_metadata(). It's a non-transactional operation anyway. It's always a part of DDL, so will have the same protection as DDL does. > > return create_partitioning_metadata(name, old_name, action_flag); > } Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 47bb2dcbcc8: MDEV-14786: Server crashes in Item_cond::transform on 2nd execution of SP querying from a view
by Sergei Golubchik 22 Jan '18

22 Jan '18

Hi, Oleksandr! On Jan 21, Oleksandr Byelkin wrote: > revision-id: 47bb2dcbcc8c3cdca43baf6259f2434a746c7b03 (mariadb-5.5.58-42-g47bb2dcbcc8) > parent(s): c0e964dcf1787a101f6e9c115751c0d02b0fac1b > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2018-01-21 23:48:32 +0100 > message: > > MDEV-14786: Server crashes in Item_cond::transform on 2nd execution of SP querying from a view > > MDEV-14957: JOIN::prepare gets unusable "conds" as argument > > Do not touch merged derived (it is irreversible) > > Fix first argument of in_optimizer for calls possible before fix_fields() > > diff --git a/sql/item.cc b/sql/item.cc > index 332e027adf1..562c0bec78d 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -10010,7 +10010,7 @@ const char *dbug_print_item(Item *item) > if (!item) > return "(Item*)NULL"; > item->print(&str ,QT_ORDINARY); > - if (str.c_ptr() == buf) > + if (str.ptr() == buf) c_ptr_safe() here. It's MDEV-14981. > return buf; > else > return "Couldn't fit into buffer"; > diff --git a/sql/item.h b/sql/item.h > index 4d0860af547..add408602fc 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -4478,4 +4478,11 @@ class Item_iterator_row: public Item_iterator > void close() {} > }; > > +#ifdef DBUG_OFF > +static inline const char *dbug_print_item(Item *item) { return NULL; } > +#else > +extern const char *dbug_print_item(Item *item); > +#endif Put it not at the end, but at the beginning - above 'class Protocol;' That's where someone had it in 10.2, it'll be easier to merge if you add it in the same place. And no 'extern' in the #else branch, just as in 10.2. > #endif /* SQL_ITEM_INCLUDED */ > diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc > index 62e76922c0e..a40340baae9 100644 > --- a/sql/item_cmpfunc.cc > +++ b/sql/item_cmpfunc.cc > @@ -1443,6 +1443,33 @@ bool Item_in_optimizer::eval_not_null_tables(uchar *opt_arg) > } > > > +void Item_in_optimizer::print(String *str, enum_query_type query_type) > +{ > + restore_first_argumet(); > + Item_func::print(str, query_type); > +} > + > + > +/** > + "Restore" first argument before fix_fields() call (after it is harmless). > + > + @Note: Main pointer to left part of IN/ALL/ANY subselect is subselect's > + lest_expr (see Item_in_optimizer::fix_left) so changes made during > + fix_fields will be rolled back there which can make > + Item_in_optimizer::args[0] unusable on second execution before fix_left() > + call. This call fix the pointer. > +*/ > + > +void Item_in_optimizer::restore_first_argumet() > +{ > + if (args[1]->type() == Item::SUBSELECT_ITEM && > + ((Item_subselect *)args[1])->is_in_predicate()) > + { > + args[0]= ((Item_in_subselect *)args[1])->left_expr; > + } > +} Any way to trigger the bug without dbug_print_item() ? Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0