Hello Sergei, This is a new version: https://github.com/MariaDB/server/commit/0f52062f908a3dbe64f7ba6929584a89611... Please see comments below: On 12/28/23 22:44, Sergei Golubchik wrote:
Hi, Alexander,
Few comments (I've omitted everything where I'd reply simply "ok").
On Dec 28, Alexander Barkov wrote:
Could you instead implement a constructor that understands
const Lex_ident_column primary_key_name= {STRING_WITH_LEN("PRIMARY")};
The are constructors: Lex_ident_xxx(const char *str, size_t length) Lex_ident_xxx(const LEX_CSTRING &str)
All these notations work:
Lex_ident_db MYSQL_SCHEMA_NAME("mysql"_LEX_CSTRING); Lex_ident_db MYSQL_SCHEMA_NAME2{"mysql"_LEX_CSTRING}; Lex_ident_db MYSQL_SCHEMA_NAME3= {STRING_WITH_LEN("mysql")}; Lex_ident_db MYSQL_SCHEMA_NAME4{STRING_WITH_LEN("mysql")};
Then let's use a shorter syntax, if possible? Not
const Lex_ident_column primary_key_name= Lex_ident_column({STRING_WITH_LEN("PRIMARY")});
but
const Lex_ident_column primary_key_name= {STRING_WITH_LEN("PRIMARY")};
or anything else out of these four variants, they're all fine.
The client library should be fixed to know new collations utf8mb3_general_as_ci and utf8mb4_general_as_ci and their IDs.
is there a CONC issue for that?
I just created it: CONC-682 Collation IDs for utf8mbX_general_as_ci
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;
These classes are small enough. I think there are no needs for a template. We have developers who don't like templates, unless they are absolutely necessary. And this not an absolutly necessary case :)
We also have developers who don't like multiple nearly-identical copies of the same copy-pasted code. But ok, it's a matter of taste, so up to you here.
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?
I agree it should be Lex_ident_func. However, "grep LEX_CSTRING.func_name_cstring" shows 388 instances. It's going to be a huge change. Can we do it in a separate patch?
okay. it just looks wrong.
+ 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/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?
I'm not sure how exactly it should be compared here: 1. Like a normal table, according to Lex_ident_table 2. Like an I_S table, according to Lex_ident_i_s_table 3. Case sensitively, like it was in the old code.
I decided to go with #3, to preserve the old behavior.
Should we report it as a separate MDEV and ask Sergey Petrunya to check?
It's an I_S table name, right? So, should be #2.
Changed to #2.
return true; } return false; 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 @@ -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
I'd like to avoid to change the logic that's not directly related to the task.
that's not a logic change, it's a dummy argument that someone used as a hack to trick a compiler into not optimizing something out.
by passing alias.str you give it a deeper meaning, surely it must be important to pass specifically this member of the alias structure, otherwise why would anyone would've written it like that?
So, please change it back to something simple. Even reverting to the old (uchar *)&alias is fine. Or use NULL, it doesn't matter.
Changed to &alias.
DBUG_RETURN(TRUE);
if (!(flags & MYSQL_OPEN_IGNORE_KILLED) && thd->killed) 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?
Reported as:
MDEV-33119 User is case insensitive in INFORMATION_SCHEMA.VIEWS
This patch fixes the problem. Should be backported to 10.4.
I love how all these bugs came into light now
+ if (my_charset_utf8mb3_general1400_as_ci.streq( + tables->definer.user, + Lex_cstring_strlen(sctx->priv_user)) &&
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org