- developers - lists.mariadb.org

Re: [Maria-developers] 99e2a49acfc: MDEV-27018 IF and COALESCE lose "json" property
by Sergei Golubchik 20 Jan '22

20 Jan '22

Hi, Alexander! On Jan 13, Alexander Barkov wrote: > revision-id: 99e2a49acfc (mariadb-10.5.13-33-g99e2a49acfc) > parent(s): 2776635cb98 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-01-10 18:05:55 +0400 > message: > > MDEV-27018 IF and COALESCE lose "json" property > > Hybrid functions (IF, COALESCE, etc) did not preserve the JSON property > from their arguments. The same problem was repeatable for single row subselects. > > The problem happened because the method Item::is_json_type() was inconsistently > implemented across the Item hierarchy. For example, Item_hybrid_func > and Item_singlerow_subselect did not override is_json_type(). > > Solution: > > - Removing Item::is_json_type() > > - Implementing specific JSON type handlers: > Type_handler_string_json > Type_handler_varchar_json > Type_handler_tiny_blob_json > Type_handler_blob_json > Type_handler_medium_blob_json > Type_handler_long_blob_json > > - Reusing the existing data type infrastructure to pass JSON > type handlers across all item types, including classes Item_hybrid_func > and Item_singlerow_subselect. Note, these two classes themselves do not > need any changes! > > - Extending the data type infrastructure so data types can inherit > their properties (e.g. aggregation rules) from their base data types. > E.g. VARCHAR/JSON acts as VARCHAR, LONGTEXT/JSON acts as LONGTEXT > when mixed to a non-JSON data type. This is done by: > - adding virtual method Type_handler::type_handler_base() > - adding class Recursive_type_pair_iterator > - refactoring Type_handler_hybrid_field_type methods > aggregate_for_result(), aggregate_for_min_max(), > aggregate_for_num_op() to use Recursive_type_pair_iterator. > > This change also fixes: > > MDEV-27361 Hybrid functions with JSON arguments do not send format metadata > > diff --git a/sql/field.cc b/sql/field.cc > index 2c768527ced..8fa3bbd538c 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -7277,6 +7277,19 @@ bool Field_longstr::send(Protocol *protocol) > } > > > +const Type_handler *Field_string::type_handler() const > +{ > + if (is_var_string()) > + return &type_handler_var_string; shouldn't it be after json check? > + /* > + This is a temporary solution and will be fixed soon (in 10.9?). > + Type_handler_string_json will provide its own Field_string_json. > + */ > + if (Type_handler_json_common::has_json_valid_constraint(this)) > + return &type_handler_string_json; > + return &type_handler_string; > +} > + > /* Copy a string and fill with space */ > > int Field_string::store(const char *from, size_t length,CHARSET_INFO *cs) > diff --git a/sql/item_jsonfunc.cc b/sql/item_jsonfunc.cc > index ddf5fc32ea4..81003be4656 100644 > --- a/sql/item_jsonfunc.cc > +++ b/sql/item_jsonfunc.cc > @@ -1441,6 +1441,32 @@ longlong Item_func_json_contains_path::val_int() > } > > > +/* > + This reproduces behavior according to the former > + Item_func_conv_charset::is_json_type() which returned args[0]->is_json_type(). > + JSON functions with multiple string input with different character sets > + wrap some arguments into Item_func_conv_charset. So the former > + Item_func_conv_charset::is_json_type() took the JSON propery from args[0], > + i.e. from the original argument before the conversion. > + This is probably not always correct because an *explicit* > + `CONVERT(arg USING charset)` is actually a general purpose string > + expression, not a JSON expression. > +*/ > +static bool is_json_type(const Item *item) > +{ > + for ( ; ; ) > + { > + if (Type_handler_json_common::is_json_type_handler(item->type_handler())) > + return true; > + const Item_func_conv_charset *func; > + if (!(func= dynamic_cast<const Item_func_conv_charset*>(item))) > + return false; > + item= func->arguments()[0]; can you have nested CONVERT()'s ? > + } > + return false; > +} > + > + > static int append_json_value(String *str, Item *item, String *tmp_val) > { > if (item->type_handler()->is_bool_type()) > diff --git a/sql/sql_type_json.h b/sql/sql_type_json.h > index 6c4ee8cb2eb..4a394809a06 100644 > --- a/sql/sql_type_json.h > +++ b/sql/sql_type_json.h > @@ -21,18 +21,145 @@ ... > +template <class BASE, const Named_type_handler<BASE> &thbase> > +class Type_handler_general_purpose_string_to_json: > + public BASE, > + public Type_handler_json_common > { ... > + bool Item_hybrid_func_fix_attributes(THD *thd, > + const char *name, > + Type_handler_hybrid_field_type *hybrid, > + Type_all_attributes *attr, > + Item **items, uint nitems) > + const override > + { > + if (BASE::Item_hybrid_func_fix_attributes(thd, name, hybrid, attr, > + items, nitems)) > + return true; > + /* > + The above call can change the type handler on "hybrid", e.g. > + choose a proper BLOB type handler according to the calculated max_length. > + Convert general purpose string type handler to its JSON counterpart. > + This makes hybrid functions preserve JSON data types, e.g.: > + COALESCE(json_expr1, json_expr2) -> JSON > + */ > + hybrid->set_handler(json_type_handler_from_generic(hybrid->type_handler())); When this line would change hybrid->type_handler() ? > + return false; > + } > }; ..... > diff --git a/sql/sql_type.cc b/sql/sql_type.cc > index c1801c1ae3e..3b2753d80a6 100644 > --- a/sql/sql_type.cc > +++ b/sql/sql_type.cc > @@ -1755,19 +1755,110 @@ const Type_handler *Type_handler_typelib::cast_to_int_type_handler() const > > /***************************************************************************/ > > +class Recursive_type_pair_iterator > +{ > + const Type_handler *m_a; > + const Type_handler *m_b; > + uint m_switched_to_base_count; > +public: > + Recursive_type_pair_iterator(const Type_handler *a, > + const Type_handler *b, > + uint switched_to_base_count= 0) > + :m_a(a), m_b(b), m_switched_to_base_count(switched_to_base_count) > + { } > + const Type_handler *a() const { return m_a; } > + const Type_handler *b() const { return m_b; } > + Recursive_type_pair_iterator base() const > + { > + Recursive_type_pair_iterator res(m_a->type_handler_base(), > + m_b->type_handler_base()); > + res.m_switched_to_base_count= (res.m_a != NULL) + (res.m_b != NULL); > + if (res.m_a == NULL) > + res.m_a= m_a; > + if (res.m_b == NULL) > + res.m_b= m_b; > + return res; that's an unusual semantics, not what I would expect from an iterator. It'd expect it to iterare with it++ or it.next() but not it = it.base() > + } > + bool done() const > + { > + switch (m_switched_to_base_count) > + { > + case 2: I don't think case 2 is possible anymore. ... > + } > + } > +}; > + > + > bool > Type_handler_hybrid_field_type::aggregate_for_result(const Type_handler *other) > { > - const Type_handler *hres; > - const Type_collection *c; > - if (!(c= Type_handler::type_collection_for_aggregation(m_type_handler, other)) || > - !(hres= c->aggregate_for_result(m_type_handler, other))) > - hres= type_handler_data-> > - m_type_aggregator_for_result.find_handler(m_type_handler, other); > - if (!hres) > - return true; > - m_type_handler= hres; > - return false; > + Recursive_type_pair_iterator it(m_type_handler, other); > + for ( ; ; ) do you really still need to do recursive? in what cases? > + { > + const Type_handler *hres; > + const Type_collection *c; > + if (((c= Type_handler::type_collection_for_aggregation(it.a(), it.b())) && > + (hres= c->aggregate_for_result(it.a(), it.b()))) || > + (hres= type_handler_data-> > + m_type_aggregator_for_result.find_handler(it.a(), it.b()))) > + { > + m_type_handler= hres; > + return false; > + } > + if ((it= it.base()).done()) > + break; > + } > + return true; > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 4

Re: [Maria-developers] e4ea1544097: MDEV-17124: mariadb 10.1.34, views and prepared statements: ERROR 1615 (HY000): Prepared statement needs to be re-prepared
by Sergei Golubchik 20 Jan '22

20 Jan '22

Hi, Oleksandr, On Jan 19, Oleksandr Byelkin wrote: > revision-id: e4ea1544097 (mariadb-10.2.40-198-ge4ea1544097) > parent(s): fd7a553cb80 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2022-01-19 11:12:57 +0100 > message: > > MDEV-17124: mariadb 10.1.34, views and prepared statements: ERROR 1615 (HY000): Prepared statement needs to be re-prepared > > The problem is that if table definition cache (TDC) is full of real tables > which are in tables cache, view definition can not stay there so will be > removed by its own underlying tables. > In situation above old mechanism of detection matching definition in PS > and current version always require reprepare and so prevent executing > the PS. > > One work around is to increase TDC, other - improve version check for > views/triggers (which is done here). Now in suspicious cases we check: > - MD5 of the view to be sure that version really have changed; > - time of creation of a trigger related to time of statement preparation. > > diff --git a/mysql-test/r/ps_ddl.result b/mysql-test/r/ps_ddl.result > index e69c6e06193..005c78a0319 100644 > --- a/mysql-test/r/ps_ddl.result > +++ b/mysql-test/r/ps_ddl.result > @@ -214,7 +214,7 @@ new trigger: 10 > drop trigger t1_bd; > set @val=11; > execute stmt using @val; > -call p_verify_reprepare_count(1); > +call p_verify_reprepare_count(0); > SUCCESS > > select @message; > @@ -831,8 +831,8 @@ a b c > 20 40 100 > 30 60 150 > call p_verify_reprepare_count(1); > -SUCCESS > - > +ERROR > +Expected: 1, actual: 0 > # Check that we properly handle ALTER VIEW statements. > execute stmt; > a b c > @@ -882,9 +882,9 @@ a b c > 10 50 60 > 20 100 120 > 30 150 180 > -call p_verify_reprepare_count(1); > -SUCCESS > - > +call p_verify_reprepare_count(0); > +ERROR > +Expected: 0, actual: 1 > execute stmt; > a b c you forgot to update the expected number or reprepares in two places. > diff --git a/sql/parse_file.cc b/sql/parse_file.cc > index 999f53bd681..3c750f4df29 100644 > --- a/sql/parse_file.cc > +++ b/sql/parse_file.cc > @@ -145,6 +145,7 @@ write_parameter(IO_CACHE *file, uchar* base, File_option *parameter) > > switch (parameter->type) { > case FILE_OPTIONS_STRING: > + case FILE_OPTIONS_FIXSTRING: > { > LEX_STRING *val_s= (LEX_STRING *)(base + parameter->offset); > if (my_b_write(file, (const uchar *)val_s->str, val_s->length)) > @@ -830,6 +831,22 @@ File_parser::parse(uchar* base, MEM_ROOT *mem_root, > } > ptr= eol+1; > break; > + case FILE_OPTIONS_FIXSTRING: > + { > + /* string have to be allocated already and length set */ > + LEX_STRING *val= (LEX_STRING *)(base + parameter->offset); > + DBUG_ASSERT(val->length != 0); > + if (ptr[val->length] != '\n') > + { > + my_error(ER_FPARSER_ERROR_IN_PARAMETER, MYF(0), > + parameter->name.str, line); > + DBUG_RETURN(TRUE); > + } > + memcpy(val->str, ptr, val->length); > + val->str[val->length]= '\0'; > + ptr+= (val->length + 1); > + break; I'd suggest to use FILE_OPTIONS_STRING here, it's much simpler and it's known to work. This doesn't check that end - ptr < val->length, and it doesn't check that even if ptr[val->length] == '\n', then it's the first '\n' after ptr. It's all possible to fix, but using FILE_OPTIONS_STRING is just easier. > + } > case FILE_OPTIONS_TIMESTAMP: > { > /* string have to be allocated already */ > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc > index bb1a99d9eef..b9198647f98 100644 > --- a/sql/sql_prepare.cc > +++ b/sql/sql_prepare.cc > @@ -4177,6 +4177,10 @@ bool Prepared_statement::prepare(const char *packet, uint packet_len) > Query_arena *old_stmt_arena; > DBUG_ENTER("Prepared_statement::prepare"); > DBUG_ASSERT(m_sql_mode == thd->variables.sql_mode); > + > + // The same format as for triggers to compare > + create_time= make_trigger_time(thd->query_start(), > + thd->query_start_sec_part()); Better use hrtime here, you might compare it with more than triggers in the future. For triggers, you can multiply it by 10000 for comparison. Or you can start storing trigger time with a microsecond precision - that's even better. > /* > If this is an SQLCOM_PREPARE, we also increase Com_prepare_sql. > However, it seems handy if com_stmt_prepare is increased always, > diff --git a/sql/sql_trigger.cc b/sql/sql_trigger.cc > index 52817ef65ae..38febce4fef 100644 > --- a/sql/sql_trigger.cc > +++ b/sql/sql_trigger.cc > @@ -929,7 +933,21 @@ bool Table_triggers_list::create_trigger(THD *thd, TABLE_LIST *tables, > > if (!sql_create_definition_file(NULL, &file, &triggers_file_type, > (uchar*)this, triggers_file_parameters)) > + { > + // delay if needed to be sure that time of creation is unique > + for (;;) > + { > + my_hrtime_t hrtime= my_hrtime(); > + ulonglong current_time= make_trigger_time(hrtime_to_my_time(hrtime), > + hrtime_sec_part(hrtime)); > + if (trigger->create_time == current_time) > + pthread_yield(); > + else > + break; > + } This shouldn't be needed. Better 1. store statement->create_time with microsecond precision 2. store trigger->create_time with microsecond precision 3. at the end of prepare wait (like above) to make sure statement->create_time is larger than all triggers' create_time. this will be a no-op unless someone created a trigger and prepared in the same microsecond. Safety check will ensure you won't wait longer than one microsecond. > DBUG_RETURN(false); > + } > > err_with_cleanup: > /* Delete .TRN file */ > diff --git a/sql/sql_view.cc b/sql/sql_view.cc > index 020830483c5..830a42b3cf3 100644 > --- a/sql/sql_view.cc > +++ b/sql/sql_view.cc > @@ -1131,7 +1141,31 @@ static int mysql_register_view(THD *thd, TABLE_LIST *view, > DBUG_RETURN(error); > } > > +/** > + Check is TABLE_LEST and SHARE match > + @param[in] view TABLE_LIST of the view > + @param[in] share Share object of view > + > + @return false on error or misspatch > +*/ > > +bool mariadb_view_version_get(TABLE_SHARE *share) > +{ > + DBUG_ASSERT(share->is_view); > + > + if (!(share->tabledef_version.str= > + (uchar*) alloc_root(&share->mem_root, VIEW_MD5_LEN + 1))) > + return TRUE; > + share->tabledef_version.length= VIEW_MD5_LEN; > + > + DBUG_ASSERT(share->view_def != NULL); > + if (share->view_def->parse((uchar *) &share->tabledef_version, NULL, > + view_md5_parameters, 1, > + &file_parser_dummy_hook)) > + return TRUE; > + DBUG_ASSERT(share->tabledef_version.length == VIEW_MD5_LEN); > + return FALSE; > +} I don't like using md5 as a uuid. md5 collisions are very much possible. Better options would be 1. use a timestamp, but store it with microsecond precisions, same logic as with triggers. 2. use a true tabledef uuid, new field in the view frm, same logic as with tables. timestamp is better, because it already exists in frms, and any old view will naturally have a timestamp before now(), so it doesn't matter that it won't have microseconds. uuid is better, because it's more universal, can be used for more than just reprepare checks. But for now I think timestamp benefits outweigh that. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 8f8e7ad5c80: MDEV-27217 DELETE partition selection doesn't work for history partitions
by Sergei Golubchik 13 Jan '22

13 Jan '22

Hi, Aleksey! ok to push. see one comment below. On Jan 13, Aleksey Midenkov wrote: > revision-id: 8f8e7ad5c80 (mariadb-10.3.31-80-g8f8e7ad5c80) > parent(s): 88f8aa20bba > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2022-01-12 21:47:03 +0300 > message: > > MDEV-27217 DELETE partition selection doesn't work for history partitions > > LIMIT history switching requires the number of history partitions to > be marked for read: from first to last non-empty plus one empty. The > least we can do is to fail with error message if the needed partition > was not marked for read. As this is handler interface we require new > handler error code to display user-friendly error message. > > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc > index bd73642cd0d..2f945f29ed1 100644 > --- a/sql/ha_partition.cc > +++ b/sql/ha_partition.cc > @@ -9872,6 +9874,11 @@ void ha_partition::print_error(int error, myf errflag) > m_part_info->print_no_partition_found(table, errflag); > DBUG_VOID_RETURN; > } > + else if (error == HA_ERR_PARTITION_LIST) > + { > + handler::print_error(error, errflag); > + DBUG_VOID_RETURN; > + } this seems to be redundant, the else branch at the end covers it. > else if (error == HA_ERR_ROW_IN_WRONG_PARTITION) > { Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] ef70e809a40: MDEV-27217 DELETE partition selection doesn't work for history partitions
by Sergei Golubchik 12 Jan '22

12 Jan '22

Hi, Aleksey! On Jan 12, Aleksey Midenkov wrote: > revision-id: ef70e809a40 (mariadb-10.3.31-80-gef70e809a40) > parent(s): 88f8aa20bba > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2021-12-23 04:15:39 +0300 > message: > > MDEV-27217 DELETE partition selection doesn't work for history partitions > > LIMIT history switching requires the number of history partitions to > be marked for read: from first to last non-empty plus one empty. The > least we can do is to fail with error message if the needed partition > was not marked for read. As this is handler interface we require new > handler error code to display user-friendly error message. > > Switching by INTERVAL works out-of-the-box with > ER_ROW_DOES_NOT_MATCH_GIVEN_PARTITION_SET error. Code-wise it's pretty much fine. Two problems related to error messages: We cannot have part of the error message a hard-coded English phrase. It doesn't work with localized error messages: "Невозможно выбрать раздел: modifying system versioned table" Also we cannot add new error messages in 10.3. Or any old GA. Currently you can add new error messages to 10.6 or any later version. You can reuse ER_UNUSED_1 for your new error message, that'll work. And, because you cannot have English phrases in it, I'd suggest something more generic, like Not allowed for system-versioned table %`s.%`s In the context of delete from t1 partition (p1); ERROR HY000: Not allowed for system-versioned table `test`.`t1` it seems to be quite clear. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 7a746072a5e: MDEV-26878: Query failing with syntax error sets ROW_NUMBER to non-zero
by Sergei Golubchik 12 Jan '22

12 Jan '22

Hi, Rucha! On Jan 12, Rucha Deodhar wrote: > On Tue, Jan 11, 2022 at 11:49 PM Sergei Golubchik <serg(a)mariadb.org> wrote: > > > +--error ER_PARSE_ERROR > > > +INSERT INTO t1 VALUES XXX (1),(2); > > > > > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > > > index 8ebefbb3d82..47ed3ded4a7 100644 > > > --- a/sql/sql_yacc.yy > > > +++ b/sql/sql_yacc.yy > > > @@ -8426,6 +8426,8 @@ query_specification_start: > > > { > > > SELECT_LEX *sel; > > > LEX *lex= Lex; > > > + if (lex->sql_command == SQLCOM_INSERT) > > > + thd->get_stmt_da()->inc_current_row_for_warning(); > > > if (!(sel= lex->alloc_select(TRUE)) || lex->push_select(sel)) > > > MYSQL_YYABORT; > > > > better not to have an if() in this rule that's used almost for > > everything. > > When we have INSERT...SELECT counter has to be 1 for errors like > ER_ILLEGAL_VALUE_FOR_TYPE which are caught during parsing. > Example: > INSERT INTO t1 SELECT a, b, 1.79769313486232e+308 FROM t2; > > So I added if() to increment the counter if we are parsing INSERT so > that ROW_NUMBER for statements like the one above gives ROW_NUMBER=1. I see. But you put a condition on every query's execution path to fix the case that, I believe, literally nobody cares about. I don't think any user ever will check the ROW_NUMBER after the syntax error :) And if someone will, we can say that in INSERT INTO t1 VALUES XXX (1),(2); the first row starts after VALUES, so ROW_NUMBER=1 is expected. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 9ea85a70a75: MDEV-24654 GTID event falsely marked transactional
by Sergei Golubchik 11 Jan '22

11 Jan '22

Hi, Andrei! On Jan 11, Andrei Elkin wrote: > Howdy, Sergei! > > To the question of the usage of trX cache by non-trX let me answer > broadly to mention @@binlog_direct_non_transactional_update = false > leads to aggregation of mixed, say innodb + myisam, events in trx > cache. That's different. The bug summary is "GTID event falsely marked transactional", meaning, the group of events is not transactional and it's falsely marked as transactional. If you have an innodb/myisam mix, it is correctly marked transactional. You need to have only non-transactional events in the transactional cache. And I'm trying to understand how can that happen. > I may need to process deeper the referred comments though. > > Next to the commit's idea, it actually covers your concern. I wonder > on the 'Wrong' conclusion. There is no intent to xidify anything > extra. Th commit assumes xid eVent is created, before Gtid one, > accordingly. And that fact is propagated to Gtid ctor. Your commit, if I'm not mistaken, assumes that "last event is Xid if and only if the cache contains a transaction". Which is incorrect, a transaction does not necesarily have to end with a Xid event. /Sergei > On Mon, 10 Jan 2022, 20:56 Sergei Golubchik, <serg(a)mariadb.org> wrote: > > > Hi, Andrei! > > > > On Jan 10, Andrei Elkin wrote: > > > revision-id: 9ea85a70a75 (mariadb-10.2.40-4-g9ea85a70a75) > > > parent(s): 160d97a4aaa > > > author: Andrei Elkin > > > committer: Andrei Elkin > > > timestamp: 2021-08-08 14:20:57 +0300 > > > message: > > > > > > MDEV-24654 GTID event falsely marked transactional > > > > > > GTID event can be falsely marked transactional in few cases including > > > binary-logging on the slave side upon execution. > > > In some execution branches bin-logging of non-transactional > > > events can be done through transactional cache. E.g see comments > > > in THD::binlog_write_table_map(). > > > > I saw that and still couldn't understand why bin-logging of > > non-transactional events can be done thr-ough transactional cache. > > > > > In order to not create the false 'trans' tag the fact of Xid event > > > logging is checked to compute correct argument to Gtid_log_event > > > ctor. > > > > This is wrong. Transactions only end with a Xid event if all > > participating engines support XA. Try to create an InnoDB+FederatedX > > transaction (I just did) and it'll end with Query_log_event("COMMIT") > > > > > diff --git a/sql/log.cc b/sql/log.cc > > > index 1d9b4645421..7fef2e0d739 100644 > > > --- a/sql/log.cc > > > +++ b/sql/log.cc > > > @@ -8118,9 +8118,11 @@ > > MYSQL_BIN_LOG::write_transaction_or_stmt(group_commit_entry *entry, > > > uint64 commit_id) > > > { > > > binlog_cache_mngr *mngr= entry->cache_mngr; > > > + bool has_xid= entry->end_event->get_type_code() == XID_EVENT; > > > DBUG_ENTER("MYSQL_BIN_LOG::write_transaction_or_stmt"); > > > > > > - if (write_gtid_event(entry->thd, false, entry->using_trx_cache, > > commit_id)) > > > + if (write_gtid_event(entry->thd, false, > > > + entry->using_trx_cache && has_xid, commit_id)) > > > DBUG_RETURN(ER_ERROR_ON_WRITE); > > > > > > if (entry->using_stmt_cache && !mngr->stmt_cache.empty() && > > > > > Regards, > > Sergei > > VP of MariaDB Server Engineering > > and security(a)mariadb.org > > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

Re: [Maria-developers] 9ea85a70a75: MDEV-24654 GTID event falsely marked transactional
by Sergei Golubchik 11 Jan '22

11 Jan '22

Hi, Andrei! On Jan 11, Andrei Elkin wrote: > On Tue, Jan 11, 2022 at 1:06 PM Sergei Golubchik <serg(a)mariadb.org> wrote: > > > Hi, Andrei! > > > > On Jan 11, Andrei Elkin wrote: > > > Howdy, Sergei! > > > > > > To the question of the usage of trX cache by non-trX let me answer > > > broadly to mention @@binlog_direct_non_transactional_update = false > > > leads to aggregation of mixed, say innodb + myisam, events in trx > > > cache. > > > > That's different. The bug summary is "GTID event falsely marked > > transactional", meaning, the group of events is not transactional and > > it's falsely marked as transactional. If you have an innodb/myisam > > mix, it is correctly marked transactional. > > > > You need to have only non-transactional events in the transactional > > cache. And I'm trying to understand how can that happen. > > Got your very specific point is about a pure myisam can be trx-cached. > This is OPTION_BINLOG_GTID > commit 5426facdcbfba2d78dab3c709cbf278073383b7c 's decision > >> - All tables are regarded as transactional tables in the binary log > (to ensure things are executed exactly as on the master) Okay, thanks, that answers my question Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 7a746072a5e: MDEV-26878: Query failing with syntax error sets ROW_NUMBER to non-zero
by Sergei Golubchik 11 Jan '22

11 Jan '22

Hi, Rucha! On Jan 11, Rucha Deodhar wrote: > revision-id: 7a746072a5e (mariadb-10.7.1-26-g7a746072a5e) > parent(s): 7dfaded9625 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2022-01-05 12:05:22 +0530 > message: > > MDEV-26878: Query failing with syntax error sets ROW_NUMBER to non-zero > > Analysis: m_current_row_for_warning counter is set to 0 at the beginning > of parsing. > Fix: Reset it to 1 and increment it before starting to parse insert values, > during SET and SELECT for INSERT...SET statement and INSERT...SELECT > respectively. > > diff --git a/mysql-test/main/get_diagnostics.result b/mysql-test/main/get_diagnostics.result > diff --git a/mysql-test/main/get_diagnostics.test b/mysql-test/main/get_diagnostics.test > index e8d81dca1e6..467f624c49c 100644 > --- a/mysql-test/main/get_diagnostics.test > +++ b/mysql-test/main/get_diagnostics.test > @@ -1687,3 +1687,18 @@ GET DIAGNOSTICS CONDITION 3 @n = ROW_NUMBER; > SELECT @n; > > DROP TABLE t; > + > +--echo # > +--echo # MDEV-26878: Query failing with syntax error sets ROW_NUMBER to > +--echo # non-zero > +--echo # > + > +CREATE TABLE t1 (id1 INT); > + > +--error ER_PARSE_ERROR > +INSERT INTO t1 VALUES XXX (1),(2); please add a test, like, INTO t1 VALUES (1), (ALL); what ROW_NUMBER will it show? > + > +GET DIAGNOSTICS CONDITION 1 @n= ROW_NUMBER; > +SELECT @n; > + > +DROP TABLE t1; > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 8ebefbb3d82..47ed3ded4a7 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -8426,6 +8426,8 @@ query_specification_start: > { > SELECT_LEX *sel; > LEX *lex= Lex; > + if (lex->sql_command == SQLCOM_INSERT) > + thd->get_stmt_da()->inc_current_row_for_warning(); > if (!(sel= lex->alloc_select(TRUE)) || lex->push_select(sel)) > MYSQL_YYABORT; better not to have an if() in this rule that's used almost for everything. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 8e21b91fd31: MDEV-26875: Wrong user in SET DEFAULT ROLE error
by Sergei Golubchik 11 Jan '22

11 Jan '22

Hi, Anel! On Jan 11, Anel Husakovic wrote: > revision-id: 8e21b91fd31 (mariadb-10.5.12-71-g8e21b91fd31) > parent(s): e5a9dcfda20 > author: Anel Husakovic > committer: Anel Husakovic > timestamp: 2021-10-25 14:43:38 +0200 > message: > > MDEV-26875: Wrong user in SET DEFAULT ROLE error > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 65f053aa09b..0a59f5fce37 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -3273,7 +3273,7 @@ static int check_user_can_set_role(THD *thd, const char *user, > { > /* 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, > + MYF(0), user, > thd->security_ctx->priv_host, rolename); That's strange. you'll use `user` and `thd->security_ctx->priv_host` ? They generally correspond to two different accounts. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] b14b6f31fa2: MDEV-26843: Inconsistent behavior of ROW_NUMBER upon resignalling from function Analysis: ROW_NUMBER should be 0 because the error produced is not related to statement that modifies table data. It is just wrong number of arguments. However, ROW_NUMBER is 1 because m_current_row_for_warning is not set to 0 before executing function. Fix: Set m_current_row_for_warning to 0.
by Sergei Golubchik 11 Jan '22

11 Jan '22

Hi, Rucha! What if you have INSERT t1 VALUES (f1(1)), (f1(2)), (f1()); ? On Jan 11, Rucha Deodhar wrote: > revision-id: b14b6f31fa2 (mariadb-10.6.1-143-gb14b6f31fa2) > parent(s): d555ae38381 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-10-21 17:13:13 +0530 > message: > > MDEV-26843: Inconsistent behavior of ROW_NUMBER upon resignalling from > function > Analysis: ROW_NUMBER should be 0 because the error produced is not related to > statement that modifies table data. It is just wrong number of arguments. > However, ROW_NUMBER is 1 because m_current_row_for_warning is not set to 0 > before executing function. > Fix: Set m_current_row_for_warning to 0. > > diff --git a/sql/sp_head.cc b/sql/sp_head.cc > index d1cf5c2d9bd..ebae2f356bd 100644 > --- a/sql/sp_head.cc > +++ b/sql/sp_head.cc > @@ -2033,6 +2033,7 @@ sp_head::execute_function(THD *thd, Item **argp, uint argcount, > */ > if (argcount != m_pcont->context_var_count()) > { > + thd->get_stmt_da()->reset_current_row_for_warning(0); > /* > Need to use my_error here, or it will not terminate the > invoking query properly. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 394bb653b0b: MDEV-26695: Number of an invalid row is not calculated for table value constructor
by Sergei Golubchik 11 Jan '22

11 Jan '22

Hi, Rucha! please, see below On Jan 11, Rucha Deodhar wrote: > revision-id: 394bb653b0b (mariadb-10.6.1-140-g394bb653b0b) > parent(s): 3c857a07d92 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-10-20 01:30:04 +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 49f319b3856..a191ec9c328 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; > > if (select_options & SELECT_DESCRIBE) > DBUG_RETURN(false); > @@ -438,16 +440,21 @@ 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; > + goto reset_counter_and_exit; Why goto, break worked here just fine, didn't it? Also, note that if you jump from here out of the loop, you'll do if (rc>0) where rc wasn't initialized yet. > - 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); > if (!rc) > send_records++; > else if (rc > 0) > - DBUG_RETURN(true); > + goto reset_counter_and_exit; break here too? > } > > +reset_counter_and_exit: > + cur_thd->get_stmt_da()->reset_current_row_for_warning(0); why do you need to reset here? > + if (rc>0) > + DBUG_RETURN(true); > + > if (result->send_eof()) > DBUG_RETURN(true); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] ce69e0c8e78: MDEV-24920: Merge "old" SQL variable to "old_mode" sql variable
by Sergei Golubchik 10 Jan '22

10 Jan '22

Hi, Rucha! First, note that it's now a 10.9 task, so don't push it into 10.8, please. As for the patch - code-wise it's all good. New mode names are confusing and should be clarified, see the comment below. On Jan 10, Rucha Deodhar wrote: > revision-id: ce69e0c8e78 (mariadb-10.6.1-88-gce69e0c8e78) > parent(s): 22556499397 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-10-06 13:25:29 +0530 > message: > > MDEV-24920: Merge "old" SQL variable to "old_mode" sql variable > > Analysis: There are 2 server variables- "old_mode" and "old". "old" is > no longer needed as "old_mode" has replaced it (however still used in > some places in the code). "old_mode" and "old" has same purpose- > emulate behavior from previous MariaDB versions. So they can be merged > to avoid confusion. Fix: Deprecate "old" variable and create another > mode for @@old_mode to mimic behavior of previous "old" variable. > Create specific modes for specifix task that --old sql variable was > doing earlier and use the new modes instead. > > diff --git a/mysql-test/suite/sys_vars/r/old_mode_basic.result b/mysql-test/suite/sys_vars/r/old_mode_basic.result > index a6b95f1c60c..2f3742160eb 100644 > --- a/mysql-test/suite/sys_vars/r/old_mode_basic.result > +++ b/mysql-test/suite/sys_vars/r/old_mode_basic.result > @@ -264,3 +264,42 @@ SET @@collation_database = @save_collation_database; > # > # End of 10.6 test > # > +# > +# Beginning of 10.7 test > +# 10.9 now > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index c2a219ee015..d1eaacd777c 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -3755,6 +3774,8 @@ static const char *old_mode_names[]= > "NO_PROGRESS_INFO", > "ZERO_DATE_TIME_CAST", > "UTF8_IS_UTF8MB3", > + "INDEX_MASKING", > + "CHECKSUM_FORMATTING", that's confusing. I don't understsand from the name of the option what "CHECKSUM_FORMATTING" or "INDEX_MASKING" changes. "NO_PROGRESS_INFO" is clear, there won't be progress info "UTF8_IS_UTF8MB3" is clear "ZERO_DATE_TIME_CAST" is kind of understandable too, CAST(<time> AS DATETIME) will be "0000-00-00 <time>", that is zero date CHECKSUM_FORMATTING and INDEX_MASKING is unclear. For CHECKSUM_FORMATTING I'd suggest CHECKSUM_SLOW_NULLS (see the commit that introduced this behavior: 496741d5761f) "INDEX_MASKING" was introduced in the commit 79542930ea1c, see the comment there, it explains what was done. This should help you to suggest a better name for this mode. > 0 > }; > > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 9ea85a70a75: MDEV-24654 GTID event falsely marked transactional
by Sergei Golubchik 10 Jan '22

10 Jan '22

Hi, Andrei! On Jan 10, Andrei Elkin wrote: > revision-id: 9ea85a70a75 (mariadb-10.2.40-4-g9ea85a70a75) > parent(s): 160d97a4aaa > author: Andrei Elkin > committer: Andrei Elkin > timestamp: 2021-08-08 14:20:57 +0300 > message: > > MDEV-24654 GTID event falsely marked transactional > > GTID event can be falsely marked transactional in few cases including > binary-logging on the slave side upon execution. > In some execution branches bin-logging of non-transactional > events can be done through transactional cache. E.g see comments > in THD::binlog_write_table_map(). I saw that and still couldn't understand why bin-logging of non-transactional events can be done through transactional cache. > In order to not create the false 'trans' tag the fact of Xid event > logging is checked to compute correct argument to Gtid_log_event > ctor. This is wrong. Transactions only end with a Xid event if all participating engines support XA. Try to create an InnoDB+FederatedX transaction (I just did) and it'll end with Query_log_event("COMMIT") > diff --git a/sql/log.cc b/sql/log.cc > index 1d9b4645421..7fef2e0d739 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -8118,9 +8118,11 @@ MYSQL_BIN_LOG::write_transaction_or_stmt(group_commit_entry *entry, > uint64 commit_id) > { > binlog_cache_mngr *mngr= entry->cache_mngr; > + bool has_xid= entry->end_event->get_type_code() == XID_EVENT; > DBUG_ENTER("MYSQL_BIN_LOG::write_transaction_or_stmt"); > > - if (write_gtid_event(entry->thd, false, entry->using_trx_cache, commit_id)) > + if (write_gtid_event(entry->thd, false, > + entry->using_trx_cache && has_xid, commit_id)) > DBUG_RETURN(ER_ERROR_ON_WRITE); > > if (entry->using_stmt_cache && !mngr->stmt_cache.empty() && > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 734aee3cd3c: MDEV-27018 IF and COALESCE lose "json" property
by Sergei Golubchik 10 Jan '22

10 Jan '22

Hi, Alexander! On Dec 28, Alexander Barkov wrote: > revision-id: 734aee3cd3c (mariadb-10.5.13-33-g734aee3cd3c) > parent(s): 2776635cb98 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2021-12-27 17:54:31 +0400 > message: > > MDEV-27018 IF and COALESCE lose "json" property > > Hybrid functions (IF, COALESCE, etc) did not preserve the JSON property > from their arguments. The same problem was repeatable for single row subselects. > > The problem happened because the method Item::is_json_type() was inconsistently > implemented across the Item hierarchy. For example, Item_hybrid_func > and Item_singlerow_subselect did not override is_json_type(). Okay. The approach is fine, but see some questions below: > diff --git a/sql/field.cc b/sql/field.cc > index 2c768527ced..8fa3bbd538c 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -7277,6 +7277,19 @@ bool Field_longstr::send(Protocol *protocol) > } > > > +const Type_handler *Field_string::type_handler() const > +{ > + if (is_var_string()) > + return &type_handler_var_string; > + /* > + This is a temporary solution and will be fixed soon (in 10.9?). > + Type_handler_string_json will provide its own Field_string_json. why? > + */ > + if (Type_handler_json_common::has_json_valid_constraint(this)) could you please use names that don't depend on how exactly we detect json? especially if the goal is to move to FORMAT JSON syntax > + return &type_handler_string_json; > + return &type_handler_string; > +} > + > /* Copy a string and fill with space */ > > int Field_string::store(const char *from, size_t length,CHARSET_INFO *cs) > diff --git a/sql/sql_type.cc b/sql/sql_type.cc > index c1801c1ae3e..3b2753d80a6 100644 > --- a/sql/sql_type.cc > +++ b/sql/sql_type.cc > @@ -1755,19 +1755,110 @@ const Type_handler *Type_handler_typelib::cast_to_int_type_handler() const > > /***************************************************************************/ > > +class Recursive_type_pair_iterator > +{ > + const Type_handler *m_a; > + const Type_handler *m_b; > + uint m_switched_to_base_count; > +public: > + Recursive_type_pair_iterator(const Type_handler *a, > + const Type_handler *b, > + uint switched_to_base_count= 0) > + :m_a(a), m_b(b), m_switched_to_base_count(switched_to_base_count) > + { } > + const Type_handler *a() const { return m_a; } > + const Type_handler *b() const { return m_b; } > + Recursive_type_pair_iterator base() const > + { > + Recursive_type_pair_iterator res(m_a->type_handler_base(), > + m_b->type_handler_base()); > + res.m_switched_to_base_count= (res.m_a != NULL) + (res.m_b != NULL); > + if (res.m_a == NULL) > + res.m_a= m_a; > + if (res.m_b == NULL) > + res.m_b= m_b; > + return res; > + } > + bool done() const > + { > + switch (m_switched_to_base_count) > + { > + case 2: > + /* > + Expression: > + COALESCE(TYPE1, TYPE2) > + > + where both type handlers derive from some other handlers, e.g. > + VARCHAR -> TYPE1 > + TEXT -> TYPE2 > + > + Continue aggregation as: > + SELECT COALESCE(VARCHAR, TEXT) > + > + Note, we now have only one level of data type inheritance, > + e.g. VARCHAR -> VARCHAR/JSON > + > + This code branch will change when we have longer inheritance chains: > + A0 -> A1 -> A2 -> A3 > + B0 -> B1 -> B2 > + > + Functions like COALESE(A2, B2) will need to do full overload resolution: > + - iterate through all pairs from the below matrix > + - find all existing candidates > + - resolve ambiguities in case multiple candidates, etc. and I don't see how you're going to do that. you need to try m_a+m_b.base(), m_a.base()+m_b, and m_a.base()+m_b.base(). And that's not what you're doing. I suggest to add an assert that m_switched_to_base_count < 2 and stop trying to solve complex cases until we have at least one. > + > + A0 A1 A2 A3 > + B0 . . . . > + B1 . . . . > + B2 . . . . > + */ > + return false; > + > + case 1: > + /* > + Expression: > + COALESCE(TYPE1, TEXT) > + > + If only one handler derives from some other handler: > + VARCHAR-> TYPE1 > + > + Continue aggregation as: > + SELECT COALESCE(VARCHAR, TEXT) > + */ > + return false; > + > + case 0: > + default: > + /* > + Non of the two handlers are derived from other handlers. > + Nothing left to try. > + */ > + return true; > + } > + } > +}; > diff --git a/sql/sql_type_json.h b/sql/sql_type_json.h > index 6c4ee8cb2eb..4a394809a06 100644 > --- a/sql/sql_type_json.h > +++ b/sql/sql_type_json.h > @@ -21,18 +21,145 @@ ... > + bool Item_append_extended_type_info(Send_field_extended_metadata *to, > + const Item *item) const > + { > + return set_format_name(to); // Send "format=json" in the protocol > + } > + > + bool Item_hybrid_func_fix_attributes(THD *thd, > + const char *name, > + Type_handler_hybrid_field_type *hybrid, > + Type_all_attributes *attr, > + Item **items, uint nitems) > + const override > + { > + if (BASE::Item_hybrid_func_fix_attributes(thd, name, hybrid, attr, > + items, nitems)) > + return true; > + /* > + The above call can change the type handler on "hybrid", e.g. > + choose a proper BLOB type handler according to the calculated max_length. > + Convert general purpose string type handler to its JSON counterpart. > + This makes hybrid functions preserve JSON data types, e.g.: > + COALESCE(json_expr1, json_expr2) -> JSON > + */ > + hybrid->set_handler(json_type_handler_from_generic(hybrid->type_handler())); I don't get it. It's supposed to "fix attributes", like signed/unsigned, max length, etc. The correct type handler should've been set already, otherwise this type handler method wouldn't even be called. > + return false; > + } > }; > > + > +class Type_handler_string_json: > + public Type_handler_general_purpose_string_to_json<Type_handler_string, > + type_handler_string> > +{ }; > diff --git a/sql/sql_type_json.cc b/sql/sql_type_json.cc > index a804366ec03..a84f720bcc9 100644 > --- a/sql/sql_type_json.cc > +++ b/sql/sql_type_json.cc > @@ -20,17 +20,111 @@ > #include "sql_class.h" > > > -Type_handler_json_longtext type_handler_json_longtext; > +Named_type_handler<Type_handler_string_json> > + type_handler_string_json("char/json"); do you mean these names are not shown anywhere? > +Named_type_handler<Type_handler_varchar_json> > + type_handler_varchar_json("varchar/json"); > + > +Named_type_handler<Type_handler_tiny_blob_json> > + type_handler_tiny_blob_json("tinyblob/json"); > + > +Named_type_handler<Type_handler_blob_json> > + type_handler_blob_json("blob/json"); > + > +Named_type_handler<Type_handler_medium_blob_json> > + type_handler_medium_blob_json("mediumblob/json"); > + > +Named_type_handler<Type_handler_long_blob_json> > + type_handler_long_blob_json("longblob/json"); ... > +/* > + This method ressembles what Field_blob::type_handler() resembles (also below) > + does for general purpose BLOB type handlers. > +*/ > +const Type_handler * > +Type_handler_json_common::json_blob_type_handler_by_length_bytes(uint len) > +{ > + switch (len) { > + case 1: return &type_handler_tiny_blob_json; > + case 2: return &type_handler_blob_json; > + case 3: return &type_handler_medium_blob_json; > + } > + return &type_handler_long_blob_json; > +} Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] ce507903d0c: MDEV-22742 UBSAN: Many overflow issues in strings/decimal.c - runtime error: signed integer overflow: x * y cannot be represented in type 'long long int' (on optimized builds).
by Sergei Golubchik 09 Jan '22

09 Jan '22

Hi, Alexey! On Jan 09, Alexey Botchkov wrote: > revision-id: ce507903d0c (mariadb-10.2.40-161-gce507903d0c) > parent(s): 0dae41637ab > author: Alexey Botchkov > committer: Alexey Botchkov > timestamp: 2021-11-22 09:58:46 +0400 > message: > > MDEV-22742 UBSAN: Many overflow issues in strings/decimal.c - runtime error: signed integer overflow: x * y cannot be represented in type 'long long int' (on optimized builds). > > Avoid integer overflow, do the check before the calculation. > > diff --git a/strings/decimal.c b/strings/decimal.c > index 9d18a9ce52a..6249d7e097a 100644 > --- a/strings/decimal.c > +++ b/strings/decimal.c > @@ -1128,13 +1128,16 @@ int decimal2ulonglong(const decimal_t *from, ulonglong *to) > > for (intg=from->intg; intg > 0; intg-=DIG_PER_DEC1) > { > - ulonglong y=x; > - x=x*DIG_BASE + *buf++; > - if (unlikely(y > ((ulonglong) ULONGLONG_MAX/DIG_BASE) || x < y)) > + if (unlikely ( > + x >= ULONGLONG_MAX/DIG_BASE && > + (x > ULONGLONG_MAX/DIG_BASE || > + *buf > (dec1) (ULONGLONG_MAX%DIG_BASE)))) This took me a while. Personally I find it easier to understand an exclusive condition, like x > ULONGLONG_MAX/DIG_BASE || (x == ULONGLONG_MAX/DIG_BASE && *buf > (dec1) (ULONGLONG_MAX%DIG_BASE)) but it's equivalent to your version, so ok to push. Did you check that this commit fixes all UBSAN issues mentioned in the MDEV-22742? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 0dfe5a31573: MDEV-22441 implement a generic way to change a value of a variable in a scope
by Sergei Golubchik 09 Jan '22

09 Jan '22

Hi, Eugene! On Jan 09, Eugene Kosov wrote: > revision-id: 0dfe5a31573 (mariadb-10.7.1-13-g0dfe5a31573) > parent(s): 1e54a9716d2 > author: Eugene Kosov > committer: Eugene Kosov > timestamp: 2021-12-03 16:12:27 +0600 > message: > > MDEV-22441 implement a generic way to change a value of a variable in a scope > > Example: > { > auto _= make_scope_value(var, tmp_value); > } > > make_scope_value(): a function which returns RAII object which temporary > changes a value of a variable 1. Why have you not created a macro, like SCOPE_EXIT to avoid `auto _=` ? Particularly as one cannot use `_` to backup more than one variable. 2. better remove the new value from make_scope_value(). Creating a RAII backup and an assignment of a new value are two separate actions. The assignment can happen much later or conditionally or not at all. 3. (not part of MDEV-22441) See how SCOPE_EXIT is implemented in rocksdb, perhaps we should do it similarly? It's used like SCOPE_EXIT { mem_heap_free(heap); } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 8a84f5a40c1: MDEV-24176 Preparations
by Sergei Golubchik 06 Jan '22

06 Jan '22

Hi, Aleksey! On Oct 17, Aleksey Midenkov wrote: > commit 8a84f5a40c1 > Author: Aleksey Midenkov <midenok(a)gmail.com> > Date: Thu May 27 17:00:14 2021 +0300 > > MDEV-24176 Preparations > > 1. moved fix_vcol_exprs() call to open_table() > > mysql_alter_table() doesn't do lock_tables() so it cannot win from > fix_vcol_exprs() from there. Tests affected: main.default_session This is likely wrong, but the old code was wrong too. Neither open_table nor lock_tables is called under LOCK TABLES, but the session environment can change, I suspect. > > 2. cleanup_excluding_fields_processor removed. > > That was just a quick hack to exclude wrongly working Item_field from > processing. Now it works due to correct execution environment (see > next commit). Related to MDEV-10355 does it work now, in that commit, or only after the next commit? what exactly do you mean by correct execution environment? > > 3. Vanilla cleanups and comments. > > diff --git a/sql/item.h b/sql/item.h > index cc1914a7ad4..7b7fe04f0b2 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -2586,6 +2585,12 @@ class Item_ident :public Item_result_field > const char *db_name; > const char *table_name; > const char *field_name; > + /* > + NOTE: came from TABLE::alias_name_used and this is only a hint! It works > + only in need_correct_ident() condition. On other cases it is FALSE even if > + table_name is alias! It cannot be TRUE in these cases, lots of spaghetti > + logic depends on that. could you elaborate on that? > + */ > bool alias_name_used; /* true if item was resolved against alias */ > /* > Cached value of index for this field in table->field array, used by prep. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 5

Re: [Maria-developers] 3617add6975: MDEV-27208: Extend CRC32() and implement CRC32C()
by Sergei Golubchik 05 Jan '22

05 Jan '22

Hi, Marko! On Jan 03, Marko Mäkelä wrote: > revision-id: 3617add6975 (mariadb-10.6.1-252-g3617add6975) > parent(s): bc90f5d6967 > author: Marko Mäkelä > committer: Sergei Golubchik > timestamp: 2021-12-15 19:54:34 +0100 > message: > > MDEV-27208: Extend CRC32() and implement CRC32C() > > We used to define a native unary function crc32() that computes the CRC-32 > of a string using the ISO 3309 polynomial that is being used by zlib > and many others. Suggestions: 1. one class for both CRC functions, like class Item_func_crc32 :public Item_long_func { extern "C" unsigned (*crc_func)(unsigned int, const char *, size_t); and in the constructor crc_func= crc_variant ? my_crc32 : my_crc32c; there's no need to duplicate everything. 2. swap the arguments: CRC32(CRC32("Maria"), "DB") = CRC32("MariaDB") looks kind of better than CRC32("DB", CRC32("Maria")) = CRC32("MariaDB") Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 803b977229c: MDEV-26698: Incorrect row number upon INSERT .. SELECT from the same table: rows are counted twice
by Sergei Golubchik 31 Dec '21

31 Dec '21

Hi, Rucha! Ok to push. One suggestion below. On Dec 31, Rucha Deodhar wrote: > revision-id: 803b977229c (mariadb-10.2.40-192-g803b977229c) > parent(s): 42fea34d4a9 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-12-29 16:09:10 +0530 > message: > > MDEV-26698: Incorrect row number upon INSERT .. SELECT from the same > table: rows are counted twice > > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index a331f4f3dbc..c8d5170f568 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -18972,7 +18972,7 @@ evaluate_join_record(JOIN *join, JOIN_TAB *join_tab, > bool shortcut_for_distinct= join_tab->shortcut_for_distinct; > ha_rows found_records=join->found_records; > COND *select_cond= join_tab->select_cond; > - bool select_cond_result= TRUE; > + bool select_cond_result= TRUE, unlock_row= TRUE; > > DBUG_ENTER("evaluate_join_record"); > DBUG_PRINT("enter", > @@ -19124,6 +19124,7 @@ evaluate_join_record(JOIN *join, JOIN_TAB *join_tab, > > if (found) > { > + unlock_row= false; > enum enum_nested_loop_state rc; > /* A match from join_tab is found for the current partial join. */ > rc= (*join_tab->next_select)(join, join_tab+1, 0); > @@ -19147,11 +19148,6 @@ evaluate_join_record(JOIN *join, JOIN_TAB *join_tab, > if (shortcut_for_distinct && found_records != join->found_records) > DBUG_RETURN(NESTED_LOOP_NO_MORE_ROWS); instead of unlock_row() you can put DBUG_RETURN(NESTED_LOOP_OK); here. Might be a bit simpler to read. > } > - else > - { > - join->thd->get_stmt_da()->inc_current_row_for_warning(); > - join_tab->read_record.unlock_row(join_tab); > - } > } > else > { > @@ -19160,6 +19156,9 @@ evaluate_join_record(JOIN *join, JOIN_TAB *join_tab, > with the beginning coinciding with the current partial join. > */ > join->join_examined_rows++; > + } > + if (unlock_row) > + { > join->thd->get_stmt_da()->inc_current_row_for_warning(); > join_tab->read_record.unlock_row(join_tab); > } > @@ -26945,6 +26944,12 @@ AGGR_OP::end_send() > table->reginfo.lock_type= TL_UNLOCK; > > bool in_first_read= true; > + > + /* > + Reset the counter before copying rows from internal temporary table to > + INSERT table. > + */ > + join_tab->join->thd->get_stmt_da()->reset_current_row_for_warning(); > while (rc == NESTED_LOOP_OK) > { > int error; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 4774601a3cd: MDEV-26698: Incorrect row number upon INSERT .. SELECT from the same table: rows are counted twice
by Sergei Golubchik 31 Dec '21

31 Dec '21

Hi, Rucha! On Dec 29, Rucha Deodhar wrote: > On Wed, Dec 29, 2021 at 12:50 AM Sergei Golubchik <serg(a)mariadb.org> wrote: > > > Why did put your fix here in a common code path in select, and not > > in, say, select_insert::send_data() ? > > Yes, the counter can be incremented in select_insert::send_data() too. > > But after writing record to INSERT table, we go back to > evaluate_join_records() where the counter is incremented > unconditionally. And CREATE TABLE...SELECT was failing with wrong > output because it also uses this code. > > I could have incremented counter conditionally so that it doesn't > increment twice like so: > > increment counter in select_insert::send_data() after record is written > if (thd->lex->sql_command == SQLCOM_INSERT_SELECT || > thd->lex->sql_command == SQLCOM_CREATE_TABLE) I'd think you wouldn't need this if(), as select_insert::send_data() is not used for other sql commands. > And increment counter in evalaute_join_records() after one nested loop > iteration ends (end_send()) > if (join->thd->lex->sql_command != SQLCOM_INSERT_SELECT && > join->thd->lex->sql_command != SQLCOM_CREATE_TABLE) I see. That's not good. > I had initially implemented it this way but it kinda looked hacky to me. > So I did a more general solution. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 4774601a3cd: MDEV-26698: Incorrect row number upon INSERT .. SELECT from the same table: rows are counted twice
by Sergei Golubchik 28 Dec '21

28 Dec '21

Hi, Rucha! Note test failures in buildbot, don't forget to update all result files before pushing. On Dec 28, Rucha Deodhar wrote: > revision-id: 4774601a3cd (mariadb-10.2.40-192-g4774601a3cd) > parent(s): 42fea34d4a9 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-12-27 16:48:04 +0530 > message: > > MDEV-26698: Incorrect row number upon INSERT .. SELECT from the same > table: rows are counted twice > > Analysis: When the table we are trying to insert into and the SELECT table > are same for INSERT ... SELECT, rows from the SELECT table are copied into > internal temporary table and then to the INSERT table. We only want to > count the rows when we start inserting into the table. > Fix: Reset the counter to 1 before starting to copy from internal temporary > table to select table and then increment the counter. > > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index a331f4f3dbc..7a50f3d74ac 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -3832,6 +3832,8 @@ mysql_select(THD *thd, > } > } > > + thd->get_stmt_da()->reset_current_row_for_warning(); What is this for? It wasn't needed before. > if ((err= join->optimize())) > { > goto err; // 1 > @@ -19124,10 +19126,10 @@ evaluate_join_record(JOIN *join, JOIN_TAB *join_tab, > > if (found) > { > + unlock_row= false; > enum enum_nested_loop_state rc; > /* A match from join_tab is found for the current partial join. */ > rc= (*join_tab->next_select)(join, join_tab+1, 0); > - join->thd->get_stmt_da()->inc_current_row_for_warning(); By moving this down you skip few returns, incl NESTED_LOOP_OK and NESTED_LOOP_NO_MORE_ROWS. Are you sure it's fine? Won't the counter be off, if the query execution continues after, say, NESTED_LOOP_OK? > if (rc != NESTED_LOOP_OK && rc != NESTED_LOOP_NO_MORE_ROWS) > DBUG_RETURN(rc); > if (return_tab < join->return_tab) > @@ -26945,6 +26943,12 @@ AGGR_OP::end_send() > table->reginfo.lock_type= TL_UNLOCK; > > bool in_first_read= true; > + > + /* > + Reset the counter before copying rows from internal temporary table to > + INSERT table. this code is also used for group by. Please, add a test for insert ... select ... group by Why did put your fix here in a common code path in select, and not in, say, select_insert::send_data() ? > + */ > + join_tab->join->thd->get_stmt_da()->reset_current_row_for_warning(); > while (rc == NESTED_LOOP_OK) > { > int error; > @@ -26968,6 +26972,8 @@ AGGR_OP::end_send() > else > { > rc= evaluate_join_record(join, join_tab, 0); > + /* Increment the counter after copying rows. */ > + join_tab->join->thd->get_stmt_da()->inc_current_row_for_warning(); > } > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] MDEV-26996: On removing SEL_ARG::is_ascending
by Sergey Petrunia 20 Dec '21

20 Dec '21

Hello Igor, Please check out this: https://github.com/MariaDB/server/tree/bb-10.8-mdev26996-no-sel-arg-ascendi… This is a patch that removes SEL_ARG::is_ascending. Does it make the code better in your opinion? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 0

Re: [Maria-developers] [Commits] 677643a: MDEV-27262 Unexpected index intersection with full index scan for an index
by Sergey Petrunia 20 Dec '21

20 Dec '21

Hello Igor, I'm trying examples with this patch: create table t1 (a int not null, key(a)); insert into t1 select seq from seq_1_to_1000; First, I debug this example: explain select * from t1 where a <10 or a>=10; The execution enters key_or(), passes through the new code this patch has added and key_or() returns NULL. Good. But if I modify the example slightly: explain select * from t1 where a <20 or a>=10 ; Here, key_or() still returns a SEL_ARG object that represents an infinite range: (gdb) fini Run till exit from #0 key_or (...) 0x0000555555f1fca2 in tree_or (...) Value returned is $24 = (SEL_ARG *) 0x7fff70064d88 (gdb) p dbug_print_sel_arg($24) $26 = 0x7fff70024390 "SEL_ARG(-inf<=a<=+inf)" Should this case be fixed as well? On Fri, Dec 17, 2021 at 02:11:40PM -0800, IgorBabaev wrote: > revision-id: 677643a80986107491b7886441f2828384f0494b (mariadb-10.2.31-1286-g677643a) > parent(s): 8bb55633699612279744c055e22eeca8d4058273 > author: Igor Babaev > committer: Igor Babaev > timestamp: 2021-12-17 14:11:39 -0800 > message: > > MDEV-27262 Unexpected index intersection with full index scan for an index > > If when extracting a range condition foran index from the WHERE condition I think the comment wording could be improved also. > Range Optimizer sees that the range condition covers the whole index then > such condition should be discarded because cannot it be used in any range > scan. In some cases Range Optimizer really does it, but there remained some > conditions for which it was not done. As a result the optimizer could > produce index merge plans with the full index scan for one of the indexes > participating in the index merge. > This could be observed in one of the test cases from index_merge1.inc > where a plan with index_merge_sort_union was produced and in the test case > reported for this bug where a plan with index_merge_sort_intersect was > produced. In both cases one of two index scans participating in index merge > ran over the whole index. > The patch slightly changes the original above mentioned test case from > index_merge1.inc to be able to produce an intended plan employing > index_merge_sort_union. The original query was left to show that index > merge is not used for it anymore. > It should be noted that for the plan with index_merge_sort_intersect could > be chosen for execution only due to a defect in the InnoDB code that > returns wrong estimates for the cardinality of big ranges. > > This bug led to serious problems in 10.4+ where the optimization using > Rowid filters is employed (see mdev-26446). > > Approved by Oleksandr Byelkin <sanja(a)mariadb.com> > > --- > mysql-test/include/index_merge1.inc | 8 +++- > mysql-test/r/index_merge_myisam.result | 12 +++-- > mysql-test/r/range_innodb.result | 81 ++++++++++++++++++++++++++++++++++ > mysql-test/t/range_innodb.test | 78 ++++++++++++++++++++++++++++++++ > sql/opt_range.cc | 7 +++ > 5 files changed, 181 insertions(+), 5 deletions(-) > > diff --git a/mysql-test/include/index_merge1.inc b/mysql-test/include/index_merge1.inc > index b168a76..440f1f7 100644 > --- a/mysql-test/include/index_merge1.inc > +++ b/mysql-test/include/index_merge1.inc > @@ -150,15 +150,19 @@ explain select * from t0 where > (((key3 <7 and key7 < 6) or key5 < 2) and (key5 < 5 or key6 < 6)); > > explain select * from t0 where > - ((key3 <5 or key5 < 4) and (key1 < 4 or key2 < 4)) > + ((key3 < 4 or key5 < 4) and (key1 < 4 or key2 < 4)) > or > ((key3 >=5 or key5 < 2) and (key5 < 5 or key6 < 6)); > > explain select * from t0 force index(i1, i2, i3, i4, i5, i6 ) where > - ((key3 <5 or key5 < 4) and (key1 < 4 or key2 < 4)) > + ((key3 < 4 or key5 < 4) and (key1 < 4 or key2 < 4)) > or > ((key3 >=5 or key5 < 2) and (key5 < 5 or key6 < 6)); > > +explain select * from t0 force index(i1, i2, i3, i4, i5, i6 ) where > + ((key3 < 5 or key5 < 4) and (key1 < 4 or key2 < 4)) > + or > + ((key3 >=5 or key5 < 2) and (key5 < 5 or key6 < 6)); > # 8. Verify that "order by" after index merge uses filesort > select * from t0 where key1 < 5 or key8 < 4 order by key1; > > diff --git a/mysql-test/r/index_merge_myisam.result b/mysql-test/r/index_merge_myisam.result > index 5a23092..a096c34 100644 > --- a/mysql-test/r/index_merge_myisam.result > +++ b/mysql-test/r/index_merge_myisam.result > @@ -173,17 +173,23 @@ or > id select_type table type possible_keys key key_len ref rows Extra > 1 SIMPLE t0 index_merge i1,i2,i3,i5,i6,i7 i3,i5 4,4 NULL 11 Using sort_union(i3,i5); Using where > explain select * from t0 where > -((key3 <5 or key5 < 4) and (key1 < 4 or key2 < 4)) > +((key3 < 4 or key5 < 4) and (key1 < 4 or key2 < 4)) > or > ((key3 >=5 or key5 < 2) and (key5 < 5 or key6 < 6)); > id select_type table type possible_keys key key_len ref rows Extra > 1 SIMPLE t0 ALL i1,i2,i3,i5,i6 NULL NULL NULL 1024 Using where > explain select * from t0 force index(i1, i2, i3, i4, i5, i6 ) where > -((key3 <5 or key5 < 4) and (key1 < 4 or key2 < 4)) > +((key3 < 4 or key5 < 4) and (key1 < 4 or key2 < 4)) > or > ((key3 >=5 or key5 < 2) and (key5 < 5 or key6 < 6)); > id select_type table type possible_keys key key_len ref rows Extra > -1 SIMPLE t0 index_merge i1,i2,i3,i5,i6 i3,i5 0,4 NULL 1024 Using sort_union(i3,i5); Using where > +1 SIMPLE t0 index_merge i1,i2,i3,i5,i6 i3,i5 4,4 NULL 1024 Using sort_union(i3,i5); Using where > +explain select * from t0 force index(i1, i2, i3, i4, i5, i6 ) where > +((key3 < 5 or key5 < 4) and (key1 < 4 or key2 < 4)) > +or > +((key3 >=5 or key5 < 2) and (key5 < 5 or key6 < 6)); > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t0 ALL i1,i2,i3,i5,i6 NULL NULL NULL 1024 Using where > select * from t0 where key1 < 5 or key8 < 4 order by key1; > key1 key2 key3 key4 key5 key6 key7 key8 > 1 1 1 1 1 1 1 1023 > diff --git a/mysql-test/r/range_innodb.result b/mysql-test/r/range_innodb.result > index f2349f2..ccb6da3 100644 > --- a/mysql-test/r/range_innodb.result > +++ b/mysql-test/r/range_innodb.result > @@ -108,3 +108,84 @@ DROP TABLE t0,t1; > SET @@GLOBAL.debug_dbug = @saved_dbug; > set @@optimizer_switch= @optimizer_switch_save; > # End of 10.1 tests > +# > +# MDEV-27262: Index intersection with full scan over an index > +# > +CREATE TABLE t1 ( > +id int(10) unsigned NOT NULL AUTO_INCREMENT, > +p char(32) DEFAULT NULL, > +es tinyint(3) unsigned NOT NULL DEFAULT 0, > +er tinyint(3) unsigned NOT NULL DEFAULT 0, > +x mediumint(8) unsigned NOT NULL DEFAULT 0, > +PRIMARY KEY (id), > +INDEX es (es), > +INDEX x (x), > +INDEX er (er,x), > +INDEX p (p) > +) ENGINE=InnoDB DEFAULT CHARSET=latin1; > +insert into t1(es,er) select 0, 1 from seq_1_to_45; > +insert into t1(es,er) select 0, 2 from seq_1_to_49; > +insert into t1(es,er) select 0, 3 from seq_1_to_951; > +insert into t1(es,er) select 0, 3 from seq_1_to_1054; > +insert into t1(es,er) select 0, 6 from seq_1_to_25; > +insert into t1(es,er) select 0, 11 from seq_1_to_1; > +insert into t1(es,er) select 1, 1 from seq_1_to_45; > +insert into t1(es,er) select 1, 2 from seq_1_to_16; > +insert into t1(es,er) select 1, 3 from seq_1_to_511; > +insert into t1(es,er) select 1, 4 from seq_1_to_687; > +insert into t1(es,er) select 1, 6 from seq_1_to_50; > +insert into t1(es,er) select 1, 7 from seq_1_to_4; > +insert into t1(es,er) select 1, 11 from seq_1_to_1; > +insert into t1(es,er) select 2, 1 from seq_1_to_82; > +insert into t1(es,er) select 2, 2 from seq_1_to_82; > +insert into t1(es,er) select 2, 3 from seq_1_to_1626; > +insert into t1(es,er) select 2, 4 from seq_1_to_977; > +insert into t1(es,er) select 2, 6 from seq_1_to_33; > +insert into t1(es,er) select 2, 11 from seq_1_to_1; > +insert into t1(es,er) select 3, 1 from seq_1_to_245; > +insert into t1(es,er) select 3, 2 from seq_1_to_81; > +insert into t1(es,er) select 3, 3 from seq_1_to_852; > +insert into t1(es,er) select 3, 4 from seq_1_to_2243; > +insert into t1(es,er) select 3, 6 from seq_1_to_44; > +insert into t1(es,er) select 3, 11 from seq_1_to_1; > +insert into t1(es,er) select 4, 1 from seq_1_to_91; > +insert into t1(es,er) select 4, 2 from seq_1_to_83; > +insert into t1(es,er) select 4, 3 from seq_1_to_297; > +insert into t1(es,er) select 4, 4 from seq_1_to_2456; > +insert into t1(es,er) select 4, 6 from seq_1_to_19; > +insert into t1(es,er) select 4, 11 from seq_1_to_1; > +update t1 set p='foobar'; > +update t1 set x=0; > +set @save_isp=@@innodb_stats_persistent; > +set global innodb_stats_persistent= 1; > +analyze table t1; > +Table Op Msg_type Msg_text > +test.t1 analyze status OK > +set optimizer_switch='index_merge_sort_intersection=on'; > +SELECT * FROM t1 > +WHERE ((p = 'foo' AND er != 4) OR er = 4 ) AND (es >= 4) LIMIT 2; > +id p es er x > +14645 foobar 4 4 0 > +14646 foobar 4 4 0 > +EXPLAIN EXTENDED SELECT * FROM t1 > +WHERE ((p = 'foo' AND er != 4) OR er = 4 ) AND (es >= 4) LIMIT 2; > +id select_type table type possible_keys key key_len ref rows filtered Extra > +1 SIMPLE t1 range es,er,p es 1 NULL # 100.00 Using index condition; Using where > +Warnings: > +Note 1003 select `test`.`t1`.`id` AS `id`,`test`.`t1`.`p` AS `p`,`test`.`t1`.`es` AS `es`,`test`.`t1`.`er` AS `er`,`test`.`t1`.`x` AS `x` from `test`.`t1` where (`test`.`t1`.`p` = 'foo' and `test`.`t1`.`er` <> 4 or `test`.`t1`.`er` = 4) and `test`.`t1`.`es` >= 4 limit 2 > +set optimizer_switch='index_merge_sort_intersection=off'; > +SELECT * FROM t1 > +WHERE ((p = 'foo' AND er != 4) OR er = 4 ) AND (es >= 4) LIMIT 2; > +id p es er x > +14645 foobar 4 4 0 > +14646 foobar 4 4 0 > +EXPLAIN EXTENDED SELECT * FROM t1 > +WHERE ((p = 'foo' AND er != 4) OR er = 4 ) AND (es >= 4) LIMIT 2; > +id select_type table type possible_keys key key_len ref rows filtered Extra > +1 SIMPLE t1 range es,er,p es 1 NULL # 100.00 Using index condition; Using where > +Warnings: > +Note 1003 select `test`.`t1`.`id` AS `id`,`test`.`t1`.`p` AS `p`,`test`.`t1`.`es` AS `es`,`test`.`t1`.`er` AS `er`,`test`.`t1`.`x` AS `x` from `test`.`t1` where (`test`.`t1`.`p` = 'foo' and `test`.`t1`.`er` <> 4 or `test`.`t1`.`er` = 4) and `test`.`t1`.`es` >= 4 limit 2 > +set optimizer_switch='index_merge_sort_intersection=default'; > +set global innodb_stats_persistent= @save_isp; > +DROP TABLE t1; > +# End of 10.2 tests > diff --git a/mysql-test/t/range_innodb.test b/mysql-test/t/range_innodb.test > index 428e5c2..7420e72 100644 > --- a/mysql-test/t/range_innodb.test > +++ b/mysql-test/t/range_innodb.test > @@ -116,3 +116,81 @@ SET @@GLOBAL.debug_dbug = @saved_dbug; > set @@optimizer_switch= @optimizer_switch_save; > > --echo # End of 10.1 tests > + > +--echo # > +--echo # MDEV-27262: Index intersection with full scan over an index > +--echo # > + > +--source include/have_sequence.inc > + > +CREATE TABLE t1 ( > + id int(10) unsigned NOT NULL AUTO_INCREMENT, > + p char(32) DEFAULT NULL, > + es tinyint(3) unsigned NOT NULL DEFAULT 0, > + er tinyint(3) unsigned NOT NULL DEFAULT 0, > + x mediumint(8) unsigned NOT NULL DEFAULT 0, > + PRIMARY KEY (id), > + INDEX es (es), > + INDEX x (x), > + INDEX er (er,x), > + INDEX p (p) > +) ENGINE=InnoDB DEFAULT CHARSET=latin1; > + > +insert into t1(es,er) select 0, 1 from seq_1_to_45; > +insert into t1(es,er) select 0, 2 from seq_1_to_49; > +insert into t1(es,er) select 0, 3 from seq_1_to_951; > +insert into t1(es,er) select 0, 3 from seq_1_to_1054; > +insert into t1(es,er) select 0, 6 from seq_1_to_25; > +insert into t1(es,er) select 0, 11 from seq_1_to_1; > +insert into t1(es,er) select 1, 1 from seq_1_to_45; > +insert into t1(es,er) select 1, 2 from seq_1_to_16; > +insert into t1(es,er) select 1, 3 from seq_1_to_511; > +insert into t1(es,er) select 1, 4 from seq_1_to_687; > +insert into t1(es,er) select 1, 6 from seq_1_to_50; > +insert into t1(es,er) select 1, 7 from seq_1_to_4; > +insert into t1(es,er) select 1, 11 from seq_1_to_1; > +insert into t1(es,er) select 2, 1 from seq_1_to_82; > +insert into t1(es,er) select 2, 2 from seq_1_to_82; > +insert into t1(es,er) select 2, 3 from seq_1_to_1626; > +insert into t1(es,er) select 2, 4 from seq_1_to_977; > +insert into t1(es,er) select 2, 6 from seq_1_to_33; > +insert into t1(es,er) select 2, 11 from seq_1_to_1; > +insert into t1(es,er) select 3, 1 from seq_1_to_245; > +insert into t1(es,er) select 3, 2 from seq_1_to_81; > +insert into t1(es,er) select 3, 3 from seq_1_to_852; > +insert into t1(es,er) select 3, 4 from seq_1_to_2243; > +insert into t1(es,er) select 3, 6 from seq_1_to_44; > +insert into t1(es,er) select 3, 11 from seq_1_to_1; > +insert into t1(es,er) select 4, 1 from seq_1_to_91; > +insert into t1(es,er) select 4, 2 from seq_1_to_83; > +insert into t1(es,er) select 4, 3 from seq_1_to_297; > +insert into t1(es,er) select 4, 4 from seq_1_to_2456; > +insert into t1(es,er) select 4, 6 from seq_1_to_19; > +insert into t1(es,er) select 4, 11 from seq_1_to_1; > +update t1 set p='foobar'; > +update t1 set x=0; > +set @save_isp=@@innodb_stats_persistent; > +set global innodb_stats_persistent= 1; > +analyze table t1; > + > +let $q= > +SELECT * FROM t1 > + WHERE ((p = 'foo' AND er != 4) OR er = 4 ) AND (es >= 4) LIMIT 2; > + > +set optimizer_switch='index_merge_sort_intersection=on'; > +eval $q; > +--replace_column 9 # > +eval EXPLAIN EXTENDED $q; > + > +set optimizer_switch='index_merge_sort_intersection=off'; > +# execution of $q and explain for it led to an assertion failure in 10.4 > +# (with the optimizer switch rowid_filter set to 'on') > +eval $q; > +--replace_column 9 # > +eval EXPLAIN EXTENDED $q; > +set optimizer_switch='index_merge_sort_intersection=default'; > + > +set global innodb_stats_persistent= @save_isp; > +DROP TABLE t1; > + > +--echo # End of 10.2 tests > diff --git a/sql/opt_range.cc b/sql/opt_range.cc > index f3f1843..2e05b88 100644 > --- a/sql/opt_range.cc > +++ b/sql/opt_range.cc > @@ -9413,6 +9413,13 @@ key_or(RANGE_OPT_PARAM *param, SEL_ARG *key1,SEL_ARG *key2) > key2->copy_min(tmp); > if (!(key1=key1->tree_delete(tmp))) > { // Only one key in tree > + if (key2->min_flag & NO_MIN_RANGE && > + key2->max_flag & NO_MAX_RANGE) > + { > + if (key2->maybe_flag) > + return new SEL_ARG(SEL_ARG::MAYBE_KEY); > + return 0; // Always true OR > + } > key1=key2; > key1->make_root(); > key2=key2_next; BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 0

[Maria-developers] Review: bb-10.8-wlad-MDEV-26713
by Alexander Barkov 13 Dec '21

13 Dec '21

Hi Wlad, You've done a great piece of work. Thanks and congrats. I generally like all the changes. Have a few comments only, please see below. commit 1411d5f40ceaf2eecabbcf68588743439003f4a2 Author: Vladislav Vaintroub <wlad(a)mariadb.com> Date: Fri Nov 19 11:42:46 2021 +0100 Windows : incorrect handling of non-ASCIIs in get_tty_password Why no MDEV? + wchar_t wbuf[80]; + char to[80*3]; /* there is at most 3 bytes per wchar, in any codepage */ + wchar_t *pos=wbuf,*end=wbuf + array_elements(wbuf)-1; Isn't it "at most 4 bytes per char"? ############################################################# commit 7af691ca11c95a385aff4889a56870d70843e78d Author: Vladislav Vaintroub <wlad(a)mariadb.com> Date: Fri Nov 19 12:03:48 2021 +0100 MDEV-27090: Windows client - ReadConsoleA does not work correctly with UTF8 codepage Corresponding Windows bug https://github.com/microsoft/terminal/issues/4551 Use ReadConsoleW instead and convert to console's input codepage, to workaround. Also, disable VT sequences in the console output, as we do not knows what type of data comes with SELECT, we do not want VT escapes there. Remove my_cgets() + while (nchars > 0) + { + if (wstrbuf[nchars - 1] != '\n' && wstrbuf[nchars - 1] != '\r') + break; + wstrbuf[--nchars]= 0; + } + + wstrbuf[nchars]= 0; The inner assignment wstrbuf[--nchars]=0 inside every iteration is not needed. I'd write it this way: for ( ; nchars > 0 ; nchars--) { if (wstrbuf[nchars - 1] != '\n' && wstrbuf[nchars - 1] != '\r') break; } wstrbuf[nchars]= 0; But the outer assignment to 0 does not seem to be needed either. Why do it? my_convert() does not need the trailing zero. It uses only sizes passed. So this should be enough: + strbuf[len]= 0; - pthread_auto_mutex_decl(my_conio_cs); - - /* lock the console for the current process*/ - if (pthread_auto_mutex_lock(my_conio_cs, GetCurrentProcessId(), INFINITE)) - { There was a mutex lock inside my_cgets(). Why was it needed before the patch and why is it not needed any more? ##################################################### commit 5c09e9ce4f922fe9674f3f85329742c0c5498a28 Author: Vladislav Vaintroub <wlad(a)mariadb.com> Date: Fri Nov 19 14:03:51 2021 +0100 MDEV-27092 Windows - services that have non-ASCII characters do not work with activeCodePage=UTF8 CreateServiceA, OpenServiceA, and couple of other functions do not work correctly with non-ASCII character, in the special case where application has defined activeCodePage=UTF8. ... - Copyright (c) 2011, 2012, Monty Program Ab + Copyright (c) 2011, 2021 Monty Program Ab MariaDB Corporation? This code: + AWSTRDUP(w_ServiceName,lpServiceName); + AWSTRDUP(w_DisplayName,lpDisplayName); + AWSTRDUP(w_BinaryPathName, lpBinaryPathName); + AWSTRDUP(w_LoadOrderGroup, lpLoadOrderGroup); + AWSTRDUP(w_Dependencies, lpDependencies); + AWSTRDUP(w_ServiceStartName, lpServiceStartName); + AWSTRDUP(w_Password, lpPassword); + + free(w_ServiceName); + free(w_DisplayName); + free(w_BinaryPathName); + free(w_LoadOrderGroup); + free(w_Dependencies); + free(w_ServiceStartName); + free(w_Password); duplicates two times. Consider wrapping these variables into a class and reuse the class in both cases. ########################################################## commit d4056a28ec5ae0c9a0c3144a8e90357dc574723f Author: Vladislav Vaintroub <wlad(a)mariadb.com> Date: Fri Nov 19 14:14:38 2021 +0100 MDEV-27093 Do not pass root password in clear text from mariadb-install-db.exe to bootstrap Pass the hash instead, so that bootstrap would not misinterpret it for string with different charset. The password was passed using hex encoding. But the comment says it was clear text. Wrong comment? I did not understand the change. Reading the MDEV did not make it clearer. What was wrong? How does the patch fix the problem? Can we discuss on slack? ######################################################## commit fa29a64de918d2045df3cce1d4a546de5597c0bd Author: Vladislav Vaintroub <wlad(a)mariadb.com> Date: Mon Nov 22 12:29:15 2021 +0100 MDEV-26713 set activeCodePage=UTF8 for windows programs - Use corresponding entry in the manifest, as described in https://docs.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-c… - Set console codepage(s) to UTF8, if ANSI codepage is UTF8 This effectively causes UTF8 to be used as default client encoding, for Windows 1903 and later - Allow UTF8 on the command line for MTR, for the affected Windows versions Looks OK for me. ######################################################## commit b88d54132369b938ba041c0d3f97a5a212210b37 Author: Vladislav Vaintroub <wlad(a)mariadb.com> Date: Mon Nov 22 12:34:10 2021 +0100 MDEV-26713 : Treat codepage 65001 as utf8mb4, not just utf8 Also, fix the "UTF8" option in MSI, which is responsible for character-set-server setting Looks OK for me. ######################################################## commit b9c38470ed6e04185f7f84a056034b7254336d51 Author: Vladislav Vaintroub <wlad(a)mariadb.com> Date: Mon Nov 22 13:22:05 2021 +0100 MDEV-26713 utf8 support on Windows, mysql_upgrade_service part Handle upgrade - on Windows that is capable of UTF8 codepage, convert entries in my.ini from ANSI to UTF8, during upgrade Reason is that paths must now be utf8. For the client , user name, database etc should be in UTF8, too, as this is now the default charset. Should not we add an UTF8 BOM to the beginning of the file, so text editors do not break it later. The patch looks too large for me. It seems it does more than the comments above tell. Looks like convenience refactoring, but it's hard to follow. It would be easier to understand changes if you split it into two parts: 1. the refactoring that adds functions like upgrade_config_file(), fix_section(), is_mariadb_section(), fix_and_check_datadir(), I guess this refactoring is a non-functional change that should not change the behaviour, right? 2. the actual conversion part on top of it ######################################################## commit 0700393b570c6b9a7b4347d3381a7cf05d1499ba Author: Vladislav Vaintroub <wlad(a)mariadb.com> Date: Tue Nov 23 12:28:28 2021 +0100 mysqltest, Windows - support background execution in 'exec' command If last character in command is ampersand, do not use my_popen/my_pclose in "exec", instead do CreateProcess() without a window. I suggest you add an MDEV for this change and explain in there why we did not need this feature earlier and why we need it now and how it relates to MDEV-26713. ######################################################## commit 7d50d83b3bcab5a3be761f98722e33ed0b01ab23 Author: Vladislav Vaintroub <wlad(a)mariadb.com> Date: Tue Nov 23 13:05:25 2021 +0100 MDEV-26713 utf8 support on Windows , add mysql_install_db tests Add mysql_install_db test with some i18n, for data dir and root password Looks OK for me. ######################################################## commit b2cc09fe76f526bd4cd76131a52263b13b3828a5 Author: Vladislav Vaintroub <wlad(a)mariadb.com> Date: Wed Nov 24 10:15:11 2021 +0100 MDEV-26713 Add test for mysql_install_db creating service, with i18 Looks OK for me. ######################################################## commit 51ce069376fec45d340563a4f7630f8a155afd64 Author: Vladislav Vaintroub <wlad(a)mariadb.com> Date: Fri Nov 26 18:41:35 2021 +0100 MDEV-26713 Introduce --password-codepage client setting I'm not sure we need this new options. Looks like over-engineering a bit. Is the scenario it covers really deserves documenting, learning, etc? I'd better disallow non-ASCII passwords in the future, and right now just add a warning: You're using a non-ASCII password, which is not portable. If you have problems connecting to the database, please ask the administrator to change the password. ######################################################## commit b155c4036cfb3ffdaa7c272048ac9d32e3182d78 Author: Vladislav Vaintroub <wlad(a)mariadb.com> Date: Mon Nov 29 19:47:36 2021 +0100 MDEV-26713 set console codepage to what user set in --default-character-set I prefer to keep the comment that you removed: - if (!fs_cset_cache) - { - char buf[10]= "cp"; - GetLocaleInfo(LOCALE_SYSTEM_DEFAULT, LOCALE_IDEFAULTANSICODEPAGE, - buf+2, sizeof(buf)-3); - /* - We cannot call get_charset_by_name here - because fs_character_set() is executed before - LOCK_THD_charset mutex initialization, which - is used inside get_charset_by_name. - As we're now interested in cp932 only, - let's just detect it using strcmp(). - */ - fs_cset_cache= - #ifdef HAVE_CHARSET_cp932 - !strcmp(buf, "cp932") ? &my_charset_cp932_japanese_ci : - #endif - &my_charset_bin; This function uses both default_charset and charset_info->cs_name.str: +static void adjust_console_codepage() +{ +#ifdef _WIN32 + const char *name= charset_info->cs_name.str; + if (my_set_console_cp(default_charset) < 0) And it seems the former is set to the latter just before the function call: default_charset= (char *)charset_info->cs_name.str; <------ HERE put_info("Charset changed", INFO_INFO); adjust_console_codepage(); I find this hard to follow. Can you please add a parameter to adjust_console_codepage() and pass either charset_info->cs_name.str (or default_charset) when calling? Also, I think this function should only be called when default_charset was some explicit name, not MYSQL_AUTODETECT_CHARSET_NAME. I think the logic should be: 1. If --default-character-set=auto, then detect it from the console. 2. If --default-character-set=csname, then set the console CP to csname. Now in case of N1 it seems to detect the character set from the console, then sets the console to what has been detected. Is the last step really needed in case of auto-detection? ######################################################## commit b38c341cc392630c798eb754f87338d37ec95cdf Author: Vladislav Vaintroub <wlad(a)mariadb.com> Date: Sun Dec 5 17:08:18 2021 +0100 Fix race condition in test. Do not change console codepage in a possibly shared console. Start new console for chcp. Can you extend the comment and tell what actually happened before this change? Greetings.

2 2

[Maria-developers] Review for: MDEV-26938 Support descending indexes internally in InnoDB (server part)
by Sergey Petrunia 11 Dec '21

11 Dec '21

Hi Serg, There is some non-trivial input too, after all. Please find below. > commit da2903f03de039693354176fda836e6642d6d0f0 > Author: Sergei Golubchik <serg(a)mariadb.org> > Date: Wed Nov 24 16:50:21 2021 +0100 > > MDEV-26938 Support descending indexes internally in InnoDB (server part) > > * preserve DESC index property in the parser > * store it in the frm (only for HA_KEY_ALG_BTREE) > * read it from the frm > * show it in SHOW CREATE > * skip DESC indexes in opt_range.cc and opt_sum.cc > * ORDER BY test > == Trivial input == > diff --git a/sql/unireg.cc b/sql/unireg.cc > index 2726b9a68c2..d6449eeca4c 100644 > --- a/sql/unireg.cc > +++ b/sql/unireg.cc > @@ -685,6 +685,14 @@ static uint pack_keys(uchar *keybuff, uint key_count, KEY *keyinfo, > DBUG_PRINT("loop", ("flags: %lu key_parts: %d key_part: %p", > key->flags, key->user_defined_key_parts, > key->key_part)); > + /* For SPATIAL, FULLTEXT and HASH indexes (anything other than B-tree), > + ignore the ASC/DESC attribute of columns. */ > + const uchar ha_reverse_sort= key->algorithm == HA_KEY_ALG_BTREE > + ? HA_REVERSE_SORT : > + (key->algorithm == HA_KEY_ALG_UNDEF && > + !(key->flags & (HA_FULLTEXT | HA_SPATIAL))) > + ? HA_REVERSE_SORT : 0; The above is hard to read for me. I think if-statement form would be more readable: const uchar ha_reverse_sort= 0; if (key->algorithm == HA_KEY_ALG_BTREE || (key->algorithm == HA_KEY_ALG_UNDEF && !(key->flags & (HA_FULLTEXT | HA_SPATIAL)) ha_reverse_sort= HA_REVERSE_SORT; == Issue #1: ordered index scan is not handled in ha_partition == Testcase: create table t10 (a int, b int, key(a desc)) partition by hash(a) partitions 4; insert into t10 select seq, seq from seq_0_to_999; MariaDB [test]> select * from t10 order by a limit 3; +------+------+ | a | b | +------+------+ | 3 | 3 | | 7 | 7 | | 11 | 11 | +------+------+ 3 rows in set (0.002 sec) Correct output: MariaDB [test]> select * from t10 use index() order by a limit 3; +------+------+ | a | b | +------+------+ | 0 | 0 | | 1 | 1 | | 2 | 2 | +------+------+ 3 rows in set (0.011 sec) == Issue #2: extra warning == MariaDB [test]> create table t1 (a int, b int, key(a), key(a)); Query OK, 0 rows affected, 1 warning (0.016 sec) MariaDB [test]> show warnings; +-------+------+--------------------------------------------------------------------------------------+ | Level | Code | Message | +-------+------+--------------------------------------------------------------------------------------+ | Note | 1831 | Duplicate index `a_2`. This is deprecated and will be disallowed in a future release | +-------+------+--------------------------------------------------------------------------------------+ 1 row in set (0.000 sec) This is ok. MariaDB [test]> create table t2 (a int, b int, key(a), key(a desc)); Query OK, 0 rows affected, 1 warning (0.004 sec) MariaDB [test]> show warnings; +-------+------+--------------------------------------------------------------------------------------+ | Level | Code | Message | +-------+------+--------------------------------------------------------------------------------------+ | Note | 1831 | Duplicate index `a_2`. This is deprecated and will be disallowed in a future release | +-------+------+--------------------------------------------------------------------------------------+ 1 row in set (0.000 sec) This is probably not? MySQL doesn't produce this warning in this case. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

2 3