Re: [Maria-developers] 96b40a5e2f5: MDEV-27744 InnoDB: Failing assertion: !cursor->index->is_committed() in row0ins.cc (from row_ins_sec_index_entry_by_modify) | Assertion `0' failed in row_upd_sec_index_entry (debug) | Corruption
![](https://secure.gravatar.com/avatar/39b623a1559cf9c69ac3d9d4fb44e7fe.jpg?s=120&d=mm&r=g)
Hi, Alexander, Some comments/questions here. I'll need to look at the patch again after I understand why you did it this way better. On Apr 18, Alexander Barkov wrote:
revision-id: 96b40a5e2f5 (mariadb-10.3.33-61-g96b40a5e2f5) parent(s): 7355f7b1f5c author: Alexander Barkov committer: Alexander Barkov timestamp: 2022-04-07 16:22:17 +0400 message:
MDEV-27744 InnoDB: Failing assertion: !cursor->index->is_committed() in row0ins.cc (from row_ins_sec_index_entry_by_modify) | Assertion `0' failed in row_upd_sec_index_entry (debug) | Corruption
A detailed comment here is a must. What was the problem, how it was fixed.
diff --git a/sql/item_func.h b/sql/item_func.h index 754b1cd1eb2..d844e45280a 100644 --- a/sql/item_func.h +++ b/sql/item_func.h @@ -35,6 +35,36 @@ extern "C" /* Bug in BSDI include file */ #include <cmath>
+template<class FUNC> +FUNC* item_func_create_2_3_args(THD *thd, const LEX_CSTRING &name, + List<Item> *item_list) +{ + int arg_count= item_list ? item_list->elements : 0; + + switch (arg_count) { + case 2: + { + Item *param_1= item_list->pop(); + Item *param_2= item_list->pop(); + return new (get_thd_memroot(thd)) FUNC(thd, param_1, param_2); + } + case 3: + { + Item *param_1= item_list->pop(); + Item *param_2= item_list->pop(); + Item *param_3= item_list->pop(); + return new (get_thd_memroot(thd)) FUNC(thd, param_1, param_2, param_3);
A constructor that takes a List<Item> wouldn't need a switch. And wouldn't need a template either, your create_check_args_ge() isn't a template.
+ break; + } + default: + my_error(ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT, MYF(0), name.str); + break; + } + + return NULL; +} + + class Item_func :public Item_func_or_sum { void sync_with_sum_func_and_with_field(List<Item> &list); @@ -56,8 +86,41 @@ class Item_func :public Item_func_or_sum bool check_argument_types_can_return_text(uint start, uint end) const; bool check_argument_types_can_return_date(uint start, uint end) const; bool check_argument_types_can_return_time(uint start, uint end) const; + + void print_schema_name_if_needed(String *to) const + { + const LEX_CSTRING schema_name= schema_name_cstring(); + if (schema_name.length) + { + to->append(schema_name); + to->append('.'); + }
this should check that schema_name isn't the same as the schema in the path (implicit path, assumed by the current sql_mode).
+ } + void print_maybe_qualified_name(String *to) const + { + print_schema_name_if_needed(to); + to->append(func_name()); + } + public:
+ // Print an error message for a builtin-schema qualified function call + static void wrong_param_count_error(const LEX_CSTRING &schema_name, + const LEX_CSTRING &func_name); + + // Check that the number of arguments is greater or equal to "expected" + static bool create_check_args_ge(const LEX_CSTRING &name, + const List<Item> *item_list, + uint expected) + { + if (!item_list || item_list->elements < expected) + { + my_error(ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT, MYF(0), name.str);
what's the point of your wrong_param_count_error() if you aren't using it?
+ return true; + } + return false; + } + table_map not_null_tables_cache;
enum Functype { UNKNOWN_FUNC,EQ_FUNC,EQUAL_FUNC,NE_FUNC,LT_FUNC,LE_FUNC, diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc index 75abd9906cd..e45fe6ef6d7 100644 --- a/sql/item_strfunc.cc +++ b/sql/item_strfunc.cc @@ -3204,6 +3239,39 @@ static String *default_pad_str(String *pad_str, CHARSET_INFO *collation) return pad_str; }
+ +Item_func_lpad* +Item_func_lpad::create(THD *thd, const LEX_CSTRING &name, + List<Item> *item_list) +{ + return item_func_create_2_3_args
(thd, name, item_list);
may be item_func_create_2_3_args should be in Item_func? Like create_check_args_ge() is? and may be create_check_args_ge() should be a template too?
+} + + +Item_func_lpad_oracle* +Item_func_lpad_oracle::create(THD *thd, const LEX_CSTRING &name, + List<Item> *item_list) +{ + return item_func_create_2_3_args
(thd, name, item_list); +} + + +Item_func_rpad* +Item_func_rpad::create(THD *thd, const LEX_CSTRING &name, + List<Item> *item_list) +{ + return item_func_create_2_3_args (thd, name, item_list); +} + + +Item_func_rpad_oracle* +Item_func_rpad_oracle::create(THD *thd, const LEX_CSTRING &name, + List<Item> *item_list) +{ + return item_func_create_2_3_args (thd, name, item_list); +} + + bool Item_func_pad::fix_length_and_dec() { if (arg_count == 3) diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc index 7f1b362d91d..6b0a351f76e 100644 --- a/sql/sql_lex.cc +++ b/sql/sql_lex.cc @@ -1542,7 +1541,18 @@ int Lex_input_stream::lex_one_token(YYSTYPE *yylval, THD *thd) if (lex->parsing_options.lookup_keywords_after_qualifier) next_state= MY_LEX_IDENT_OR_KEYWORD; else - next_state= MY_LEX_IDENT_START; // Next is ident (not keyword) + { + /* + Next is: + - A qualified func with a special syntax: + mariadb_schema.REPLACE('a','b','c') + mariadb_schema.SUSTRING('a',1,2) + mariadb_schema.TRIM('a') + - Or an identifier otherwise. No keyword lookup is done, + all keywords are treated as identifiers. + */ + next_state= MY_LEX_IDENT_OR_QUALIFIED_SPECIAL_FUNC;
I don't understand why you did it in the lexer and not in the parser, like - | REPLACE '(' expr ',' expr ',' expr ')' + | opt_schema REPLACE '(' expr ',' expr ',' expr ')' or (if the above won't work in LARL(1) parser) - | ident_cli '.' ident_cli '(' opt_expr_list ')' + | ident_cli '.' func_2nd_part with + func_2nd_part: ident_cli '(' opt_expr_list ')' + | REPLACE '(' expr ',' expr ',' expr ')' etc (not reviewing lexer changes anymore, until I understand the above)
+ } if (!ident_map[(uchar) yyPeek()]) // Probably ` or " next_state= MY_LEX_START; return((int) c);
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
![](https://secure.gravatar.com/avatar/e1d656a55b3b29f2f0872cde35c6143c.jpg?s=120&d=mm&r=g)
Hello Sergei, On 4/19/22 1:17 AM, Sergei Golubchik wrote:
Hi, Alexander,
Some comments/questions here. I'll need to look at the patch again after I understand why you did it this way better.
Thanks for the review. My answers follow below. A new version is here: https://github.com/MariaDB/server/commit/50bb0987e10e660326763de38231d63cfc1...
On Apr 18, Alexander Barkov wrote:
revision-id: 96b40a5e2f5 (mariadb-10.3.33-61-g96b40a5e2f5) parent(s): 7355f7b1f5c author: Alexander Barkov committer: Alexander Barkov timestamp: 2022-04-07 16:22:17 +0400 message:
MDEV-27744 InnoDB: Failing assertion: !cursor->index->is_committed() in row0ins.cc (from row_ins_sec_index_entry_by_modify) | Assertion `0' failed in row_upd_sec_index_entry (debug) | Corruption
A detailed comment here is a must. What was the problem, how it was fixed.
Added.
diff --git a/sql/item_func.h b/sql/item_func.h index 754b1cd1eb2..d844e45280a 100644 --- a/sql/item_func.h +++ b/sql/item_func.h @@ -35,6 +35,36 @@ extern "C" /* Bug in BSDI include file */ #include <cmath>
+template<class FUNC> +FUNC* item_func_create_2_3_args(THD *thd, const LEX_CSTRING &name, + List<Item> *item_list) +{ + int arg_count= item_list ? item_list->elements : 0; + + switch (arg_count) { + case 2: + { + Item *param_1= item_list->pop(); + Item *param_2= item_list->pop(); + return new (get_thd_memroot(thd)) FUNC(thd, param_1, param_2); + } + case 3: + { + Item *param_1= item_list->pop(); + Item *param_2= item_list->pop(); + Item *param_3= item_list->pop(); + return new (get_thd_memroot(thd)) FUNC(thd, param_1, param_2, param_3);
A constructor that takes a List<Item> wouldn't need a switch. And wouldn't need a template either, your create_check_args_ge() isn't a template.
Right. I added new constructors with List<Item>. Now the template item_func_create_2_3_args is not needed any more. create() methods now look like this in the new version: Item_func_lpad_oracle* Item_func_lpad_oracle::create(THD *thd, const LEX_CSTRING &name, List<Item> *item_list) { if (create_check_args_between(name, item_list, 2, 3)) return NULL; return new (thd->mem_root) Item_func_lpad_oracle(thd, *item_list); }
+ break; + } + default: + my_error(ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT, MYF(0), name.str); + break; + } + + return NULL; +} + + class Item_func :public Item_func_or_sum { void sync_with_sum_func_and_with_field(List<Item> &list); @@ -56,8 +86,41 @@ class Item_func :public Item_func_or_sum bool check_argument_types_can_return_text(uint start, uint end) const; bool check_argument_types_can_return_date(uint start, uint end) const; bool check_argument_types_can_return_time(uint start, uint end) const; + + void print_schema_name_if_needed(String *to) const + { + const LEX_CSTRING schema_name= schema_name_cstring(); + if (schema_name.length) + { + to->append(schema_name); + to->append('.'); + }
this should check that schema_name isn't the same as the schema in the path (implicit path, assumed by the current sql_mode).
I will try to do this.
+ } + void print_maybe_qualified_name(String *to) const + { + print_schema_name_if_needed(to); + to->append(func_name()); + } + public:
+ // Print an error message for a builtin-schema qualified function call + static void wrong_param_count_error(const LEX_CSTRING &schema_name, + const LEX_CSTRING &func_name); + + // Check that the number of arguments is greater or equal to "expected" + static bool create_check_args_ge(const LEX_CSTRING &name, + const List<Item> *item_list, + uint expected) + { + if (!item_list || item_list->elements < expected) + { + my_error(ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT, MYF(0), name.str);
what's the point of your wrong_param_count_error() if you aren't using it?
I use it for DECODE(), because DECODE() has a different argument number in sql_mode='' and sql_mode=ORACLE. MariaDB [test]> select decode(); ERROR 1582 (42000): Incorrect parameter count in the call to native function 'mariadb_schema.decode' I did not use it for the other sql_mode-dependent functions yet. Which way of printing the schema part do you prefer in error messages for sql_mode-dependent functions? 1a. Print always 1b. Print if the number of arguments differ (like now) 1c. Print if the Item_func_xxx qualifier is not equal to the current sql_mode implied qualifier. 2a. Always print the explicit qualifier if it was typed by the user 2b. Handle explicit qualifiers like implicit qualifiers <cut>
diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc index 7f1b362d91d..6b0a351f76e 100644 --- a/sql/sql_lex.cc +++ b/sql/sql_lex.cc @@ -1542,7 +1541,18 @@ int Lex_input_stream::lex_one_token(YYSTYPE *yylval, THD *thd) if (lex->parsing_options.lookup_keywords_after_qualifier) next_state= MY_LEX_IDENT_OR_KEYWORD; else - next_state= MY_LEX_IDENT_START; // Next is ident (not keyword) + { + /* + Next is: + - A qualified func with a special syntax: + mariadb_schema.REPLACE('a','b','c') + mariadb_schema.SUSTRING('a',1,2) + mariadb_schema.TRIM('a') + - Or an identifier otherwise. No keyword lookup is done, + all keywords are treated as identifiers. + */ + next_state= MY_LEX_IDENT_OR_QUALIFIED_SPECIAL_FUNC;
I don't understand why you did it in the lexer and not in the parser, like
- | REPLACE '(' expr ',' expr ',' expr ')' + | opt_schema REPLACE '(' expr ',' expr ',' expr ')'
or (if the above won't work in LARL(1) parser)
- | ident_cli '.' ident_cli '(' opt_expr_list ')' + | ident_cli '.' func_2nd_part
with
+ func_2nd_part: ident_cli '(' opt_expr_list ')' + | REPLACE '(' expr ',' expr ',' expr ')'
etc
(not reviewing lexer changes anymore, until I understand the above)
+ } if (!ident_map[(uchar) yyPeek()]) // Probably ` or " next_state= MY_LEX_START; return((int) c);
My grammar effectively looks like this exactly. But the problem is that we don't resolve keywords after the dot character. This is a performance optimization. So what I changed in the tokenizer is that now in this sequence: ident1.ident2( the "ident2" part is resolved for keywords.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
![](https://secure.gravatar.com/avatar/e1d656a55b3b29f2f0872cde35c6143c.jpg?s=120&d=mm&r=g)
Hello Sergei, On 4/19/22 7:15 PM, Alexander Barkov wrote:
Hello Sergei, On 4/19/22 1:17 AM, Sergei Golubchik wrote:
Hi, Alexander,
Some comments/questions here. I'll need to look at the patch again after I understand why you did it this way better.
Thanks for the review. My answers follow below.
A new version is here:
https://github.com/MariaDB/server/commit/50bb0987e10e660326763de38231d63cfc1...
A new version is here: https://github.com/MariaDB/server/commit/447f241e17e54f170e14a11bb6320fba193... Now qualifiers are printed only if the current (according to sql_mode) does not match. But EXPLAIN EXTENDED forces the qualifier.
On Apr 18, Alexander Barkov wrote:
revision-id: 96b40a5e2f5 (mariadb-10.3.33-61-g96b40a5e2f5) parent(s): 7355f7b1f5c author: Alexander Barkov committer: Alexander Barkov timestamp: 2022-04-07 16:22:17 +0400 message:
MDEV-27744 InnoDB: Failing assertion: !cursor->index->is_committed() in row0ins.cc (from row_ins_sec_index_entry_by_modify) | Assertion `0' failed in row_upd_sec_index_entry (debug) | Corruption
A detailed comment here is a must. What was the problem, how it was fixed.
Added.
diff --git a/sql/item_func.h b/sql/item_func.h index 754b1cd1eb2..d844e45280a 100644 --- a/sql/item_func.h +++ b/sql/item_func.h @@ -35,6 +35,36 @@ extern "C" /* Bug in BSDI include file */ #include <cmath> +template<class FUNC> +FUNC* item_func_create_2_3_args(THD *thd, const LEX_CSTRING &name, + List<Item> *item_list) +{ + int arg_count= item_list ? item_list->elements : 0; + + switch (arg_count) { + case 2: + { + Item *param_1= item_list->pop(); + Item *param_2= item_list->pop(); + return new (get_thd_memroot(thd)) FUNC(thd, param_1, param_2); + } + case 3: + { + Item *param_1= item_list->pop(); + Item *param_2= item_list->pop(); + Item *param_3= item_list->pop(); + return new (get_thd_memroot(thd)) FUNC(thd, param_1, param_2, param_3);
A constructor that takes a List<Item> wouldn't need a switch. And wouldn't need a template either, your create_check_args_ge() isn't a template.
Right. I added new constructors with List<Item>.
Now the template item_func_create_2_3_args is not needed any more.
create() methods now look like this in the new version:
Item_func_lpad_oracle* Item_func_lpad_oracle::create(THD *thd, const LEX_CSTRING &name, List<Item> *item_list) { if (create_check_args_between(name, item_list, 2, 3)) return NULL; return new (thd->mem_root) Item_func_lpad_oracle(thd, *item_list); }
+ break; + } + default: + my_error(ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT, MYF(0), name.str); + break; + } + + return NULL; +} + + class Item_func :public Item_func_or_sum { void sync_with_sum_func_and_with_field(List<Item> &list); @@ -56,8 +86,41 @@ class Item_func :public Item_func_or_sum bool check_argument_types_can_return_text(uint start, uint end) const; bool check_argument_types_can_return_date(uint start, uint end) const; bool check_argument_types_can_return_time(uint start, uint end) const; + + void print_schema_name_if_needed(String *to) const + { + const LEX_CSTRING schema_name= schema_name_cstring(); + if (schema_name.length) + { + to->append(schema_name); + to->append('.'); + }
this should check that schema_name isn't the same as the schema in the path (implicit path, assumed by the current sql_mode).
I will try to do this.
+ } + void print_maybe_qualified_name(String *to) const + { + print_schema_name_if_needed(to); + to->append(func_name()); + } + public: + // Print an error message for a builtin-schema qualified function call + static void wrong_param_count_error(const LEX_CSTRING &schema_name, + const LEX_CSTRING &func_name); + + // Check that the number of arguments is greater or equal to "expected" + static bool create_check_args_ge(const LEX_CSTRING &name, + const List<Item> *item_list, + uint expected) + { + if (!item_list || item_list->elements < expected) + { + my_error(ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT, MYF(0), name.str);
what's the point of your wrong_param_count_error() if you aren't using it?
I use it for DECODE(), because DECODE() has a different argument number in sql_mode='' and sql_mode=ORACLE.
MariaDB [test]> select decode(); ERROR 1582 (42000): Incorrect parameter count in the call to native function 'mariadb_schema.decode'
I did not use it for the other sql_mode-dependent functions yet.
Which way of printing the schema part do you prefer in error messages for sql_mode-dependent functions?
1a. Print always 1b. Print if the number of arguments differ (like now) 1c. Print if the Item_func_xxx qualifier is not equal to the current sql_mode implied qualifier.
2a. Always print the explicit qualifier if it was typed by the user 2b. Handle explicit qualifiers like implicit qualifiers
<cut>
diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc index 7f1b362d91d..6b0a351f76e 100644 --- a/sql/sql_lex.cc +++ b/sql/sql_lex.cc @@ -1542,7 +1541,18 @@ int Lex_input_stream::lex_one_token(YYSTYPE *yylval, THD *thd) if (lex->parsing_options.lookup_keywords_after_qualifier) next_state= MY_LEX_IDENT_OR_KEYWORD; else - next_state= MY_LEX_IDENT_START; // Next is ident (not keyword) + { + /* + Next is: + - A qualified func with a special syntax: + mariadb_schema.REPLACE('a','b','c') + mariadb_schema.SUSTRING('a',1,2) + mariadb_schema.TRIM('a') + - Or an identifier otherwise. No keyword lookup is done, + all keywords are treated as identifiers. + */ + next_state= MY_LEX_IDENT_OR_QUALIFIED_SPECIAL_FUNC;
I don't understand why you did it in the lexer and not in the parser, like
- | REPLACE '(' expr ',' expr ',' expr ')' + | opt_schema REPLACE '(' expr ',' expr ',' expr ')'
or (if the above won't work in LARL(1) parser)
- | ident_cli '.' ident_cli '(' opt_expr_list ')' + | ident_cli '.' func_2nd_part
with
+ func_2nd_part: ident_cli '(' opt_expr_list ')' + | REPLACE '(' expr ',' expr ',' expr ')'
etc
(not reviewing lexer changes anymore, until I understand the above)
+ } if (!ident_map[(uchar) yyPeek()]) // Probably ` or " next_state= MY_LEX_START; return((int) c);
My grammar effectively looks like this exactly.
But the problem is that we don't resolve keywords after the dot character. This is a performance optimization.
So what I changed in the tokenizer is that now in this sequence:
ident1.ident2(
the "ident2" part is resolved for keywords.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
![](https://secure.gravatar.com/avatar/e1d656a55b3b29f2f0872cde35c6143c.jpg?s=120&d=mm&r=g)
Hello Sergei, Here's a new version: https://github.com/MariaDB/server/commit/23a0a8becc68c867821350e6edd2e287c94... Now sql_mode dependent functions print themselves to FRM like this: - substr() - MariaDB version - substr_oracle() - Oracle version for downgrade compatibility. On 4/20/22 3:25 PM, Alexander Barkov wrote:
Hello Sergei,
On 4/19/22 7:15 PM, Alexander Barkov wrote:
Hello Sergei, On 4/19/22 1:17 AM, Sergei Golubchik wrote:
Hi, Alexander,
Some comments/questions here. I'll need to look at the patch again after I understand why you did it this way better.
Thanks for the review. My answers follow below.
A new version is here:
https://github.com/MariaDB/server/commit/50bb0987e10e660326763de38231d63cfc1...
A new version is here:
https://github.com/MariaDB/server/commit/447f241e17e54f170e14a11bb6320fba193...
Now qualifiers are printed only if the current (according to sql_mode) does not match.
But EXPLAIN EXTENDED forces the qualifier.
On Apr 18, Alexander Barkov wrote:
revision-id: 96b40a5e2f5 (mariadb-10.3.33-61-g96b40a5e2f5) parent(s): 7355f7b1f5c author: Alexander Barkov committer: Alexander Barkov timestamp: 2022-04-07 16:22:17 +0400 message:
MDEV-27744 InnoDB: Failing assertion: !cursor->index->is_committed() in row0ins.cc (from row_ins_sec_index_entry_by_modify) | Assertion `0' failed in row_upd_sec_index_entry (debug) | Corruption
A detailed comment here is a must. What was the problem, how it was fixed.
Added.
diff --git a/sql/item_func.h b/sql/item_func.h index 754b1cd1eb2..d844e45280a 100644 --- a/sql/item_func.h +++ b/sql/item_func.h @@ -35,6 +35,36 @@ extern "C" /* Bug in BSDI include file */ #include <cmath> +template<class FUNC> +FUNC* item_func_create_2_3_args(THD *thd, const LEX_CSTRING &name, + List<Item> *item_list) +{ + int arg_count= item_list ? item_list->elements : 0; + + switch (arg_count) { + case 2: + { + Item *param_1= item_list->pop(); + Item *param_2= item_list->pop(); + return new (get_thd_memroot(thd)) FUNC(thd, param_1, param_2); + } + case 3: + { + Item *param_1= item_list->pop(); + Item *param_2= item_list->pop(); + Item *param_3= item_list->pop(); + return new (get_thd_memroot(thd)) FUNC(thd, param_1, param_2, param_3);
A constructor that takes a List<Item> wouldn't need a switch. And wouldn't need a template either, your create_check_args_ge() isn't a template.
Right. I added new constructors with List<Item>.
Now the template item_func_create_2_3_args is not needed any more.
create() methods now look like this in the new version:
Item_func_lpad_oracle* Item_func_lpad_oracle::create(THD *thd, const LEX_CSTRING &name, List<Item> *item_list) { if (create_check_args_between(name, item_list, 2, 3)) return NULL; return new (thd->mem_root) Item_func_lpad_oracle(thd, *item_list); }
+ break; + } + default: + my_error(ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT, MYF(0), name.str); + break; + } + + return NULL; +} + + class Item_func :public Item_func_or_sum { void sync_with_sum_func_and_with_field(List<Item> &list); @@ -56,8 +86,41 @@ class Item_func :public Item_func_or_sum bool check_argument_types_can_return_text(uint start, uint end) const; bool check_argument_types_can_return_date(uint start, uint end) const; bool check_argument_types_can_return_time(uint start, uint end) const; + + void print_schema_name_if_needed(String *to) const + { + const LEX_CSTRING schema_name= schema_name_cstring(); + if (schema_name.length) + { + to->append(schema_name); + to->append('.'); + }
this should check that schema_name isn't the same as the schema in the path (implicit path, assumed by the current sql_mode).
I will try to do this.
+ } + void print_maybe_qualified_name(String *to) const + { + print_schema_name_if_needed(to); + to->append(func_name()); + } + public: + // Print an error message for a builtin-schema qualified function call + static void wrong_param_count_error(const LEX_CSTRING &schema_name, + const LEX_CSTRING &func_name); + + // Check that the number of arguments is greater or equal to "expected" + static bool create_check_args_ge(const LEX_CSTRING &name, + const List<Item> *item_list, + uint expected) + { + if (!item_list || item_list->elements < expected) + { + my_error(ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT, MYF(0), name.str);
what's the point of your wrong_param_count_error() if you aren't using it?
I use it for DECODE(), because DECODE() has a different argument number in sql_mode='' and sql_mode=ORACLE.
MariaDB [test]> select decode(); ERROR 1582 (42000): Incorrect parameter count in the call to native function 'mariadb_schema.decode'
I did not use it for the other sql_mode-dependent functions yet.
Which way of printing the schema part do you prefer in error messages for sql_mode-dependent functions?
1a. Print always 1b. Print if the number of arguments differ (like now) 1c. Print if the Item_func_xxx qualifier is not equal to the current sql_mode implied qualifier.
2a. Always print the explicit qualifier if it was typed by the user 2b. Handle explicit qualifiers like implicit qualifiers
<cut>
diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc index 7f1b362d91d..6b0a351f76e 100644 --- a/sql/sql_lex.cc +++ b/sql/sql_lex.cc @@ -1542,7 +1541,18 @@ int Lex_input_stream::lex_one_token(YYSTYPE *yylval, THD *thd) if (lex->parsing_options.lookup_keywords_after_qualifier) next_state= MY_LEX_IDENT_OR_KEYWORD; else - next_state= MY_LEX_IDENT_START; // Next is ident (not keyword) + { + /* + Next is: + - A qualified func with a special syntax: + mariadb_schema.REPLACE('a','b','c') + mariadb_schema.SUSTRING('a',1,2) + mariadb_schema.TRIM('a') + - Or an identifier otherwise. No keyword lookup is done, + all keywords are treated as identifiers. + */ + next_state= MY_LEX_IDENT_OR_QUALIFIED_SPECIAL_FUNC;
I don't understand why you did it in the lexer and not in the parser, like
- | REPLACE '(' expr ',' expr ',' expr ')' + | opt_schema REPLACE '(' expr ',' expr ',' expr ')'
or (if the above won't work in LARL(1) parser)
- | ident_cli '.' ident_cli '(' opt_expr_list ')' + | ident_cli '.' func_2nd_part
with
+ func_2nd_part: ident_cli '(' opt_expr_list ')' + | REPLACE '(' expr ',' expr ',' expr ')'
etc
(not reviewing lexer changes anymore, until I understand the above)
+ } if (!ident_map[(uchar) yyPeek()]) // Probably ` or " next_state= MY_LEX_START; return((int) c);
My grammar effectively looks like this exactly.
But the problem is that we don't resolve keywords after the dot character. This is a performance optimization.
So what I changed in the tokenizer is that now in this sequence:
ident1.ident2(
the "ident2" part is resolved for keywords.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
![](https://secure.gravatar.com/avatar/e1d656a55b3b29f2f0872cde35c6143c.jpg?s=120&d=mm&r=g)
Hello Sergei, On 4/21/22 6:44 PM, Alexander Barkov wrote:
Hello Sergei,
Here's a new version:
https://github.com/MariaDB/server/commit/23a0a8becc68c867821350e6edd2e287c94...
Now sql_mode dependent functions print themselves to FRM like this:
- substr() - MariaDB version - substr_oracle() - Oracle version
for downgrade compatibility.
Yet another update: https://github.com/MariaDB/server/commit/d67c3f88883b616a9adf3abba43938c3a07... Now the view_body_utf8 field in a view FRM file is written with forced qualifiers. It's used for I_S.VIEW.VIEW_DEFINITION output as is.
On 4/20/22 3:25 PM, Alexander Barkov wrote:
Hello Sergei,
On 4/19/22 7:15 PM, Alexander Barkov wrote:
Hello Sergei, On 4/19/22 1:17 AM, Sergei Golubchik wrote:
Hi, Alexander,
Some comments/questions here. I'll need to look at the patch again after I understand why you did it this way better.
Thanks for the review. My answers follow below.
A new version is here:
https://github.com/MariaDB/server/commit/50bb0987e10e660326763de38231d63cfc1...
A new version is here:
https://github.com/MariaDB/server/commit/447f241e17e54f170e14a11bb6320fba193...
Now qualifiers are printed only if the current (according to sql_mode) does not match.
But EXPLAIN EXTENDED forces the qualifier.
On Apr 18, Alexander Barkov wrote:
revision-id: 96b40a5e2f5 (mariadb-10.3.33-61-g96b40a5e2f5) parent(s): 7355f7b1f5c author: Alexander Barkov committer: Alexander Barkov timestamp: 2022-04-07 16:22:17 +0400 message:
MDEV-27744 InnoDB: Failing assertion: !cursor->index->is_committed() in row0ins.cc (from row_ins_sec_index_entry_by_modify) | Assertion `0' failed in row_upd_sec_index_entry (debug) | Corruption
A detailed comment here is a must. What was the problem, how it was fixed.
Added.
diff --git a/sql/item_func.h b/sql/item_func.h index 754b1cd1eb2..d844e45280a 100644 --- a/sql/item_func.h +++ b/sql/item_func.h @@ -35,6 +35,36 @@ extern "C" /* Bug in BSDI include file */ #include <cmath> +template<class FUNC> +FUNC* item_func_create_2_3_args(THD *thd, const LEX_CSTRING &name, + List<Item> *item_list) +{ + int arg_count= item_list ? item_list->elements : 0; + + switch (arg_count) { + case 2: + { + Item *param_1= item_list->pop(); + Item *param_2= item_list->pop(); + return new (get_thd_memroot(thd)) FUNC(thd, param_1, param_2); + } + case 3: + { + Item *param_1= item_list->pop(); + Item *param_2= item_list->pop(); + Item *param_3= item_list->pop(); + return new (get_thd_memroot(thd)) FUNC(thd, param_1, param_2, param_3);
A constructor that takes a List<Item> wouldn't need a switch. And wouldn't need a template either, your create_check_args_ge() isn't a template.
Right. I added new constructors with List<Item>.
Now the template item_func_create_2_3_args is not needed any more.
create() methods now look like this in the new version:
Item_func_lpad_oracle* Item_func_lpad_oracle::create(THD *thd, const LEX_CSTRING &name, List<Item> *item_list) { if (create_check_args_between(name, item_list, 2, 3)) return NULL; return new (thd->mem_root) Item_func_lpad_oracle(thd, *item_list); }
+ break; + } + default: + my_error(ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT, MYF(0), name.str); + break; + } + + return NULL; +} + + class Item_func :public Item_func_or_sum { void sync_with_sum_func_and_with_field(List<Item> &list); @@ -56,8 +86,41 @@ class Item_func :public Item_func_or_sum bool check_argument_types_can_return_text(uint start, uint end) const; bool check_argument_types_can_return_date(uint start, uint end) const; bool check_argument_types_can_return_time(uint start, uint end) const; + + void print_schema_name_if_needed(String *to) const + { + const LEX_CSTRING schema_name= schema_name_cstring(); + if (schema_name.length) + { + to->append(schema_name); + to->append('.'); + }
this should check that schema_name isn't the same as the schema in the path (implicit path, assumed by the current sql_mode).
I will try to do this.
+ } + void print_maybe_qualified_name(String *to) const + { + print_schema_name_if_needed(to); + to->append(func_name()); + } + public: + // Print an error message for a builtin-schema qualified function call + static void wrong_param_count_error(const LEX_CSTRING &schema_name, + const LEX_CSTRING &func_name); + + // Check that the number of arguments is greater or equal to "expected" + static bool create_check_args_ge(const LEX_CSTRING &name, + const List<Item> *item_list, + uint expected) + { + if (!item_list || item_list->elements < expected) + { + my_error(ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT, MYF(0), name.str);
what's the point of your wrong_param_count_error() if you aren't using it?
I use it for DECODE(), because DECODE() has a different argument number in sql_mode='' and sql_mode=ORACLE.
MariaDB [test]> select decode(); ERROR 1582 (42000): Incorrect parameter count in the call to native function 'mariadb_schema.decode'
I did not use it for the other sql_mode-dependent functions yet.
Which way of printing the schema part do you prefer in error messages for sql_mode-dependent functions?
1a. Print always 1b. Print if the number of arguments differ (like now) 1c. Print if the Item_func_xxx qualifier is not equal to the current sql_mode implied qualifier.
2a. Always print the explicit qualifier if it was typed by the user 2b. Handle explicit qualifiers like implicit qualifiers
<cut>
diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc index 7f1b362d91d..6b0a351f76e 100644 --- a/sql/sql_lex.cc +++ b/sql/sql_lex.cc @@ -1542,7 +1541,18 @@ int Lex_input_stream::lex_one_token(YYSTYPE *yylval, THD *thd) if (lex->parsing_options.lookup_keywords_after_qualifier) next_state= MY_LEX_IDENT_OR_KEYWORD; else - next_state= MY_LEX_IDENT_START; // Next is ident (not keyword) + { + /* + Next is: + - A qualified func with a special syntax: + mariadb_schema.REPLACE('a','b','c') + mariadb_schema.SUSTRING('a',1,2) + mariadb_schema.TRIM('a') + - Or an identifier otherwise. No keyword lookup is done, + all keywords are treated as identifiers. + */ + next_state= MY_LEX_IDENT_OR_QUALIFIED_SPECIAL_FUNC;
I don't understand why you did it in the lexer and not in the parser, like
- | REPLACE '(' expr ',' expr ',' expr ')' + | opt_schema REPLACE '(' expr ',' expr ',' expr ')'
or (if the above won't work in LARL(1) parser)
- | ident_cli '.' ident_cli '(' opt_expr_list ')' + | ident_cli '.' func_2nd_part
with
+ func_2nd_part: ident_cli '(' opt_expr_list ')' + | REPLACE '(' expr ',' expr ',' expr ')'
etc
(not reviewing lexer changes anymore, until I understand the above)
+ } if (!ident_map[(uchar) yyPeek()]) // Probably ` or " next_state= MY_LEX_START; return((int) c);
My grammar effectively looks like this exactly.
But the problem is that we don't resolve keywords after the dot character. This is a performance optimization.
So what I changed in the tokenizer is that now in this sequence:
ident1.ident2(
the "ident2" part is resolved for keywords.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Alexander Barkov
-
Sergei Golubchik