October 2021 - developers - lists.mariadb.org

Re: [Maria-developers] b3844107287: MDEV-16546 System versioning setting to allow history modification
by Sergei Golubchik 24 Aug '22

24 Aug '22

Hi, Aleksey! On Jun 08, Aleksey Midenkov wrote: > revision-id: b3844107287 (mariadb-10.5.2-424-gb3844107287) > parent(s): 27d66d644cf > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2021-03-01 17:43:34 +0300 > message: > > MDEV-16546 System versioning setting to allow history modification > > 1. force_fields_visible session variable makes system-invisible and > user-invisible fields visible on next table open. FLUSH TABLES > required for already opened tables. * system_versioning_insert_history * allows pseudocolumns ROW_START and ROW_END be specified in INSERT * doesn't affect invisible columns * FLUSH TABLES requirememnt is rather confusing, but let's have it, at least until I could suggest something better > 2. If secure_timestamp allows to modify timestamp variable then > following DML commands: INSERT, INSERT..SELECT and LOAD DATA can > specify row_start and row_end system field values. when system_versioning_insert_history=1 > 3. Cleaned up select_insert::send_data() from setting vers_write as > this parameter is now set on TABLE initialization. I don't see where it's set. This commit removes two table->vers_write= table->versioned(); and adds one from_field->table->vers_write= false; in Item_field::fix_fields. But where is vers_write set to true? Was it done in some other commit? > diff --git a/mysql-test/suite/versioning/r/insert.result b/mysql-test/suite/versioning/r/insert.result > index 2645d0184e8..00d1bb705c4 100644 > --- a/mysql-test/suite/versioning/r/insert.result > +++ b/mysql-test/suite/versioning/r/insert.result > @@ -112,3 +112,95 @@ x y > 9 9001 > drop table t1; > drop table t2; > +# > +# MDEV-16546 System versioning setting to allow history modification > +# > +# restart: --secure-timestamp=NO > +set @@session.time_zone='+00:00'; > +create table t1(x int) with system versioning; > +insert into t1(x) values (1); > +insert into t1(x, row_start, row_end) values (2, '1980-01-01 00:00:00', '1980-01-01 00:00:01'); > +ERROR 42S22: Unknown column 'row_start' in 'field list' > +set session force_fields_visible= 1; > +flush tables; > +show create table t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `x` int(11) DEFAULT NULL, > + `row_start` timestamp(6) GENERATED ALWAYS AS ROW START, > + `row_end` timestamp(6) GENERATED ALWAYS AS ROW END, shouldn't be visible here, only in insert and load. > + PERIOD FOR SYSTEM_TIME (`row_start`, `row_end`) > +) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > +insert into t1(x, row_start, row_end) values (3, '1980-01-01 00:00:00', '1980-01-01 00:00:01'); please, add tests for insert into t1 values (4) this should work, row_start/row_end must be mentioned explicitly. Also insert into t1 set x=5, row_start=..., row_end=... this should work too. And when only one row_start or row_end is mentioned as well. But update t1 set row_start=... insert into t1 (x, row_start, row_end) values (...) on duplicate key update row_start=... this should fail. While replace load data ... replace should work, but as DELETE+INSERT (I mean that DELETE in versioned tables doesn't actually delete history). If that's too difficult, then an error too. > +update t1 set x= x + 1; > +select x, check_row_ts(row_start, row_end) from t1 for system_time all order by x; ... > diff --git a/mysql-test/suite/versioning/t/insert.opt b/mysql-test/suite/versioning/t/insert.opt > new file mode 100644 > index 00000000000..a74d68957ef > --- /dev/null > +++ b/mysql-test/suite/versioning/t/insert.opt > @@ -0,0 +1 @@ > +--secure-timestamp=yes why is this? > -- source suite/versioning/common_finish.inc > diff --git a/mysql-test/suite/versioning/t/insert2.opt b/mysql-test/suite/versioning/t/insert2.opt > new file mode 100644 > index 00000000000..a74d68957ef > --- /dev/null > +++ b/mysql-test/suite/versioning/t/insert2.opt > @@ -0,0 +1 @@ > +--secure-timestamp=yes and this? > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index d23b190b2c5..93f8a8434e4 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -7604,6 +7604,20 @@ void THD::set_last_commit_gtid(rpl_gtid &gtid) > #endif > } > > +bool THD::vers_modify_sys_field() const don't call it vers_modify_sys_field, your previous name vers_modify_history was better. There may be other sys fields that will never be directly writable (like ROWID, for example). > +{ > + if (lex->sql_command != SQLCOM_INSERT && > + lex->sql_command != SQLCOM_INSERT_SELECT && > + lex->sql_command != SQLCOM_LOAD) > + return false; I don't think this is enough to reject insert ... on duplicate key update row_start=xxx > + if (opt_secure_timestamp >= SECTIME_REPL || this is fixed in a later commit, so ok > + (opt_secure_timestamp == SECTIME_SUPER && > + !(security_ctx->master_access & SUPER_ACL))) > + return false; > + return true; > +} > + > + > void > wait_for_commit::reinit() > { > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 2913a4a8350..520ea1d63bf 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -820,6 +820,7 @@ typedef struct system_variables > > vers_asof_timestamp_t vers_asof_timestamp; > ulong vers_alter_history; > + my_bool force_fields_visible; system_versioning_insert_history > } SV; > > /** > @@ -5400,6 +5401,7 @@ class THD: public THD_count, /* this must be first */ > Item *sp_prepare_func_item(Item **it_addr, uint cols= 1); > bool sp_eval_expr(Field *result_field, Item **expr_item_ptr); > > + bool vers_modify_sys_field() const; vers_modify_history > }; > > > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index f0b84ab126f..84c2ae4ab36 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -2014,7 +2014,7 @@ int write_record(THD *thd, TABLE *table, COPY_INFO *info, select_result *sink) > !table->file->referenced_by_foreign_key() && > (!table->triggers || !table->triggers->has_delete_triggers())) > { > - if (table->versioned(VERS_TRX_ID)) > + if (table->versioned(VERS_TRX_ID) && table->vers_write) does it even make sense? I mean direct insertion of transaction ids. > { > bitmap_set_bit(table->write_set, table->vers_start_field()->field_index); > table->file->column_bitmaps_signal(); > @@ -2029,7 +2029,7 @@ int write_record(THD *thd, TABLE *table, COPY_INFO *info, select_result *sink) > info->deleted++; > if (!table->file->has_transactions()) > thd->transaction->stmt.modified_non_trans_table= TRUE; > - if (table->versioned(VERS_TIMESTAMP)) > + if (table->versioned(VERS_TIMESTAMP) && table->vers_write) This (and the previous hunk) are in the DUP_REPLACE branch. There should be tests for these changes. > { > store_record(table, record[2]); > error= vers_insert_history_row(table); > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index f817f9086a9..4429308a097 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -6737,3 +6737,7 @@ static Sys_var_ulonglong Sys_max_rowid_filter_size( > SESSION_VAR(max_rowid_filter_size), CMD_LINE(REQUIRED_ARG), > VALID_RANGE(1024, (ulonglong)~(intptr)0), DEFAULT(128*1024), > BLOCK_SIZE(1)); > + > +static Sys_var_mybool Sys_force_fields_visible( > + "force_fields_visible", "Make invisible fields visible on next table open", "system_versioning_insert_history" "Allows direct inserts into ROW_START and ROW_END columns" > + SESSION_VAR(force_fields_visible), CMD_LINE(OPT_ARG), DEFAULT(FALSE)); > diff --git a/sql/table.cc b/sql/table.cc > index f4bdbdeac5a..520f3738c9e 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -4024,6 +4024,9 @@ enum open_frm_error open_table_from_share(THD *thd, TABLE_SHARE *share, > { > if (!((*field_ptr)= share->field[i]->clone(&outparam->mem_root, outparam))) > goto err; > + if (thd->variables.force_fields_visible && > + (*field_ptr)->invisible <= INVISIBLE_SYSTEM) > + (*field_ptr)->invisible= VISIBLE; Not really, system_versioning_insert_history should not affect visibility of anything besides ROW_START and ROW_END pseudocolumns in the INSERT and LOAD DATA statements. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 5

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] 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 30 Oct '21

30 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 24 Oct '21

24 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

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

21 Oct '21

Hi, Seppo, Jan! Note, this is 10.2 patch below. > commit 4b164f176e6 > Author: Seppo Jaakola <seppo.jaakola(a)galeracluster.com> > Date: Wed Sep 15 09:16:44 2021 +0300 > > MDEV-25114 Crash: WSREP: invalid state ROLLED_BACK (FATAL) I think this should say MDEV-23328 Server hang due to Galera lock conflict resolution it'd be easier to understand the commit comment and code changes if it'll refer to the correct problem it is fixing. The fix for MDEV-25114 was the previous commit, the one that This reverts commit 29bbcac0ee841faaa68eeb09c86ff825eabbe6b6 it should have MDEV-25114 in the commit comment. > > This patch is the plan D variant for fixing potetial mutex locking "potential" > order exercised by BF aborting and KILL command execution. > > In this approach, KILL command is replicated as TOI operation. > This guarantees total isolation for the KILL command execution > in the first node: there is no concurrent replication applying > and no concurrent DDL executing. Therefore there is no risk of > BF aborting to happen in parallel with KILL command execution > either. Potential mutex deadlocks between the different mutex > access paths with KILL command execution and BF aborting cannot > therefore happen. > > TOI replication is used, in this approach, purely as means > to provide isolated KILL command execution in the first node. > KILL command should not (and must not) be applied in secondary > nodes. In this patch, we make this sure by skipping KILL > execution in secondary nodes, in applying phase, where we > bail out if applier thread is trying to execute KILL command. > This is effective, but skipping the applying of KILL command > could happen much earlier as well. I like the idea of using TOI apparatus to provide total isolation for KILL. Actually replicating it to nodes is kind of crude, it'd be more elegant not to replicate at all. But it'll do. > > This patch also fixes mutex locking order and unprotected > THD member accesses on bf aborting case. We try to hold > THD::LOCK_thd_data during bf aborting. Only case where it > is not possible is at wsrep_abort_transaction before > call wsrep_innobase_kill_one_trx where we take InnoDB > mutexes first and then THD::LOCK_thd_data. > > This will also fix possible race condition during > close_connection and while wsrep is disconnecting > connections. > > Added wsrep_bf_kill_debug test case > > Reviewed-by: Jan Lindström <jan.lindstrom(a)mariadb.com> > > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 5ada018e540..05ec9a1c369 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -1804,11 +1804,11 @@ void THD::awake(killed_state state_to_set) > the Vio might be disassociated concurrently. > */ > > -void THD::disconnect() > +void THD::disconnect_mutexed() should be disconnect_no_mutex, cf. awake_no_mutex, set_killed_no_mutex. > { > Vio *vio= NULL; > > - mysql_mutex_lock(&LOCK_thd_data); > + mysql_mutex_assert_owner(&LOCK_thd_data); > > set_killed(KILL_CONNECTION); > > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 3e1f248b082..2bec9c6b6cd 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -9069,6 +9069,18 @@ static > void sql_kill(THD *thd, longlong id, killed_state state, killed_type type) > { > uint error; > +#ifdef WITH_WSREP > + if (WSREP(thd)) > + { > + WSREP_DEBUG("sql_kill called"); > + if (thd->wsrep_applier) > + { > + WSREP_DEBUG("KILL in applying, bailing out here"); > + return; > + } > + WSREP_TO_ISOLATION_BEGIN(WSREP_MYSQL_DB, NULL, NULL) when can it fail and jump to wsrep_error_label? under what conditions? > + } > +#endif /* WITH_WSREP */ > if (!(error= kill_one_thread(thd, id, state, type))) > { > if (!thd->killed) > diff --git a/sql/wsrep_mysqld.cc b/sql/wsrep_mysqld.cc > index e60100e2e90..00a6dfe2f8a 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) > +{ > + 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. */ How do you know victim_thd wasn't deleted before you locked LOCK_thd_data below? > + mysql_mutex_lock(&victim_thd->LOCK_thd_data); > + int res= wsrep_abort_thd(bf_thd_ptr, victim_thd_ptr, signal); > + mysql_mutex_unlock(&victim_thd->LOCK_thd_data); > + return res; > +} > > diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc > index 1f666f19d8a..921b8282e45 100644 > --- a/storage/innobase/handler/ha_innodb.cc > +++ b/storage/innobase/handler/ha_innodb.cc > @@ -19739,27 +19739,65 @@ wsrep_abort_transaction( > { > DBUG_ENTER("wsrep_abort_transaction"); > > + ut_ad(bf_thd); > + ut_ad(victim_thd); > trx_t* victim_trx= thd_to_trx(victim_thd); > - trx_t* bf_trx = (bf_thd) ? thd_to_trx(bf_thd) : NULL; > + trx_t* bf_trx= thd_to_trx(bf_thd); > > > if (victim_trx) { > + wsrep_thd_UNLOCK(victim_thd); what keeps victim_trx from disappearing here? > lock_mutex_enter(); > trx_mutex_enter(victim_trx); > wsrep_innobase_kill_one_trx(bf_thd, bf_trx, victim_trx, signal); > lock_mutex_exit(); > trx_mutex_exit(victim_trx); > wsrep_srv_conc_cancel_wait(victim_trx); > + wsrep_thd_LOCK(victim_thd); > DBUG_VOID_RETURN; > } else { > - wsrep_thd_LOCK(victim_thd); > wsrep_thd_set_conflict_state(victim_thd, MUST_ABORT); > - wsrep_thd_UNLOCK(victim_thd); > wsrep_thd_awake(victim_thd, signal); > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 20

Re: [Maria-developers] 688ad5ed67f: MDEV-26830: Wrong ROW_NUMBER in diagnostics upon INSERT IGNORE with CHECK violation
by Sergei Golubchik 18 Oct '21

18 Oct '21

Hi, Rucha! On Oct 18, Rucha Deodhar wrote: > revision-id: 688ad5ed67f (mariadb-10.6.1-138-g688ad5ed67f) > parent(s): c27f04ede5a > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-10-18 13:40:56 +0530 > message: > > MDEV-26830: Wrong ROW_NUMBER in diagnostics upon INSERT IGNORE with > CHECK violation > > Analysis: When there is constraint fail we return non-zero value for > view_check_option(). So we continue the loop which doesn't increment the > counter because it increments at the end of the loop. > Fix: Increment m_current_row_for_warning() at the beginning of loop. This > will also fix similar bugs if any, about counter not incrementing > correctly because of continue. > > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index 3dff6722a3d..11289779f7c 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -1007,8 +1007,10 @@ bool mysql_insert(THD *thd, TABLE_LIST *table_list, > goto values_loop_end; > } > > + thd->get_stmt_da()->reset_current_row_for_warning(0); This changes the behavior in array binding case, let's not do it in this commit. Better change the previous reset_current_row_for_warning() to reset to 0, not to 1. The rest is ok. > while ((values= its++)) > { > + thd->get_stmt_da()->inc_current_row_for_warning(); > if (fields.elements || !value_count) > { > /* > @@ -1125,7 +1127,6 @@ bool mysql_insert(THD *thd, TABLE_LIST *table_list, > if (unlikely(error)) > break; > info.accepted_rows++; > - thd->get_stmt_da()->inc_current_row_for_warning(); > } > its.rewind(); > iteration++; > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] a895fce3277: MDEV-26842: MDEV-26842: ROW_NUMBER is not set and differs from the message upon WARN_DATA_TRUNCATED produced by inplace ALTER
by Sergei Golubchik 17 Oct '21

17 Oct '21

Hi, Rucha! On Oct 17, Rucha Deodhar wrote: > revision-id: a895fce3277 (mariadb-10.6.1-138-ga895fce3277) > parent(s): c27f04ede5a > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-10-17 17:59:11 +0530 > message: > > MDEV-26842: MDEV-26842: ROW_NUMBER is not set and differs from the message > upon WARN_DATA_TRUNCATED produced by inplace ALTER > > Analysis: When row number is passed as parameter to set_warning() it is only > used for error/warning text but m_current_row_for_warning is not updated. > Hence default value of m_current_row_for_warning is assumed. > Fix: update m_current_row_for_warning when error/warning occurs. > > diff --git a/sql/field.cc b/sql/field.cc > index a5cd595f05b..048798ea509 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -11086,6 +11086,10 @@ Field::set_warning(Sql_condition::enum_warning_level level, uint code, > will have table == NULL. > */ > THD *thd= get_thd(); > + ulong row_count= current_row ? current_row > + : thd->get_stmt_da()->current_row_for_warning(); > + > + thd->get_stmt_da()->reset_current_row_for_warning(row_count); Isn't it a bit confusing, why do you set current_row_for_warning to its own actual value? May be it'd be clearer to if (current_row) thd->get_stmt_da()->reset_current_row_for_warning(current_row); And also, please, mention either here or in the commit comment that it's for INPLACE alter, where the server cannot know what row has generated the warning, so the value of current_row is supplied by the engine. > if (thd->count_cuted_fields > CHECK_FIELD_EXPRESSION) > { > thd->cuted_fields+= cut_increment; > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 351a0a7d6b7: MDEV-26841: ROW_NUMBER is not set and differs from the message upon ER_WRONG_VALUE_COUNT_ON_ROW for the 1st row
by Sergei Golubchik 17 Oct '21

17 Oct '21

Hi, Rucha! On Oct 17, Rucha Deodhar wrote: > revision-id: 351a0a7d6b7 (mariadb-10.6.1-138-g351a0a7d6b7) > parent(s): c27f04ede5a > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-10-17 16:24:47 +0530 > message: > > MDEV-26841: ROW_NUMBER is not set and differs from the message upon > ER_WRONG_VALUE_COUNT_ON_ROW for the 1st row > > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index 3dff6722a3d..9e81c33ff8f 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -229,6 +229,8 @@ static int check_insert_fields(THD *thd, TABLE_LIST *table_list, > } > if (values.elements != table->s->visible_fields) > { > + if (values.elements > table->s->visible_fields) why do you need an if() here? > + thd->get_stmt_da()->inc_current_row_for_warning(); > my_error(ER_WRONG_VALUE_COUNT_ON_ROW, MYF(0), 1L); > DBUG_RETURN(-1); > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] fcf2c5fff14: MDEV-26299: Some views force server (and mysqldump) to generate invalid SQL for their definitions
by Sergei Golubchik 16 Oct '21

16 Oct '21

Hi, Oleksandr! On Oct 16, Oleksandr Byelkin wrote: > revision-id: fcf2c5fff14 (mariadb-10.2.40-78-gfcf2c5fff14) > parent(s): d5a15f04f4a > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2021-10-01 14:46:22 +0200 > message: > > MDEV-26299: Some views force server (and mysqldump) to generate invalid SQL for their definitions > > Do not print illegal table field names for non-top-level SELECT list, > they will not be refered in any case but create problem for parsing > of printed result. > > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index ff584e936b7..ebee9c85791 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -25797,6 +25797,11 @@ void st_select_lex::print(THD *thd, String *str, enum_query_type query_type) > > //Item List > bool first= 1; > + /* > + outer_select() can not be used here because it is for name resolution > + and will return NULL at any end of name resolution chain (view/derived) > + */ > + bool top_level= (get_master()->get_master() == 0); > List_iterator_fast<Item> it(item_list); > Item *item; > while ((item= it++)) > @@ -25806,7 +25811,8 @@ void st_select_lex::print(THD *thd, String *str, enum_query_type query_type) > else > str->append(','); > > - if (is_subquery_function() && item->is_autogenerated_name) > + if ((is_subquery_function() && item->is_autogenerated_name) || > + !item->name) Where can item->name be NULL? I thought the either specified by the user or autogenerated, but never NULL. > { > /* > Do not print auto-generated aliases in subqueries. It has no purpose > @@ -25815,7 +25821,26 @@ void st_select_lex::print(THD *thd, String *str, enum_query_type query_type) > item->print(str, query_type); > } > else > - item->print_item_w_name(str, query_type); > + { > + /* > + Despite presence of item->name_length it is incorrect in many cases > + till we start using LEX_STRING for the name. > + */ > + size_t len= strlen(item->name); > + > + /* > + Do not print illegal names (if it is not top level SELECT). > + Top level view checked (and correct name are assigned), > + other cases of top level SELECT are not important, because > + it is not "table field". > + */ > + if (top_level || > + !item->is_autogenerated_name || > + !check_table_name(item->name, len, TRUE)) why check_table_name() if it isn't a table? it's an incorrect check to use for columns. > + item->print_item_w_name(str, query_type); > + else > + item->print(str, query_type); > + } > } > > /* > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

Re: [Maria-developers] baaaa971662: MDEV-22883: Assertion `!parse_error || lex.sphead == 0' failed in Table_triggers_list::check_n_load | SIGSEGV in strxmov
by Sergei Golubchik 15 Oct '21

15 Oct '21

Hi, Oleksandr! On Oct 15, Oleksandr Byelkin wrote: > revision-id: baaaa971662 (mariadb-10.2.40-61-gbaaaa971662) > parent(s): dce490e9d4d > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2021-09-19 19:23:49 +0200 > message: > > MDEV-22883: Assertion `!parse_error || lex.sphead == 0' failed in Table_triggers_list::check_n_load | SIGSEGV in strxmov > > Abort execution of SHOW in case of fatal error. Does this patch fixes all test cases from the bug report? There are quite a few. > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index 721bb053343..105b2719882 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -4849,7 +4849,8 @@ static int fill_schema_table_from_frm(THD *thd, TABLE *table, > */ > DBUG_ASSERT(thd->open_tables == NULL); > thd->mdl_context.rollback_to_savepoint(open_tables_state_backup->mdl_system_tables_svp); > - thd->clear_error(); > + if (!thd->is_fatal_error) > + thd->clear_error(); Hmm. THD::clear_error() starts with a comment: /** Clear the current error, if any. We do not clear is_fatal_error or is_fatal_sub_stmt_error since we assume this is never called if the fatal error is set. Unfortunately, it doesn't have an assert to go with this, otherwise all these bugs would've probably failed identically and early. Please add DBUG_ASSERT(!is_fatal_error); DBUG_ASSERT(!is_fatal_sub_stmt_error); or something like that to THD::clear_error() > return res; > } > > @@ -5064,7 +5065,7 @@ int get_all_tables(THD *thd, TABLE_LIST *tables, COND *cond) > continue; > } > > - if (thd->killed == ABORT_QUERY) > + if (thd->killed == ABORT_QUERY || thd->is_fatal_error) > { > error= 0; > goto err; that's kind of weird. Why does it continue executing if the statement is killed? Perhaps it'd better be if (thd->killed || thd->is_fatal_error) { if (thd->killed == ABORT_QUERY) error= 0; goto err; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 2f812e800c0: MDEV-22447: SIGSEGV in Item::val_uint on EXPLAIN
by Sergei Golubchik 15 Oct '21

15 Oct '21

Hi, Oleksandr! On Oct 15, Oleksandr Byelkin wrote: > revision-id: 2f812e800c0 (mariadb-10.2.40-57-g2f812e800c0) > parent(s): 696de6d06c0 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2021-09-16 14:12:48 +0200 > message: > > MDEV-22447: SIGSEGV in Item::val_uint on EXPLAIN > > take into account possible NULL > > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index ff584e936b7..d1b738e94a0 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -389,7 +389,7 @@ bool handle_select(THD *thd, LEX *lex, select_result *result, > ER_QUERY_EXCEEDED_ROWS_EXAMINED_LIMIT, > ER_THD(thd, ER_QUERY_EXCEEDED_ROWS_EXAMINED_LIMIT), > thd->accessed_rows_and_keys, > - thd->lex->limit_rows_examined->val_uint()); > + thd->lex->get_limit_rows_examined()); > thd->abort_on_warning= saved_abort_on_warning; > thd->reset_killed(); > } How comes, it gets into ER_QUERY_EXCEEDED_ROWS_EXAMINED_LIMIT if limit_rows_examined is NULL? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] cd4cac2b95a: MDEV-17964: Assertion `status == 0' failed in add_role_user_mapping_action
by Sergei Golubchik 15 Oct '21

15 Oct '21

Hi, Vicențiu! The patch is ok, please push. But given the time it took us to understand the issue, I'm afraid that simply stating that "HASH implementation shuffles elements around" is not enough. Better to explain how elements are shuffled around. For example: in this case the hash has 131 element, DROP ROLE removes the element [119]. Its [119]->next was element [129], so [129] is moved to [119]. Now we need to compact the hash, removing the last element [130]. It gets one bit off its hash value and becomes element [2]. The existing element [2] is moved to [129], and old [130] is moved to [2]. We cannot simply move [130] to [129] and make [2]->next=130, it won't work if [2] is itself in the collision list and doesn't belong in [2]. On Oct 15, Vicențiu Ciorbaru wrote: > revision-id: cd4cac2b95a (mariadb-10.2.40-91-gcd4cac2b95a) > parent(s): 2bb8d7c2f36 > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2021-10-13 13:28:50 +0300 > message: > > MDEV-17964: Assertion `status == 0' failed in add_role_user_mapping_action > > This happens upon CREATE USER and DROP ROLE. > > The underlying problem is that our HASH implementation shuffles > elements around when performing an update or delete. This means that > when doing a scan through the HASH table by index, in search of > elements to delete or update one must restart the scan to make sure > nothing is missed if at least one delete / update happened. > > The handle_grant_struct code assumed that it is safe to continue by > only reexamining the currently modified / deleted element index, but > that is not true. > > Missing to delete an element in the hash triggered the assertion in > the test case. DROP ROLE would not clear all necessary role->role or > role->user mappings. > > To fix the problem we ensure that the scan is restarted, only if an > element was deleted / updated, similar to how bubble-sort keeps > sorting until it finds no more elements to swap. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 97458a7767b: MDEV-26832: ROW_NUMBER in SIGNAL/RESIGNAL causes a syntax error
by Sergei Golubchik 15 Oct '21

15 Oct '21

Hi, Rucha! Looks quite good. A couple of small comments, see below. After you fix them, no need to ask for a new review, just push into bb-10.7-row_number On Oct 15, Rucha Deodhar wrote: > revision-id: 97458a7767b (mariadb-10.6.1-131-g97458a7767b) > parent(s): a6cf8b34a83 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-10-15 01:54:51 +0530 > message: > > MDEV-26832: ROW_NUMBER in SIGNAL/RESIGNAL causes a syntax error > > Analysis: Parser was missing ROW_NUMBER as syntax for SIGNAL and RESIGNAL. > Fix: Fix parser and fix how m_row_number is copied like other attributes > to avoid ROW_NUMBER from assuming default value. > > diff --git a/mysql-test/main/get_diagnostics.test b/mysql-test/main/get_diagnostics.test > index 83ea1dee343..186d1bfc438 100644 > --- a/mysql-test/main/get_diagnostics.test > +++ b/mysql-test/main/get_diagnostics.test > @@ -1552,3 +1552,53 @@ GET DIAGNOSTICS CONDITION 2 @rnum = ROW_NUMBER, @msg = MESSAGE_TEXT, @err = MYSQ ... > + > +CREATE PROCEDURE resignal_syntax() > +BEGIN > + DECLARE CONTINUE HANDLER > + FOR 1146 > + BEGIN > + RESIGNAL SET > + MESSAGE_TEXT = '`temptab` does not exist', ROW_NUMBER= 105; > + END; > + SELECT `c` FROM `temptab`; > +END| > + > +DELIMITER ;| > + > +--error ER_NO_SUCH_TABLE > +CALL resignal_syntax(); You test that the above isn't a syntax error. But you also need to verify that row_number was, indeed, set. Like, do GET DIAGNOSTICS and print the ROW_NUMBER. > + > +DROP PROCEDURE resignal_syntax; > diff --git a/sql/sql_error.cc b/sql/sql_error.cc > index a2178831697..84065ddce15 100644 > --- a/sql/sql_error.cc > +++ b/sql/sql_error.cc > @@ -190,6 +190,11 @@ static void copy_string(MEM_ROOT *mem_root, String* dst, const String* src) > dst->length(0); > } > > +void copy_value(ulong *dst, const ulong *src) > +{ > + memcpy(dst, src, sizeof(*src)); > +} > + > void > Sql_condition::copy_opt_attributes(const Sql_condition *cond) > { > @@ -204,6 +209,7 @@ Sql_condition::copy_opt_attributes(const Sql_condition *cond) > copy_string(m_mem_root, & m_table_name, & cond->m_table_name); > copy_string(m_mem_root, & m_column_name, & cond->m_column_name); > copy_string(m_mem_root, & m_cursor_name, & cond->m_cursor_name); > + copy_value(& m_row_number, & cond->m_row_number); That makes no sense. Other values were strings and had to be strdup-ed, but that's a number, you can simply do m_row_number= cond->m_row_number; > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 6ec64caec5d: MDEV-26650: Failed ALTER USER/GRANT statement removes the password from the cache
by Sergei Golubchik 15 Oct '21

15 Oct '21

Hi, Oleksandr! All good, ok to push. Just add a comment right after memcpy, that /* if acl_user->nauth was >= nauth, then we've just updated the actual acl_user, not the copy. We rely on the fact that acl_update_user() cannot fail anywhere below. */ On Oct 15, Oleksandr Byelkin wrote: > revision-id: 6ec64caec5d (mariadb-10.4.21-64-g6ec64caec5d) > parent(s): a736a3174a4 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2021-10-14 16:19:09 +0200 > message: > > MDEV-26650: Failed ALTER USER/GRANT statement removes the password from the cache > > Starting from 10.4 AUTH is not part of ACL_USER so changes have to be done > over a copy, and bring in the cache only in case of success. > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] MDEV-26742 Assertion `field->type_handler() == this' failed...
by Sergei Golubchik 12 Oct '21

12 Oct '21

Hi, Alexander! See comments below > diff --git a/plugin/type_inet/mysql-test/type_inet/type_inet6_innodb.test b/plugin/type_inet/mysql-test/type_inet/type_inet6_innodb.test > index dd6049abbf3..3f3a6fdac2c 100644 > --- a/plugin/type_inet/mysql-test/type_inet/type_inet6_innodb.test > +++ b/plugin/type_inet/mysql-test/type_inet/type_inet6_innodb.test > @@ -12,6 +12,30 @@ > SET default_storage_engine=InnoDB; > --source type_inet6_engines.inc > > +--echo # > +--echo # MDEV-26742 Assertion `field->type_handler() == this' failed in FixedBinTypeBundle<NATIVE_LEN, MAX_CHAR_LEN>::Type_handler_fbt::stored_field_cmp_to_item > +--echo # > + > +CREATE TABLE t1 (pk inet6, c text) engine=myisam; > +INSERT INTO t1 VALUES ('::',1); > +CREATE TABLE t2 (d text, KEY (d)) engine=innodb ; > +INSERT INTO t2 VALUES (2); > +--error ER_TRUNCATED_WRONG_VALUE > +UPDATE t2 JOIN t1 ON ( t1.pk > t2.d) SET t1.c = 1; > +SET sql_mode=''; > +UPDATE t2 JOIN t1 ON ( t1.pk > t2.d) SET t1.c = 1; > +SET sql_mode=DEFAULT; > +SELECT * FROM t1; > +SELECT * FROM t2; > +DROP TABLE t1, t2; > + > +CREATE TABLE t1 (pk inet6, c text) engine=myisam; > +INSERT INTO t1 VALUES ('::',1); > +CREATE TABLE t2 ( d text, KEY (d)) engine=innodb ; I don't get it, why do you recreate tables with exactly the same definition and exactly the same content? > +INSERT INTO t2 VALUES (2); > +SELECT * FROM t2 JOIN t1 ON ( t1.pk > t2.d); > +DROP TABLE t1, t2; > + > > --echo # > --echo # End of 10.5 tests > diff --git a/plugin/type_inet/mysql-test/type_inet/type_inet6_myisam.test b/plugin/type_inet/mysql-test/type_inet/type_inet6_myisam.test > index c5183f01cf0..1ccd654181c 100644 > --- a/plugin/type_inet/mysql-test/type_inet/type_inet6_myisam.test > +++ b/plugin/type_inet/mysql-test/type_inet/type_inet6_myisam.test > @@ -10,6 +10,21 @@ > SET default_storage_engine=MyISAM; > --source type_inet6_engines.inc > > +--echo # > +--echo # MDEV-26742 Assertion `field->type_handler() == this' failed in FixedBinTypeBundle<NATIVE_LEN, MAX_CHAR_LEN>::Type_handler_fbt::stored_field_cmp_to_item > +--echo # > + > +CREATE TABLE t1 (c varchar(64), key(c)) engine=myisam; > +INSERT INTO t1 VALUES ('::1'),('::2'); > +SELECT * FROM t1 WHERE c>CAST('::1' AS INET6); > +DROP TABLE t1; > + > +CREATE TABLE t1 (c varchar(64), key(c)) engine=myisam; > +INSERT INTO t1 VALUES ('::1'),('0::1'); also don't see much point in recreating > +SELECT * FROM t1 WHERE c=CAST('::1' AS INET6); > +EXPLAIN SELECT * FROM t1 WHERE c=CAST('::1' AS INET6); > +DROP TABLE t1; > + > > --echo # > --echo # End of 10.5 tests > diff --git a/plugin/type_uuid/mysql-test/type_uuid/type_uuid_innodb.test b/plugin/type_uuid/mysql-test/type_uuid/type_uuid_innodb.test > index 062e25d366b..27e3214af15 100644 > --- a/plugin/type_uuid/mysql-test/type_uuid/type_uuid_innodb.test > +++ b/plugin/type_uuid/mysql-test/type_uuid/type_uuid_innodb.test > @@ -12,5 +12,27 @@ SET default_storage_engine=InnoDB; > --source type_uuid_engines.inc > > --echo # > ---echo # End of 10.5 tests > +--echo # MDEV-26742 Assertion `field->type_handler() == this' failed in FixedBinTypeBundle<NATIVE_LEN, MAX_CHAR_LEN>::Type_handler_fbt::stored_field_cmp_to_item > +--echo # > + > +CREATE TABLE t1 ( pk uuid, c text) engine=myisam; > +INSERT INTO t1 VALUES ('00000000-0000-0000-0000-000000000000',1); > +CREATE TABLE t2 ( d text, KEY (d)) engine=innodb ; > +INSERT INTO t2 VALUES (2); > +--error ER_TRUNCATED_WRONG_VALUE > +UPDATE t2 JOIN t1 ON ( t1.pk > t2.d) SET t1.c = 1; > +SET sql_mode=''; > +UPDATE t2 JOIN t1 ON ( t1.pk > t2.d) SET t1.c = 1; > +SET sql_mode=DEFAULT; > +DROP TABLE t1, t2; > + > +CREATE TABLE t1 (pk uuid, c text) engine=myisam; > +INSERT INTO t1 VALUES ('00000000-0000-0000-0000-000000000000',1); > +CREATE TABLE t2 ( d text, KEY (d)) engine=innodb ; > +INSERT INTO t2 VALUES (2); same here > +SELECT * FROM t2 JOIN t1 ON ( t1.pk > t2.d); > +DROP TABLE t1, t2; > + > +--echo # > +--echo # End of 10.7 tests > --echo # > diff --git a/sql/field.cc b/sql/field.cc > index 46a3a1deea3..fc3456e2ccb 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -7368,7 +7368,9 @@ bool > Field_longstr::cmp_to_string_with_same_collation(const Item_bool_func *cond, > const Item *item) const > { > + Type_handler_hybrid_field_type cmp(type_handler_for_comparison()); Can this be anything else but type_handler_long_blob ? if not, then I'd suggest Type_handler_hybrid_field_type cmp(type_handler_long_blob); DBUG_ASSERT(type_handler_for_comparison() == &type_handler_long_blob); > + return !cmp.aggregate_for_comparison(item->type_handler_for_comparison()) && > + cmp.type_handler() == type_handler_for_comparison() && > - return item->cmp_type() == STRING_RESULT && > charset() == cond->compare_collation(); This, basically, means, that cmp_type() is now meaningless, one cannot use it anymore? There's no INET6_RESULT and UUID_RESULT for comparison, so what's the point of cmp_type() now? One more question. What type do we have where you need to aggregate here and simple item->type_handler_for_comparison() != &type_handler_long_blob won't do? > } > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 4

[Maria-developers] How can I separate networking test from mysql_test
by Lukas Javorsky 11 Oct '21

11 Oct '21

Hello there, I've open this topic on the maria-discuss mailing list, but, I didn't get any response. Maybe this mailing list could be a better fit for this request. > Hi guys, > I've found few particular tests from the test suite that is failing due to the OpenSSL-3 version and I want to separate them and give them to the OpenSSL team in Red Hat. > > The idea behind this is to give them a set of commands/config options (reproducer) for each test and they will try to find out what's the exact problem there and help us develop the patch. > > The tests that I'm interested in are (there are more of them, I just need to see how to reproduce few of them and I'll do the same for the other ones as well): > encryption.corrupted_during_recovery > encryption.innodb-checksum-algorithm > encryption.innodb-discard-import-change > encryption.innodb_page_encryption_key_change > binlog_encryption.rpl_loaddata_local > binlog_encryption.rpl_parallel_free_deferred_event > > IMPORTANT: These tests are failing only with the 'cbc' cipher in use, the 'ctr' is passing. > > Could you please help me with how to make a reproducer for each test, so I can create them and give them to the OpenSSL team? > > Thank you for any help you can provide > Lukas -- S pozdravom/ Best regards Lukáš Javorský Associate Software Engineer, Core service - Databases Red Hat <https://www.redhat.com> Purkyňova 115 (TPB-C) 612 00 Brno - Královo Pole ljavorsk(a)redhat.com <https://www.redhat.com>

1 0

Re: [Maria-developers] dcbdec60893: MDEV-26650: Failed ALTER USER/GRANT statement removes the password from the cache
by Sergei Golubchik 07 Oct '21

07 Oct '21

Hi, Oleksandr! Ok to push! A couple of typos below On Oct 07, Oleksandr Byelkin wrote: > revision-id: dcbdec60893 (mariadb-10.4.21-56-gdcbdec60893) > parent(s): a75813d4673 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2021-10-07 11:39:02 +0200 > message: > > MDEV-26650: Failed ALTER USER/GRANT statement removes the password from the cache > > Starting from 10.4 AUTH is not part of ACL_USER so have to be safely copied. > Patch made in line with preriouse code ideas. "previous" > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 0e2c044e3a1..cef51038540 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -188,6 +188,9 @@ class ACL_USER_PARAM > class ACL_USER :public ACL_USER_BASE, > public ACL_USER_PARAM > { > +protected: > + // default shellow copy constructor (protect from non-authorized usage) "shallow" > + ACL_USER(const ACL_USER &)= default; > public: Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0