- developers - lists.mariadb.org

Re: [Maria-developers] MDEV-23766:Optimizer tracing code is prone to producing invalid JSON
by Sergey Petrunia 31 Oct '21

31 Oct '21

Hi Serg, Ok the patch moving in the right direction but it's not there, yet. Please find the input below. First, input on the commit comment. Please consider it as a request applying to ALL further commits to the MariaDB codebase. > MDEV-23766: implemented requested debug checks Imagine somebody looking at this in a few years. Will they know what checks were requested? They might get a clue by looking at the Jira text, but the idea to make the commit comments concise and self-contained descriptions. Something like this: Line #1: one-line description of the patch: >> MDEV-23766: Make attempts to write invalid JSON cause assertion failures Subsequent lines: >> Make JSON writing API (class Json_writer) check the produced JSON document >> is valid. The following checks are made: >> - JSON objects must contain named members >> - JSON arrays must contain unnamed members. >> - (TODO: add other restrictions we're enforcing). It is not clear how the checker interacts with Single_line_formatting_helper. The code like one the one I'm quoting below shows some non-trivial overlap: > @@ -28,7 +36,16 @@ void Json_writer::append_indent() > > void Json_writer::start_object() > { > +#ifndef NDEBUG > + DBUG_ASSERT(got_name == named_item_expected()); > + named_items_expectation.push_back(true); > + size_t depth= named_items_expectation.size(); > +#endif > fmt_helper.on_start_object(); > +#ifndef NDEBUG > + if (depth != named_items_expectation.size()) > + named_items_expectation.pop_back(); > +#endif > > if (!element_started) > start_element(); This looks very cryptic. Can you elaborate about this? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 1

Re: [Maria-developers] 85aa3f8cf66: MDEV-26102: dgcov: add support for *.gcda.gcov.json.gz files of gcov 9.1+
by Sergei Golubchik 31 Oct '21

31 Oct '21

Hi, Anel! Looks ok. Except that you have that pointless $gcc_version=~ line that I wrote about in a previous review. Did you verify that this script actually delivers meaningful results? On Oct 30, Anel Husakovic wrote: > revision-id: 85aa3f8cf66 (mariadb-10.2.39-96-g85aa3f8cf66) > parent(s): afe00bb7cce > author: Anel Husakovic > committer: Anel Husakovic > timestamp: 2021-07-29 12:42:44 +0200 > message: > > MDEV-26102: dgcov: add support for *.gcda.gcov.json.gz files of gcov 9.1+ > > Reviewed by: serg(a)mariadb.com > > --- > mysql-test/dgcov.pl | 55 +++++++++++++++++++++++++++++++++++------------------ > 1 file changed, 37 insertions(+), 18 deletions(-) > > diff --git a/mysql-test/dgcov.pl b/mysql-test/dgcov.pl > index 2c00c64d1ff..543099c1b4c 100755 > --- a/mysql-test/dgcov.pl > +++ b/mysql-test/dgcov.pl > @@ -62,12 +62,15 @@ my $res; > + > +my $gcc_version= `gcc -dumpversion`; > +$gcc_version=~ s/(\d).*$/$1/; > > find(\&gcov_one_file, $root); > find(\&write_coverage, $root) if $opt_generate; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 2c8f52ea53d: MDEV-23328 Server hang due to Galera lock conflict resolution
by Sergei Golubchik 29 Oct '21

29 Oct '21

Hi, Seppo, Jan! this is pretty much the same as in 10.4, so same comments apply, otherwise ok. On Oct 29, sjaakola wrote: > revision-id: 2c8f52ea53d (mariadb-10.5.12-108-g2c8f52ea53d) > parent(s): 910fc72a7f1 > author: sjaakola > committer: Jan Lindström > timestamp: 2021-10-28 12:00:41 +0300 > message: > > MDEV-23328 Server hang due to Galera lock conflict resolution Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 778d88ea2d9: MDEV-23328 Server hang due to Galera lock conflict resolution
by Sergei Golubchik 29 Oct '21

29 Oct '21

Hi, Seppo, Jan! Looks pretty much ok too. A few comments, see below On Oct 29, sjaakola wrote: > revision-id: 778d88ea2d9 (mariadb-10.4.21-76-g778d88ea2d9) > parent(s): e2e4e06f081 > author: sjaakola > committer: Jan Lindström > timestamp: 2021-10-28 11:06:22 +0300 > message: > > MDEV-23328 Server hang due to Galera lock conflict resolution > > diff --git a/sql/log_event.cc b/sql/log_event.cc > index c1c753d69fd..a1562016aa4 100644 > --- a/sql/log_event.cc > +++ b/sql/log_event.cc > @@ -8987,6 +8987,18 @@ bool Xid_log_event::print(FILE* file, PRINT_EVENT_INFO* print_event_info) > > > #if defined(HAVE_REPLICATION) && !defined(MYSQL_CLIENT) > +static bool wsrep_must_replay(THD *thd) > +{ > +#ifdef WITH_WSREP > + mysql_mutex_lock(&thd->LOCK_thd_data); > + bool res= WSREP(thd) && thd->wsrep_trx().state() == wsrep::transaction::s_must_replay; check WSREP(thd) outside of the mutex > + mysql_mutex_unlock(&thd->LOCK_thd_data); > + return res; > +#else > + return false; > +#endif > +} > + > int Xid_log_event::do_apply_event(rpl_group_info *rgi) > { > bool res; > diff --git a/sql/service_wsrep.cc b/sql/service_wsrep.cc > index 6566bb372d8..18067a4d0ec 100644 > --- a/sql/service_wsrep.cc > +++ b/sql/service_wsrep.cc > @@ -29,12 +29,14 @@ extern "C" my_bool wsrep_on(const THD *thd) > > extern "C" void wsrep_thd_LOCK(const THD *thd) > { > + mysql_mutex_lock(&thd->LOCK_thd_kill); > mysql_mutex_lock(&thd->LOCK_thd_data); I presume, you can remove wsrep_thd_kill_LOCK() and wsrep_thd_kill_UNLOCK() that I've added > } > > extern "C" void wsrep_thd_UNLOCK(const THD *thd) > { > mysql_mutex_unlock(&thd->LOCK_thd_data); > + mysql_mutex_unlock(&thd->LOCK_thd_kill); > } > > extern "C" void wsrep_thd_kill_LOCK(const THD *thd) > @@ -295,8 +301,6 @@ extern "C" my_bool wsrep_thd_is_aborting(const MYSQL_THD thd) > default: > return false; > } > - > - return false; this looks like it begs for the compiler warning. please, keep the return where it was. > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 002192b92cf: MDEV-23328 Server hang due to Galera lock conflict resolution
by Sergei Golubchik 27 Oct '21

27 Oct '21

Hi, Seppo, Jan! On Oct 27, sjaakola wrote: > revision-id: 002192b92cf (mariadb-10.3.31-60-g002192b92cf) > parent(s): 736da112b65 > author: sjaakola > committer: Jan Lindström > timestamp: 2021-10-27 12:15:48 +0300 > message: > > MDEV-23328 Server hang due to Galera lock conflict resolution This 10.3 version is also ok to push when you have 10.4 and 10.5 versions ready. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] d0e18d694f0: MDEV-23328 Server hang due to Galera lock conflict resolution
by Sergei Golubchik 27 Oct '21

27 Oct '21

Hi, Seppo, Jan! On Oct 27, sjaakola wrote: > revision-id: d0e18d694f0 (mariadb-10.2.40-128-gd0e18d694f0) > parent(s): 408277e8012 > author: sjaakola > committer: Jan Lindström > timestamp: 2021-10-27 12:12:53 +0300 > message: > > MDEV-23328 Server hang due to Galera lock conflict resolution This commit is ok to push, thanks! Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] d1ec0231553: MDEV-26732 Assertion `0' failed in Item::val_native
by Sergei Golubchik 26 Oct '21

26 Oct '21

Hi, Alexander! On Oct 25, Alexander Barkov wrote: > revision-id: d1ec0231553 (mariadb-10.5.4-550-gd1ec0231553) > parent(s): 8b115503563 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2021-10-07 20:58:18 +0400 > message: > > MDEV-26732 Assertion `0' failed in Item::val_native > > Also fixes MDEV-24619 Wrong result or Assertion `0' in Item::val_native / Type_handler_inet6::Item_val_native_with_conversion would be good to have some explanation here, what was wrong, what was the fix. > diff --git a/plugin/type_inet/mysql-test/type_inet/type_inet6.result b/plugin/type_inet/mysql-test/type_inet/type_inet6.result > index da949481337..ac16f5c06ce 100644 > --- a/plugin/type_inet/mysql-test/type_inet/type_inet6.result > +++ b/plugin/type_inet/mysql-test/type_inet/type_inet6.result > @@ -2159,3 +2159,38 @@ IFNULL(c, '::1') > ::1 > DROP TABLE t2; > DROP TABLE t1; > +# > +# MDEV-26732 Assertion `0' failed in Item::val_native > +# > +SELECT CAST(CONCAT('::', REPEAT('',RAND())) AS INET6) AS f, var_pop('x') FROM dual HAVING f > ''; do you need RAND() here for a test case? can you replace it with a literal? do you need var_pop() here? > +f var_pop('x') > +Warnings: > +Warning 1292 Truncated incorrect DOUBLE value: 'x' > +Warning 1292 Incorrect inet6 value: '' > +SELECT CAST(CONCAT('::', REPEAT('',RAND())) AS INET6) AS f, var_pop(1) FROM dual HAVING f >= '::'; > +f var_pop(1) > +:: 0.0000 > +CREATE TABLE t1(id INET6 NOT NULL PRIMARY KEY, dsc INET6); > +INSERT INTO t1 VALUES ('::1', '1::1'),('::3', '1::3'),('::4', NULL); > +CREATE TABLE t2 SELECT COALESCE(t1.dsc), COUNT(*) FROM t1 GROUP BY t1.id; > +SELECT * FROM t2 ORDER BY 1,2; > +COALESCE(t1.dsc) COUNT(*) > +NULL 1 > +1::1 1 > +1::3 1 > +DROP TABLE t1, t2; > +# > +# MDEV-24619 Wrong result or Assertion `0' in Item::val_native / Type_handler_inet6::Item_val_native_with_conversion > +# > +CREATE TABLE t1 (a INET6); > +INSERT INTO t1 VALUES ('::'),('::'); > +SELECT IF(1, '::', a) AS f FROM t1 GROUP BY 'foo' HAVING f != ''; > +f > +Warnings: > +Warning 1292 Incorrect inet6 value: '' > +SELECT IF(1, '::', a) AS f FROM t1 GROUP BY 'foo' HAVING f != '::'; > +f > +SELECT IF(1, '::', a) AS f FROM t1 GROUP BY 'foo' HAVING f != '::1'; > +f > +:: > +DROP TABLE t1; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 5

Re: [Maria-developers] 6d0c1f3ae12: MDEV-23328 Server hang due to Galera lock conflict resolution
by Sergei Golubchik 25 Oct '21

25 Oct '21

Hi, Jan! This was good! Just a few questions, see below On Oct 24, Jan Lindström wrote: > revision-id: 6d0c1f3ae12 (mariadb-10.2.40-114-g6d0c1f3ae12) > parent(s): 3153cc799e2 > author: Jan Lindström > committer: Jan Lindström > timestamp: 2021-10-22 07:02:39 +0300 > message: > > MDEV-23328 Server hang due to Galera lock conflict resolution ... > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 3e1f248b082..5d40106d0ad 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -2159,6 +2159,7 @@ bool dispatch_command(enum enum_server_command command, THD *thd, > } > DBUG_PRINT("quit",("Got shutdown command for level %u", level)); > general_log_print(thd, command, NullS); > + DBUG_EXECUTE_IF("shutdown_unireg_abort", { unireg_abort(1); }); what's the point of this test? > my_eof(thd); > kill_mysql(thd); > error=TRUE; > diff --git a/sql/wsrep_mysqld.cc b/sql/wsrep_mysqld.cc > index e60100e2e90..7466654eead 100644 > --- a/sql/wsrep_mysqld.cc > +++ b/sql/wsrep_mysqld.cc > @@ -835,13 +835,25 @@ void wsrep_thr_init() > DBUG_VOID_RETURN; > } > > +/* This is wrapper for wsrep_break_lock in thr_lock.c */ > +static int wsrep_thr_abort_thd(void *bf_thd_ptr, void *victim_thd_ptr, my_bool signal) When is it called? Under what conditions? > +{ > + THD* victim_thd= (THD *) victim_thd_ptr; > + /* We need to lock THD::LOCK_thd_data to protect victim > + from concurrent usage or disconnect or delete. */ > + mysql_mutex_lock(&victim_thd->LOCK_thd_data); > + int res= wsrep_abort_thd(bf_thd_ptr, victim_thd_ptr, signal); > + return res; > +} > + > diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc > index 73b3625b6fd..56400264245 100644 > --- a/storage/innobase/handler/ha_innodb.cc > +++ b/storage/innobase/handler/ha_innodb.cc > @@ -19720,46 +19716,113 @@ wsrep_innobase_kill_one_trx( > +/******************************************************************* > +This function is used to kill one transaction in BF. */ > +void > +wsrep_innobase_kill_one_trx( > + MYSQL_THD const bf_thd, > + const trx_t * const bf_trx, > + trx_t *victim_trx, > + my_bool signal) > +{ > + ut_ad(bf_thd); > + ut_ad(victim_trx); > + ut_ad(lock_mutex_own()); > + ut_ad(trx_mutex_own(victim_trx)); > + > + DBUG_ENTER("wsrep_innobase_kill_one_trx"); > + THD *thd= (THD *) victim_trx->mysql_thd; > + > + /* Here we need to lock THD::LOCK_thd_data to protect from > + concurrent usage or disconnect or delete. */ > + DEBUG_SYNC(bf_thd, "wsrep_before_BF_victim_lock"); > + my_sleep(100000); Eh? Forgot to remove after debugging? > + wsrep_thd_LOCK(thd); > + my_sleep(100000); and here > + DEBUG_SYNC(bf_thd, "wsrep_after_BF_victim_lock"); > + Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

3 5

Re: [Maria-developers] 5c4db10d6ff: MDEV-25114: Crash: WSREP: invalid state ROLLED_BACK (FATAL)
by Sergei Golubchik 25 Oct '21

25 Oct '21

Hi, Jan! On Oct 24, Jan LindstrÃ¶m wrote: > revision-id: 5c4db10d6ff (mariadb-10.3.31-58-g5c4db10d6ff) > parent(s): f9b856b0525 > author: Jan Lindström > committer: Jan Lindström > timestamp: 2021-10-22 07:23:14 +0300 > message: > > MDEV-25114: Crash: WSREP: invalid state ROLLED_BACK (FATAL) > > Revert "MDEV-23328 Server hang due to Galera lock conflict resolution" > > This reverts commit 29bbcac0ee841faaa68eeb09c86ff825eabbe6b6. This was incorrectly reverted. 29bbcac0ee841faaa68eeb09c86ff825eabbe6b6 didn't apply to 10.3 verbatim, there were some merge related changes. To revert it properly you need to look at affected files before the corresponding 10.2->10.3 merge. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] dfa33fb309d: MDEV-26664 Store UUIDs in a more efficient manner
by Sergei Golubchik 22 Oct '21

22 Oct '21

Hi, Alexander! On Oct 19, Alexander Barkov wrote: > revision-id: dfa33fb309d (mariadb-10.6.1-84-gdfa33fb309d) > parent(s): 5cfb31854a4 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2021-10-19 13:27:19 +0400 > message: > > MDEV-26664 Store UUIDs in a more efficient manner > > UUID values > > llllllll-mmmm-Vhhh-vsss-nnnnnnnnnnnn > > are now stored as > > nnnnnnnnnnnn-vsss-Vhhh-mmmm-llllllll > > inside the record: > > - the groups (segments separated by dash) are reordered right-to-left. > - the bytes inside the groups are not reordered (stored as before, > in big-endian format). > > This provides a better sorting order: the earlier UUID was generated, > the higher it appears in the ORDER BY output. > > Also, this change enables a good key prefix compression, > because the constant part is now in the beginning, while > the non-constant part (the timestamp) is in the end. > > diff --git a/plugin/type_uuid/mysql-test/type_uuid/type_uuid.result b/plugin/type_uuid/mysql-test/type_uuid/type_uuid.result > index 6a08ffe8c23..6e2e1cc87ef 100644 > --- a/plugin/type_uuid/mysql-test/type_uuid/type_uuid.result > +++ b/plugin/type_uuid/mysql-test/type_uuid/type_uuid.result > @@ -596,10 +596,10 @@ INSERT INTO t1 VALUES (CAST(CONCAT('2','0000000-0000-0000-0000-000000000003') AS > SELECT * FROM t1 ORDER BY a; > a > 10000000-0000-0000-0000-000000000001 > -10000000-0000-0000-0000-000000000002 > -10000000-0000-0000-0000-000000000003 > 20000000-0000-0000-0000-000000000001 > +10000000-0000-0000-0000-000000000002 > 20000000-0000-0000-0000-000000000002 > +10000000-0000-0000-0000-000000000003 > 20000000-0000-0000-0000-000000000003 please, add a test for `ORDER BY CONCAT(a)` just to test the lexicographical ordering of UUIDs. Or may be `ORDER BY HEX(a)` ? Or `UNHEX` ? Whatever the recommended approach should be. > DROP TABLE t1; > # > diff --git a/plugin/type_uuid/mysql-test/type_uuid/type_uuid_memory.result b/plugin/type_uuid/mysql-test/type_uuid/type_uuid_memory.result > index 42bb74d4b01..eb7a51895b9 100644 > --- a/plugin/type_uuid/mysql-test/type_uuid/type_uuid_memory.result > +++ b/plugin/type_uuid/mysql-test/type_uuid/type_uuid_memory.result > @@ -25,7 +25,7 @@ a > 00000000-0000-0000-0000-0000000000ff > EXPLAIN SELECT * FROM t1 WHERE a='00000000-0000-0000-0000-0000000000ff'; > id select_type table type possible_keys key key_len ref rows Extra > -1 SIMPLE t1 ref a a 17 const 2 Using where > +1 SIMPLE t1 ref a a 17 const 4 Using where why? > diff --git a/plugin/type_uuid/sql_type_uuid.h b/plugin/type_uuid/sql_type_uuid.h > index 9076c874e98..be9fea8ebc9 100644 > --- a/plugin/type_uuid/sql_type_uuid.h > +++ b/plugin/type_uuid/sql_type_uuid.h > @@ -24,8 +24,162 @@ class UUID: public FixedBinTypeStorage<MY_UUID_SIZE, MY_UUID_STRING_LENGTH> ... > + // Compare two in-memory values > + static int cmp(const LEX_CSTRING &a, const LEX_CSTRING &b) > + { this is way more LoC that I would've used. but ok, it's your code and it's a plugin, so as you like > diff --git a/sql/sql_string.h b/sql/sql_string.h > index fe57c8153bb..795f80c3e08 100644 > --- a/sql/sql_string.h > +++ b/sql/sql_string.h > @@ -484,6 +484,11 @@ class Binary_string: public Sql_alloc > if (str.Alloced_length) > Alloced_length= (uint32) (str.Alloced_length - offset); > } > + LEX_CSTRING to_lex_cstring() const > + { > + LEX_CSTRING tmp= {Ptr, str_length}; > + return tmp; > + } > inline LEX_CSTRING *get_value(LEX_CSTRING *res) may be you could remove get_value()? In a separate commit. the name is quite bad and the method makes rather little sense, especially if there's to_lex_cstring() now. > { > res->str= Ptr; > diff --git a/sql/sql_type_fixedbin.h b/sql/sql_type_fixedbin.h > index c6e3d20bcfa..5141cb9fad4 100644 > --- a/sql/sql_type_fixedbin.h > +++ b/sql/sql_type_fixedbin.h > @@ -154,18 +160,13 @@ class FixedBinTypeBundle > FbtImpl::max_char_length()+1)); > return false; > } > - int cmp(const char *str, size_t length) const > - { > - DBUG_ASSERT(length == sizeof(m_buffer)); > - return memcmp(m_buffer, str, length); > - } > int cmp(const Binary_string &other) const what is this one used for? > { > - return cmp(other.ptr(), other.length()); > + return FbtImpl::cmp(FbtImpl::to_lex_cstring(), other.to_lex_cstring()); > } > int cmp(const Fbt &other) const > { > - return memcmp(m_buffer, other.m_buffer, sizeof(m_buffer)); > + return FbtImpl::cmp(FbtImpl::to_lex_cstring(), other.to_lex_cstring()); > } > }; > > diff --git a/sql/sql_type_fixedbin_storage.h b/sql/sql_type_fixedbin_storage.h > index 0e8ac81ab59..6e18335bd4c 100644 > --- a/sql/sql_type_fixedbin_storage.h > +++ b/sql/sql_type_fixedbin_storage.h > @@ -21,6 +21,38 @@ > format and their own (variable size) canonical string representation. > > Examples are INET6 and UUID types. > + > + The MariaDB server uses three binary representations of a data type: > + > + 1. In-memory binary representation (user visible) > + This representation: > + - can be used in INSERT..VALUES (X'AABBCC') > + - can be used in WHERE conditions: WHERE c1=X'AABBCC' > + - is returned by CAST(x AS BINARY(N)) > + - is returned by Field::val_native() and Item::val_native() > + > + 2. In-record binary representation (user invisible) > + This representation: > + - is used in records (is pointed by Field::ptr) > + - must be comparable by memcmp() > + > + 3. Binlog binary (row) representation > + Usually, for string data types the binlog representation > + is based on the in-record representation with trailing byte compression: > + - trailing space compression for text string data types > + - trailing zero compression for binary string data types > + > + We have to have separate in-memory and in-record representations > + because we use HA_KEYTYPE_BINARY for indexing. The engine API > + does not have a way to pass a comparison function as a parameter. > + > + The default implementation below assumes that: > + - the in-memory and in-record representations are equal > + - the binlog representation is compatible with BINARY(N) > + This is OK for simple data types, like INET6. > + > + Data type implementations that need different representations > + can override the default implementation (like e.g. UUID does). > */ thanks, this is very good > > /***********************************************************************/ > @@ -64,5 +137,37 @@ class FixedBinTypeStorage > return true; > } > > + static ulong KEY_pack_flags(uint column_nr) > + { > + /* > + Return zero by default. A particular data type can override > + this method return some flags, e.g. HA_PACK_KEY to enable > + key prefix compression. what if you change it later, will it make old tables corrupted? > + */ > + return 0; > + } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3