- developers - lists.mariadb.org

Re: [Maria-developers] 06ce67c644b: MDEV-27653 long uniques don't work with unicode collations
by Sergei Golubchik 16 Jan '23

16 Jan '23

Hi, Alexander, On Jan 15, Alexander Barkov wrote: > revision-id: 06ce67c644b (mariadb-10.4.27-41-g06ce67c644b) > parent(s): 6cb84346e1b > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2023-01-10 18:27:16 +0400 > message: > > MDEV-27653 long uniques don't work with unicode collations > > diff --git a/sql/sql_admin.cc b/sql/sql_admin.cc > index 213d77f8237..5dd19c877af 100644 > --- a/sql/sql_admin.cc > +++ b/sql/sql_admin.cc > @@ -772,11 +772,14 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, > int check_for_upgrade= file->ha_check_for_upgrade(check_opt); > > if (check_old_types == HA_ADMIN_NEEDS_ALTER || > - check_for_upgrade == HA_ADMIN_NEEDS_ALTER) > + check_old_types == HA_ADMIN_NEEDS_UPGRADE || > + check_for_upgrade == HA_ADMIN_NEEDS_ALTER || > + check_for_upgrade == HA_ADMIN_NEEDS_UPGRADE) eh. So old code was returning HA_ADMIN_NEEDS_ALTER actually quite intentionally. REPAIR TABLE was automatically switching to ALTER if these checks were returning HA_ADMIN_NEEDS_ALTER. So it didn't matter if the message confusingly said "Please use REPAIR", because it would've been ALTER internally anyway. Meaning, I suspect, that the old code was fine and you didn't need to change `case HA_ADMIN_NEEDS_ALTER` and didn't need to replace `return HA_ADMIN_NEEDS_UPGRADE` with HA_ADMIN_NEEDS_ALTER. All you needed to do was to return HA_ADMIN_NEEDS_ALTER from the check_long_hash_compatibility() method and the rest would've likely worked automatically. Or at least with much smaller changes. > { > /* We use extra_open_options to be able to open crashed tables */ > thd->open_options|= extra_open_options; > - result_code= admin_recreate_table(thd, table); > + result_code= admin_recreate_table(thd, table) ? HA_ADMIN_FAILED : > + HA_ADMIN_OK; good catch > thd->open_options&= ~extra_open_options; > goto send_result; > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 0bef50e50b5: MDEV-20501: Assertion `maybe_null || !null_value' failed in Item_func_round::date_op
by Sergei Golubchik 14 Jan '23

14 Jan '23

Hi, Sergei, On Jan 13, Sergei Petrunia wrote: > revision-id: 0bef50e50b5 (mariadb-10.4.27-33-g0bef50e50b5) > parent(s): 5db970fc760 > author: Sergei Petrunia > committer: Sergei Petrunia > timestamp: 2023-01-04 16:50:12 +0300 > message: > > MDEV-20501: Assertion `maybe_null || !null_value' failed in Item_func_round::date_op > > When the optimizer finds a constant (or system) table $TBL which is empty > or has no matching row, it would set table->null_row=true. This is done > even for tables with table->maybe_null==0. > > Then, it would proceed to perform query optimization phases (what for?) > which will attempt to evaluate Item expressions referring to $TBL. > > Eventually some Item expression will get the value of $TBL.not_null_field, > get SQL NULL and fail an assertion. > > Fixed by not performing any query optimization steps after we've got > constant/empty tables with no matching rows. > > Test result changes contain a lot of changes like > - ... Impossible WHERE noticed after reading const tables > + ... no matching row in const table > > as well as other changes caused by slightly-different processing of > the special case of empty constant tables. > > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 6a441c5047b..57ffd58f8b4 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -2382,6 +2382,9 @@ int JOIN::optimize_stage2() > if (subq_exit_fl) > goto setup_subq_exit; > > + if (zero_result_cause) > + goto setup_subq_exit; In many other cases it's, like zero_result_cause= "No matching min/max row"; subq_exit_fl= true; that is, subq_exit_fl is set whenever zero_result_cause is set. Meaning, perhaps you should do the same in your new code below and then you wouldn't need those lines you've added above? > if (unlikely(thd->check_killed())) > DBUG_RETURN(1); > > @@ -5392,7 +5398,32 @@ make_join_statistics(JOIN *join, List<TABLE_LIST> &tables_list, > } > } > } while (ref_changed); > - > + > + /* > + ConstRowNotFoundShortcut-1: > + Some constant/system tables have mo matching rows. This means that > + the join operation output will be empty. > + Short-cut further optimization steps. > + Note that some query plan steps will still be performed to handle > + implicit grouping, join result setup, etc. > + See also: ConstRowNotFoundShortcut-{2,3}. > + > + Do not do this if we're optimizing for some UNION's fake_select_lex. > + We might be running UglySubqueryReoptimization (grep for name) and > + in this case constant tables are not reliable. > + */ > + if ((join->const_table_map & ~found_const_table_map) && > + !(join->select_lex->master_unit() && > + join->select_lex->master_unit()->fake_select_lex == join->select_lex)) why wouldn't you check for `describe` here? "reoptimization" hack is only needed for EXPLAIN. > + { > + join->zero_result_cause= "no matching row in const table"; > + join->table_count=0; > + join->const_tables= 0; > + join->join_tab= NULL; > + > + DBUG_RETURN(0); > + } > + > join->sort_by_table= get_sort_by_table(join->order, join->group_list, > join->select_lex->leaf_tables, > join->const_table_map); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 5dcde8f6523: MDEV-27653 long uniques don't work with unicode collations
by Sergei Golubchik 06 Jan '23

06 Jan '23

Hi, Alexander, Looks good. A couple of comments, see below On Jan 06, Alexander Barkov wrote: > revision-id: 5dcde8f6523 (mariadb-10.4.26-64-g5dcde8f6523) > parent(s): ce443c85547 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-10-28 15:37:44 +0400 > message: > > MDEV-27653 long uniques don't work with unicode collations > > diff --git a/mysql-test/main/ctype_utf8.test b/mysql-test/main/ctype_utf8.test > index cc61c2ae0fe..e2d4e4ab906 100644 > --- a/mysql-test/main/ctype_utf8.test > +++ b/mysql-test/main/ctype_utf8.test > @@ -2310,3 +2310,133 @@ VALUES (_latin1 0xDF) UNION VALUES(_utf8'a' COLLATE utf8_bin); > --echo # > --echo # End of 10.3 tests > --echo # > + > + > +--echo # > +--echo # Start of 10.4 tests > +--echo # > + > +--echo # > +--echo # MDEV-27653 long uniques don't work with unicode collations > +--echo # > + > +SET NAMES utf8mb3; > + > +# CHAR > + > +CREATE TABLE t1 ( > + a CHAR(30) COLLATE utf8mb3_general_ci, > + UNIQUE KEY(a) USING HASH > +); > +SHOW CREATE TABLE t1; > +INSERT INTO t1 VALUES ('a'); > +--error ER_DUP_ENTRY > +INSERT INTO t1 VALUES ('ä'); > +SELECT * FROM t1; > +DROP TABLE t1; > + > +CREATE TABLE t1 ( > + a CHAR(30) COLLATE utf8mb3_general_ci, > + UNIQUE KEY(a(10)) USING HASH > +); > +SHOW CREATE TABLE t1; > +INSERT INTO t1 VALUES ('a'); > +--error ER_DUP_ENTRY > +INSERT INTO t1 VALUES ('ä'); > +SELECT * FROM t1; > +DROP TABLE t1; > + > + > +# VARCHAR > + > +CREATE TABLE t1 ( > + a VARCHAR(30) COLLATE utf8mb3_general_ci, > + UNIQUE KEY(a) USING HASH > +); > +SHOW CREATE TABLE t1; > +INSERT INTO t1 VALUES ('a'); > +--error ER_DUP_ENTRY > +INSERT INTO t1 VALUES ('ä'); > +SELECT * FROM t1; > +DROP TABLE t1; > + > +CREATE TABLE t1 ( > + a VARCHAR(30) COLLATE utf8mb3_general_ci, > + UNIQUE KEY(a(10)) USING HASH > +); > +SHOW CREATE TABLE t1; > +INSERT INTO t1 VALUES ('a'); > +--error ER_DUP_ENTRY > +INSERT INTO t1 VALUES ('ä'); > +SELECT * FROM t1; > +DROP TABLE t1; > + > + > +# TEXT > + > +CREATE TABLE t1 (a TEXT COLLATE utf8mb3_general_ci UNIQUE); > +SHOW CREATE TABLE t1; > +INSERT INTO t1 VALUES ('a'); > +--error ER_DUP_ENTRY > +INSERT INTO t1 VALUES ('ä'); > +SELECT * FROM t1; > +DROP TABLE t1; > + > +CREATE TABLE t1 ( > + a LONGTEXT COLLATE utf8mb3_general_ci, > + UNIQUE KEY(a(10)) USING HASH > +); > +SHOW CREATE TABLE t1; > +INSERT INTO t1 VALUES ('a'); > +--error ER_DUP_ENTRY > +INSERT INTO t1 VALUES ('ä'); > +SELECT * FROM t1; > +DROP TABLE t1; > + > + > +# Testing upgrade: > +# Prior to MDEV-27653, the UNIQUE HASH function errorneously > +# took into account string octet length. > +# Old tables should still open and work, but with a wrong results. > + > +copy_file std_data/mysql_upgrade/mdev27653_100422_text.frm $MYSQLD_DATADIR/test/t1.frm; > +copy_file std_data/mysql_upgrade/mdev27653_100422_text.MYD $MYSQLD_DATADIR/test/t1.MYD; > +copy_file std_data/mysql_upgrade/mdev27653_100422_text.MYI $MYSQLD_DATADIR/test/t1.MYI; > +SHOW CREATE TABLE t1; > +SELECT a, OCTET_LENGTH(a) FROM t1 ORDER BY BINARY a; > +CHECK TABLE t1; > + > +# There is already a one byte value 'a' in the table > +--error ER_DUP_ENTRY > +INSERT INTO t1 VALUES ('A'); > + > +# There is already a two-byte value 'ä' in the table > +--error ER_DUP_ENTRY > +INSERT INTO t1 VALUES ('Ä'); > + > +# There were no three-byte values in the table so far. > +# The below value violates UNIQUE, but it gets inserted. > +# This is wrong but expected for a pre-MDEV-27653 table. > +INSERT INTO t1 VALUES ('Ấ'); > +SELECT a, OCTET_LENGTH(a) FROM t1 ORDER BY BINARY a; > +CHECK TABLE t1; > + > +# ALTER FORCE fails: it tries to rebuild the table > +# with a correct UNIQUE HASH function, but there are duplicates! > +--error ER_DUP_ENTRY > +ALTER TABLE t1 FORCE; please, try ALTER IGNORE TABLE too > + > +# Let's remove all duplicate values, so only the one-byte 'a' stays. > +# ALTER..FORCE should work after that. > +DELETE FROM t1 WHERE OCTET_LENGTH(a)>1; > +ALTER TABLE t1 FORCE; > + > +# Make sure that 'a' and 'ä' cannot co-exists any more, > +# because the table was recreated with a correct UNIQUE HASH function. > +--error ER_DUP_ENTRY > +INSERT INTO t1 VALUES ('ä'); > +DROP TABLE t1; > + > +--echo # > +--echo # End of 10.4 tests > +--echo # > diff --git a/sql/table.cc b/sql/table.cc > index b9260853381..97f71284005 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -1267,7 +1286,11 @@ bool parse_vcol_defs(THD *thd, MEM_ROOT *mem_root, TABLE *table, > list_item= new (mem_root) Item_field(thd, keypart->field); > field_list->push_back(list_item, mem_root); > } > - Item_func_hash *hash_item= new(mem_root)Item_func_hash(thd, *field_list); > + > + Item_func_hash *hash_item= make_unique_hash_func(thd, mem_root, > + table->s->mysql_version, > + field_list); would be good to fix CHECK ... FOR UPGRADE too. > + > Virtual_column_info *v= new (mem_root) Virtual_column_info(); > field->vcol_info= v; > field->vcol_info->expr= hash_item; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 92ff948d021: MDEV-29231 View returns wrong value with SQL_MODE 'NO_BACKSLASH_ESCAPES'
by Sergei Golubchik 06 Jan '23

06 Jan '23

Hi, Oleksandr, On Jan 06, Oleksandr Byelkin wrote: > revision-id: 92ff948d021 (mariadb-10.3.37-53-g92ff948d021) > parent(s): 180b2bcd538 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2022-12-05 20:42:03 +0100 > message: > > MDEV-29231 View returns wrong value with SQL_MODE 'NO_BACKSLASH_ESCAPES' > > 1. make view printing sql_mode independent > 1.1. Print always current escape character > 1.2. Allow escape character usage independent of sql_mode > 2. Add workaround for parsing "like 3 in (0,1) escape 3" problem > (alwaysuse parances in case of printing ESCAPE) > > diff --git a/mysql-test/main/ctype_cp1250_ch.result b/mysql-test/main/ctype_cp1250_ch.result > index b0aa4cff382..88175ae5013 100644 > --- a/mysql-test/main/ctype_cp1250_ch.result > +++ b/mysql-test/main/ctype_cp1250_ch.result > @@ -129,7 +129,7 @@ EXPLAIN EXTENDED SELECT * FROM t1 WHERE CONCAT(c1)='a' AND CONCAT(c1) LIKE 'a '; > id select_type table type possible_keys key key_len ref rows filtered Extra > 1 SIMPLE t1 ALL NULL NULL NULL NULL 2 100.00 Using where > Warnings: > -Note 1003 select `test`.`t1`.`c1` AS `c1` from `test`.`t1` where concat(`test`.`t1`.`c1`) = 'a' and concat(`test`.`t1`.`c1`) like 'a ' > +Note 1003 select `test`.`t1`.`c1` AS `c1` from `test`.`t1` where concat(`test`.`t1`.`c1`) = 'a' and concat(`test`.`t1`.`c1`) like ('a ') escape '\\' I don't think there's a need to print the escape clause here, it only makes the output less readable. > diff --git a/mysql-test/main/default.result b/mysql-test/main/default.result > index c9ac96367aa..5c72b894bf6 100644 > --- a/mysql-test/main/default.result > +++ b/mysql-test/main/default.result > @@ -1936,7 +1936,7 @@ CREATE OR REPLACE TABLE t1 ( col INT DEFAULT ( 1 LIKE ( NOW() BETWEEN '2000-01-0 > SHOW CREATE TABLE t1; > Table Create Table > t1 CREATE TABLE `t1` ( > - `col` int(11) DEFAULT (1 like (current_timestamp() between '2000-01-01' and '2012-12-12')) > + `col` int(11) DEFAULT (1 like (current_timestamp() between '2000-01-01' and '2012-12-12') escape '\\') So, there're basically two approaches to a fix and we're inconsistently using both for different items. 1. one can always print expressions in the environment-independent way - create longer less readable text - not all items support it, new syntax might be needed (cannot emulate NO_BACKSLASH_ESCAPES with the ESCAPE clause) 2. one can set the environment (sql_mode) to the same hard-coded value before parsing views and vcols - one cannot dump the view/table definition in one sql_mode and load in another + this isn't normally a problem, as mysqldump also uses a fixed sql_mode - some item properties (what are they?) might be only set at fix_fields, fixing sql_mode during parsing won't help there Could we try to agree to one approach and use it consistently for all items from now on? > diff --git a/mysql-test/main/parser.result b/mysql-test/main/parser.result > index 0bb4e82c8b8..79ec9f539eb 100644 > --- a/mysql-test/main/parser.result > +++ b/mysql-test/main/parser.result > @@ -1325,11 +1325,11 @@ select 1 between 2 in (3,4) and 5 AS `1 between (2 in (3,4)) and 5` > create or replace view v1 as select 1 between (2 like 3) and 4; > Select view_definition from information_schema.views where table_schema='test' and table_name='v1'; > view_definition > -select 1 between 2 like 3 and 4 AS `1 between (2 like 3) and 4` > +select 1 between 2 like (3) escape '\\' and 4 AS `1 between (2 like 3) and 4` > create or replace view v1 as select 1 not between (2 like 3) and 4; > Select view_definition from information_schema.views where table_schema='test' and table_name='v1'; > view_definition > -select 1 not between 2 like 3 and 4 AS `1 not between (2 like 3) and 4` > +select 1 not between 2 like (3) escape '\\' and 4 AS `1 not between (2 like 3) and 4` > drop view v1; > # > # Start of 10.2 tests > diff --git a/mysql-test/main/precedence.result b/mysql-test/main/precedence.result > index fc6579651b4..ce26ca2acf2 100644 > --- a/mysql-test/main/precedence.result > +++ b/mysql-test/main/precedence.result > @@ -113,7 +113,7 @@ NOT 2 != 3 NOT (2 != 3) (NOT 2) != 3 > create or replace view v1 as select NOT 2 LIKE 3, NOT (2 LIKE 3), (NOT 2) LIKE 3; > Select view_definition from information_schema.views where table_schema='test' and table_name='v1'; > view_definition > -select 2 not like 3 AS `NOT 2 LIKE 3`,2 not like 3 AS `NOT (2 LIKE 3)`,!2 like 3 AS `(NOT 2) LIKE 3` > +select 2 not like (3) escape '\\' AS `NOT 2 LIKE 3`,2 not like (3) escape '\\' AS `NOT (2 LIKE 3)`,!2 like (3) escape '\\' AS `(NOT 2) LIKE 3` > select NOT 2 LIKE 3, NOT (2 LIKE 3), (NOT 2) LIKE 3 union select * from v1; > NOT 2 LIKE 3 NOT (2 LIKE 3) (NOT 2) LIKE 3 > 1 1 0 > diff --git a/mysql-test/main/precedence_bugs.result b/mysql-test/main/precedence_bugs.result > index 723ab823b48..9654e3c31db 100644 > --- a/mysql-test/main/precedence_bugs.result > +++ b/mysql-test/main/precedence_bugs.result > @@ -54,7 +54,7 @@ drop table t1; > create view v1 as select 1 like (now() between '2000-01-01' and '2012-12-12' ); > show create view v1; > View v1 > -Create View CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select 1 like (current_timestamp() between '2000-01-01' and '2012-12-12') AS `1 like (now() between '2000-01-01' and '2012-12-12' )` > +Create View CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select 1 like (current_timestamp() between '2000-01-01' and '2012-12-12') escape '\\' AS `1 like (now() between '2000-01-01' and '2012-12-12' )` > character_set_client latin1 > collation_connection latin1_swedish_ci > drop view v1; if you'll always print ESCAPE clause, it'll make many tests in parser.test, precedence.test and precedence_bugs.test meaningless. Don't just update result files, please, remove tests that no longer make sense. Or rewrite them, when possible (but in precedence.test it's not). > diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc > index b7b0c981c2d..702689a7496 100644 > --- a/sql/item_cmpfunc.cc > +++ b/sql/item_cmpfunc.cc > @@ -5262,10 +5262,15 @@ void Item_func_like::print(String *str, enum_query_type query_type) > str->append(STRING_WITH_LEN(" not ")); > str->append(func_name()); > str->append(' '); > - if (escape_used_in_parsing) > + if (escape_used_in_parsing || (query_type | QT_VIEW_INTERNAL)) > { > - args[1]->print_parenthesised(str, query_type, precedence()); > - str->append(STRING_WITH_LEN(" escape ")); > + /* > + Explicit parencies is workaround against parser bug which üprevent > + patsing "like 3 in (0,1) escape 3" correctly > + */ > + str->append('('); > + args[1]->print(str, query_type); > + str->append(STRING_WITH_LEN(") escape ")); no, this is wrong. Don't just force parentheses there. Instead, fix the precedence, so that the parentheses would be printed automatically when needed. > escape_item->print_parenthesised(str, query_type, higher_precedence()); > } > else > @@ -5390,10 +5395,7 @@ bool fix_escape_item(THD *thd, Item *escape_item, String *tmp_str, > if (escape_str) > { > const char *escape_str_ptr= escape_str->ptr(); > - if (escape_used_in_parsing && ( > - (((thd->variables.sql_mode & MODE_NO_BACKSLASH_ESCAPES) && > - escape_str->numchars() != 1) || > - escape_str->numchars() > 1))) > + if (escape_used_in_parsing && escape_str->numchars() > 1) I still think that ESCAPE '' doesn't mean "no escape character". I tried and in my tests it didn't. > { > my_error(ER_WRONG_ARGUMENTS,MYF(0),"ESCAPE"); > return TRUE; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 6ff49e48e4f: MDEV-26161: fix of view protocol
by Sergei Golubchik 06 Jan '23

06 Jan '23

Hi, Oleksandr, On Jan 06, Oleksandr Byelkin wrote: > revision-id: 6ff49e48e4f (mariadb-10.3.37-54-g6ff49e48e4f) > parent(s): 92ff948d021 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2022-12-06 10:22:06 +0100 > message: > > MDEV-26161: fix of view protocol could you please amend the commit comment to explain why the statement produces a different error in a view protocol? > diff --git a/mysql-test/main/gis.result b/mysql-test/main/gis.result > index 358be520b06..37613bc71a0 100644 > --- a/mysql-test/main/gis.result > +++ b/mysql-test/main/gis.result > @@ -4980,11 +4980,11 @@ ERROR HY000: Illegal parameter data type geometry for operation 'is_used_lock' > # MDEV-26161 crash in Gis_point::calculate_haversine > # > select st_distance_sphere(x'01030000000400000004000000000000', multipoint(point(124,204)), 10); > -ERROR 22003: Cannot get geometry object from data you send to the GEOMETRY field > +Got one of the listed errors > select st_distance_sphere(x'010300000004000000040000', multipoint(point(124,204)), 10); > -ERROR 22003: Cannot get geometry object from data you send to the GEOMETRY field > +Got one of the listed errors > select st_distance_sphere(x'010300000001000000040000', multipoint(point(124,204)), 10); > -ERROR 22003: Cannot get geometry object from data you send to the GEOMETRY field > +Got one of the listed errors > # > # End of 10.3 tests > # > diff --git a/mysql-test/main/gis.test b/mysql-test/main/gis.test > index 716fab9bfeb..07b207dcdc9 100644 > --- a/mysql-test/main/gis.test > +++ b/mysql-test/main/gis.test > @@ -3093,11 +3093,11 @@ SELECT IS_USED_LOCK(POINT(1,1)); > --echo # > --echo # MDEV-26161 crash in Gis_point::calculate_haversine > --echo # > ---error ER_CANT_CREATE_GEOMETRY_OBJECT > +--error ER_CANT_CREATE_GEOMETRY_OBJECT,ER_INTERNAL_ERROR > select st_distance_sphere(x'01030000000400000004000000000000', multipoint(point(124,204)), 10); > ---error ER_CANT_CREATE_GEOMETRY_OBJECT > +--error ER_CANT_CREATE_GEOMETRY_OBJECT,ER_INTERNAL_ERROR > select st_distance_sphere(x'010300000004000000040000', multipoint(point(124,204)), 10); > ---error ER_CANT_CREATE_GEOMETRY_OBJECT > +--error ER_CANT_CREATE_GEOMETRY_OBJECT,ER_INTERNAL_ERROR > select st_distance_sphere(x'010300000001000000040000', multipoint(point(124,204)), 10); > > --echo # > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] MDEV-28602 Wrong result with outer join, merged derived table and view
by Sergey Petrunia 04 Jan '23

04 Jan '23

Hi Rex, Please find below input for the commit. (I've actually committed an alternative fix now, but before doing that I wrote all the trivial comments below so I'm sending these anyway in order to show what is expected of a patch. I'm also including the rationale for creating an alternative patch) > commit 75af3195482d3da277825663d1150c0dfc55420a > Author: Rex <rex.johnston(a)mariadb.com> > Date: Fri Dec 23 05:28:59 2022 +1200 > > MDEV-28602 Wrong result with outer join, merged derived table and view > > const item reference being mishandled in outer to inner join on > filling in null values. > First, fixVersion. It is currently set to 10.11, but affectsVersion starts from 10.3 I think the bug and the fix are applicable for all versions. Second: commit comment. This is not descriptive enough: > const item reference being mishandled in outer to inner join on > filling in null values. Something like this would be better: <commit-comment> A LEFT JOIN with a constant as a column of the inner table produced wrong query result if the optimizer had to write the inner table column into a temp table: SELECT ... FROM (SELECT /*non-mergeable select*/ FROM t1 LEFT JOIN (SELECT 'Y' as Val) t2 ON ...) Fixed this by: 1. Making Item_ref::save_in_field() call check_null_ref() to see if the referred table has a NULL-complemented row. 2. In order to do #1, moved null_ref_table() and check_null_ref() from Item_direct_view_ref to Item_ref. </commit-comment> Note: it starts with a siccint problem description, which is followed by a description of the fix. > diff --git a/sql/sql_join_cache.cc b/sql/sql_join_cache.cc > index 620c52a3f40..65bedc3d337 100644 > --- a/sql/sql_join_cache.cc > +++ b/sql/sql_join_cache.cc > @@ -2622,7 +2622,7 @@ enum_nested_loop_state JOIN_CACHE::join_null_complements(bool skip_last) > get_record(); > /* The outer row is complemented by nulls for each inner table */ > restore_record(join_tab->table, s->default_values); > - mark_as_null_row(join_tab->table); > + mark_as_null_row(join_tab->table); In the future please make sure patches do not contain unrelated meaningless changes like this one. > rc= generate_full_extensions(get_curr_rec()); > if (rc != NESTED_LOOP_OK && rc != NESTED_LOOP_NO_MORE_ROWS) > goto finish; > diff --git a/mysql-test/main/merge.test b/mysql-test/main/merge.test > index 0485f3ed1c3..05b05287e1b 100644 > --- a/mysql-test/main/merge.test > +++ b/mysql-test/main/merge.test Why is the patch in the merge.test ? merge.test at the top says clearly "Test of MERGE TABLES". That is, this is a test for tables with ENGINE=MERGE, a feature not related to this bug. The right test files for this bug would be main/join_outer.test and main/derived.test, I would prefer the first one. > @@ -2886,3 +2886,50 @@ drop table tm, t; > --echo # > --echo # End of 10.8 tests > --echo # > + > +--echo # > +--echo # MDEV-28602 Wrong result with outer join, merged derived table and view > +--echo # > + > +drop table if exists t1, t2; ^^^ This is not needed as there is a DROP TABLE right above > +create table t1 ( > + Election int(10) unsigned NOT NULL > +); > + > +insert into t1 (Election) values (1); > + +create table t2 ( + VoteID int(10), + ElectionID int(10), + UserID int(10) +); + +insert into t2 (ElectionID, UserID) values (2, 30), (3, 30); +# INSERT INTO t2 (ElectionID, UserID) VALUES (1, 30); ^^ No need to have commented-out lines in the commit. +drop view if exists v1; ^^^ same as above: this is not needed. +create view v1 as select * from t1 + left join ( select 'Y' AS Voted, ElectionID from t2 ) AS T + on T.ElectionID = t1.Election +limit 9; +# limit X causes merge algorithm select as opposed to temp table +select * from v1; ... > diff --git a/sql/item.h b/sql/item.h > index 2d598546b91..8684477b558 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -5531,18 +5531,42 @@ class Item_sp > > class Item_ref :public Item_ident > { > +public: > + > +#define NO_NULL_TABLE (reinterpret_cast<TABLE *>(0x1)) > + > + void set_null_ref_table() > + { > + null_ref_table= NO_NULL_TABLE; > + } > + > + bool check_null_ref() This works, but looks like a change that is 1. Too big 2. Looks like a partial implementation I'm looking at methods of Item_direct_view_ref: - save_val() - save_org_in_field() - save_in_result_field() - val_real() and other val_XXX() and they all follow the same pattern: if (check_null_ref()) produce SQL NULL; else Call Item_direct_ref::same_method(); This suggests we could add Item_direct_view_ref::save_in_field() and use the same approach. Conversely, if we move check_null_ref() to be in Item_ref, we will end up with: - check_null_ref() is in Item_ref - methods that make use of it are in Item_direct_view_ref() - methods of Item_ref() do not make check_null_ref() check, except for save_in_field() which does it. I think the former looks more logical than the latter. I've made the patch for it and I'll ask Sanja (the Item_ref/views expert) to check it. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 0

Re: [Maria-developers] 6a8268b7893: MDEV-28915: mysql_upgrade fails due to old_mode="", with "Cannot load from mysql.proc. The table is probably corrupted"
by Sergei Golubchik 28 Dec '22

28 Dec '22

Hi, Rucha, On Dec 28, Rucha Deodhar wrote: > revision-id: 6a8268b7893 (mariadb-10.6.11-62-g6a8268b7893) > parent(s): 4ca5a0ec98d > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2022-12-15 17:45:25 +0530 > message: > > MDEV-28915: mysql_upgrade fails due to old_mode="", with "Cannot load from > mysql.proc. The table is probably corrupted" > > Analysis: When mysql_upgrade runs statements for upgrade, characterset is > converted to utf8mb4 because server starts with old_mode that interprets > utf8 to utf8mb4, but mysql.proc table has "utf8mb3" as hardcoded, so > it crashes with corrupted table. > > Fix: Changed Table_check_intact::check() definition to allow both > utf8mb3 and utf8mb4 by checking prefix and changing the upgrade scripts > to explicitly use utf8mb3 > > diff --git a/mysql-test/main/mysql_upgrade-28915.opt b/mysql-test/main/mysql_upgrade-28915.opt > new file mode 100644 > index 00000000000..0be5567a64d > --- /dev/null > +++ b/mysql-test/main/mysql_upgrade-28915.opt > @@ -0,0 +1 @@ > +--old-mode=NO_DUP_KEY_WARNINGS_WITH_IGNORE it's a bit confusing, try to use --old-mode= that is, empty old_mode, otherwise one starts thinking what this bug has to do with NO_DUP_KEY_WARNINGS_WITH_IGNORE. > diff --git a/mysql-test/main/mysql_upgrade-28915.test b/mysql-test/main/mysql_upgrade-28915.test > new file mode 100644 > index 00000000000..61b1fc263cb > --- /dev/null > +++ b/mysql-test/main/mysql_upgrade-28915.test > @@ -0,0 +1,78 @@ > +--source include/mysql_upgrade_preparation.inc > + > +let $MYSQLD_DATADIR= `select @@datadir`; > + > +--echo # > +--echo # Beginning of 10.6 test > +--echo # > +--echo # MDEV-28915: mysql_upgrade fails due to old_mode="", with "Cannot load > +--echo # from mysql.proc. The table is probably corrupted" > + > +--echo # Show that tables are created with utf8mb3 even without UTF8_IS_UTF8MB3 (see the .opt file) I don't think you've shown it. mtr does not recreate system tables for every test. You need to force it to re-bootstrap with one of the options that do it, for example, --innodb-page-size=16k (search mtr for "rebootstrap" for details). But even that won't help, because it'll only add "rebootstrap" options to the server's command line when bootstrapping. May be a combination of .cnf and .opt can do what you want, I didn't try. > + > +SHOW CREATE TABLE mysql.proc; > +SHOW CREATE TABLE mysql.event; > + > +--exec $MYSQL_UPGRADE --force 2>&1 > + > +--remove_file $MYSQLD_DATADIR/mysql_upgrade_info > + > +SHOW CREATE TABLE mysql.proc; > +SHOW CREATE TABLE mysql.event; > + > +--echo # Emulate that tables were created with utf8mb4 by an older version > + > +ALTER TABLE mysql.proc MODIFY db CHAR(64) CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci NOT NULL, > + MODIFY name CHAR(64) CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci NOT NULL, > + MODIFY specific_name CHAR(64) CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci DEFAULT NULL, > + MODIFY definer VARCHAR(64) CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci DEFAULT NULL, > + MODIFY comment TEXT CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci DEFAULT NULL, > + MODIFY character_set_client CHAR(32) CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci DEFAULT NULL, > + MODIFY collation_connection CHAR(32) CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci DEFAULT NULL, > + MODIFY db_collation CHAR(32) CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci DEFAULT NULL; > + > +ALTER TABLE mysql.event MODIFY db CHAR(64) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin NOT NULL DEFAULT '', > + MODIFY name CHAR(64) CHARACTER SET utf8mb4 NOT NULL DEFAULT '', > + MODIFY definer VARCHAR(384) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin NOT NULL DEFAULT '', > + MODIFY comment CHAR(64) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin NOT NULL DEFAULT '', > + MODIFY character_set_client CHAR(32) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin DEFAULT NULL, > + MODIFY collation_connection CHAR(32) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin DEFAULT NULL, > + MODIFY db_collation CHAR(32) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin DEFAULT NULL; > + > +--echo # mysql_upgrade changes columns from utf8mb4 to utf8mb3 > + > +--exec $MYSQL_UPGRADE --force 2>&1 > +--remove_file $MYSQLD_DATADIR/mysql_upgrade_info > + > +--vertical_results > + > +SHOW CREATE TABLE mysql.proc; > +SHOW CREATE TABLE mysql.event; > + > +CREATE TABLE t1 (id1 INT, val1 VARCHAR(5)); > + > +DELIMITER |; > + > +CREATE PROCEDURE sp1 () > + BEGIN > + SELECT val1 FROM t1; > + END| You've made changes in sp.cc and event_db_repository.cc to ensure that these tables are usable even with utf8mb4 charset. But you don't test it if it works. You create a procedure and an event here, *after* tables were converted back to utf8mb3. you likely want to do it earlier, before mysql_upgrade, just after ALTER TABLE. > + > +DELIMITER ;| > + > +SELECT name, body_utf8, body FROM mysql.proc WHERE name like 'sp1'; > +CALL sp1(); > +SELECT name, body_utf8, body FROM mysql.proc WHERE name like 'sp1'; > + > +SET GLOBAL event_scheduler=ON; > + > +SELECT name, body_utf8, body FROM mysql.event; > +CREATE EVENT ev1 ON SCHEDULE EVERY 1 SECOND DO INSERT INTO t1 VALUES (1, 'abc'); > +SELECT name, body_utf8, body FROM mysql.event; > + > +SET GLOBAL event_scheduler=OFF; > +DROP EVENT ev1; > +DROP PROCEDURE sp1; > +DROP TABLE t1; > + > +--echo # end of 10.6 test Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 4ddf606debf: MDEV-30151 parse error 1=2 not between/in
by Sergei Golubchik 28 Dec '22

28 Dec '22

Hi, Alexander, I'm not sure I understand everything you did there, so questions below On Dec 28, Alexander Barkov wrote: > revision-id: 4ddf606debf (mariadb-10.3.37-59-g4ddf606debf) > parent(s): 8f30973234d > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-12-13 10:35:16 +0400 > message: > > MDEV-30151 parse error 1=2 not between/in > > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 1f6485dac6a..ca614ff63e9 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -1687,7 +1687,8 @@ bool my_yyoverflow(short **a, YYSTYPE **b, size_t *yystacksize); > > %left PREC_BELOW_NOT > > -%nonassoc NOT_SYM > +/* The precendence of boolean NOT is in fact here. See the comment below. */ > + > %left '=' EQUAL_SYM GE '>' LE '<' NE > %nonassoc IS > %right BETWEEN_SYM > @@ -1699,6 +1700,24 @@ bool my_yyoverflow(short **a, YYSTYPE **b, size_t *yystacksize); > %left '*' '/' '%' DIV_SYM MOD_SYM > %left '^' > %left MYSQL_CONCAT_SYM > +/* > + Boolean negation has a special branch in "expr" starting with NOT_SYM. > + The precedence of logical negation is determined by the grammar itself > + (without using Bison terminal symbol precedence) in this order > + - Boolean factor (i.e. logical AND) > + - Boolean NOT > + - Boolean test (such as '=', IS NULL, IS TRUE) > + > + But we also need a precedence for NOT_SYM in other contexts, > + to shift (without reduce) in these cases: > + predicate <here> NOT IN ... > + predicate <here> NOT BETWEEN ... > + predicate <here> NOT LIKE ... > + predicate <here> NOT REGEXP ... > + If the precedence of NOT_SYM was low, it would reduce immediately > + after scanning "predicate" and then produce a syntax error on "NOT". > +*/ > +%nonassoc NOT_SYM > %nonassoc NEG '~' NOT2_SYM BINARY > %nonassoc COLLATE_SYM > > @@ -1938,6 +1957,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, size_t *yystacksize); > literal insert_ident order_ident temporal_literal > simple_ident expr sum_expr in_sum_expr > variable variable_aux > + boolean_test boolean_factor > predicate bit_expr parenthesized_expr > table_wild simple_expr column_default_non_parenthesized_expr udf_expr > primary_expr string_factor_expr mysql_concatenation_expr > @@ -9840,79 +9860,87 @@ expr: > MYSQL_YYABORT; > } > } > - | NOT_SYM expr %prec NOT_SYM > + | boolean_factor what does that achieve? > + ; > + > +boolean_factor: > + NOT_SYM boolean_factor > { > $$= negate_expression(thd, $2); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr IS TRUE_SYM %prec IS > + | boolean_test %prec PREC_BELOW_NOT > + ; > + > +boolean_test: > + boolean_test IS TRUE_SYM %prec IS > { > $$= new (thd->mem_root) Item_func_istrue(thd, $1); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr IS not TRUE_SYM %prec IS > + | boolean_test IS not TRUE_SYM %prec IS > { > $$= new (thd->mem_root) Item_func_isnottrue(thd, $1); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr IS FALSE_SYM %prec IS > + | boolean_test IS FALSE_SYM %prec IS > { > $$= new (thd->mem_root) Item_func_isfalse(thd, $1); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr IS not FALSE_SYM %prec IS > + | boolean_test IS not FALSE_SYM %prec IS > { > $$= new (thd->mem_root) Item_func_isnotfalse(thd, $1); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr IS UNKNOWN_SYM %prec IS > + | boolean_test IS UNKNOWN_SYM %prec IS > { > $$= new (thd->mem_root) Item_func_isnull(thd, $1); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr IS not UNKNOWN_SYM %prec IS > + | boolean_test IS not UNKNOWN_SYM %prec IS > { > $$= new (thd->mem_root) Item_func_isnotnull(thd, $1); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr IS NULL_SYM %prec PREC_BELOW_NOT > + | boolean_test IS NULL_SYM %prec PREC_BELOW_NOT can you try %prec IS here? I suspect that if this affects anything, it'll cause bugs > { > $$= new (thd->mem_root) Item_func_isnull(thd, $1); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr IS not NULL_SYM %prec IS > + | boolean_test IS not NULL_SYM %prec IS > { > $$= new (thd->mem_root) Item_func_isnotnull(thd, $1); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr EQUAL_SYM predicate %prec EQUAL_SYM > + | boolean_test EQUAL_SYM predicate %prec EQUAL_SYM > { > $$= new (thd->mem_root) Item_func_equal(thd, $1, $3); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr comp_op predicate %prec '=' > + | boolean_test comp_op predicate %prec '=' > { > $$= (*$2)(0)->create(thd, $1, $3); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr comp_op all_or_any '(' subselect ')' %prec '=' > + | boolean_test comp_op all_or_any '(' subselect ')' %prec '=' > { > $$= all_any_subquery_creator(thd, $1, $2, $3, $5); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | predicate > + | predicate %prec BETWEEN_SYM > ; > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 4d9f8a3c31e: MDEV-28669 addendum: additional tests and mtr changes
by Sergei Golubchik 27 Dec '22

27 Dec '22

Hi, Julius, On Dec 27, Julius Goryavsky wrote: > commit 4d9f8a3c31e > Author: Julius Goryavsky <julius.goryavsky(a)mariadb.com> > Date: Mon Dec 19 10:29:55 2022 +0100 > > diff --git a/mysql-test/mysql-test-run.pl b/mysql-test/mysql-test-run.pl > index 12afab4d28c..8df8198fc99 100755 > --- a/mysql-test/mysql-test-run.pl > +++ b/mysql-test/mysql-test-run.pl > @@ -313,7 +313,7 @@ my %mysqld_logs; > my $opt_debug_sync_timeout= 300; # Default timeout for WAIT_FOR actions. > my $warn_seconds = 60; > > -my $rebootstrap_re= '--innodb[-_](?:page[-_]size|checksum[-_]algorithm|undo[-_]tablespaces|log[-_]group[-_]home[-_]dir|data[-_]home[-_]dir)|data[-_]file[-_]path|force_rebootstrap'; > +my $rebootstrap_re= '--(?:loose[-_])?(?:innodb[-_](?:page[-_]size|file(?:[-_]format|per[-_]table)|compression[-_](?:default|algorithm)|checksum(?:s|[-_]algorithm)|undo[-_](?:directory|tablespaces)|(?:data|log[-_]group)[-_]home[-_]dir|buffer[-_]pool[-_]filename|data[-_]file[-_]path|default[-_](?:encryption[-_]key[-_]id|page[-_]encryption)|sys[-_]|(?:index|table)[-_]stats)|aria[-_]log[-_](?:dir[-_]path|file[-_]size))|force_rebootstrap'; 1. drop force_rebootstrap please 2. this code was already not very readable, but got fairly ridiculous now. Let's change the approach. E.g. put them all in a hash, like my %rebootstrap_check = map { $_ => 1 } qw( innodb-page-size innodb-file-format ... ); you'll need to normalize an option before looking it up in the hash > > sub testcase_timeout ($) { return $opt_testcase_timeout * 60; } > sub check_timeout ($) { return testcase_timeout($_[0]); } > @@ -2762,7 +2762,29 @@ sub mysql_server_start($) { > return; > } > > - my $datadir= $mysqld->value('datadir'); > + my $extra_opts= get_extra_opts($mysqld, $tinfo); > + > + # The test options can contain the --datadir parameter, but > + # the bootstrap function can only read datadir location from > + # a .cnf file. So we need to parse the options to get the > + # actual location of the data directory, considering both > + # the options and the .cnf file, and then store the resulting > + # value in a "$mysqld" hash - to use later, when we need to > + # know the actual location of the data directory: > + my $datadir=""; > + foreach my $opt ( @$extra_opts ) > + { > + if ($opt =~ /^--datadir=/) { > + $datadir = substr($opt, 10); > + last; strictly speaking, `last` is incorrect, there can be more than one --datadir > + } > + } > + # If datadir is not in the options, then read it from .cnf: > + if (! $datadir) { > + $datadir = $mysqld->value('datadir'); > + } > + $mysqld->{'datadir'} = $datadir; > + > if (not $opt_start_dirty) > { > > @@ -2785,17 +2807,59 @@ sub mysql_server_start($) { > } > } > > + # Run <tname>-master.sh > + if ($mysqld->option('#!run-master-sh') and > + defined $tinfo->{master_sh} and > + run_system('/bin/sh ' . $tinfo->{master_sh}) ) > + { > + $tinfo->{'comment'}= "Failed to execute '$tinfo->{master_sh}'"; > + return 1; > + } > + > + # Run <tname>-slave.sh > + if ($mysqld->option('#!run-slave-sh') and > + defined $tinfo->{slave_sh} and > + run_system('/bin/sh ' . $tinfo->{slave_sh})) > + { > + $tinfo->{'comment'}= "Failed to execute '$tinfo->{slave_sh}'"; > + return 1; > + } > + > my $mysqld_basedir= $mysqld->value('basedir'); > - my $extra_opts= get_extra_opts($mysqld, $tinfo); > > if ( $basedir eq $mysqld_basedir ) > { > if (! $opt_start_dirty) # If dirty, keep possibly grown system db > { > + # Find the name of the current section in the configuration > + # file and its suffix: > + my $section = $mysqld->{name}; > + my $server_name; > + my $suffix = ""; > + if (index($section, '.') != -1) { > + ($server_name, $suffix) = $section =~ /^\s*([^\s.]+)\s*\.\s*([^\s]+)/; > + } > + else { > + $server_name = $section =~ /^\s*([^\s]+)/; > + } Replace all 11 lines above with: my ($suffix) = ($mysqld->{name} =~ /\.(.*)/); > # Some InnoDB options are incompatible with the default bootstrap. > # If they are used, re-bootstrap > my @rebootstrap_opts; > @rebootstrap_opts = grep {/$rebootstrap_re/o} @$extra_opts if $extra_opts; > + # Let's store the additional bootstrap options in > + # the environment variable - they may be used later > + # in the mtr tests - for re-bootstrap or run the > + # recovery, etc: > + my $extra_text = "--datadir=$datadir"; that's rather random. Why do you add only this one option, that doesn't make sense. Better handle any additional requirements in the test. > + if (@rebootstrap_opts) { why do you need an if() ? > + $extra_text .= ' '.join(' ', @rebootstrap_opts); > + } > + if ($suffix) { > + $ENV{'MTR_BOOTSTRAP_OPTS_'.$suffix} = $extra_text; > + } > + else { > + $ENV{'MTR_BOOTSTRAP_OPTS'} = $extra_text; Is that even possible? > + } > if (@rebootstrap_opts) > { > mtr_verbose("Re-bootstrap with @rebootstrap_opts"); > diff --git a/mysql-test/suite/galera/disabled.def b/mysql-test/suite/galera/disabled.def > index 84babda2fa0..18e7c074059 100644 > --- a/mysql-test/suite/galera/disabled.def > +++ b/mysql-test/suite/galera/disabled.def > @@ -28,3 +28,4 @@ query_cache: MDEV-15805 Test failure on galera.query_cache > versioning_trx_id: MDEV-18590: galera.versioning_trx_id: Test failure: mysqltest: Result content mismatch > galera_wsrep_provider_unset_set: wsrep_provider is read-only for security reasons > pxc-421: wsrep_provider is read-only for security reasons > +galera_sst_rsync_innodb_nest : MDEV-29591 nesting innodb subdirectories in datadir causes SST to fail Why do you disable it if the whole commit is about fixing the test? How can I see that the test works? > diff --git a/mysql-test/suite/galera/r/galera_sst_rsync_innodb__innodb_dir,debug.rdiff b/mysql-test/suite/galera/r/galera_sst_rsync_innodb__innodb_dir,debug.rdiff > new file mode 100644 > index 00000000000..027f4860a39 > --- /dev/null > +++ b/mysql-test/suite/galera/r/galera_sst_rsync_innodb__innodb_dir,debug.rdiff > @@ -0,0 +1,114 @@ looks like a wrong file name? > +--- galera_sst_rsync_innodb_nest.result > ++++ galera_sst_rsync_innodb_nest.reject > +@@ -286,3 +286,111 @@ > + DROP TABLE t1; > + COMMIT; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] f984f49fba7: MDEV-30203 - deb fix piuparts
by Sergei Golubchik 21 Dec '22

21 Dec '22

Hi, Daniel, This is a review of `git diff 12786f0e779f 5f76981eb649`, that is, for commits: 5f76981eb64 rpm: packages that dont match component name 25eaa0aebfa MDEV-30205: [Cmtr: suite look in mariadb-test path 0981201f027 mtr - wsrep use mariadb executables eba0b35035a mtr: maradb names for *embedded and mariadb-client-test 974ca88f0fe deb: fix autotest - exclude for MDEV-30205 b19d75408bb Deb: Breaks/Replaces/COnflicts c60f0341950 MDEV-30203 deb breaks/replaces on compat packages f984f49fba7 MDEV-30203 - deb fix piuparts 310d025ff0d MDEV-30205: fix deb autopkgtest 89f6cf47ee2 rpm: fix packaging of compat/non-compat scripts links 494ed76d15a deb: mysqlhotcopy to mariadb-client-compat d03075f3a1f deb: server-core mysql* -> server-compat 9453812193d wsrep_sst_mariabackup to use mariadb-backup (and message accordingly) a4f843a7a5a Fix uncollected symlink eb9a469bf9a Add package recommended dependencies c135be62274 Remove duplicate entry 84c7cd29c61 Fix Debian packaging for mariabackup 5dbc7ae8bb4 MDEV-30203: compat packages - RPM remove mariabackup link eb4fa5d750f MDEV-30203: debian - make compat packages d4be0b9be73 MDEV-30203: Create mysql symlink packages (RPM) - scripts 441879fd493 MDEV-30203: Create mysql symlink packages (RPM) On Dec 19, Daniel Black wrote: > diff --git a/cmake/cpack_rpm.cmake b/cmake/cpack_rpm.cmake > index 339363b2169..7b48fd64f4c 100644 > --- a/cmake/cpack_rpm.cmake > +++ b/cmake/cpack_rpm.cmake > @@ -85,6 +85,35 @@ SET(CPACK_RPM_server_PACKAGE_DESCRIPTION "${CPACK_RPM_PACKAGE_DESCRIPTION}") > SET(CPACK_RPM_test_PACKAGE_SUMMARY "MariaDB database regression test suite") > SET(CPACK_RPM_test_PACKAGE_DESCRIPTION "${CPACK_RPM_PACKAGE_DESCRIPTION}") > > +# "set/append array" - append a set of strings, separated by a space > +MACRO(SETA var) > + FOREACH(v ${ARGN}) > + SET(${var} "${${var}} ${v}") > + ENDFOREACH() > +ENDMACRO(SETA) > + > +FOREACH(SYM_COMPONENT Server Client Test) > + SET(SYM ${SYM_COMPONENT}_symlinks) > + string(TOLOWER ${SYM} SYM) * why all cmake commands are uppercase, but `string` is lowercase? * SET is redundant, you can do STRING(TOLOWER ${SYM_COMPONENT}_symlinks SYM) * may be, better to use ${SYM_COMPONENT}-symlinks ? Like in MariaDB-server-symlinks ? > + SET(SYMCOMP ${SYM_COMPONENT}Symlinks) > + string(TOUPPER ${SYMCOMP} SYMCOMP_UPPER) > + SET(CPACK_COMPONENT_${SYMCOMP_UPPER}_GROUP "${SYM}") > + SET(CPACK_COMPONENTS_ALL "${CPACK_COMPONENTS_ALL}" "${SYMCOMP}") > + SET(CPACK_RPM_${SYM}_PACKAGE_SUMMARY "MySQL compatible symlinks for MariaDB database ${SYM_COMPONENT} binaries/scripts") > + SET(CPACK_RPM_${SYM}_PACKAGE_DESCRIPTION "${CPACK_RPM_PACKAGE_DESCRIPTION}") > + SET(CPACK_RPM_${SYM}_PACKAGE_ARCHITECTURE "noarch") > + SETA(CPACK_RPM_${SYM_COMPONENT}_PACKAGE_RECOMMENDS "MariaDB-${SYM}") You forgot to say that symlink package DEPENDS on the non-symlink package > +ENDFOREACH() > + > +# TODO change to 11.0 on rebase > +SETA(CPACK_RPM_client_symlinks_PACKAGE_CONFLICTS > + "MariaDB-client < 10.11.2" > + "MariaDB-server < 10.11.2") > +SETA(CPACK_RPM_server_symlinks_PACKAGE_CONFLICTS > + "MariaDB-server < 10.11.2") > +SETA(CPACK_RPM_test_symlinks_PACKAGE_CONFLICTS > + "MariaDB-test < 10.11.2") > + > # libmariadb3 > SET(CPACK_RPM_shared_PACKAGE_SUMMARY "LGPL MariaDB database client library") > SET(CPACK_RPM_shared_PACKAGE_DESCRIPTION "This is LGPL MariaDB client library that can be used to connect to MySQL > diff --git a/cmake/symlinks.cmake b/cmake/symlinks.cmake > index 3f3b4e4a9b5..a4eb6c2b42c 100644 > --- a/cmake/symlinks.cmake > +++ b/cmake/symlinks.cmake > @@ -12,7 +12,6 @@ endmacro() > REGISTER_SYMLINK("mariadb" "mysql") > REGISTER_SYMLINK("mariadb-access" "mysqlaccess") > REGISTER_SYMLINK("mariadb-admin" "mysqladmin") > -REGISTER_SYMLINK("mariadb-backup" "mariabackup") better not. there're scripts that might be using it. This is why I suggested `strncmp(my_progname, "mariadb", 7)` and not `strncmp(my_progname, "maria", 5)` in your PR > REGISTER_SYMLINK("mariadb-binlog" "mysqlbinlog") > REGISTER_SYMLINK("mariadb-check" "mysqlcheck") > REGISTER_SYMLINK("mariadb-client-test-embedded" "mysql_client_test_embedded") > diff --git a/debian/control b/debian/control > index 81418c13656..6b225ca4b3d 100644 > --- a/debian/control > +++ b/debian/control > @@ -548,7 +548,8 @@ Provides: default-mysql-client, > virtual-mysql-client > Recommends: libdbd-mariadb-perl | libdbd-mysql-perl, > libdbi-perl, > - libterm-readkey-perl > + libterm-readkey-perl, > + mariadb-client-compat Please, no. deb and rpm packages are different enough already. don't use different suffixes for them. Choose either -symlink or -compat (or something else) and use it consistently both in deb and in rpm. > Description: MariaDB database client binaries > MariaDB is a fast, stable and true multi-user, multi-threaded SQL database > server. SQL (Structured Query Language) is the most popular database query > @@ -558,6 +559,110 @@ Description: MariaDB database client binaries > This package includes the client binaries and the additional tools > innotop and mariadb-report (mysqlreport). > > +Package: mariadb-client-compat > +Architecture: all > +Depends: mariadb-client > +Multi-Arch: foreign > +Description: MySQL compatibility links to mariadb-client binaries/scripts. > +Conflicts: mariadb-client (<< ${source:Version}), > + mariadb-client-10.0, I wonder, would it work, if you won't set any Conflicts/Breaks, but only set Depends? It'd be a lot simpler that way (if it'll work). > + mariadb-client-10.1, > + mariadb-client-10.2, > + mariadb-client-10.3, > + mariadb-client-10.4, > + mariadb-client-10.5, > + mariadb-client-10.6, > + mariadb-client-10.7, > + mariadb-client-10.8, > + mariadb-client-5.1, > + mariadb-client-5.2, > + mariadb-client-5.3, > + mariadb-client-5.5, > + mariadb-client-core (<< ${source:Version}), > + mariadb-client-core-10.0, > + mariadb-client-core-10.1, > + mariadb-client-core-10.2, > + mariadb-client-core-10.3, > + mariadb-client-core-10.4, > + mariadb-client-core-10.5, > + mariadb-client-core-10.6, > + mariadb-client-core-10.7, > + mariadb-client-core-10.8, > + mariadb-server (<< ${source:Version}), > + mariadb-server-10.0, > + mariadb-server-10.1, > + mariadb-server-10.2, > + mariadb-server-10.3, > + mariadb-server-10.4, > + mariadb-server-10.5, > + mariadb-server-10.6, > + mariadb-server-10.7, > + mariadb-server-10.8, > + mysql-client (<< 5.0.51), > + mysql-client-5.0, > + mysql-client-5.1, > + mysql-client-5.5, > + mysql-client-5.6, > + mysql-client-5.7, > + mysql-client-8.0, > + mysql-client-core-5.0, > + mysql-client-core-5.1, > + mysql-client-core-5.5, > + mysql-client-core-5.6, > + mysql-client-core-5.7, > + mysql-client-core-8.0, > + mysql-server-core-5.7, > + mysql-server-core-8.0, > + percona-server-server-5.6, > + percona-server-server, > + percona-xtradb-cluster-server-5.6, > + percona-xtradb-cluster-server-5.7, > + percona-xtradb-cluster-server > +Breaks: mariadb-client (<< ${source:Version}), > + mariadb-client-10.0, > + mariadb-client-10.1, > + mariadb-client-10.2, > + mariadb-client-10.3, > + mariadb-client-10.4, > + mariadb-client-10.5, > + mariadb-client-10.6, > + mariadb-client-10.7, > + mariadb-client-10.8, > + mariadb-client-5.1, > + mariadb-client-core (<< ${source:Version}), > + mariadb-client-core-10.0, > + mariadb-client-core-10.1, > + mariadb-client-core-10.2, > + mariadb-client-core-10.3, > + mariadb-client-core-10.4, > + mariadb-client-core-10.5, > + mariadb-client-core-10.6, > + mariadb-client-core-10.7, > + mariadb-client-core-10.8, > + mariadb-server (<< ${source:Version}), > + mariadb-server-10.0, > + mariadb-server-10.1, > + mariadb-server-10.2, > + mariadb-server-10.3, > + mariadb-server-10.4, > + mariadb-server-10.5, > + mariadb-server-10.6, > + mariadb-server-10.7, > + mariadb-server-10.8, > + mysql-server-5.5, > + mysql-server-5.6, > + mysql-server-5.7, > + mysql-server-8.0, > + mysql-server-core-5.5, > + mysql-server-core-5.6, > + mysql-server-core-5.7, > + mysql-server-core-8.0, > + percona-server-server-5.6, > + percona-server-server, > + percona-xtradb-cluster-server-5.6, > + percona-xtradb-cluster-server-5.7, > + percona-xtradb-cluster-server > + > Package: mariadb-server-core > Architecture: any > Depends: mariadb-common (>= ${source:Version}), > diff --git a/debian/mariadb-backup.install b/debian/mariadb-backup.install > index e450f8f46a0..a288377ddd0 100644 > --- a/debian/mariadb-backup.install > +++ b/debian/mariadb-backup.install > @@ -1,6 +1,4 @@ > -usr/bin/mariabackup same here. needs mariadb-backup-compat > usr/bin/mariadb-backup > usr/bin/mbstream > -usr/share/man/man1/mariabackup.1 > usr/share/man/man1/mariadb-backup.1 > usr/share/man/man1/mbstream.1 > diff --git a/debian/mariadb-client.links b/debian/mariadb-client.links > index 62e3651daf5..8ccce12c609 100644 > --- a/debian/mariadb-client.links > +++ b/debian/mariadb-client.links > @@ -2,11 +2,6 @@ usr/bin/mariadb-check usr/bin/mariadb-analyze > usr/bin/mariadb-check usr/bin/mariadb-optimize > usr/bin/mariadb-check usr/bin/mariadb-repair > usr/bin/mariadb-check usr/bin/mariadbcheck > -usr/bin/mariadb-check usr/bin/mysqlanalyze > -usr/bin/mariadb-check usr/bin/mysqlcheck > -usr/bin/mariadb-check usr/bin/mysqloptimize > -usr/bin/mariadb-check usr/bin/mysqlrepair > -usr/bin/mariadb-report usr/bin/mysqlreport > usr/share/man/man1/mariadb-check.1.gz usr/share/man/man1/mariadb-analyze.1.gz > usr/share/man/man1/mariadb-check.1.gz usr/share/man/man1/mariadb-optimize.1.gz > usr/share/man/man1/mariadb-check.1.gz usr/share/man/man1/mariadb-repair.1.gz you forgot to move symlinks to man pages > diff --git a/debian/mariadb-server-compat.install b/debian/mariadb-server-compat.install > new file mode 100644 > index 00000000000..36701d8e0c6 > --- /dev/null > +++ b/debian/mariadb-server-compat.install > @@ -0,0 +1,12 @@ > +usr/bin/mysqld_multi > +usr/bin/mysqld_safe > +usr/bin/mysqld_safe_helper > +usr/bin/mysql_install_db > +usr/bin/mysql_upgrade > +usr/sbin/mysqld > +usr/share/man/man1/mysqld_multi.1 > +usr/share/man/man1/mysqld_safe.1 > +usr/share/man/man1/mysqld_safe_helper.1 great. do you do the same for rpms? I didn't see where manpages are moved to -symlinks packages in rpms. > +usr/share/man/man1/mysql_install_db.1 > +usr/share/man/man1/mysql_upgrade.1 > +usr/share/man/man8/mysqld.8 > diff --git a/debian/mariadb-test-compat.install b/debian/mariadb-test-compat.install > new file mode 100644 > index 00000000000..7b498d7d1c8 > --- /dev/null > +++ b/debian/mariadb-test-compat.install > @@ -0,0 +1 @@ > +usr/share/mysql/mysql-test/mysql-test-run.pl You forgot mysql-test-run, mysqltest, mysql_client_test, and their *embedded variants. But, you know, it's probably not worth a new package at all. It's the *test* package (not interesting for users, never used in production, so no immutability requirements). I suggest to keep those symlinks in the mariadb-test package. Your other PR will add a warning. And eventually we drop them. Optional/Recommended/Suggested - sounds like too much toubles for a *test* package. > diff --git a/mysql-test/lib/mtr_cases.pm b/mysql-test/lib/mtr_cases.pm > index fe202279ac7..ad1ac6fdf88 100644 > --- a/mysql-test/lib/mtr_cases.pm > +++ b/mysql-test/lib/mtr_cases.pm > @@ -387,14 +387,14 @@ sub collect_suite_name($$) > else > { > my @dirs = my_find_dir(dirname($::glob_mysql_test_dir), > - ["mysql-test/suite", @plugin_suitedirs ], > + ["mariadb-test/suite", "mysql-test/suite", @plugin_suitedirs ], Eh, no. That is, yes, but not in this set of commits, not in this MDEV > $suitename); > # > # if $suitename contained wildcards, we'll have many suites and > # their overlays here. Let's group them appropriately. > # > for (@dirs) { > - m@^.*/(?:mysql-test/suite|$plugin_suitedir_regex)/(.*)$@o or confess $_; > + m@^.*/(?:mariadb-test/suite|mysql-test/suite|$plugin_suitedir_regex)/(.*)$@o or confess $_; > push @{$suites{$1}}, $_; > } > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3