- developers - lists.mariadb.org

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

20 Dec '21

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

1 0

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

13 Dec '21

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

2 2

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

11 Dec '21

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

2 3

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

10 Dec '21

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

1 0

Re: [Maria-developers] b95c5105e28: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
by Sergei Golubchik 07 Dec '21

07 Dec '21

Hi, Aleksey! This is a review of diff b95c5105e2 2552bce607 This was quite good, thanks! Just a couple of questions/comments, see below. On Nov 23, Aleksey Midenkov wrote: > diff --git a/mysql-test/suite/versioning/t/partition.test b/mysql-test/suite/versioning/t/partition.test > index 006a65e1a16..a5bb8426ae4 100644 > --- a/mysql-test/suite/versioning/t/partition.test > +++ b/mysql-test/suite/versioning/t/partition.test > @@ -1070,15 +1080,572 @@ alter table t1 add partition partitions 2; ... > +--let $datadir= `select @@datadir` > +--let $dummy= $datadir/test/t1#P#p1.ibd > +--write_file $dummy > +EOF > + > +set timestamp= unix_timestamp('2000-01-01 01:00:00'); > +--error ER_GET_ERRNO > +update t1 set x= x + 2; That's a good one! > +show warnings; > +--remove_file $dummy > + > diff --git a/sql/table.h b/sql/table.h > index 2e074abcea0..d861db261bf 100644 > --- a/sql/table.h > +++ b/sql/table.h > @@ -910,6 +911,11 @@ struct TABLE_SHARE > vers_kind_t versioned; > period_info_t vers; > period_info_t period; > + /* > + Protect multiple threads from repeating partition auto-create over > + single share. could you please add here (to the comment) TODO remove it when partitioning metadata will be in TABLE_SHARE > + */ > + bool vers_skip_auto_create; > > bool init_period_from_extra2(period_info_t *period, const uchar *data, > const uchar *end); > @@ -2557,6 +2567,11 @@ struct TABLE_LIST > bool merged; > bool merged_for_insert; > bool sequence; /* Part of NEXTVAL/CURVAL/LASTVAL */ > + /* > + Protect single thread from repeating partition auto-create over > + multiple share instances (as the share is closed on backoff action). > + */ > + bool vers_skip_create; I don't understand it. What does it mean "multiple share instances"? Can there be multiple TABLE_SHARE objects for the same table? How? > > /* > Items created by create_view_field and collected to change them in case > diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc > index d7bb02bbd4e..56d5a68efd0 100644 > --- a/sql/sql_partition.cc > +++ b/sql/sql_partition.cc > @@ -2587,11 +2587,15 @@ char *generate_partition_syntax(THD *thd, partition_info *part_info, > err+= str.append(ctime, ctime_len); > err+= str.append('\''); > } > + if (vers_info->auto_hist) > + err+= str.append(STRING_WITH_LEN(" AUTO")); > } > - if (vers_info->limit) > + else if (vers_info->limit) > { > err+= str.append(STRING_WITH_LEN("LIMIT ")); > err+= str.append_ulonglong(vers_info->limit); > + if (vers_info->auto_hist) > + err+= str.append(STRING_WITH_LEN(" AUTO")); Can you do this str.appent("AUTO") once, after the if()? Not in both branches? > } > } > else if (part_info->part_expr) > diff --git a/sql/partition_info.cc b/sql/partition_info.cc > index fd92e437cac..ddcc6f413ba 100644 > --- a/sql/partition_info.cc > +++ b/sql/partition_info.cc > @@ -865,9 +870,167 @@ void partition_info::vers_set_hist_part(THD *thd) > { > vers_info->hist_part= next; > if (next->range_value > thd->query_start()) > - return; > + { > + error= false; > + break; > + } > + } > + if (error) > + { > + if (auto_hist) > + { > + *create_count= 0; > + const my_time_t hist_end= (my_time_t) vers_info->hist_part->range_value; > + DBUG_ASSERT(thd->query_start() >= hist_end); > + MYSQL_TIME h0, q0; > + my_tz_OFFSET0->gmt_sec_to_TIME(&h0, hist_end); > + my_tz_OFFSET0->gmt_sec_to_TIME(&q0, thd->query_start()); > + longlong q= pack_time(&q0); > + longlong h= pack_time(&h0); > + while (h <= q) > + { > + if (date_add_interval(thd, &h0, vers_info->interval.type, > + vers_info->interval.step)) > + return true; > + h= pack_time(&h0); > + ++*create_count; > + if (*create_count == MAX_PARTITIONS - 2) > + { > + my_error(ER_TOO_MANY_PARTITIONS_ERROR, MYF(ME_WARNING)); > + my_error(ER_VERS_HIST_PART_FAILED, MYF(0), > + table->s->db.str, table->s->table_name.str); > + return true; > + } > + } > + } > + else > + { > + DBUG_ASSERT(!vers_info->auto_hist); ^^^ This assert fails for me in the update-big test > + my_error(WARN_VERS_PART_FULL, MYF(ME_WARNING|ME_ERROR_LOG), > + table->s->db.str, table->s->table_name.str, > + vers_info->hist_part->partition_name, "INTERVAL"); > + } > + } > + } > + > + return false; > +} > + > + > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index ac22a63bdba..459edb614f7 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -4203,6 +4444,15 @@ bool open_tables(THD *thd, const DDL_options_st &options, > } > > thd->current_tablenr= 0; > + > +#ifdef WITH_PARTITION_STORAGE_ENGINE > + if (!thd->stmt_arena->is_conventional()) > + { > + for (tables= *start; tables; tables= tables->next_global) > + tables->vers_skip_create= false; > + } This looks a bit like an overkill, you do it for every prepare, while auto-adding a partition is a rare event, as you said. May be you can do instead in TABLE::vers_switch_partition uint *create_count= !thd->stmt_arena->is_conventional() && table_list->vers_skip_create ? > +#endif > + > restart: > /* > Close HANDLER tables which are marked for flush or against which there Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

[Maria-developers] some questions for the community
by 杨凡 26 Nov '21

26 Nov '21

Hello, I am a student of Xi'an University of Posts and Telecommunications. My name is yangfan.I want to ask something about community code sharing and GSOC. I am now in my sophomore year, and I am very interested in databases, especially your marria database. I am now familiar with the c/c++ language and master the basic Linux system programming and network programming. Currently learning C++ multi-threaded programming. I can use basic SQL statements, databases, basic Linux commands, and some knowledge about operating systems and networks. I joined the xiyou linux group. Now, I want to learn quickly and contribute code to your community. Can you tell me what to do? And I want to participate in GSOC in 2022. If you can give me some help, I will be very happy. I only learned about open source culture from university. After a long period of study and thinking, I feel that open source is a very mea ningful thing. It not only helps personal growth but also helps a product iteration. The slogan of our group is free sharing and openness. At the same time, I continue to promote open source culture. I am fascinated by this culture. I also want to join a community and make my contribution to you. Although my current ability is not very strong, but I will continue to work hard. I am very interested in programming, and I want to focus on distributed and database work now and in the future. Maybe my English is not very good, I hope you can be more tolerant.

1 0

Re: [Maria-developers] a5fca9a6e30: MENT-651 [6/8] store cache managers in a list
by Sergei Golubchik 26 Nov '21

26 Nov '21

Hi, Nikita! On Nov 14, Nikita Malyavin wrote: > revision-id: a5fca9a6e30 (mariadb-10.5.2-478-ga5fca9a6e30) > parent(s): ad6e7b87107 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2021-01-27 17:28:05 +1000 > message: > > MENT-651 [6/8] store cache managers in a list again, a couple of lines with a description would be good here > diff --git a/mysql-test/suite/binlog/t/online_alter.test b/mysql-test/suite/binlog/t/online_alter.test > index 804e847d008..4ed2db74bd6 100644 > --- a/mysql-test/suite/binlog/t/online_alter.test > +++ b/mysql-test/suite/binlog/t/online_alter.test > @@ -218,7 +219,7 @@ set autocommit = 0; > start transaction; > update t1 set b= 7007 where a = 7; > --error ER_DUP_ENTRY > -update t1 set a= 8 where a = 8 or a = 9; > +update t1 set a= 8, b= 8008 where a = 8 or a = 9 order by a; why did you change tests for what is, as far as I understand, just an optimization? > commit; > set autocommit = 1; > > diff --git a/sql/handler.cc b/sql/handler.cc > index 8b95653e69f..8b22167e729 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -6660,7 +6660,16 @@ static int binlog_log_row_online_alter(TABLE* table, > THD *thd= table->in_use; > > if (!table->online_alter_cache) > - table->online_alter_cache= thd->binlog_setup_cache_data(); > + { > + auto *cache_mngr= online_alter_binlog_get_cache_mngr(thd, table); > + // Use transaction cache directly, if it is not multi-transaction mode > + table->online_alter_cache= binlog_get_cache_data(cache_mngr, > + !thd->in_multi_stmt_transaction_mode()); > + > + trans_register_ha(thd, false, binlog_hton, 0); > + if (thd->in_multi_stmt_transaction_mode()) > + trans_register_ha(thd, true, binlog_hton, 0); I still don't understand that. Why do you need a handlerton, why do you fake a transaction? And why do you need to search a list in THD, instead of storing cache_mngr in TABLE_SHARE? > + } > > // We need to log all columns for the case if alter table changes primary key. > table->use_all_columns(); > diff --git a/sql/log.cc b/sql/log.cc > index 6c678624230..c58096e6d9a 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -2258,14 +2298,18 @@ static int binlog_rollback(handlerton *hton, THD *thd, bool all) > { > DBUG_ENTER("binlog_rollback"); > > - for (TABLE *table= thd->open_tables; table; table= table->next) > - { > - if (!table->online_alter_cache) > - continue; > - table->online_alter_cache->reset(); > - delete table->online_alter_cache; > - table->online_alter_cache= NULL; > - } > + bool is_ending_trans= ending_trans(thd, all); > + > + /* > + This is a crucial moment that we are running through > + thd->online_alter_cache_list, and not through thd->open_tables to cleanup > + stmt cache, though both have it. The reason is that the tables can be closed > + to that moment in case of an error. > + The same reason applies to the fact we don't store cache_mngr in the table > + itself -- because it can happen to be not existing. > + Still in case if tables are left opened > + */ > + binlog_online_alter_cleanup(thd->online_alter_cache_list, is_ending_trans); still don't understand that comment either :( Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

[Maria-developers] Squash aaed3436 MENT-651 [4/8] ALTER ONLINE TABLE
by Nikita Malyavin 25 Nov '21

25 Nov '21

Sergei, I think all the commits starting from [4/8] ALTER ONLINE TABLE should be squashed. I will also add a better description in the commit message covering our latest discussions. I would also embed a reference to the original unsquashed branch. It seems to me as a good idea in sense of helping the reader investigate the code -- Yours truly, Nikita Malyavin

2 5

Re: [Maria-developers] 4af7a583802: Refactor MYSQL_BIN_LOG: extract Event_log ancestor
by Sergei Golubchik 24 Nov '21

24 Nov '21

Hi, Nikita! On Nov 12, Nikita Malyavin wrote: > revision-id: 4af7a583802 (mariadb-10.5.2-474-g4af7a583802) > parent(s): 2da3dc5d422 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2021-01-08 20:59:09 +1000 > message: > > Refactor MYSQL_BIN_LOG: extract Event_log ancestor Would be good to have some explanation here. I understand that you'll use this refactoring in following commits, but conceptually, what Event_log is? I mean, like Event_log is a log where Log_event_xxx events are written, but not _something else that MYSQL_BIN_LOG is and Event_log isn't_ or Event_log is a log where Log_event_xxx events are written. While MYSQL_BIN_LOG is Event_log that also does .... > diff --git a/sql/log.cc b/sql/log.cc > index 827a34c2883..6f4e3191588 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -3701,64 +3765,25 @@ bool MYSQL_BIN_LOG::open(const char *log_name, > write_file_name_to_index_file= 1; > } a bit strange that MYSQL_BIN_LOG::open doesn't use Event_log::open > diff --git a/sql/log.h b/sql/log.h > index 73cd66d5a4a..74c409e1ac7 100644 > --- a/sql/log.h > +++ b/sql/log.h > @@ -327,6 +327,7 @@ class MYSQL_LOG > bool strip_ext, char *buff); > virtual int generate_new_name(char *new_name, const char *log_name, > ulong next_log_number); > + inline mysql_mutex_t* get_log_lock() { return &LOCK_log; } > protected: > /* LOCK_log is inited by init_pthread_objects() */ > mysql_mutex_t LOCK_log; > @@ -376,6 +377,64 @@ struct Rows_event_factory > } > }; > > +class Event_log: public MYSQL_LOG > +{ > +public: > +#if !defined(MYSQL_CLIENT) > + Rows_log_event* > + prepare_pending_rows_event(THD *thd, TABLE* table, > + binlog_cache_data *cache_data, > + uint32 serv_id, size_t needed, > + bool is_transactional, > + Rows_event_factory event_factory); forgot to delete the same from MYSQL_BIN_LOG > +#endif > + > + bool flush_and_sync(bool *synced); same > + void cleanup() > + { > + if (inited) > + mysql_mutex_destroy(&LOCK_binlog_end_pos); > + > + MYSQL_LOG::cleanup(); > + } > + void init_pthread_objects() > + { > + MYSQL_LOG::init_pthread_objects(); > + > + mysql_mutex_init(m_key_LOCK_binlog_end_pos, &LOCK_binlog_end_pos, > + MY_MUTEX_INIT_SLOW); > + } > + > + bool open( > + const char *log_name, > + enum_log_type log_type, > + const char *new_name, ulong next_file_number, > + enum cache_type io_cache_type_arg); > + IO_CACHE *get_log_file() { return &log_file; } > + > + int write_description_event(enum_binlog_checksum_alg checksum_alg, > + bool encrypt, bool dont_set_created); > + > + bool write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file); > +}; > + > /* Tell the io thread if we can delay the master info sync. */ > #define SEMI_SYNC_SLAVE_DELAY_SYNC 1 > /* Tell the io thread if the current event needs a ack. */ > @@ -451,7 +510,7 @@ class binlog_cache_data; > struct rpl_gtid; > struct wait_for_commit; > > -class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG > +class MYSQL_BIN_LOG: public TC_LOG, private Event_log > { > /** The instrumentation key to use for @ LOCK_index. */ > PSI_mutex_key m_key_LOCK_index; > @@ -849,15 +901,13 @@ class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG > > - bool write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file); > + bool write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file) > + { return Event_log::write_event(ev, data, file); } Perhaps using Event_log::write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file); ? > bool write_event(Log_event *ev) { return write_event(ev, 0, &log_file); } > > bool write_event_buffer(uchar* buf,uint len); > @@ -918,7 +968,6 @@ class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG > uint next_file_id(); > inline char* get_index_fname() { return index_file_name;} > inline char* get_log_fname() { return log_file_name; } > - inline char* get_name() { return name; } > inline mysql_mutex_t* get_log_lock() { return &LOCK_log; } should remove that too, I reckon > inline mysql_cond_t* get_bin_log_cond() { return &COND_bin_log_updated; } > inline IO_CACHE* get_log_file() { return &log_file; } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] f3603cbfaeb: report progress
by Sergei Golubchik 24 Nov '21

24 Nov '21

Hi, Nikita! On Nov 15, Nikita Malyavin wrote: > revision-id: f3603cbfaeb (mariadb-10.5.2-483-gf3603cbfaeb) > parent(s): 3b6038bee88 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2021-01-29 03:47:52 +1000 > message: > > report progress Perhaps, squash this with your commit "test progress reporting" ? > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 29d0f487303..87dbe226c2e 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -11300,6 +11300,9 @@ static int online_alter_read_from_binlog(THD *thd, rpl_group_info *rgi, > > IO_CACHE *log_file= log->flip(); > > + thd_progress_report(thd, 0, my_b_write_tell(log_file) > + / rgi->tables_to_lock->m_conv_table->s->reclength); 1. why do you do this "/ rgi->...->s->reclength" ? 2. How does it work with your "cache flipping"? While data is copied between tables, concurrent connections write into an IO_CACHE. Then you flip, they what, write into the second IO_CACHE? What file length will you use here in thd_progress_report()? > + > do > { > const auto *descr_event= rgi->rli->relay_log.description_event_for_exec; > @@ -11319,6 +11322,9 @@ static int online_alter_read_from_binlog(THD *thd, rpl_group_info *rgi, > free_root(&event_mem_root, MYF(MY_KEEP_PREALLOC)); > if (ev != rgi->rli->relay_log.description_event_for_exec) > delete ev; > + thd_progress_report(thd, my_b_tell(log_file) > + / rgi->tables_to_lock->m_conv_table->s->reclength, > + thd->progress.max_counter); > } > while(!error); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3