January 2022 - developers - lists.mariadb.org

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