- developers - lists.mariadb.org

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

05 Jan '22

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

2 1

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

31 Dec '21

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

1 0

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

31 Dec '21

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

1 0

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

28 Dec '21

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

1 0

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

20 Dec '21

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

1 0

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

20 Dec '21

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

1 0

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

13 Dec '21

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

2 2

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

11 Dec '21

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

2 3

[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