December 2023 - developers - lists.mariadb.org

PR#2779: Fix merge commit 5ea5291: No test file or result files should be executable
by Otto Kekäläinen 28 Jan '24

28 Jan '24

Hi Sanja! Did you notice my PR https://github.com/MariaDB/server/pull/2779? I tagged you about a minor bug in you previous merge. I was hoping you would notice it and review that PR, but Daniel Black jumped on it. I am asking now because it seems the exact same thing happened for mysql-test/main/lowercase_table2.result in your commit fecd78b8378 from November 8th, so I thought I'd send an extra email now to ensure you are aware of this happening. - Otto

2 3

Re: 0c0721cb2f7: MDEV-31616 Problems with a stored function EMPTY() on upgrade to 10.6.
by Sergei Golubchik 22 Jan '24

22 Jan '24

Hi, Alexey, On Dec 27, Alexey Botchkov wrote: > revision-id: 0c0721cb2f7 (mariadb-10.6.16-45-g0c0721cb2f7) > parent(s): aff5ed3988a > author: Alexey Botchkov > committer: Alexey Botchkov > timestamp: 2023-12-19 00:00:48 +0400 > message: > > MDEV-31616 Problems with a stored function EMPTY() on upgrade to 10.6. > > The IDENT_sys doesn't include keywords, so the fucntion with the typo: fucntion > keyword name can be created, but cannot be called. > Moving some keywords to keyword_funcname set so the functions with these > names are allowed. The current logic in the code is: * built-in functions use IDENT_sys (no keywords allowed) in function_call_generic * functions with keyword names are explicitly listed in function_call_conflict. * stored function invocations use IDENT_sys via function_call_generic but * stored function declaration uses sp_name (that is keyword_ident) rule so, it'd make sense to introduce IDENT_funcname and use it instead of IDENT_sys in function_call_generic. That was a good idea. But also, I expect it to make function_call_conflict redundant, because all names from there should be in IDENT_funcname. And the patch doesn't solve the issue that many keywords from IDENT_sys are not in IDENT_funcname. So it's still possible to create functions that cannot be invoked. Note, that simply changing sp_name to use IDENT_funcname is not correct, because stored procedure names have different limitations. Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 2

Re: 02b85f1cf9b: MDEV-31340 Remove MY_COLLATION_HANDLER::strcasecmp()
by Sergei Golubchik 19 Jan '24

19 Jan '24

Hi, Alexander, Phew. It was a big patch. Mostly mechanical changes, admittedly, but still a lot to read. Generally I quite liked it, great work! Shorter way to write an identifier comparison, type safety that enforces namespaces and correct collations. Nice. But there are, of course, comments and questions, please, see below. On Dec 10, Alexander Barkov wrote: > commit 02b85f1cf9b > Author: Alexander Barkov <bar(a)mariadb.com> > Date: Wed Apr 26 15:27:01 2023 +0400 > > MDEV-31340 Remove MY_COLLATION_HANDLER::strcasecmp() > > - Removing the virtual function strnncoll() from MY_COLLATION_HANDLER > > - Adding a wrapper function CHARSET_INFO::streq(), to compare > two strings for equality. For now it calls strnncoll() internally. > In the future it will turn into a virtual function. why would you want it as a virtual function? What is collation-dependent there? > > - Adding new accent sensitive case insensitive collations: > - utf8mb4_general1400_as_ci > - utf8mb3_general1400_as_ci > They implements accent sensitive case insensitive comparison. > The weight of a character is equal to the code point of its > upper case variant. These collation use Unicode-14.0.0 casefolding data. > > The result of > my_charset_utf8mb3_general1400_as_ci.strcoll() > is very close to the former > my_charset_utf8mb3_general_ci.strcasecmp() > > There is only a difference in a couple dozen rare characters, because: > - the switch from "tolower" to "toupper" comparison, to make > utf8mb3_general1400_as_ci closer to utf8mb3_general_ci > - the switch from Unicode-3.0.0 to Unicode-14.0.0 > This difference should be tolarable. See the list of affected > characters in the MDEV description. > > Note, utf8mb4_general1400_as_ci correctly handles non-BMP characters! > Unlike utf8mb4_general_ci, it does not treat all BMP characters > as equal. > > - Adding classes representing names of the file based database objects: > > Lex_ident_db > Lex_ident_table > Lex_ident_trigger > > Their comparison collation depends on the underlying > file system case sensitivity and on --lower-case-table-names > and can be either my_charset_bin or my_charset_utf8mb3_general1400_as_ci. > > - Adding classes representing names of other database objects, > whose names have case insensitive comparison style, > using my_charset_utf8mb3_general1400_as_ci: > > Lex_ident_column > Lex_ident_sys_var > Lex_ident_user_var > Lex_ident_sp_var I don't think Lex_ident_sp_var is a separate domain, it's the same as Lex_ident_column. Both logically: select a from t1 where b > c (here you don't know what's a column and what's an sp var) and also in the code, in fill_spvar_definition() you yourself cast from Lex_ident_sp_var to Lex_ident_column, which kind of defeats the purpose of having two different types and emphasizes that it's actually just one and the same. > Lex_ident_ps > Lex_ident_i_s_table > Lex_ident_window > Lex_ident_func > Lex_ident_partition > Lex_ident_with_element > Lex_ident_rpl_filter > Lex_ident_master_info > Lex_ident_host > Lex_ident_locale > Lex_ident_plugin > Lex_ident_engine > Lex_ident_server > Lex_ident_savepoint > Lex_ident_table_option_name > Lex_ident_table_option_value option value is a string, not an ident > Lex_ident_charset > > - All the mentioned Lex_ident_xxx classes implement a method streq(): > > if (ident1.streq(ident2)) > do_equal(); > > This method works as a wrapper for CHARSET_INFO::streq(). > > - Changing a lot of "LEX_CSTRING name" to "Lex_ident_xxx name" > in class members and in function/method parameters. > > - Replacing all calls like > system_charset_info->coll->strcasecmp(ident1, ident2) > to > ident1.streq(ident2) > > - Taking advantage of the c++11 user defined literal operator > for Lex_ident_xxx data type. See lex_ident.h. > Use example: > > const Lex_ident_column primary_key_name= "PRIMARY"_Lex_ident_column; > > is now a shorter version of: > > const Lex_ident_column primary_key_name= > Lex_ident_column({STRING_WITH_LEN("PRIMARY")}); Of all the things in your patch this will be the first thing that Monty will remove when he'll stumble upon it. Could you instead implement a constructor that understands const Lex_ident_column primary_key_name= {STRING_WITH_LEN("PRIMARY")}; > diff --git a/include/m_ctype.h b/include/m_ctype.h > index 9d13989d1fe..e5c5145b256 100644 > --- a/include/m_ctype.h > +++ b/include/m_ctype.h > @@ -997,6 +1006,18 @@ struct charset_info_st > return state & MY_CS_COMPILED; > } > > + /* > + There will be a separate virtual function streq() in > + MY_COLLATION_HANDLER eventually. > + */ > + my_bool streq(const LEX_CSTRING a, const LEX_CSTRING b) const > + { > + DBUG_ASSERT(is_valid_comparison_operand(a)); > + DBUG_ASSERT(is_valid_comparison_operand(b)); > + return 0 == (coll->strnncoll)(this, (const uchar *) a.str, a.length, > + (const uchar *) b.str, b.length, FALSE); as you have also added strnncoll helper, you can write the above as return 0 == strnncoll(a, b); also, perhaps you should move these asserts into strnncoll helper? > + } > + > int strnncoll(const uchar *a, size_t alen, > const uchar *b, size_t blen, my_bool b_is_prefix= FALSE) const > { > diff --git a/mysql-test/main/ctype_utf8mb4_general1400_as_ci_casefold.result b/mysql-test/main/ctype_utf8mb4_general1400_as_ci_casefold.result > new file mode 100644 > index 00000000000..c6ccf662d16 > --- /dev/null > +++ b/mysql-test/main/ctype_utf8mb4_general1400_as_ci_casefold.result > @@ -0,0 +1,1446 @@ > +# > +# Start of 11.4 tests > +# > +# > +# MDEV-31340 Remove MY_COLLATION_HANDLER::strcasecmp() > +# > +SET NAMES utf8mb4 COLLATE utf8mb4_general1400_as_ci; > +EXECUTE IMMEDIATE SFORMAT(' > +CREATE VIEW v_bmp AS > +SELECT > + seq AS codepoint, > + LPAD(HEX(seq),4,''0'') AS codepoint_hex4, > + CONVERT(CHAR(seq USING utf32) USING {}) COLLATE {} AS c > +FROM > + seq_0_to_65535', @@character_set_connection, @@collation_connection); > +SELECT COLLATION(c) FROM v_bmp LIMIT 1; > +COLLATION(c) > +utf8mb4_general1400_as_ci > +SELECT > +codepoint_hex4, > +HEX(CAST(LOWER(c) AS CHAR CHARACTER SET ucs2)), > +HEX(CAST(UPPER(c) AS CHAR CHARACTER SET ucs2)) > +FROM v_bmp > +WHERE BINARY(c)<>BINARY(LOWER(c)) OR BINARY(c)<>BINARY(UPPER(c)); > +codepoint_hex4 HEX(CAST(LOWER(c) AS CHAR CHARACTER SET ucs2)) HEX(CAST(UPPER(c) AS CHAR CHARACTER SET ucs2)) what is the point of this table? detect changes when going to the next unicode version? > +0041 0061 0041 ... > +# > +# End of 11.4 tests > +# > diff --git a/mysql-test/main/ctype_utf8mb4_general1400_as_ci_ws.result b/mysql-test/main/ctype_utf8mb4_general1400_as_ci_ws.result > new file mode 100644 > index 00000000000..49f43777feb > --- /dev/null > +++ b/mysql-test/main/ctype_utf8mb4_general1400_as_ci_ws.result > diff --git a/mysql-test/main/ctype_utf8mb4_general1400_as_ci_ws.test b/mysql-test/main/ctype_utf8mb4_general1400_as_ci_ws.test > new file mode 100644 > index 00000000000..b5eb369ae9a > --- /dev/null > +++ b/mysql-test/main/ctype_utf8mb4_general1400_as_ci_ws.test > @@ -0,0 +1,39 @@ > +--source include/have_utf32.inc > +--source include/have_sequence.inc > + > +--echo # > +--echo # Start of 11.1 tests > +--echo # > + > +--echo # > +--echo # Collation utfmb4_general1400_as_ci > +--echo # > + > +SET NAMES utf8mb4 COLLATE utf8mb4_general1400_as_ci; > + > +EXECUTE IMMEDIATE SFORMAT(' > +CREATE VIEW v_bmp AS > +SELECT > + seq AS codepoint, > + LPAD(HEX(seq),6,''0'') AS codepoint_hex6, > + CONVERT(CHAR(seq USING utf32) USING {}) COLLATE {} AS c > +FROM > + seq_0_to_1114111', @@character_set_connection, @@collation_connection); > + > +SELECT COLLATION(c) FROM v_bmp LIMIT 1; > + > +SELECT > + SUM(codepoint_hex6=HEX(LOWER(c))) AS count_bmp_weight_is_lower, > + SUM(codepoint_hex6<>HEX(LOWER(c))) AS count_bmp_weight_is_not_lower > +FROM v_bmp; > + > +SELECT codepoint_hex6,HEX(WEIGHT_STRING(c)) > +FROM v_bmp > +WHERE codepoint_hex6<>HEX(WEIGHT_STRING(c)); > + > +DROP VIEW v_bmp; > + > + > +--echo # > +--echo # End of 11.1 tests 11.4 ? > +--echo # > diff --git a/mysql-test/suite/compat/oracle/r/lower_case_table_names.result b/mysql-test/suite/compat/oracle/r/lower_case_table_names.result > new file mode 100644 > index 00000000000..2518be31402 > --- /dev/null > +++ b/mysql-test/suite/compat/oracle/r/lower_case_table_names.result > @@ -0,0 +1,9 @@ > +SELECT @@lower_case_table_names; > +@@lower_case_table_names > +1 > +# > +# TODO: Report this problem as MDEV what is the problem? isn't the comparison accent-sensitive? > +# > +SET NAMES utf8; > +CREATE TABLE t1 (a öracle_schema.date); > +ERROR HY000: Unknown data type: 'öracle_schema.date' > diff --git a/mysql-test/suite/unit/disabled.def b/mysql-test/suite/unit/disabled.def > new file mode 100644 > index 00000000000..3a68bea32d0 > --- /dev/null > +++ b/mysql-test/suite/unit/disabled.def > @@ -0,0 +1,12 @@ > +############################################################################## > +# > +# List the test cases that are to be disabled temporarily. > +# > +# Separate the test case name and the comment with ':'. > +# > +# <testcasename> : BUG#<xxxx> <date disabled> <disabler> <comment> > +# > +# Do not use any TAB characters for whitespace. > +# > +############################################################################## > +conc_charset : TODO: client side ID for utf8mb4_tolower_ci 1. There is no utf8mb4_tolower_ci 2. what do you mean "client side ID"? > diff --git a/sql/ddl_log.cc b/sql/ddl_log.cc > index 61c8f50d1e3..fdfb605a57f 100644 > --- a/sql/ddl_log.cc > +++ b/sql/ddl_log.cc > @@ -1146,22 +1146,23 @@ static void execute_rename_table(DDL_LOG_ENTRY *ddl_log_entry, handler *file, > static void rename_triggers(THD *thd, DDL_LOG_ENTRY *ddl_log_entry, > bool swap_tables) > { > - LEX_CSTRING to_table, from_table, to_db, from_db, from_converted_name; > + Lex_ident_table to_table, from_table, from_converted_name; > + Lex_ident_db to_db, from_db; > char to_path[FN_REFLEN+1], from_path[FN_REFLEN+1], conv_path[FN_REFLEN+1]; > > if (!swap_tables) > { > - from_db= ddl_log_entry->db; > - from_table= ddl_log_entry->name; > - to_db= ddl_log_entry->from_db; > - to_table= ddl_log_entry->from_name; > + from_db= Lex_ident_db(ddl_log_entry->db); this would look better with implicit casting, that is, if you'd have an assignment operator from LEX_CSTRING > + from_table= Lex_ident_table(ddl_log_entry->name); > + to_db= Lex_ident_db(ddl_log_entry->from_db); > + to_table= Lex_ident_table(ddl_log_entry->from_name); > } > else > { > - from_db= ddl_log_entry->from_db; > - from_table= ddl_log_entry->from_name; > - to_db= ddl_log_entry->db; > - to_table= ddl_log_entry->extra_name; > + from_db= Lex_ident_db(ddl_log_entry->from_db); > + from_table= Lex_ident_table(ddl_log_entry->from_name); > + to_db= Lex_ident_db(ddl_log_entry->db); > + to_table= Lex_ident_table(ddl_log_entry->extra_name); > } > > build_filename_and_delete_tmp_file(from_path, sizeof(from_path), > @@ -1211,11 +1212,11 @@ static void rename_triggers(THD *thd, DDL_LOG_ENTRY *ddl_log_entry, > > (void) Table_triggers_list::prepare_for_rename(thd, > &trigger_param, > - &from_db, > - &from_table, > - &from_converted_name, > - &to_db, > - &to_table); > + from_db, > + from_table, > + from_converted_name, > + to_db, > + to_table); You rather inconsistently replace pointers with const references in some methods, like above, but not in others, like below or in the next hunk. > (void) Table_triggers_list::change_table_name(thd, > &trigger_param, > &from_db, > @@ -1707,9 +1708,8 @@ static int ddl_log_execute_action(THD *thd, MEM_ROOT *mem_root, > > switch (ddl_log_entry->phase) { > case DDL_DROP_DB_PHASE_INIT: > - drop_database_objects(thd, &path, &db, > - !my_strcasecmp(system_charset_info, > - MYSQL_SCHEMA_NAME.str, db.str)); > + // QQ: was a bug here? This comment makes little sense in the code, as one cannot see what *was* here before. Just drop it. > + drop_database_objects(thd, &path, &db, MYSQL_SCHEMA_NAME.streq(db)); > > strxnmov(to_path, sizeof(to_path)-1, path.str, MY_DB_OPT_FILE, NullS); > mysql_file_delete_with_symlink(key_file_misc, to_path, "", MYF(0)); > diff --git a/sql/debug_sync.cc b/sql/debug_sync.cc > index 362463a7f22..4704691615a 100644 > --- a/sql/debug_sync.cc > +++ b/sql/debug_sync.cc > @@ -858,6 +858,13 @@ static st_debug_sync_action *debug_sync_get_action(THD *thd, > } > > > +class Debug_token: public Lex_ident_ci > +{ > +public: > + using Lex_ident_ci::Lex_ident_ci; > +}; I wonder if you can create those types with less boilerplate will typedef work? will using work? if not, may be, like template<> Lex_ident_templ: Lex_ident_ci { ... } Lex_ident_templ<> Debug_token; > + > + > /** > Set a debug sync action. > > @@ -909,7 +916,8 @@ static bool debug_sync_set_action(THD *thd, st_debug_sync_action *action) > } > else > { > - const char *dsp_name= action->sync_point.c_ptr(); > + const Debug_token dsp_name(action->sync_point.c_ptr(), > + action->sync_point.length()); or dsp_name(action->sync_point->to_lex_cstring()) ? > #ifdef DBUG_TRACE > DBUG_EXECUTE("debug_sync", { > /* Functions as DBUG_PRINT args can change keyword and line nr. */ > diff --git a/sql/grant.cc b/sql/grant.cc > index 1ba197bc1d9..0ebf47431bc 100644 > --- a/sql/grant.cc > +++ b/sql/grant.cc > @@ -31,8 +31,8 @@ bool Grant_privilege::add_column_privilege(THD *thd, > class LEX_COLUMN *point; > while ((point=iter++)) > { > - if (!my_strcasecmp(system_charset_info, > - point->column.c_ptr(), new_str->c_ptr())) > + const Lex_ident_column tmp(point->column.c_ptr(), point->column.length()); That feels like an overkill, creating Lex_ident_column just to compare two strings. What is it compiled to at -O3? > + if (tmp.streq(new_str->to_lex_cstring())) > break; > } > m_column_privilege_total|= which_grant; > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc > index a615d26b9a9..fc840c21191 100644 > --- a/sql/ha_partition.cc > +++ b/sql/ha_partition.cc > @@ -118,7 +118,7 @@ int ha_partition::notify_tabledef_changed(LEX_CSTRING *db, > LEX_CSTRING table_name; > const char *table_name_ptr; > if (create_partition_name(from_buff, sizeof(from_buff), > - from_path, name_buffer_ptr, > + from_path, Lex_cstring_strlen(name_buffer_ptr), Don't, please. You _always_ use Lex_cstring_strlen, in _all_ invocations of create_partition_name(). Just pass a char* down as before and do strlen inside, if you need it. > NORMAL_PART_NAME, FALSE)) > res=1; > table_name_ptr= from_buff + dirname_length(from_buff); > diff --git a/sql/handler.cc b/sql/handler.cc > index a977054d0da..7b9d11d00a2 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -7531,7 +7528,8 @@ int handler::check_duplicate_long_entry_key(const uchar *new_rec, uint key_no) > else > { > Item_func_left *fnc= static_cast<Item_func_left *>(arguments[j]); > - DBUG_ASSERT(!my_strcasecmp(system_charset_info, "left", fnc->func_name())); > + DBUG_ASSERT(Lex_ident_func(fnc->func_name_cstring()). eh, why func_name_cstring() doesn't return Lex_ident_func? > + streq("left"_Lex_cstring)); > DBUG_ASSERT(fnc->arguments()[0]->type() == Item::FIELD_ITEM); > t_field= static_cast<Item_field *>(fnc->arguments()[0])->field; > uint length= (uint)fnc->arguments()[1]->val_int(); > diff --git a/sql/item.cc b/sql/item.cc > index 5c4f1704ebe..5689756eca9 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -3463,13 +3463,11 @@ bool Item_field::eq(const Item *item, bool binary_cmp) const > (In cases where we would choose wrong we would have to generate a > ER_NON_UNIQ_ERROR). > */ > - return (!lex_string_cmp(system_charset_info, &item_field->name, > - &field_name) && > - (!item_field->table_name.str || !table_name.str || > - (!my_strcasecmp(table_alias_charset, item_field->table_name.str, > - table_name.str) && > - (!item_field->db_name.str || !db_name.str || > - (item_field->db_name.str && !strcmp(item_field->db_name.str, > + return (item_field->name.streq(field_name) && > + (!item_field->table_name.str || !table_name.str || > + (item_field->table_name.streq(table_name) && > + (!item_field->db_name.str || !db_name.str || > + (item_field->db_name.str && !strcmp(item_field->db_name.str, > db_name.str)))))); I presume, # db_name should also be compared with streq > } > > diff --git a/sql/item_create.cc b/sql/item_create.cc > index 4f0df8f732d..501db19c1b8 100644 > --- a/sql/item_create.cc > +++ b/sql/item_create.cc > @@ -2638,7 +2639,8 @@ Create_qfunc::create_func(THD *thd, const LEX_CSTRING *name, > if (thd->lex->copy_db_to(&db)) > return NULL; > > - return create_with_db(thd, &db, name, false, item_list); Again, can LEX_CSTRING be implicitly cast to the correct Lex_ident variant? > + return create_with_db(thd, Lex_ident_db(db), Lex_ident_func(*name), > + false, item_list); > } > > > diff --git a/sql/item_windowfunc.cc b/sql/item_windowfunc.cc > index 5438cade27a..f0a5737bdf7 100644 > --- a/sql/item_windowfunc.cc > +++ b/sql/item_windowfunc.cc > @@ -28,7 +28,7 @@ Item_window_func::resolve_window_name(THD *thd) > return false; > } > DBUG_ASSERT(window_name != NULL && window_spec == NULL); > - const char *ref_name= window_name->str; > + const LEX_CSTRING &ref_name= *window_name; Why LEX_CSTRING ? > > /* !TODO: Add the code to resolve ref_name in outer queries */ > /* > diff --git a/sql/opt_trace.cc b/sql/opt_trace.cc > index 1008690ccaa..dbb2708f487 100644 > --- a/sql/opt_trace.cc > +++ b/sql/opt_trace.cc > @@ -43,7 +43,7 @@ bool list_has_optimizer_trace_table(const TABLE_LIST *tbl) > for (; tbl; tbl= tbl->next_global) > { > if (tbl->schema_table && > - 0 == strcmp(tbl->schema_table->table_name, I_S_table_name)) > + 0 == strcmp(tbl->schema_table->table_name.str, I_S_table_name)) Why not streq? > return true; > } > return false; > @@ -190,9 +190,8 @@ void opt_trace_disable_if_no_security_context_access(THD *thd) > if (!(thd->main_security_ctx.check_access(GLOBAL_ACLS & ~GRANT_ACL)) && > (0 != strcmp(thd->main_security_ctx.priv_user, > thd->security_context()->priv_user) || > - 0 != my_strcasecmp(system_charset_info, > - thd->main_security_ctx.priv_host, > - thd->security_context()->priv_host))) > + !Lex_ident_host(Lex_cstring_strlen(thd->main_security_ctx.priv_host)). > + streq(Lex_cstring_strlen(thd->security_context()->priv_host)))) Doesn't really look like an improvement :) May be you can get rid of intermediate Lex_cstring_strlen()'s ? > trace->missing_privilege(); > } > > diff --git a/sql/partition_info.cc b/sql/partition_info.cc > index 1b65de6e3c0..7b76c6d65b1 100644 > --- a/sql/partition_info.cc > +++ b/sql/partition_info.cc > @@ -1403,8 +1397,8 @@ bool partition_info::check_partition_info(THD *thd, handlerton **eng_type, > num_parts_not_set++; > part_elem->engine_type= default_engine_type; > } > - if (check_table_name(part_elem->partition_name, > - strlen(part_elem->partition_name), FALSE)) > + if (check_table_name(part_elem->partition_name.str, > + part_elem->partition_name.length, FALSE)) better change check_table_name to accept a LEX_CSTRING or Lex_ident, it'll simplify almost all (except one) invocations of it > { > my_error(ER_WRONG_PARTITION_NAME, MYF(0)); > goto end; > @@ -2604,8 +2596,8 @@ bool partition_info::has_same_partitioning(partition_info *new_part_info) > part_state must be PART_NORMAL! > */ > if (!part_elem || !new_part_elem || > - strcmp(part_elem->partition_name, > - new_part_elem->partition_name) || > + strcmp(part_elem->partition_name.str, > + new_part_elem->partition_name.str) || why strcmp? > part_elem->part_state != PART_NORMAL || > new_part_elem->part_state != PART_NORMAL || > part_elem->max_value != new_part_elem->max_value || > @@ -2666,8 +2658,8 @@ bool partition_info::has_same_partitioning(partition_info *new_part_info) > sub_part_elem->engine_type != new_sub_part_elem->engine_type) > DBUG_RETURN(false); > > - if (strcmp(sub_part_elem->partition_name, > - new_sub_part_elem->partition_name) || > + if (strcmp(sub_part_elem->partition_name.str, > + new_sub_part_elem->partition_name.str) || same here > sub_part_elem->part_state != PART_NORMAL || > new_sub_part_elem->part_state != PART_NORMAL || > sub_part_elem->part_min_rows != > diff --git a/sql/partition_info.h b/sql/partition_info.h > index 3a8c3a37524..92e8906274f 100644 > --- a/sql/partition_info.h > +++ b/sql/partition_info.h > @@ -526,11 +527,13 @@ void partition_info::vers_update_el_ids() > } > > > -inline > -bool make_partition_name(char *move_ptr, uint i) > +static inline > +Lex_ident_partition make_partition_name_ls(char *move_ptr, uint i) it's not _ls really, is it? why not to keep the old name? > { > int res= snprintf(move_ptr, MAX_PART_NAME_SIZE + 1, "p%u", i); > - return res < 0 || res > MAX_PART_NAME_SIZE; > + return res < 0 || res > MAX_PART_NAME_SIZE ? > + Lex_ident_partition() : > + Lex_ident_partition(move_ptr, (size_t) res); > } > > > @@ -549,15 +552,16 @@ uint partition_info::next_part_no(uint new_parts) const > for (uint cur_part= 0; cur_part < new_parts; ++cur_part, ++suffix) > { > uint32 cur_suffix= suffix; > - if (make_partition_name(part_name, suffix)) > + Lex_ident_partition part_name_ls(make_partition_name_ls(part_name, suffix)); > + if (!part_name_ls.str) > return 0; > partition_element *el; > it.rewind(); > while ((el= it++)) > { > - if (0 == my_strcasecmp(&my_charset_latin1, el->partition_name, part_name)) > + if (el->partition_name.streq(part_name_ls)) > { > - if (make_partition_name(part_name, ++suffix)) > + if (!(part_name_ls= make_partition_name_ls(part_name, ++suffix)).str) you don't seem to use part_name_ls below > return 0; > it.rewind(); > } > diff --git a/sql/set_var.h b/sql/set_var.h > index aed4955ef62..8f7e3609f5f 100644 > --- a/sql/set_var.h > +++ b/sql/set_var.h > @@ -383,7 +383,7 @@ class set_var_default_role: public set_var_base > { > LEX_USER *user, *real_user; > LEX_CSTRING role; > - const char *real_role; > + LEX_CSTRING real_role; Not Lex_ident_user ? > public: > set_var_default_role(LEX_USER *user_arg, LEX_CSTRING role_arg) : > user(user_arg), role(role_arg) {} > diff --git a/sql/slave.cc b/sql/slave.cc > index 0a15cc55146..daeb05f423e 100644 > --- a/sql/slave.cc > +++ b/sql/slave.cc > @@ -3403,8 +3403,9 @@ static bool send_show_master_info_data(THD *thd, Master_info *mi, bool full, > static int cmp_mi_by_name(const Master_info **arg1, > const Master_info **arg2) > { > - return my_strcasecmp(system_charset_info, (*arg1)->connection_name.str, > - (*arg2)->connection_name.str); > + return Lex_ident_master_info::charset_info()->strnncoll( > + (*arg1)->connection_name, > + (*arg2)->connection_name); Why not (*arg1)->connection_name.streq() ? > } > > > diff --git a/sql/sp_pcontext.cc b/sql/sp_pcontext.cc > index 848d1f0c655..761f535832d 100644 > --- a/sql/sp_pcontext.cc > +++ b/sql/sp_pcontext.cc > @@ -139,8 +139,7 @@ sp_pcontext *sp_pcontext::push_context(THD *thd, sp_pcontext::enum_scope scope) > > bool cmp_labels(sp_label *a, sp_label *b) > { > - return (lex_string_cmp(system_charset_info, &a->name, &b->name) == 0 && > - a->type == b->type); > + return a->name.streq(b->name) && a->type == b->type; put type comparison first, please > } > > sp_pcontext *sp_pcontext::pop_context() > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 09af5523c76..dd36504ed56 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -567,14 +560,17 @@ static uchar* acl_role_get_key(ACL_ROLE *entry, size_t *length, > > struct ROLE_GRANT_PAIR : public Sql_alloc > { > - char *u_uname; > - char *u_hname; > - char *r_uname; > + LEX_CSTRING u_uname; > + LEX_CSTRING u_hname; > + LEX_CSTRING r_uname; Lex_ident_user ? (and in many places below) > LEX_STRING hashkey; > bool with_admin; > > - bool init(MEM_ROOT *mem, const char *username, const char *hostname, > - const char *rolename, bool with_admin_option); > + bool init(MEM_ROOT *mem, > + const LEX_CSTRING &username, > + const LEX_CSTRING &hostname, > + const LEX_CSTRING &rolename, > + bool with_admin_option); > }; > > static uchar* acl_role_map_get_key(ROLE_GRANT_PAIR *entry, size_t *length, > @@ -2731,7 +2733,7 @@ static bool acl_load(THD *thd, const Grant_tables& tables) > char *db_name; > db.user=safe_str(get_field(&acl_memroot, db_table.user())); > const char *hostname= get_field(&acl_memroot, db_table.host()); > - if (!hostname && find_acl_role(db.user, true)) > + if (!hostname && find_acl_role(Lex_cstring_strlen(db.user), true)) Perhaps it'd be better to change user and db to be Lex_ident_* in ACL_DB? > hostname= ""; > update_hostname(&db.host, hostname); > db.db= db_name= get_field(&acl_memroot, db_table.db()); > @@ -2822,14 +2824,29 @@ static bool acl_load(THD *thd, const Grant_tables& tables) > init_alloc_root(key_memory_acl_mem, &temp_root, ACL_ALLOC_BLOCK_SIZE, 0, MYF(0)); > while (!(read_record_info.read_record())) > { > - char *hostname= safe_str(get_field(&temp_root, roles_mapping_table.host())); > - char *username= safe_str(get_field(&temp_root, roles_mapping_table.user())); > - char *rolename= safe_str(get_field(&temp_root, roles_mapping_table.role())); > + StringBuffer<MAX_FIELD_WIDTH> host_buff, user_buff, role_buff; > + /* > + find_user_by_name() called recursively later in this code block There's no find_user_by_name() and acl_find_user_by_name does not seem to be called recursively. What did you mean here? also, why you didn't change acl_find_user_by_name to take a Lex_ident? > + needs a 0-terminated string. So does sql_print_error(). > + As val_lex_cstring() does not guarantee a 0-terminated string, > + let's alloc 0-terminated copies of val_lex_cstring() results on > + the memory root. you've already added Field::val_lex_string_strmake() in August, why not to use it here? > + */ > + const Lex_cstring hostname= safe_lexcstrdup_root(&temp_root, > + roles_mapping_table.host()-> > + val_lex_cstring(&host_buff)); > + const Lex_cstring username= safe_lexcstrdup_root(&temp_root, > + roles_mapping_table.user()-> > + val_lex_cstring(&user_buff)); > + const Lex_cstring rolename= safe_lexcstrdup_root(&temp_root, > + roles_mapping_table.role()-> > + val_lex_cstring(&role_buff)); > + > bool with_grant_option= get_YN_as_bool(roles_mapping_table.admin_option()); > > if (add_role_user_mapping(username, hostname, rolename)) { > sql_print_error("Invalid roles_mapping table entry user:'%s@%s', rolename:'%s'", > - username, hostname, rolename); > + username.str, hostname.str, rolename.str); > continue; > } > > @@ -4443,28 +4484,31 @@ static ACL_USER * find_user_wild(const char *host, const char *user, const char > /* > Find a role with the specified name > */ > -static ACL_ROLE *find_acl_role(const char *role, bool allow_public) > +static ACL_ROLE *find_acl_role(const LEX_CSTRING &role, bool allow_public) > { > - size_t length= strlen(role); > DBUG_ENTER("find_acl_role"); > - DBUG_PRINT("enter",("role: '%s'", role)); > + DBUG_PRINT("enter",("role: '%s'", role.str)); > DBUG_PRINT("info", ("Hash elements: %ld", acl_roles.records)); > > mysql_mutex_assert_owner(&acl_cache->lock); > > - if (!length || (!allow_public && strcasecmp(role, public_name.str) == 0)) > + if (!role.length || > + (!allow_public && > + my_charset_utf8mb3_general1400_as_ci.streq(role, public_name))) Lex_ident_user would've been nice here too > DBUG_RETURN(NULL); > > - ACL_ROLE *r= (ACL_ROLE *)my_hash_search(&acl_roles, (uchar *)role, length); > + ACL_ROLE *r= (ACL_ROLE *)my_hash_search(&acl_roles, (uchar *)role.str, > + role.length); > DBUG_RETURN(r); > } > > /* > Finds a grantee - something that privileges or roles can be granted to. > */ > -static ACL_USER_BASE *find_acl_user_base(const char *user, const char *host) > +static ACL_USER_BASE *find_acl_user_base(const LEX_CSTRING &user, > + const LEX_CSTRING &host) > { > - if (*host) > + if (*host.str) > return find_user_exact(host, user); > > return find_acl_role(user, true); > @@ -5657,8 +5696,8 @@ static GRANT_NAME *name_hash_search(HASH *name_hash, > { > if (!grant_name->host.hostname || > (host && > - !my_strcasecmp(system_charset_info, host, > - grant_name->host.hostname)) || > + Lex_ident_host(Lex_cstring_strlen(host)). Still don't like these Lex_ident_something(Lex_cstring_strlen(xxx)) Let's get rid of them > + streq(Lex_cstring_strlen(grant_name->host.hostname))) || > (ip && !strcmp(ip, grant_name->host.hostname))) > return grant_name; > } > @@ -7373,8 +7414,9 @@ int mysql_table_grant(THD *thd, TABLE_LIST *table_list, > } > } > if (Str->is_role()) > - propagate_role_grants(find_acl_role(Str->user.str, true), > - PRIVS_TO_MERGE::TABLE_COLUMN, db_name, table_name); > + propagate_role_grants(find_acl_role(Str->user, true), > + PRIVS_TO_MERGE::TABLE_COLUMN, > + db_name.str, table_name.str); may be pass Lex_ident_* down into propagate_role_grants too? > } > > thd->mem_root= old_root; > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index 9d2415d3a4d..d7d7ebee024 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -233,33 +233,41 @@ uint get_table_def_key(const TABLE_LIST *table_list, const char **key) > # Pointer to list of names of open tables. > */ > > -struct list_open_tables_arg > +class list_open_tables_arg > { > +public: > THD *thd; > - const char *db; > + const Lex_ident_db db; > const char *wild; > TABLE_LIST table_list; > OPEN_TABLE_LIST **start_list, *open_list; > + > + list_open_tables_arg(THD *thd_arg, const LEX_CSTRING &db_arg, > + const char *wild_arg) > + :thd(thd_arg), db(db_arg), wild(wild_arg), > + start_list(&open_list), open_list(0) > + { > + bzero((char*) &table_list, sizeof(table_list)); > + } > }; > > > static my_bool list_open_tables_callback(TDC_element *element, > list_open_tables_arg *arg) > { > - const char *db= (char*) element->m_key; > - size_t db_length= strlen(db); > - const char *table_name= db + db_length + 1; > + const Lex_ident_db > + db= Lex_ident_db(Lex_cstring_strlen((const char*) element->m_key)); > + const char *table_name= db.str + db.length + 1; > > - if (arg->db && my_strcasecmp(system_charset_info, arg->db, db)) > + // QQ: a bug was here? same as above, "was" > + if (arg->db.str && !arg->db.streq(db)) > return FALSE; > if (arg->wild && wild_compare(table_name, arg->wild, 0)) > return FALSE; > > /* Check if user has SELECT privilege for any column in the table */ > - arg->table_list.db.str= db; > - arg->table_list.db.length= db_length; > - arg->table_list.table_name.str= table_name; > - arg->table_list.table_name.length= strlen(table_name); > + arg->table_list.db= db; > + arg->table_list.table_name= Lex_cstring_strlen(table_name); > arg->table_list.grant.privilege= NO_ACL; > > if (check_table_access(arg->thd, SELECT_ACL, &arg->table_list, TRUE, 1, TRUE)) > @@ -1906,7 +1904,7 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx) > DBUG_ASSERT(!table_list->table); > > /* an open table operation needs a lot of the stack space */ > - if (check_stack_overrun(thd, STACK_MIN_SIZE_FOR_OPEN, (uchar *)&alias)) > + if (check_stack_overrun(thd, STACK_MIN_SIZE_FOR_OPEN, (uchar *)&alias.str)) better pass `key` above. doesn't matter much but looks a bit less weird > DBUG_RETURN(TRUE); > > if (!(flags & MYSQL_OPEN_IGNORE_KILLED) && thd->killed) > @@ -1954,7 +1952,10 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx) > if (table->s->table_cache_key.length == key_length && > !memcmp(table->s->table_cache_key.str, key, key_length)) > { > - if (!my_strcasecmp(system_charset_info, table->alias.c_ptr(), alias) && > + // QQ: Why table_alias_charset is not used for comparison? Did you try to correct this? > + if (my_charset_utf8mb3_general1400_as_ci.streq( > + table->alias.to_lex_cstring(), > + alias) && > table->query_id != thd->query_id && /* skip tables already used */ > (thd->locked_tables_mode == LTM_LOCK_TABLES || > table->query_id == 0)) > @@ -6353,26 +6355,25 @@ find_field_in_natural_join(THD *thd, TABLE_LIST *table_ref, const char *name, si > */ > > Field * > -find_field_in_table(THD *thd, TABLE *table, const char *name, size_t length, > +find_field_in_table(THD *thd, TABLE *table, const Lex_ident_column &name, > bool allow_rowid, field_index_t *cached_field_index_ptr) > { > Field *field; > field_index_t cached_field_index= *cached_field_index_ptr; > DBUG_ENTER("find_field_in_table"); > DBUG_PRINT("enter", ("table: '%s', field name: '%s'", table->alias.c_ptr(), > - name)); > + name.str)); > > /* We assume here that table->field < NO_CACHED_FIELD_INDEX = UINT_MAX */ > if (cached_field_index < table->s->fields && > - !my_strcasecmp(system_charset_info, > - table->field[cached_field_index]->field_name.str, name)) > + table->field[cached_field_index]->field_name.streq(name)) > { > field= table->field[cached_field_index]; > DEBUG_SYNC(thd, "table_field_cached"); > } > else > { > - LEX_CSTRING fname= {name, length}; > + LEX_CSTRING fname= name; Do you need that? You can use `name` directly below > field= table->find_field_by_name(&fname); > } > > @@ -6999,9 +7000,9 @@ find_item_in_list(Item *find, List<Item> &items, uint *counter, > List_iterator<Item> li(items); > uint n_items= limit == 0 ? items.elements : limit; > Item **found=0, **found_unaliased= 0, *item; > - const char *db_name=0; > + const LEX_CSTRING *db_name=0; > const LEX_CSTRING *field_name= 0; > - const char *table_name=0; > + const LEX_CSTRING *table_name=0; Why not Lex_ident's ? > bool found_unaliased_non_uniq= 0; > /* > true if the item that we search for is a valid name reference > @@ -7059,12 +7060,11 @@ find_item_in_list(Item *find, List<Item> &items, uint *counter, > item is not fix_field()'ed yet. > */ > if (item_field->field_name.str && item_field->table_name.str && > - !lex_string_cmp(system_charset_info, &item_field->field_name, > - field_name) && > - !my_strcasecmp(table_alias_charset, item_field->table_name.str, > - table_name) && > - (!db_name || (item_field->db_name.str && > - !strcmp(item_field->db_name.str, db_name)))) > + item_field->field_name.streq(*field_name) && > + item_field->table_name.streq(*table_name) && > + (!db_name || !db_name->str || > + (item_field->db_name.str && > + !strcmp(item_field->db_name.str, db_name->str)))) why strcmp? > { > if (found_unaliased) > { > @@ -7135,11 +7132,10 @@ find_item_in_list(Item *find, List<Item> &items, uint *counter, > } > } > } > - else if (!table_name) > + else if (!table_name || !table_name->str) > { > if (is_ref_by_name && find->name.str && item->name.str && > - find->name.length == item->name.length && > - !lex_string_cmp(system_charset_info, &item->name, &find->name)) > + item->name.streq(find->name)) Do you have tests that will fail if streq will compare lengths? > { > found= li.ref(); > *counter= i; > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index a2414de645b..1b36cd9416f 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -855,10 +853,13 @@ THD::THD(my_thread_id id, bool is_wsrep_applier) > profiling.set_thd(this); > #endif > user_connect=(USER_CONN *)0; > - my_hash_init(key_memory_user_var_entry, &user_vars, system_charset_info, > + my_hash_init(key_memory_user_var_entry, &user_vars, > + Lex_ident_user_var::charset_info(), > USER_VARS_HASH_SIZE, 0, 0, (my_hash_get_key) get_var_key, > (my_hash_free_key) free_user_var, HASH_THREAD_SPECIFIC); > - my_hash_init(PSI_INSTRUMENT_ME, &sequences, system_charset_info, > + my_hash_init(PSI_INSTRUMENT_ME, &sequences, > + // QQ: why case insensitive? looks like a bug, can you do a test case for it, please? > + &my_charset_utf8mb3_general1400_as_ci, > SEQUENCES_HASH_SIZE, 0, 0, (my_hash_get_key) > get_sequence_last_key, (my_hash_free_key) free_sequence_last, > HASH_THREAD_SPECIFIC); > @@ -4011,7 +4015,7 @@ Table_ident::to_ident_db_internal_with_error(Query_arena *arena) const > if (is_derived_table()) > { > DBUG_ASSERT(db.str == empty_c_string && db.length == 0); > - return Lex_ident_db(empty_c_string, 0); > + return Lex_ident_db(empty_c_string, (size_t) 0); why? > } > // Normal table or JSON table > return arena->to_ident_db_internal_with_error(db); > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 214118d8431..5685240ceaf 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -433,13 +435,14 @@ class Alter_rename_key : public Sql_alloc > class Alter_index_ignorability: public Sql_alloc > { > public: > - Alter_index_ignorability(const char *name, bool is_ignored, bool if_exists) : > + Alter_index_ignorability(const LEX_CSTRING &name, > + bool is_ignored, bool if_exists) : > m_name(name), m_is_ignored(is_ignored), m_if_exists(if_exists) > { > - assert(name != NULL); > + assert(name.str != NULL); DBUG_ASSERT > } > > - const char *name() const { return m_name; } > + const Lex_ident_column &name() const { return m_name; } > bool if_exists() const { return m_if_exists; } > > /* The ignorability after the operation is performed. */ > @@ -7989,42 +7995,24 @@ class Identifier_chain2 > class Database_qualified_name > { > public: > - LEX_CSTRING m_db; > - LEX_CSTRING m_name; > - Database_qualified_name(const LEX_CSTRING *db, const LEX_CSTRING *name) > - :m_db(*db), m_name(*name) > + Lex_ident_db m_db; > + Lex_cstring m_name; // no comparison semantics > + Database_qualified_name() > { } > - Database_qualified_name(const LEX_CSTRING &db, const LEX_CSTRING &name) > + Database_qualified_name(const Lex_ident_db &db, const LEX_CSTRING &name) > :m_db(db), m_name(name) > { } > - Database_qualified_name(const char *db, size_t db_length, > - const char *name, size_t name_length) > - { > - m_db.str= db; > - m_db.length= db_length; > - m_name.str= name; > - m_name.length= name_length; > - } > - > - bool eq(const Database_qualified_name *other) const > + bool eq_routine_name(const Database_qualified_name *other) const eq_func_name, because the comparison semantics comes from Lex_ident_func > { > - CHARSET_INFO *cs= lower_case_table_names ? > - &my_charset_utf8mb3_general_ci : > - &my_charset_utf8mb3_bin; > - return > - m_db.length == other->m_db.length && > - m_name.length == other->m_name.length && > - !cs->strnncoll(m_db.str, m_db.length, > - other->m_db.str, other->m_db.length) && > - !cs->strnncoll(m_name.str, m_name.length, > - other->m_name.str, other->m_name.length); > + return m_db.streq(other->m_db) && no need to take lower_case_table_names into account when comparing db names here? > + Lex_ident_func(m_name).streq(other->m_name); > } > /* > Make copies of "db" and "name" on the memory root in internal format: > - Lower-case "db" if lower-case-table-names==1. > - Preserve "name" as is. > */ > - bool copy_sp_name_internal(MEM_ROOT *mem_root, const LEX_CSTRING &db, > + bool copy_sp_name_internal(MEM_ROOT *mem_root, const Lex_ident_db &db, > const LEX_CSTRING &name); > > // Export db and name as a qualified name string: 'db.name' > diff --git a/sql/sql_connect.cc b/sql/sql_connect.cc > index c5619b28f69..4e10f498e3c 100644 > --- a/sql/sql_connect.cc > +++ b/sql/sql_connect.cc > @@ -509,7 +514,9 @@ extern "C" void free_table_stats(TABLE_STATS* table_stats) > > void init_global_table_stats(void) > { > - my_hash_init(PSI_INSTRUMENT_ME, &global_table_stats, system_charset_info, > + my_hash_init(PSI_INSTRUMENT_ME, &global_table_stats, > + // QQ: why not Lex_ident_table::charset_info() ? just change it > + &my_charset_utf8mb3_general1400_as_ci, > max_connections, 0, 0, (my_hash_get_key) get_key_table_stats, > (my_hash_free_key) free_table_stats, 0); > } > diff --git a/sql/sql_db.cc b/sql/sql_db.cc > index 08441d9f067..51635bdf0ce 100644 > --- a/sql/sql_db.cc > +++ b/sql/sql_db.cc > @@ -98,12 +98,10 @@ typedef struct my_dbopt_st > */ > > static inline bool > -cmp_db_names(LEX_CSTRING *db1_name, const LEX_CSTRING *db2_name) > +cmp_db_names(const Lex_ident_db &db1_name, const Lex_ident_db &db2_name) > { > - return (db1_name->length == db2_name->length && > - (db1_name->length == 0 || can it be used with empty names, really? > - my_strcasecmp(table_alias_charset, > - db1_name->str, db2_name->str) == 0)); > + return (db1_name.length == 0 && db2_name.length == 0) || > + db1_name.streq(db2_name); > } > > #ifdef HAVE_PSI_INTERFACE > @@ -1087,8 +1085,7 @@ mysql_rm_db_internal(THD *thd, const Lex_ident_db &db, bool if_exists, > Disable drop of enabled log tables, must be done before name locking. > This check is only needed if we are dropping the "mysql" database. > */ > - if ((rm_mysql_schema= > - (my_strcasecmp(system_charset_info, MYSQL_SCHEMA_NAME.str, db.str) == 0))) > + if ((rm_mysql_schema= MYSQL_SCHEMA_NAME.streq(db))) // QQ: was a bug here? again, "was" > { > for (table= tables; table; table= table->next_local) > if (check_if_log_table(table, TRUE, "DROP")) > diff --git a/sql/sql_handler.cc b/sql/sql_handler.cc > index 406f67ff955..def1fc44392 100644 > --- a/sql/sql_handler.cc > +++ b/sql/sql_handler.cc > @@ -1080,11 +1079,14 @@ static SQL_HANDLER *mysql_ha_find_match(THD *thd, TABLE_LIST *tables) > { > if (tables->is_anonymous_derived_table()) > continue; > + /* > + TODO: for some reasons the code user case-insensitive comparison. > + Shouldn't it use Lex_ident_db/Lex_ident_table instead, > + to take into account the filesystem case sensitivity? > + */ must be a bug. try to create a test case for it. I think it could be something like (untested) create table t1 ...; create table T1; handler t1 open; handler t1 read first; handler t1 read next; drop table T1; handler t1 read next; > if ((! tables->db.str[0] || > - ! my_strcasecmp(&my_charset_latin1, hash_tables->db.str, > - tables->get_db_name())) && > - ! my_strcasecmp(&my_charset_latin1, hash_tables->table_name.str, > - tables->get_table_name())) > + Lex_ident_ci(hash_tables->db).streq(tables->get_db_name())) && > + Lex_ident_ci(hash_tables->table_name).streq(tables->get_table_name())) > { > /* Link into hash_tables list */ > hash_tables->next= head; > diff --git a/sql/sql_i_s.h b/sql/sql_i_s.h > index 263031ae2c9..127b35f6ab2 100644 > --- a/sql/sql_i_s.h > +++ b/sql/sql_i_s.h > @@ -329,7 +325,7 @@ typedef class Item COND; > > typedef struct st_schema_table > { > - const char *table_name; > + LEX_CSTRING table_name; why LEX_CSTRING? > ST_FIELD_INFO *fields_info; > /* for FLUSH table_name */ > int (*reset_table) (); > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > index a84fd29affc..b74ecb51a69 100644 > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -9370,8 +9359,8 @@ bool LEX::create_package_finalize(THD *thd, > { > if (name2 && > (name2->m_explicit_name != name->m_explicit_name || > - strcmp(name2->m_db.str, name->m_db.str) || > - !Sp_handler::eq_routine_name(name2->m_name, name->m_name))) > + // QQ: was a bug here? "was" may be it was intentional, looking for an exact match only? > + !name2->eq_routine_name(name))) > { > bool exp= name2->m_explicit_name || name->m_explicit_name; > my_error(ER_END_IDENTIFIER_DOES_NOT_MATCH, MYF(0), > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 665d2143872..1430aa473af 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -1131,8 +1131,8 @@ static bool wsrep_tables_accessible_when_detached(const TABLE_LIST *tables) > { > for (const TABLE_LIST *table= tables; table; table= table->next_global) I'd personally use const TABLE_LIST *t= tables; ... it's a 3-line loop > { > - LEX_CSTRING db= table->db, tn= table->table_name; > - if (get_table_category(&db, &tn) < TABLE_CATEGORY_INFORMATION) > + if (get_table_category(table->db, > + table->table_name) < TABLE_CATEGORY_INFORMATION) > return false; > } > return tables != NULL; > diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc > index 958c52fd95c..c8d64ea37ff 100644 > --- a/sql/sql_partition.cc > +++ b/sql/sql_partition.cc > @@ -8055,13 +8050,9 @@ void make_used_partitions_str(MEM_ROOT *alloc, > if (parts_str->length()) > parts_str->append(','); > uint index= parts_str->length(); > - parts_str->append(head_pe->partition_name, > - strlen(head_pe->partition_name), > - system_charset_info); > + parts_str->append(head_pe->partition_name, system_charset_info); > parts_str->append('_'); > - parts_str->append(pe->partition_name, > - strlen(pe->partition_name), > - system_charset_info); > + parts_str->append(pe->partition_name, system_charset_info); better pe->partition_name->charset_info() (here and above and below) > used_partitions_list.append_str(alloc, parts_str->ptr() + index); > } > partition_id++; > diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc > index 8bab244c709..f335226b9a2 100644 > --- a/sql/sql_plugin.cc > +++ b/sql/sql_plugin.cc > @@ -374,16 +374,16 @@ bool check_valid_path(const char *path, size_t len) > > static void fix_dl_name(MEM_ROOT *root, LEX_CSTRING *dl) > { > - const size_t so_ext_len= sizeof(SO_EXT) - 1; > - if (dl->length < so_ext_len || > - my_strcasecmp(&my_charset_latin1, dl->str + dl->length - so_ext_len, > - SO_EXT)) > + const Lex_ident_plugin so_ext(SO_EXT, sizeof(SO_EXT) - 1); STRING_WITH_LEN > + if (dl->length < so_ext.length || > + !so_ext.streq(Lex_cstring(dl->str + dl->length - so_ext.length, > + so_ext.length))) > { > - char *s= (char*)alloc_root(root, dl->length + so_ext_len + 1); > + char *s= (char*)alloc_root(root, dl->length + so_ext.length + 1); > memcpy(s, dl->str, dl->length); > strcpy(s + dl->length, SO_EXT); > dl->str= s; > - dl->length+= so_ext_len; > + dl->length+= so_ext.length; > } > } > > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index 1fcafd6ec08..7548ff1ec57 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -7182,10 +7170,12 @@ static int get_schema_views_record(THD *thd, TABLE_LIST *tables, > Security_context *sctx= thd->security_ctx; > if (!tables->allowed_show) > { > - if (!my_strcasecmp(system_charset_info, tables->definer.user.str, > - sctx->priv_user) && > - !my_strcasecmp(system_charset_info, tables->definer.host.str, > - sctx->priv_host)) > + // QQ: why case insensitive comparision? Same as above? Try a test case and fix if a bug? it's likely create user foo; create user FOO; create view ... definer=foo v1 ... connect(as FOO); select * from information schema.views; > + if (my_charset_utf8mb3_general1400_as_ci.streq( > + tables->definer.user, > + Lex_cstring_strlen(sctx->priv_user)) && > + Lex_ident_host(tables->definer.host). > + streq(Lex_cstring_strlen(sctx->priv_host))) > tables->allowed_show= TRUE; > #ifndef NO_EMBEDDED_ACCESS_CHECKS > else > @@ -7893,8 +7883,7 @@ static int get_schema_partitions_record(THD *thd, TABLE_LIST *tables, > > while ((part_elem= part_it++)) > { > - table->field[3]->store(part_elem->partition_name, > - strlen(part_elem->partition_name), cs); > + table->field[3]->store(part_elem->partition_name, cs); may be part_elem->partition_name->charset_info() ? > table->field[3]->set_notnull(); > /* PARTITION_ORDINAL_POSITION */ > table->field[5]->store((longlong) ++part_pos, TRUE); > diff --git a/sql/sql_trigger.cc b/sql/sql_trigger.cc > index 5304a535ad0..dcba31d06c9 100644 > --- a/sql/sql_trigger.cc > +++ b/sql/sql_trigger.cc > @@ -470,7 +470,7 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create) > /* > We don't allow creating triggers on tables in the 'mysql' schema > */ > - if (create && lex_string_eq(&tables->db, STRING_WITH_LEN("mysql"))) > + if (create && tables->db.streq(MYSQL_SCHEMA_NAME)) // QQ: was a bug here? "was" > { > my_error(ER_NO_TRIGGERS_ON_SYSTEM_SCHEMA, MYF(0)); > DBUG_RETURN(TRUE); > diff --git a/sql/sql_trigger.h b/sql/sql_trigger.h > index 493349afb9d..5648f8f15ff 100644 > --- a/sql/sql_trigger.h > +++ b/sql/sql_trigger.h > @@ -74,8 +74,10 @@ struct st_trg_execution_order > /** > Trigger name referenced in the FOLLOWS/PRECEDES clause of the > CREATE TRIGGER statement. > + Cannot be Lex_ident_trigger, > + as this structure is used in %union in sql_yacc.yy Thanks. I already wrote above "why it's not simply a Lex_ident_trigger. That explains it, deleted my previous comment :) > */ > - LEX_CSTRING anchor_trigger_name; > + LEX_CSTRING anchor_trigger_name; // Used in sql_yacc %union > }; > > > diff --git a/sql/sql_truncate.cc b/sql/sql_truncate.cc > index beeee9da72f..e3efc3122df 100644 > --- a/sql/sql_truncate.cc > +++ b/sql/sql_truncate.cc > @@ -147,14 +147,12 @@ fk_truncate_illegal_if_parent(THD *thd, TABLE *table) > /* Loop over the set of foreign keys for which this table is a parent. */ > while ((fk_info= it++)) > { > - if (lex_string_cmp(system_charset_info, fk_info->referenced_db, > - &table->s->db) || > - lex_string_cmp(system_charset_info, fk_info->referenced_table, > - &table->s->table_name) || > - lex_string_cmp(system_charset_info, fk_info->foreign_db, > - &table->s->db) || > - lex_string_cmp(system_charset_info, fk_info->foreign_table, > - &table->s->table_name)) > + // QQ: why db and table don't use table_alias_charset for comparison? Same as above? Try a test case and fix if a bug? test looks fairly straightforward here too > + if (!Lex_ident_ci(*fk_info->referenced_db).streq(table->s->db) || > + !Lex_ident_ci(*fk_info->referenced_table). > + streq(table->s->table_name) || > + !Lex_ident_ci(*fk_info->foreign_db).streq(table->s->db) || > + !Lex_ident_ci(*fk_info->foreign_table).streq(table->s->table_name)) > break; > } > > diff --git a/sql/sql_view.cc b/sql/sql_view.cc > index 10588b54fa5..751373b4571 100644 > --- a/sql/sql_view.cc > +++ b/sql/sql_view.cc > @@ -1347,12 +1347,9 @@ bool mysql_make_view(THD *thd, TABLE_SHARE *share, TABLE_LIST *table, > precedent; > precedent= precedent->referencing_view) > { > - if (precedent->view_name.length == table->table_name.length && > - precedent->view_db.length == table->db.length && > - my_strcasecmp(system_charset_info, > - precedent->view_name.str, table->table_name.str) == 0 && > - my_strcasecmp(system_charset_info, > - precedent->view_db.str, table->db.str) == 0) > + // QQ: was a bug here? "was" > + if (precedent->view_name.streq(table->table_name) && > + precedent->view_db.streq(table->db)) > { > my_error(ER_VIEW_RECURSIVE, MYF(0), > top_view->view_db.str, top_view->view_name.str); > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index fcfe490c035..d01c195cb8a 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -15676,7 +15680,8 @@ user_maybe_role: > It's OK to use in-place lowercase as long as > the character set is utf8. > */ > - my_casedn_str(system_charset_info, (char*) $$->host.str); > + $$->host.length= my_casedn_str(system_charset_info, > + (char*) $$->host.str); I thought my_casedn_str() changes were in the different commit? > } > else > { > diff --git a/sql/structs.h b/sql/structs.h > index 75bdc9e276b..d349ab72970 100644 > --- a/sql/structs.h > +++ b/sql/structs.h > @@ -321,11 +322,25 @@ typedef struct user_conn { > uint conn_per_hour, updates, questions; > /* Maximum amount of resources which account is allowed to consume. */ > USER_RESOURCES user_resources; > + > + /* > + The CHARSET_INFO used for hashes to compare the entire 'user\0hash' key. > + QQ: perhaps the user should be compared case sensitively, > + while host host should be comparad case insensitively. correct > + */ > + static CHARSET_INFO *user_host_key_charset_info_for_hash() > + { > + return &my_charset_utf8mb3_general1400_as_ci; > + } > } USER_CONN; > > typedef struct st_user_stats > { > char user[MY_MAX(USERNAME_LENGTH, LIST_PROCESS_HOST_LEN) + 1]; > + static CHARSET_INFO *user_key_charset_info_for_hash() > + { > + return &my_charset_utf8mb3_general1400_as_ci; > + } > // Account name the user is mapped to when this is a user from mapped_user. > // Otherwise, the same value as user. > char priv_user[MY_MAX(USERNAME_LENGTH, LIST_PROCESS_HOST_LEN) + 1]; > diff --git a/sql/table.cc b/sql/table.cc > index 8bc16753e68..d9bc5fabb4f 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -88,23 +88,34 @@ struct extra2_fields > static Virtual_column_info * unpack_vcol_info_from_frm(THD *, > TABLE *, String *, Virtual_column_info **, bool *); > > +/* > + Lex_ident_db does not have operator""_Lex_ident_db, > + because its constructor contains DBUG_SLOW_ASSERT > + and therefore it's not a constexpr constructor. > + So let's initialize a number of Lex_ident_db constants > + using operator""_Lex_cstring. > +*/ > + > /* INFORMATION_SCHEMA name */ > -LEX_CSTRING INFORMATION_SCHEMA_NAME= {STRING_WITH_LEN("information_schema")}; > +Lex_ident_db > + INFORMATION_SCHEMA_NAME= Lex_ident_db("information_schema"_Lex_cstring); don't think so. information_schema is case insensitive, even if fs is case sensitivity > > /* PERFORMANCE_SCHEMA name */ > -LEX_CSTRING PERFORMANCE_SCHEMA_DB_NAME= {STRING_WITH_LEN("performance_schema")}; > +Lex_ident_db > + PERFORMANCE_SCHEMA_DB_NAME= Lex_ident_db("performance_schema"_Lex_cstring); same > > /* MYSQL_SCHEMA name */ > -LEX_CSTRING MYSQL_SCHEMA_NAME= {STRING_WITH_LEN("mysql")}; > +Lex_ident_db > + MYSQL_SCHEMA_NAME= Lex_ident_db("mysql"_Lex_cstring); this uses fs case sensitivity, correct > > /* GENERAL_LOG name */ > -LEX_CSTRING GENERAL_LOG_NAME= {STRING_WITH_LEN("general_log")}; > +Lex_ident_table GENERAL_LOG_NAME= "general_log"_Lex_ident_table; > > /* SLOW_LOG name */ > -LEX_CSTRING SLOW_LOG_NAME= {STRING_WITH_LEN("slow_log")}; > +Lex_ident_table SLOW_LOG_NAME= "slow_log"_Lex_ident_table; > > -LEX_CSTRING TRANSACTION_REG_NAME= {STRING_WITH_LEN("transaction_registry")}; > -LEX_CSTRING MYSQL_PROC_NAME= {STRING_WITH_LEN("proc")}; > +Lex_ident_table TRANSACTION_REG_NAME= "transaction_registry"_Lex_ident_table; > +Lex_ident_table MYSQL_PROC_NAME= "proc"_Lex_ident_table; > > /* > Keyword added as a prefix when parsing the defining expression for a > @@ -280,42 +291,38 @@ const char *fn_frm_ext(const char *name) > } > > > -TABLE_CATEGORY get_table_category(const LEX_CSTRING *db, > - const LEX_CSTRING *name) > +TABLE_CATEGORY get_table_category(const Lex_ident_db &db, > + const Lex_ident_table &name) > { > - DBUG_ASSERT(db != NULL); > - DBUG_ASSERT(name != NULL); > - > #ifdef WITH_WSREP > - if (db->str && > - my_strcasecmp(system_charset_info, db->str, WSREP_SCHEMA) == 0) > + if (db.str && db.streq(MYSQL_SCHEMA_NAME)) //QQ: was a bug here? "was" > { > - if ((my_strcasecmp(system_charset_info, name->str, WSREP_STREAMING_TABLE) == 0 || > - my_strcasecmp(system_charset_info, name->str, WSREP_CLUSTER_TABLE) == 0 || > - my_strcasecmp(system_charset_info, name->str, WSREP_MEMBERS_TABLE) == 0)) > + if (name.streq(Lex_ident_table{STRING_WITH_LEN(WSREP_STREAMING_TABLE)}) || > + name.streq(Lex_ident_table{STRING_WITH_LEN(WSREP_CLUSTER_TABLE)}) || > + name.streq(Lex_ident_table{STRING_WITH_LEN(WSREP_MEMBERS_TABLE)})) > { > return TABLE_CATEGORY_INFORMATION; > } > } > #endif /* WITH_WSREP */ > - if (is_infoschema_db(db)) > + if (is_infoschema_db(&db)) > return TABLE_CATEGORY_INFORMATION; > > - if (is_perfschema_db(db)) > + if (is_perfschema_db(&db)) > return TABLE_CATEGORY_PERFORMANCE; > > - if (lex_string_eq(&MYSQL_SCHEMA_NAME, db)) > + if (db.streq(MYSQL_SCHEMA_NAME)) // QQ: was a bug here? "was" > { > - if (is_system_table_name(name->str, name->length)) > + if (is_system_table_name(name.str, name.length)) > return TABLE_CATEGORY_SYSTEM; > > - if (lex_string_eq(&GENERAL_LOG_NAME, name)) > + if (name.streq(GENERAL_LOG_NAME)) // QQ: was bug here? "was" > return TABLE_CATEGORY_LOG; > > - if (lex_string_eq(&SLOW_LOG_NAME, name)) > + if (name.streq(SLOW_LOG_NAME)) // QQ: was a bug here? "was" > return TABLE_CATEGORY_LOG; > > - if (lex_string_eq(&TRANSACTION_REG_NAME, name)) > + if (name.streq(TRANSACTION_REG_NAME)) // QQ: was a bug here? "was" > return TABLE_CATEGORY_LOG; > } > > diff --git a/sql/table.h b/sql/table.h > index 35be3bd4b2d..fe65025478d 100644 > --- a/sql/table.h > +++ b/sql/table.h > @@ -3402,27 +3409,27 @@ static inline int set_zone(int nr,int min_zone,int max_zone) > } > > /* performance schema */ > -extern LEX_CSTRING PERFORMANCE_SCHEMA_DB_NAME; > +extern Lex_ident_db PERFORMANCE_SCHEMA_DB_NAME; > > -extern LEX_CSTRING GENERAL_LOG_NAME; > -extern LEX_CSTRING SLOW_LOG_NAME; > -extern LEX_CSTRING TRANSACTION_REG_NAME; > +extern Lex_ident_table GENERAL_LOG_NAME; > +extern Lex_ident_table SLOW_LOG_NAME; > +extern Lex_ident_table TRANSACTION_REG_NAME; > > /* information schema */ > -extern LEX_CSTRING INFORMATION_SCHEMA_NAME; > -extern LEX_CSTRING MYSQL_SCHEMA_NAME; > +extern Lex_ident_db INFORMATION_SCHEMA_NAME; > +extern Lex_ident_db MYSQL_SCHEMA_NAME; > > /* table names */ > -extern LEX_CSTRING MYSQL_PROC_NAME; > +extern Lex_ident_table MYSQL_PROC_NAME; > > inline bool is_infoschema_db(const LEX_CSTRING *name) > { > - return lex_string_eq(&INFORMATION_SCHEMA_NAME, name); > + return lex_string_eq(&INFORMATION_SCHEMA_NAME, name); // QQ: use Lex_ident_ci ? yes > } > > inline bool is_perfschema_db(const LEX_CSTRING *name) > { > - return lex_string_eq(&PERFORMANCE_SCHEMA_DB_NAME, name); > + return lex_string_eq(&PERFORMANCE_SCHEMA_DB_NAME, name); // QQ: use Lex_ident_ci? yes > } > > inline void mark_as_null_row(TABLE *table) > diff --git a/storage/blackhole/ha_blackhole.cc b/storage/blackhole/ha_blackhole.cc > index 343f3c70286..ce48c95f837 100644 > --- a/storage/blackhole/ha_blackhole.cc > +++ b/storage/blackhole/ha_blackhole.cc > @@ -416,7 +416,9 @@ static int blackhole_init(void *p) > mysql_mutex_init(bh_key_mutex_blackhole, > &blackhole_mutex, MY_MUTEX_INIT_FAST); > (void) my_hash_init(PSI_INSTRUMENT_ME, &blackhole_open_tables, > - system_charset_info, 32, 0, 0, > + // QQ: was a bug here? "was" > + Lex_ident_table::charset_info(), > + 32, 0, 0, > (my_hash_get_key) blackhole_get_key, > (my_hash_free_key) blackhole_free_key, 0); > > diff --git a/storage/csv/ha_tina.cc b/storage/csv/ha_tina.cc > index e705ff0e7c0..9fc0bcf604b 100644 > --- a/storage/csv/ha_tina.cc > +++ b/storage/csv/ha_tina.cc > @@ -184,7 +184,9 @@ static int tina_init_func(void *p) > tina_hton= (handlerton *)p; > mysql_mutex_init(csv_key_mutex_tina, &tina_mutex, MY_MUTEX_INIT_FAST); > (void) my_hash_init(csv_key_memory_tina_share, &tina_open_tables, > - system_charset_info, 32, 0, 0, (my_hash_get_key) > + // QQ: was a bug here? "was" > + table_alias_charset, > + 32, 0, 0, (my_hash_get_key) > tina_get_key, 0, 0); > tina_hton->db_type= DB_TYPE_CSV_DB; > tina_hton->create= tina_create_handler; > diff --git a/storage/innobase/dict/dict0dict.cc b/storage/innobase/dict/dict0dict.cc > index 5545727b015..231bcc1a132 100644 > --- a/storage/innobase/dict/dict0dict.cc > +++ b/storage/innobase/dict/dict0dict.cc > @@ -284,27 +284,26 @@ otherwise table->n_def */ > ulint > dict_table_has_column( > const dict_table_t* table, > - const char* col_name, > + const LEX_CSTRING &col_name, looks ok to me, but to avoid surprises Marko should review it too > ulint col_nr) > { > ulint col_max = table->n_def; > > ut_ad(table); > - ut_ad(col_name); > + ut_ad(col_name.str); > ut_ad(table->magic_n == DICT_TABLE_MAGIC_N); > > if (col_nr < col_max > - && innobase_strcasecmp( > - col_name, dict_table_get_col_name(table, col_nr)) == 0) { > + && Lex_ident_column(col_name). > + streq(dict_table_get_col_name_ls(table, col_nr))) { > return(col_nr); > } > > /** The order of column may changed, check it with other columns */ > for (ulint i = 0; i < col_max; i++) { > if (i != col_nr > - && innobase_strcasecmp( > - col_name, dict_table_get_col_name(table, i)) == 0) { > - > + && Lex_ident_column(col_name). > + streq(dict_table_get_col_name_ls(table, i))) { > return(i); > } > } > diff --git a/storage/mroonga/ha_mroonga.cpp b/storage/mroonga/ha_mroonga.cpp > index 44fd09e74ed..71784dad475 100644 > --- a/storage/mroonga/ha_mroonga.cpp > +++ b/storage/mroonga/ha_mroonga.cpp > @@ -1936,7 +1936,10 @@ static int mrn_init(void *p) > MY_MUTEX_INIT_FAST) != 0)) { > goto err_allocated_thds_mutex_init; > } > - if (mrn_my_hash_init(&mrn_allocated_thds, system_charset_info, 32, 0, 0, > + if (mrn_my_hash_init(&mrn_allocated_thds, > + // QQ: was a bug here? "was" > + &my_charset_bin, > + 32, 0, 0, > mrn_allocated_thds_get_key, 0, 0)) { > goto error_allocated_thds_hash_init; > } > @@ -1945,7 +1948,10 @@ static int mrn_init(void *p) > MY_MUTEX_INIT_FAST) != 0)) { > goto err_allocated_open_tables_mutex_init; > } > - if (mrn_my_hash_init(&mrn_open_tables, system_charset_info, 32, 0, 0, > + if (mrn_my_hash_init(&mrn_open_tables, > + // QQ: was a bug here? "was" > + Lex_ident_table::charset_info(), > + 32, 0, 0, > mrn_open_tables_get_key, 0, 0)) { > goto error_allocated_open_tables_hash_init; > } > @@ -1954,7 +1960,10 @@ static int mrn_init(void *p) > MY_MUTEX_INIT_FAST) != 0)) { > goto error_allocated_long_term_share_mutex_init; > } > - if (mrn_my_hash_init(&mrn_long_term_share, system_charset_info, 32, 0, 0, > + if (mrn_my_hash_init(&mrn_long_term_share, > + // QQ: was a bug here? "was" > + Lex_ident_table::charset_info(), > + 32, 0, 0, > mrn_long_term_share_get_key, 0, 0)) { > goto error_allocated_long_term_share_hash_init; > } > diff --git a/storage/perfschema/pfs_engine_table.h b/storage/perfschema/pfs_engine_table.h > index 1ef1e0282de..b4f4c9f0acc 100644 > --- a/storage/perfschema/pfs_engine_table.h > +++ b/storage/perfschema/pfs_engine_table.h > @@ -25,6 +25,22 @@ > > #include "table.h" > #include "sql_acl.h" > + > +/* > + The performance schema is implemented as a storage engine, in memory. > + The current storage engine interface exposed by the server, > + and in particular handlerton::discover, uses 'FRM' files to describe a > + table structure, which are later stored on disk, by the server, > + in ha_create_table_from_engine(). > + Because the table metadata is stored on disk, the table naming rules > + used by the performance schema then have to comply with the constraints > + imposed by the disk storage, and in particular with lower_case_table_names. > +*/ > + > +using PFS_ident_db= Lex_ident_db; > +using PFS_ident_table = Lex_ident_table; No, table metadata for performance schema are never stored on disk. let's use case insensitive comparison here, if possible. > + > + > /** > @file storage/perfschema/pfs_engine_table.h > Performance schema tables (declarations). Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 5

Re: d2de9a89d80: MDEV-31531 Remove my_casedn_str() and my_caseup_str()
by Sergei Golubchik 19 Dec '23

19 Dec '23

Hi, Alexander, Looks good. A couple of minor comments below On Dec 19, Alexander Barkov wrote: > commit d2de9a89d80 > Author: Alexander Barkov <bar(a)mariadb.com> > Date: Fri Jun 23 13:24:02 2023 +0400 > > diff --git a/include/m_ctype.h b/include/m_ctype.h > index 9d13989d1fe..65ccf869992 100644 > --- a/include/m_ctype.h > +++ b/include/m_ctype.h > @@ -1512,6 +1540,14 @@ size_t my_copy_fix_mb(CHARSET_INFO *cs, > /* Functions for 8bit */ > extern size_t my_caseup_str_8bit(CHARSET_INFO *, char *); > extern size_t my_casedn_str_8bit(CHARSET_INFO *, char *); > +static inline size_t my_caseup_str_latin1(char *str) caseup variant doesn't seem to be used anywhere > +{ > + return my_caseup_str_8bit(&my_charset_latin1, str); > +} > +static inline size_t my_casedn_str_latin1(char *str) > +{ > + return my_casedn_str_8bit(&my_charset_latin1, str); > +} > extern size_t my_caseup_8bit(CHARSET_INFO *, > const char *src, size_t srclen, > char *dst, size_t dstlen); > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 09af5523c76..3166c9e56cf 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -2543,6 +2541,37 @@ static void push_new_user(const ACL_USER &user) > } > > > +/** > + Make a database name on mem_root from a String, > + apply lower-case conversion if lower_case_table_names says so. > + Perform database name length limit validation. > + > + @param thd - the THD, to get the warning text from > + @param mem_root - allocate the result on this memory root > + @param dbstr - the String, e.g. with Field::val_str() result > + > + @return - {NULL,0} in case of EOM or a bad database name, > + or a good database name otherwise. > +*/ > +static LEX_STRING make_and_check_db_name(MEM_ROOT *mem_root, > + const String &dbstr) > +{ > + LEX_STRING dbls= lower_case_table_names ? > + lex_string_casedn_root(mem_root, files_charset_info, > + dbstr.ptr(), dbstr.length()) : > + lex_string_strmake_root(mem_root, > + dbstr.ptr(), dbstr.length()); > + if (!dbls.str) > + return LEX_STRING{NULL, 0}; // EOM what's the difference between this and return {NULL, 0}; > + if (dbls.length > SAFE_NAME_LEN) > + { > + sql_print_warning(ER_DEFAULT(ER_WRONG_DB_NAME), dbls.str); > + return LEX_STRING{NULL, 0}; // Bad name > + } > + return dbls; // Good name > +} > + > + > /* > Initialize structures responsible for user/db-level privilege checking > and load information about grants from open privilege tables. > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > index a84fd29affc..f61e5085e47 100644 > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -4186,6 +4187,18 @@ bool LEX::copy_db_to(LEX_CSTRING *to) > return thd->copy_db_to(to); > } > > + > +Lex_ident_db_normalized LEX::copy_db_normalized() > +{ > + if (sphead && sphead->m_name.str) > + { > + DBUG_ASSERT(sphead->m_db.str && sphead->m_db.length); you missed that DBUG_ASSERT(sphead->m_db.str); DBUG_ASSERT(sphead->m_db.length); > + return thd->to_ident_db_normalized_with_error(sphead->m_db); > + } > + return thd->copy_db_normalized(); > +} > + > + > /** > Initialize offset and limit counters. > > diff --git a/storage/perfschema/pfs_program.cc b/storage/perfschema/pfs_program.cc > index de456610519..f3734cb383b 100644 > --- a/storage/perfschema/pfs_program.cc > +++ b/storage/perfschema/pfs_program.cc > @@ -118,31 +118,21 @@ static void set_program_key(PFS_program_key *key, > */ > > char *ptr= &key->m_hash_key[0]; > + const char *end= ptr + sizeof(key->m_hash_key) - 1; > > ptr[0]= object_type; > ptr++; > > if (object_name_length > 0) > - { > - char tmp_object_name[COL_OBJECT_NAME_SIZE + 1]; > - memcpy(tmp_object_name, object_name, object_name_length); > - tmp_object_name[object_name_length]= '\0'; > - my_casedn_str(system_charset_info, tmp_object_name); > - memcpy(ptr, tmp_object_name, object_name_length); > - ptr+= object_name_length; > - } > + ptr+= system_charset_info->casedn(object_name, object_name_length, > + ptr, end - ptr); > ptr[0]= 0; > ptr++; > > if (schema_name_length > 0) > - { > - char tmp_schema_name[COL_OBJECT_SCHEMA_SIZE + 1]; > - memcpy(tmp_schema_name, schema_name, schema_name_length); > - tmp_schema_name[schema_name_length]='\0'; > - my_casedn_str(system_charset_info, tmp_schema_name); > - memcpy(ptr, tmp_schema_name, schema_name_length); > - ptr+= schema_name_length; > - } > + ptr+= system_charset_info->opt_casedn(schema_name, schema_name_length, > + ptr, end - ptr, > + lower_case_table_names); why system_charset_info and not files_charset_info ? > ptr[0]= 0; > ptr++; > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

MTR failures on official Debian buildd after MariaDB 1:10.11.6-1 upload
by Otto Kekäläinen 17 Dec '23

17 Dec '23

Hi! I just wanted to let you know there are a bunch of MTR failures/regressions at https://buildd.debian.org/status/package.php?p=mariadb which I need to fix or add to skip-lists. The previous upload of MariaDB 1:10.11.5-3 had more 'green' than ever before, and now there are regressions on 6 archs. [image: image.png]

3 6

Re: 2821cf816a4: MDEV-31531 Remove my_casedn_str() and my_caseup_str()
by Sergei Golubchik 16 Dec '23

16 Dec '23

Hi, Alexander, > commit 2821cf816a4 > Author: Alexander Barkov <bar(a)mariadb.com> > Date: Fri Jun 23 13:24:02 2023 +0400 > > MDEV-31531 Remove my_casedn_str() and my_caseup_str() please, explain here why > > diff --git a/include/m_ctype.h b/include/m_ctype.h > index 9d13989d1fe..d2629121cfe 100644 > --- a/include/m_ctype.h > +++ b/include/m_ctype.h > @@ -848,6 +844,26 @@ struct charset_info_st > return (cset->casedn)(this, src, srclen, dst, dstlen); > } > > + /* Convert to a lower-cased 0-terminated string */ > + size_t casedn_z(const char *src, size_t srclen, > + char *dst, size_t dstlen) const > + { > + DBUG_ASSERT(dstlen); DBUG_ASSERT(src != dst) ? > + size_t len= casedn(src, srclen, dst, dstlen - 1); > + dst[len]= '\0'; > + return len; > + } > + > + /* Convert to a upper-cased 0-terminated string */ > + size_t caseup_z(const char *src, size_t srclen, > + char *dst, size_t dstlen) const > + { > + DBUG_ASSERT(dstlen); > + size_t len= caseup(src, srclen, dst, dstlen - 1); > + dst[len]= '\0'; > + return len; > + } > + > uint caseup_multiply() const > { > return (cset->caseup_multiply)(this); > @@ -1512,6 +1528,14 @@ size_t my_copy_fix_mb(CHARSET_INFO *cs, > /* Functions for 8bit */ > extern size_t my_caseup_str_8bit(CHARSET_INFO *, char *); > extern size_t my_casedn_str_8bit(CHARSET_INFO *, char *); > +static inline size_t my_caseup_str_latin1(char *str) > +{ > + return my_caseup_str_8bit(&my_charset_latin1, str); > +} > +static inline size_t my_casedn_str_latin1(char *str) > +{ > + return my_casedn_str_8bit(&my_charset_latin1, str); > +} may be my_charset_ascii? with an assert that it's indeed ascii? and a comment that it's used for generated temp table names and such > extern size_t my_caseup_8bit(CHARSET_INFO *, > const char *src, size_t srclen, > char *dst, size_t dstlen); > diff --git a/sql/char_buffer.h b/sql/char_buffer.h > index 7b855f5401c..1091775259d 100644 > --- a/sql/char_buffer.h > +++ b/sql/char_buffer.h > @@ -71,25 +76,92 @@ class CharBuffer > m_buff[m_length]= '\0'; > return *this; > } > + CharBuffer<buff_sz> & copy_caseup(CHARSET_INFO *cs, const LEX_CSTRING &str) > + { > + DBUG_ASSERT(!buffer_overlaps(str)); > + m_length= cs->cset->caseup(cs, str.str, str.length, m_buff, buff_sz); > + DBUG_ASSERT(is_sane()); > + m_buff[m_length]= '\0'; // See comments in copy_casedn() > + return *this; > + } > CharBuffer<buff_sz> & copy_casedn(CHARSET_INFO *cs, const LEX_CSTRING &str, > bool casedn) > { > casedn ? copy_casedn(cs, str) : copy_bin(str); > return *this; > } > + > + // Append one character > + CharBuffer<buff_sz> & append_char(char ch) > + { > + DBUG_ASSERT(is_sane()); > + if (available_size()) > + { > + m_buff[m_length++]= ch; > + m_buff[m_length]= '\0'; > + } > + DBUG_ASSERT(is_sane()); > + return *this; > + } > + > + // Append a string > + CharBuffer<buff_sz> & append_bin(const LEX_CSTRING &str) why _bin ? and why even have a suffix, and not just append() here and above? > + { > + DBUG_ASSERT(is_sane()); > + DBUG_ASSERT(!buffer_overlaps(str)); > + size_t len= MY_MIN(available_size(), str.length); > + memcpy(m_buff + m_length, str.str, len); > + m_length+= len; > + DBUG_ASSERT(is_sane()); > + m_buff[m_length]= '\0'; > + return *this; > + } > + > // Append a string with casedn conversion > CharBuffer<buff_sz> & append_casedn(CHARSET_INFO *cs, const LEX_CSTRING &str) > { > DBUG_ASSERT(is_sane()); > DBUG_ASSERT(!buffer_overlaps(str)); > size_t casedn_length= cs->casedn(str.str, str.length, > - m_buff + m_length, buff_sz - m_length); > + m_buff + m_length, available_size()); > + m_length+= casedn_length; > + DBUG_ASSERT(is_sane()); > + m_buff[m_length]= '\0'; > + return *this; > + } > + > + CharBuffer<buff_sz> & append_opt_casedn(CHARSET_INFO *cs, > + const LEX_CSTRING &str, > + bool casedn) > + { > + return casedn ? append_casedn(cs, str) : append_bin(str); > + } > + > + // Append a string with caseup conversion > + CharBuffer<buff_sz> & append_caseup(CHARSET_INFO *cs, const LEX_CSTRING &str) > + { > + DBUG_ASSERT(is_sane()); > + DBUG_ASSERT(!buffer_overlaps(str)); > + size_t casedn_length= cs->caseup(str.str, str.length, > + m_buff + m_length, available_size()); > m_length+= casedn_length; > DBUG_ASSERT(is_sane()); > m_buff[m_length]= '\0'; > return *this; > } > > + CharBuffer<buff_sz> & truncate(size_t length) > + { > + DBUG_ASSERT(is_sane()); > + if (m_length > length) > + { > + m_length= length; > + m_buff[m_length]= '\0'; > + DBUG_ASSERT(is_sane()); > + } > + return *this; > + } > + > LEX_CSTRING to_lex_cstring() const > { > return LEX_CSTRING{m_buff, m_length}; > diff --git a/sql/item_sum.cc b/sql/item_sum.cc > index bbd09a59267..824ed42ec91 100644 > --- a/sql/item_sum.cc > +++ b/sql/item_sum.cc > @@ -3769,20 +3769,15 @@ int group_concat_key_cmp_with_order_with_nulls(void *arg, const void *key1_arg, > } > > > -static void report_cut_value_error(THD *thd, uint row_count, const char *fname) > +static void report_cut_value_error(THD *thd, uint row_count, > + const LEX_CSTRING &fname) > { > - size_t fn_len= strlen(fname); > - char *fname_upper= (char *) my_alloca(fn_len + 1); > - if (!fname_upper) > - fname_upper= (char*) fname; // Out of memory > - else > - memcpy(fname_upper, fname, fn_len+1); > - my_caseup_str(&my_charset_latin1, fname_upper); > + CharBuffer<NAME_LEN> fname_upper; > + fname_upper.copy_caseup(&my_charset_latin1, fname); why bother? we don't do it in other error cases > push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, > ER_CUT_VALUE_GROUP_CONCAT, > ER_THD(thd, ER_CUT_VALUE_GROUP_CONCAT), > - row_count, fname_upper); > + row_count, fname_upper.ptr()); > - my_afree(fname_upper); > } > > > diff --git a/sql/lex_ident.h b/sql/lex_ident.h > index f39273b7da5..db96e40cab8 100644 > --- a/sql/lex_ident.h > +++ b/sql/lex_ident.h > @@ -45,6 +46,10 @@ class Lex_ident_fs: public LEX_CSTRING > bool is_in_lower_case() const; > bool ok_for_lower_case_names() const; > #endif > + bool check_db_name_quick() const please, use a name that makes it clear what the return value is Not if (str.check_db_name_quick()) but if (str.is_invalid_db_name()) > + { > + return !length || length > NAME_LEN || str[length-1] == ' '; > + } > }; > > > @@ -160,4 +171,95 @@ class DBNameBuffer: public CharBuffer<SAFE_NAME_LEN + MY_CS_MBMAXLEN> > }; > > > +class Identifier_chain2 > +{ > + LEX_CSTRING m_name[2]; > +public: > + Identifier_chain2() > + :m_name{Lex_cstring(), Lex_cstring()} > + { } > + Identifier_chain2(const LEX_CSTRING &a, const LEX_CSTRING &b) > + :m_name{a, b} > + { } > + > + const LEX_CSTRING& operator [] (size_t i) const > + { > + return m_name[i]; > + } > + > + static Identifier_chain2 split(const LEX_CSTRING &txt) > + { > + DBUG_ASSERT(txt.str[txt.length] == '\0'); // Expect 0-terminated input > + const char *dot= strchr(txt.str, '.'); > + if (!dot) > + return Identifier_chain2(Lex_cstring(), txt); > + size_t length0= dot - txt.str; > + Lex_cstring name0(txt.str, length0); > + Lex_cstring name1(txt.str + length0 + 1, txt.length - length0 - 1); > + return Identifier_chain2(name0, name1); > + } > + > + // Export as a qualified name string: 'db.name' > + size_t make_qname(char *dst, size_t dstlen) const > + { > + return my_snprintf(dst, dstlen, "%.*s.%.*s", > + (int) m_name[0].length, m_name[0].str, > + (int) m_name[1].length, m_name[1].str); > + } > + > + // Export as a qualified name string, allocate on mem_root. > + bool make_qname(MEM_ROOT *mem_root, LEX_CSTRING *dst) const > + { > + const uint dot= !!m_name[0].length; > + char *tmp; > + /* format: [pkg + dot] + name + '\0' */ > + dst->length= m_name[0].length + dot + m_name[1].length; > + if (unlikely(!(dst->str= tmp= (char*) alloc_root(mem_root, > + dst->length + 1)))) > + return true; > + snprintf(tmp, dst->length + 1, "%.*s%.*s%.*s", > + (int) m_name[0].length, (m_name[0].length ? m_name[0].str : ""), > + dot, ".", > + (int) m_name[1].length, m_name[1].str); > + return false; > + } > + > + /* > + Build a separated two step name, e.g. "ident1/ident2", 0-terminated. > + with an optional lower-case conversion. may be should be inside InnoDB? > + @param [OUT] dst - the destination > + @param dst_size - number of bytes available in dst > + @param sep - the separator character > + @param casedn - whether to convert components to lower case > + @return - number of bytes written to "dst", not counting > + the trailing '\0' byte. > + */ > + size_t make_sep_name_opt_casedn(char *dst, size_t dst_size, > + int sep, bool casedn) const > + { > + /* > + Minimal possible buffer is 4 bytes: 'd/t\0' > + where 'd' is the database name, 't' is the table name. > + Callers should never pass a smaller buffer. > + */ > + DBUG_ASSERT(dst_size > 4); > + DBUG_ASSERT(m_name[0].length + m_name[1].length + 2 < FN_REFLEN - 1); > + if (casedn) > + { > + CHARSET_INFO *cs= &my_charset_utf8mb3_general_ci; > + char *ptr= dst, *end= dst + dst_size; > + ptr+= cs->casedn(m_name[0].str, m_name[0].length, ptr, end - ptr - 2); > + *ptr++= (char) sep; > + ptr+= cs->casedn_z(m_name[1].str, m_name[1].length, ptr, end - ptr); > + return ptr - dst; > + } > + memcpy(dst, m_name[0].str, m_name[0].length); > + dst[m_name[0].length] = (char) sep; > + /* Copy the name and null-byte. */ > + memcpy(dst + m_name[0].length + 1, m_name[1].str, m_name[1].length + 1); > + return m_name[0].length + 1/*slash*/ + m_name[1].length; s/slash/sep/ > + } > +}; > + > + > #endif // LEX_IDENT_INCLUDED > diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc > index 6030202adc7..0fc1d4ce962 100644 > --- a/sql/log_event_server.cc > +++ b/sql/log_event_server.cc > @@ -5777,19 +5778,29 @@ int Table_map_log_event::do_apply_event(rpl_group_info *rgi) > /* Step the query id to mark what columns that are actually used. */ > thd->set_query_id(next_query_id()); > > + db_mem_alloced= tname_mem_alloced= NAME_LEN * > + (lower_case_table_names ? 1 : > + files_charset_info->casedn_multiply()) > + + 1 /* for '\0' */; 1. may be db_mem_length/tname_mem_length instead of NAME_LEN? 2. is the ?: condition right? Looks like it's inverted > if (!(memory= my_multi_malloc(PSI_INSTRUMENT_ME, MYF(MY_WME), > &table_list, (uint) sizeof(RPL_TABLE_LIST), > - &db_mem, (uint) NAME_LEN + 1, > - &tname_mem, (uint) NAME_LEN + 1, > + &db_mem, (uint) db_mem_alloced, > + &tname_mem, (uint) tname_mem_alloced, > NullS))) > DBUG_RETURN(HA_ERR_OUT_OF_MEM); > > - db_mem_length= strmov(db_mem, m_dbnam) - db_mem; > - tname_mem_length= strmov(tname_mem, m_tblnam) - tname_mem; > if (lower_case_table_names) > { > - my_casedn_str(files_charset_info, (char*)tname_mem); > - my_casedn_str(files_charset_info, (char*)db_mem); > + db_mem_length= files_charset_info->casedn_z(m_dbnam, m_dblen, > + db_mem, db_mem_alloced); > + tname_mem_length= files_charset_info->casedn_z(m_tblnam, m_tbllen, > + tname_mem, > + tname_mem_alloced); > + } > + else > + { > + db_mem_length= strmov(db_mem, m_dbnam) - db_mem; > + tname_mem_length= strmov(tname_mem, m_tblnam) - tname_mem; > } > > /* call from mysql_client_binlog_statement() will not set rli->mi */ > diff --git a/sql/sp.cc b/sql/sp.cc > index 0a8aa24789b..537d29fb67e 100644 > --- a/sql/sp.cc > +++ b/sql/sp.cc > @@ -2281,12 +2281,20 @@ Sp_handler::sp_exist_routines(THD *thd, TABLE_LIST *routines) const > for (routine= routines; routine; routine= routine->next_global) > { > sp_name *name; > - LEX_CSTRING lex_db; > + LEX_CSTRING lex_db= thd->make_ident_opt_casedn(routine->db, > + lower_case_table_names); > + if (!lex_db.str) > + DBUG_RETURN(TRUE); // EOM // EOM, error was already sent right? (also, in other places) > LEX_CSTRING lex_name; > - thd->make_lex_string(&lex_db, routine->db.str, routine->db.length); > thd->make_lex_string(&lex_name, routine->table_name.str, > routine->table_name.length); Rather inconsistent :( May be, makes sense to create LEX_CSTRING make_lex_string(const char* str, size_t length) const ? > + /* > + routine->db was earlier tested with check_db_name(). > + Now it's lower-cased according to lower_case_table_names. > + It's safe to make a Lex_ident_db_normalized. > + */ > - name= new sp_name(&lex_db, &lex_name, true); > + name= new (thd->mem_root) sp_name(Lex_ident_db_normalized(lex_db), > + lex_name, true); > sp_object_found= sp_find_routine(thd, name, false) != NULL; > thd->get_stmt_da()->clear_warning_info(thd->query_id); > if (! sp_object_found) > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 09af5523c76..70dde7dc0ed 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -2543,6 +2543,37 @@ static void push_new_user(const ACL_USER &user) > } > > > +/** > + Make a database name on mem_root from a String, > + apply lower-case conversion if lower_case_table_names says so. > + Perform database name length limit validation. > + > + @param thd - the THD, to get the warning text from > + @param mem_root - allocate the result on this memory root > + @param dbstr - the String, e.g. with Field::val_str() result > + > + @return - {NULL,0} in case of EOM or a bad database name, > + or a good database name otherwise. > +*/ > +static LEX_STRING make_and_check_db_name(THD *thd, MEM_ROOT *mem_root, > + const String &dbstr) > +{ > + LEX_STRING dbls= lower_case_table_names ? > + lex_string_casedn_root(mem_root, files_charset_info, > + dbstr.ptr(), dbstr.length()) : > + lex_string_strmake_root(mem_root, > + dbstr.ptr(), dbstr.length()); > + if (!dbls.str) > + return LEX_STRING{NULL, 0}; // EOM > + if (dbls.length > SAFE_NAME_LEN) > + { > + sql_print_warning(ER_THD(thd, ER_WRONG_DB_NAME), dbls.str); It's an error log, should every message be localized according to connection-specific locale? > + return LEX_STRING{NULL, 0}; // Bad name > + } > + return dbls; // Good name > +} > + > + > /* > Initialize structures responsible for user/db-level privilege checking > and load information about grants from open privilege tables. > @@ -3680,26 +3702,27 @@ privilege_t acl_get(const char *host, const char *ip, > { > privilege_t host_access(ALL_KNOWN_ACL), db_access(NO_ACL); > uint i; > - size_t key_length; > - char key[ACL_KEY_LENGTH],*tmp_db,*end; > + const char *tmp_db; > acl_entry *entry; > DBUG_ENTER("acl_get"); > > - tmp_db= strmov(strmov(key, safe_str(ip)) + 1, user) + 1; > - end= strnmov(tmp_db, db, key + sizeof(key) - tmp_db); > - > - if (end >= key + sizeof(key)) // db name was truncated > + constexpr size_t key_data_size= ACL_KEY_LENGTH + 2; > + CharBuffer<key_data_size + MY_CS_MBMAXLEN> key; why ACL_KEY_LENGTH + 2 + MY_CS_MBMAXLEN ? > + key.append_bin(Lex_cstring_strlen(safe_str(ip))); > + key.append_char('\0'); > + key.append_bin(Lex_cstring_strlen(user)); > + key.append_char('\0'); there was strmov(strmov(...)) pattern here before. as far as I can see, you can chain too: key.append_bin().append_char() .append_bin().append_char(); let's use that > + tmp_db= key.end(); > + key.append_opt_casedn(files_charset_info, Lex_cstring_strlen(db), > + lower_case_table_names); > + db= tmp_db; > + > + if (key.length() >= key_data_size) // db name was truncated > DBUG_RETURN(NO_ACL); // no privileges for an invalid db name > > - if (lower_case_table_names) > - { > - my_casedn_str(files_charset_info, tmp_db); > - db=tmp_db; > - } > - key_length= (size_t) (end-key); > - > mysql_mutex_lock(&acl_cache->lock); > - if (!db_is_pattern && (entry=acl_cache->search((uchar*) key, key_length))) > + if (!db_is_pattern && > + (entry= acl_cache->search((uchar*) key.ptr(), key.length()))) > { > db_access=entry->access; > mysql_mutex_unlock(&acl_cache->lock); > @@ -5630,28 +5662,26 @@ static GRANT_NAME *name_hash_search(HASH *name_hash, > const char *user, const char *tname, > bool exact, bool name_tolower) > { > - char helping[SAFE_NAME_LEN*2+USERNAME_LENGTH+3]; > - char *hend = helping + sizeof(helping); > - uint len; > + constexpr size_t key_data_size= SAFE_NAME_LEN * 2 + USERNAME_LENGTH + 3; > + CharBuffer<key_data_size + MY_CS_MBMAXLEN> key; > GRANT_NAME *grant_name,*found=0; > HASH_SEARCH_STATE state; > > - char *db_ptr= strmov(helping, user) + 1; > - char *tname_ptr= strnmov(db_ptr, db, hend - db_ptr) + 1; > - if (tname_ptr > hend) > - return 0; // invalid name = not found > - char *end= strnmov(tname_ptr, tname, hend - tname_ptr) + 1; > - if (end > hend) > + key.append_bin(Lex_cstring_strlen(user)); > + key.append_char('\0'); > + key.append_bin(Lex_cstring_strlen(db)); is db already normalized? If yes, then, please, document it with DBUG_ASSERT(ok_for_lower_case_names()); > + key.append_char('\0'); > + key.append_opt_casedn(files_charset_info, Lex_cstring_strlen(tname), > + name_tolower); > + key.append_char('\0'); > + if (key.length() >= key_data_size) > return 0; // invalid name = not found > > - len = (uint) (end - helping); > - if (name_tolower) > - my_casedn_str(files_charset_info, tname_ptr); > - for (grant_name= (GRANT_NAME*) my_hash_first(name_hash, (uchar*) helping, > - len, &state); > + for (grant_name= (GRANT_NAME*) my_hash_first(name_hash, (uchar*) key.ptr(), > + key.length(), &state); > grant_name ; > - grant_name= (GRANT_NAME*) my_hash_next(name_hash,(uchar*) helping, > - len, &state)) > + grant_name= (GRANT_NAME*) my_hash_next(name_hash, (uchar*) key.ptr(), > + key.length(), &state)) > { > if (exact) > { > @@ -8826,33 +8856,28 @@ static bool check_grant_db_routine(THD *thd, const char *db, HASH *hash) > bool check_grant_db(THD *thd, const char *db) > { > Security_context *sctx= thd->security_ctx; > - char helping [SAFE_NAME_LEN + USERNAME_LENGTH+2], *end; > - char helping2 [SAFE_NAME_LEN + USERNAME_LENGTH+2], *tmp_db; > - uint len, UNINIT_VAR(len2); > + constexpr size_t key_data_size= SAFE_NAME_LEN + USERNAME_LENGTH + 2; > + CharBuffer<key_data_size + MY_CS_MBMAXLEN> key, key2; > bool error= TRUE; > > - tmp_db= strmov(helping, sctx->priv_user) + 1; > - end= strnmov(tmp_db, db, helping + sizeof(helping) - tmp_db); > - > - if (end >= helping + sizeof(helping)) // db name was truncated > - return 1; // no privileges for an invalid db name > - > - if (lower_case_table_names) > - { > - end = tmp_db + my_casedn_str(files_charset_info, tmp_db); > - db=tmp_db; > - } > + key.append_bin(Lex_cstring_strlen(sctx->priv_user)); > + key.append_char('\0'); > + key.append_opt_casedn(files_charset_info, Lex_cstring_strlen(db), > + lower_case_table_names); > + key.append_char('\0'); > > - len= (uint) (end - helping) + 1; > + if (key.length() >= key_data_size) // db name was truncated > + return 1; // no privileges for an invalid db name > > /* > If a role is set, we need to check for privileges here as well. > */ > if (sctx->priv_role[0]) > { > - end= strmov(helping2, sctx->priv_role) + 1; > - end= strnmov(end, db, helping2 + sizeof(helping2) - end); > - len2= (uint) (end - helping2) + 1; > + key2.append_bin(Lex_cstring_strlen(sctx->priv_role)); > + key2.append_char('\0'); > + key2.append_bin(Lex_cstring_strlen(db)); No append_opt_casedn() here? Looks wrong. > + key2.append_char('\0'); > } > > > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 214118d8431..decb9a9064e 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -4918,11 +4948,46 @@ class THD: public THD_count, /* this must be first */ > my_message(ER_NO_DB_ERROR, ER(ER_NO_DB_ERROR), MYF(0)); > return TRUE; > } > + return false; > + } > + > + /* > + Copy the current database to the argument. Use the current arena to > + allocate memory for a deep copy: current database may be freed after > + a statement is parsed but before it's executed. > + > + Can only be called by owner of thd (no mutex protection) > + */ > + bool copy_db_to(LEX_CSTRING *to) > + { > + if (check_if_current_db_is_set_with_error()) > + return true; > > to->str= strmake(db.str, db.length); > to->length= db.length; > return to->str == NULL; /* True on error */ > } > + > + /* > + Make a normalized copy of the current database. > + Raise an error if no current database is set. > + Note, in case of lower_case_table_names==2, thd->db can contain the > + name in arbitrary case typed by the user, so it must be lower-cased. > + For other lower_case_table_names values the name is already in > + its normalized case, so it's copied as is. > + */ > + Lex_ident_db_normalized copy_db_normalized() > + { > + if (check_if_current_db_is_set_with_error()) > + return Lex_ident_db_normalized(); > + LEX_STRING ident= make_ident_opt_casedn(db, lower_case_table_names == 2); LEX_STRING or LEX_CSTRING ? > + /* > + A non-empty thd->db is always known to satisfy check_db_name(). > + So after optional lower-casing above it's safe to > + make Lex_ident_db_normalized. > + */ > + return Lex_ident_db_normalized(ident); > + } > /* Get db name or "". Use for printing current db */ > const char *get_db() > { return safe_str(db.str); } > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > index a84fd29affc..8cafad65538 100644 > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -4186,6 +4186,18 @@ bool LEX::copy_db_to(LEX_CSTRING *to) > return thd->copy_db_to(to); > } > > + > +Lex_ident_db_normalized LEX::copy_db_normalized() > +{ > + if (sphead && sphead->m_name.str) > + { > + DBUG_ASSERT(sphead->m_db.str && sphead->m_db.length); Please, DBUG_ASSERT(sphead->m_db.str); DBUG_ASSERT(sphead->m_db.length); > + return thd->to_ident_db_normalized_with_error(sphead->m_db); > + } > + return thd->copy_db_normalized(); > +} > + > + > /** > Initialize offset and limit counters. > > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index 1fcafd6ec08..0f39a13632a 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -3856,30 +3859,31 @@ static bool show_status_array(THD *thd, const char *wild, > names until lp:1306875 has been fixed. > TODO: remove once lp:1306875 has been addressed. > */ > - if (!(*prefix) && !strncasecmp(name_buffer, "wsrep", strlen("wsrep"))) > + if (!prefix.length && > + !strncasecmp(name_buffer.ptr(), "wsrep", strlen("wsrep"))) > { > is_wsrep_var= TRUE; > } > #endif /* WITH_WSREP */ > > if (ucase_names) > - my_caseup_str(system_charset_info, name_buffer); > + name_buffer.append_caseup(system_charset_info, var_name); > else > { > - my_casedn_str(system_charset_info, name_buffer); > - DBUG_ASSERT(name_buffer[0] >= 'a'); > - DBUG_ASSERT(name_buffer[0] <= 'z'); > - > + name_buffer.append_casedn(system_charset_info, var_name); > // WSREP_TODO: remove once lp:1306875 has been addressed. > if (IF_WSREP(is_wsrep_var == FALSE, 1) && > status_var) > - name_buffer[0]-= 'a' - 'A'; > + { > + char *ptr= (char*) name_buffer.ptr(); > + if (ptr[0] >= 'a' && ptr[0] <= 'z') > + ptr[0]-= 'a' - 'A'; > + } is it still needed? > } > > > restore_record(table, s->default_values); > - table->field[0]->store(name_buffer, strlen(name_buffer), > - system_charset_info); > + table->field[0]->store(name_buffer.to_lex_cstring(), system_charset_info); > > /* > Compare name for types that can't return arrays. We do this to not > @@ -4355,18 +4361,15 @@ bool get_lookup_field_values(THD *thd, COND *cond, bool fix_table_name_case, > > if (lower_case_table_names && !rc) > { > - /* > - We can safely do in-place upgrades here since all of the above cases > - are allocating a new memory buffer for these strings. > - */ > if (lookup_field_values->db_value.str && lookup_field_values->db_value.str[0]) > - my_casedn_str(system_charset_info, > - (char*) lookup_field_values->db_value.str); > + lookup_field_values->db_value= thd->make_ident_casedn( > + lookup_field_values->db_value); > + > if (fix_table_name_case && > lookup_field_values->table_value.str && > lookup_field_values->table_value.str[0]) > - my_casedn_str(system_charset_info, > - (char*) lookup_field_values->table_value.str); > + lookup_field_values->table_value= thd->make_ident_casedn( > + lookup_field_values->table_value); Why is it even needed? If db/table names are compared case insensitively, it shouldn't matter if they're lowercased or not, should it? > } > > return rc; > diff --git a/sql/sql_type_fixedbin.h b/sql/sql_type_fixedbin.h > index 8aaa7582cd5..e9728b78a3d 100644 > --- a/sql/sql_type_fixedbin.h > +++ b/sql/sql_type_fixedbin.h > @@ -267,8 +267,7 @@ class Type_handler_fbt: public Type_handler > void print(String *str, enum_query_type query_type) override > { > StringBuffer<FbtImpl::max_char_length()+64> tmp; > - tmp.append(singleton()->name().lex_cstring()); > - my_caseup_str(&my_charset_latin1, tmp.c_ptr()); > + tmp.copy_caseup(&my_charset_latin1, singleton()->name().lex_cstring()); why bother? > str->append(tmp); > str->append('\''); > m_value.to_string(&tmp); > diff --git a/storage/innobase/dict/dict0load.cc b/storage/innobase/dict/dict0load.cc > index bf59fed9ee4..104a8dc8780 100644 > --- a/storage/innobase/dict/dict0load.cc > +++ b/storage/innobase/dict/dict0load.cc > @@ -2955,7 +2955,7 @@ dict_load_foreign( > > foreign->foreign_table_name = mem_heap_strdupl( > foreign->heap, (char*) field, len); > - dict_mem_foreign_table_name_lookup_set(foreign, TRUE); > + foreign->foreign_table_name_lookup_set(); please, ask Marko to review InnoDB changes > > const size_t foreign_table_name_len = len; > const size_t table_name_len = strlen(table_name); > diff --git a/storage/perfschema/pfs_instr_class.h b/storage/perfschema/pfs_instr_class.h > index f353c410d4c..d083db63968 100644 > --- a/storage/perfschema/pfs_instr_class.h > +++ b/storage/perfschema/pfs_instr_class.h > @@ -265,6 +265,40 @@ struct PFS_table_share_key > char m_hash_key[PFS_TABLESHARE_HASHKEY_SIZE]; > /** Length in bytes of @c m_hash_key. */ > uint m_key_length; > + > + size_t available_length() const > + { > + return sizeof(m_hash_key) - m_key_length; > + } > + char *end() > + { > + return m_hash_key + m_key_length; > + } > + // Append and 0-terminate a string with an optional lower-case conversion > + void append_opt_casedn_z(CHARSET_INFO *cs, > + const char *str, size_t length, > + bool casedn) > + { > + DBUG_ASSERT(length <= sizeof(m_hash_key)); // Expect valid db/tbl names > + MY_STRCOPY_STATUS st; > + size_t dst_length= available_length(); > + if (dst_length > 0) > + { > + dst_length--; > + /* > + The code below uses copy_fix() instead of memcpy() to make > + sure we don't break a multi-byte character in the middle. > + */ > + m_key_length+= (uint) (casedn ? > + cs->casedn(str, length, end(), dst_length) : > + cs->copy_fix(end(), dst_length, > + str, length, length, &st)); may be let's not do copy_fix here? there are table names and in a perfschema, for PFS_table_share_key. memcpy would be enough. > + m_hash_key[m_key_length++]= '\0'; > + } > + } > + void set(bool temporary, > + const char *schema_name, size_t schema_name_length, > + const char *table_name, size_t table_name_length); > }; > > /** Table index or 'key' */ > diff --git a/storage/perfschema/pfs_program.cc b/storage/perfschema/pfs_program.cc > index de456610519..6c35a745d30 100644 > --- a/storage/perfschema/pfs_program.cc > +++ b/storage/perfschema/pfs_program.cc > @@ -118,31 +118,20 @@ static void set_program_key(PFS_program_key *key, > */ > > char *ptr= &key->m_hash_key[0]; > + const char *end= ptr + sizeof(key->m_hash_key) - 1; > > ptr[0]= object_type; > ptr++; > > if (object_name_length > 0) > - { > - char tmp_object_name[COL_OBJECT_NAME_SIZE + 1]; > - memcpy(tmp_object_name, object_name, object_name_length); > - tmp_object_name[object_name_length]= '\0'; > - my_casedn_str(system_charset_info, tmp_object_name); > - memcpy(ptr, tmp_object_name, object_name_length); > - ptr+= object_name_length; > - } > + ptr+= system_charset_info->casedn(object_name, object_name_length, > + ptr, end - ptr); > ptr[0]= 0; > ptr++; > > if (schema_name_length > 0) > - { > - char tmp_schema_name[COL_OBJECT_SCHEMA_SIZE + 1]; > - memcpy(tmp_schema_name, schema_name, schema_name_length); > - tmp_schema_name[schema_name_length]='\0'; > - my_casedn_str(system_charset_info, tmp_schema_name); > - memcpy(ptr, tmp_schema_name, schema_name_length); > - ptr+= schema_name_length; > - } > + ptr+= system_charset_info->casedn(schema_name, schema_name_length, > + ptr, end - ptr); That's strange. casedn of schema when lower_case_table_names==0 ? > ptr[0]= 0; > ptr++; > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 4

review for MDEV-32839 LONG UNIQUE gives error when used with REPLACE
by Alexander Barkov 12 Dec '23

12 Dec '23

Hello Sergei, - This patch: https://github.com/MariaDB/server/commit/4f2d0541b501a7a27bf44aa16821d09395… cleanup: remove innodb-specific code around update_auto_increment() is OK to push. Please write in the commit comments that it was a dead code and that InnoDB team does know why this code was there. - This patch https://github.com/MariaDB/server/commit/c8d0f3370b7d77ecb07f8f1357da6bb1f0… MDEV-32839 LONG UNIQUE gives error when used with REPLACE is OK. I have only a proposal about tests. You added tests above the "# End of 10.5 tests" line, but the patch is for 10.6. Also please add tests for at lease these engines: - MyISAM - InnoDB - Spider as well as partitioned tables for the same engines, because your patch for ha_partition.cc changes the code related to need_info_for_auto_inc(), which has a special implementation in ha_partition and ha_spider. - This patch: https://github.com/MariaDB/server/commit/94f26086663b7c8dbcd6b26930b64de36a… cleanup: remove partition-specific code around update_auto_increment() > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc > index 25bbcfdd75ce0..e174ef8d21940 100644 > --- a/sql/ha_partition.cc > +++ b/sql/ha_partition.cc > @@ -4308,6 +4308,7 @@ int ha_partition::external_lock(THD *thd, int lock_type) > m_part_info->part_expr->walk(&Item::register_field_in_read_map, 1, 0); > if ((error= m_part_info->vers_set_hist_part(thd))) > goto err_handler; > + need_info_for_auto_inc(); auto-increment code is often done under a condition like this: if (table->next_number_field && buf == table->record[0]) { some_auto_increment code; } This includes the code that you are removing from ha_partition::write_row() Is it ok to call need_info_for_auto_inc() unconditionally here? > } > DBUG_RETURN(0); > > @@ -4623,33 +4624,8 @@ int ha_partition::write_row(const uchar * buf) > */ > if (have_auto_increment) > { > - if (!table_share->next_number_keypart) > - if (unlikely(error= update_next_auto_inc_val())) > - goto exit; Here there was an extra condition !table_share->next_number_keypart before calling update_next_auto_inc_val(). Why was it needed? Why it's not needed in the new version of external_lock() before the call for need_info_for_auto_inc()? Also, the old code checked the return value. The new code int external_code() does not check the result of need_info_for_auto_inc(). Can't the behavior change because of this change? > - > - /* > - If we have failed to set the auto-increment value for this row, > - it is highly likely that we will not be able to insert it into > - the correct partition. We must check and fail if necessary. > - */ > if (unlikely(error= update_auto_increment())) > goto exit; > - > - /* > - Don't allow generation of auto_increment value the partitions handler. > - If a partitions handler would change the value, then it might not > - match the partition any longer. > - This can occur if 'SET INSERT_ID = 0; INSERT (NULL)', > - So allow this by adding 'MODE_NO_AUTO_VALUE_ON_ZERO' to sql_mode. > - The partitions handler::next_insert_id must always be 0. Otherwise > - we need to forward release_auto_increment, or reset it for all > - partitions. > - */ > - if (table->next_number_field->val_int() == 0) > - { > - table->auto_increment_field_not_null= TRUE; > - thd->variables.sql_mode|= MODE_NO_AUTO_VALUE_ON_ZERO; > - } Why is it safe to remove this code? > } > old_map= dbug_tmp_use_all_columns(table, &table->read_set); > error= m_part_info->get_partition_id(m_part_info, &part_id, &func_value); > @@ -11012,10 +10988,7 @@ void ha_partition::get_auto_increment(ulonglong offset, ulonglong increment, > else > { > THD *thd= ha_thd(); > - /* > - This is initialized in the beginning of the first write_row call. > - */ > - DBUG_ASSERT(part_share->auto_inc_initialized); > + update_next_auto_inc_val(); > /* > Get a lock for handling the auto_increment in part_share > for avoiding two concurrent statements getting the same number. > diff --git a/sql/ha_partition.h b/sql/ha_partition.h > index d75e38ec272a5..0ac8f696fcf55 100644 > --- a/sql/ha_partition.h > +++ b/sql/ha_partition.h > @@ -1408,9 +1408,8 @@ class ha_partition final :public handler > { > ulonglong nr= (((Field_num*) field)->unsigned_flag || > field->val_int() > 0) ? field->val_int() : 0; > + update_next_auto_inc_val(); > lock_auto_increment(); > - DBUG_ASSERT(part_share->auto_inc_initialized || > - !can_use_for_auto_inc_init()); > /* must check when the mutex is taken */ > if (nr >= part_share->next_auto_inc_val) > part_share->next_auto_inc_val= nr + 1;

2 2

MDEV-31404 Review
by Kristian Nielsen 11 Dec '23

11 Dec '23

Hi Monty, here is my review of the patch for MDEV-31404: > commit d51086a9f1b1b6a66c7ba8eb6a6876b433fa118f (origin/bb-11.4-MDEV-31404) > Author: Monty <monty(a)mariadb.org> > Date: Sun Dec 3 21:42:44 2023 +0200 > > MDEV-31404 Implement binlog_space_limit > > binlog_space_limit is a variable in Percona server used to limit the total > size of all binary logs. > > This implementation is based on code from Percona server 5.7. General comment: The --slave-connections-needed-for-purge defaults to 1. I think this means that --log-expire-days will have no effect on systems that have binlog enabled but no slaves configured. This is something to be aware of for backwards compatibility. What I do not like about this is the consequence it will have for all systems that have binlog enabled with some --expire-log-days set but no slaves configured. After upgrade, these systems will silently start accumulating binlogs without limit, possibly filling up disk eventually. On the other hand, it seems good to have --slave-connections-needed-for-purge enabled for the new --max-binlog-total-size option; as you pointed out, otherwise a single large transaction could immediately cause deletion of all binlogs when no slaves are connected. And having --slave-connections-needed-for-purge affect purge from --max-binlog-total-size but not for --expire-log-days seems inconsistent. In the end, maybe it isn't too serious. Many small and default-configured systems will probably be able to run for ages without binlogs getting excessive due to low traffic; and it will be easy for DBAs to just delete the binlogs if needed, or google the --slave-connections-needed-for-purge if they care enough. Two suggestions to make it easier for the DBAs to know how to handle this: 1. How about emitting a note or warning in the error log, if purge from --log-expire-days=N gets delayed for more than 2*N because no slaves are connected and --slave-connections-needed-for-purge=1 ? Note: Purge of binlogs prevented for <seconds> because no slaves are connected. To allow purge of binlogs with no slaves connected, set --slave-connections-needed-for-purge=0 2. Mention in the changelogs for 11.4 that binlogs will by default no longer be purged when no slaves are configured, and point to some documentation about how to handle this for the DBA. I think with those two suggestions it should be fine. Detailed comments on the patch below. - Kristian. > diff --git a/mysql-test/suite/binlog_encryption/binlog_index-master.opt b/mysql-test/suite/binlog_encryption/binlog_index-master.opt > new file mode 100644 > index 00000000000..f2673d08020 > --- /dev/null > +++ b/mysql-test/suite/binlog_encryption/binlog_index-master.opt > @@ -0,0 +1 @@ > +--slave_connections_needed_for_purge=0 Is this needed? If so, would it be better to put this in a general my.cnf for the entire binlog_encryption suite, like you did for mysql-test/suite/binlog/my.cnf ? So that a similar .opt will not need to be added manually for each new binlog_encryption test that will need it. > --- a/mysql-test/suite/perfschema/t/show_sanity.test > +++ b/mysql-test/suite/perfschema/t/show_sanity.test > @@ -533,6 +533,7 @@ insert into test.sanity values > ("JUNK: GLOBAL-ONLY", "I_S.SESSION_VARIABLES", "MAX_BINLOG_CACHE_SIZE"), > ("JUNK: GLOBAL-ONLY", "I_S.SESSION_VARIABLES", "MAX_BINLOG_SIZE"), > ("JUNK: GLOBAL-ONLY", "I_S.SESSION_VARIABLES", "MAX_BINLOG_STMT_CACHE_SIZE"), > + ("JUNK: GLOBAL-ONLY", "I_S.SESSION_VARIABLES", "MAX_BINLOGS_SIZE"), I think this is a left-over from previous naming, should probably be "MAX_BINLOG_TOTAL_SIZE ? > +FLUSH LOGS; > +FLUSH LOGS; > +FLUSH LOGS; > +source include/show_binary_logs.inc; > +show status like "binlog_disk_use"; I think you will need --source include/wait_for_binlog_checkpoint.inc after each of these FLUSH LOGS. Otherwise random delay of binlog checkpoint could cause the binlog_disk_use value to fluctuate and get rare random test failures. (Or alternatively consider if you could omit the select of binlog_disk_use completely, since it will probably differ between versions as event formats change.) Adding a small sleep in binlog_background_thread() just before calling mysql_bin_log.mark_xid_done() can usually reproduce the random failures from delayed binlog checkpoint. > +INSERT INTO t1 VALUES (0,repeat("a",3000)); > +show status like "binlog_disk_use"; > +Variable_name Value > +Binlog_disk_use 3848 > +# First binary should be binary.000004 > +show binary logs; > +Log_name File_size > +binary.000004 # > +INSERT INTO t1 VALUES (2,repeat("b",10)); > +# First binary should be binary.000004 > +show binary logs; > +Log_name File_size > +binary.000004 # > +binary.000005 # Here, I don't understand why binary.000004 is not purged. The binary.000004 contains the large transaction 'repeat("a", 3000)', so the binlog total size is too large. And the transaction 'repeat("n",10)' caused a binlog rotation. So why does that not cause binary.000004 to be purged due to --max-binlog-total-size=1500 ? > +show status like "binlog_disk_use"; > +Variable_name Value > +Binlog_disk_use 3647 > +INSERT INTO t1 VALUES (7,repeat("g",3000)); > +# binary.000001 should be deleted now > +show binary logs; > +Log_name File_size > +binary.000002 # > +binary.000003 # Similarly here, why binary.000002 is not purged? > +INSERT INTO t1 VALUES (4,repeat("d",3000)); > +--echo # First binary should be binary.000011 > +source include/show_binary_logs.inc; > + > +RESET MASTER; Idea: Here you could (or ask one of the other devs) add a test that the SHOW STATUS LIKE "binlog_disk_use" is equal to the size of the files on disk, to check that the running calculation of disk use doesn't miss the odd byte or two. > + if ((longlong) (binlog_space_total + binlog_pos - found_space <= > + binlog_space_limit)) > + break; // Found enough The (longlong) cast here looks misplaced. It's casting the result of the (X <= Y) comparison. I think the cast can just be removed, all the variables seem to be ulonglong type. > +static bool waiting_for_slave_to_change_binlog= 0; > +static ulonglong purge_sending_new_binlog_file= 0; I think a short comment here on these variables would be good, explaining their purpose. And same on the declaration of Atomic_counter<ulonglong> sending_new_binlog_file. > + DBUG_ASSERT(!is_relay_log || binlog_xid_count_list.is_empty()); > + if (!is_relay_log) > { > - I_List_iterator<xid_count_per_binlog> it(binlog_xid_count_list); > - while ((b= it++) && > - 0 != strncmp(log_file_name_arg+dirname_length(log_file_name_arg), > - b->binlog_name, b->binlog_name_len)) > - ; > + xid_count_per_binlog *b; > + mysql_mutex_lock(&LOCK_xid_list); Good catch that we were uselessly doing xid_count operations on the relay log, thanks! > +int MYSQL_BIN_LOG::count_binlog_space() > +{ > + int error; > + LOG_INFO log_info; > + DBUG_ENTER("count_binlog_space"); > + if (is_relay_log || !binlog_space_limit) > + DBUG_RETURN(0); Maybe this if (is_relay_log || !binlog_space_limit) could be an assert? We probably should not be calling this function at all in that case. It looks like that binlog_space_limit is already checked in all callers, not sure about relay log. (A small point, just something to consider as you like). > @@ -7608,7 +7807,8 @@ int MYSQL_BIN_LOG::rotate(bool force_rotate, bool* check_purge) > @retval > nonzero - error in rotating routine. > */ > -void MYSQL_BIN_LOG::purge() > + > +void MYSQL_BIN_LOG::purge(bool all) Would be good with a small comment here explaining the meaning of the new parameter `all`. Minor stuff: Commit message: > Binlog file file sizes are checked on startup Typo (s/file file/file/). > when updating the binary log on on binary log rotation Typo (s/on on/and on/ ?) > Without this option max-binlog-total-size would potentially deleted > binlogs needed by slaves on server startup or when a slave disconnects Typo s/deleted/delete/. > +static Sys_var_ulonglong Sys_max_binlog_total_size( > + "max_binlog_total_size", > + "Maximum space to use for all binary logs. Extra logs are deleted on " > + "server start, log rotation, FLUSH LOGGS or when writing to binlog. " > + "Default is 0, which means no size restrictions." > + "See also slave_connections_needed_for_purge", Missing space at end of line (s/restrictions."/restrictions. "/). > +static Sys_var_ulong Sys_slave_connections_needed_for_purge( > + "slave_connections_needed_for_purge", > + "Number of minimum connected slaves needed for automatic binary " > + "log purge with max_binlog_total_size, binlog_expire_logs_seconds " > + "or binlog_expire_logs_days.", Suggestion for better wording: "Minimum number of connected slaves required for automatic binary " "log purge with max_binlog_total_size, binlog_expire_logs_seconds " "or binlog_expire_logs_days."

2 2