Re: 2d937b62c33: 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
Hi, Alexander, Few questions and suggestions below. Nothing major. On Oct 27, Alexander Barkov wrote:
commit 2d937b62c33 Author: Alexander Barkov <bar@mariadb.com> Date: Mon Apr 4 14:50:21 2022 +0400
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
would it make sense to change the MDEV title in Jira to better describe the problem? I sometimes do it with my bugs. Like "LPAD in vcol created in ORACLE mode makes table corrupted in non-ORACLE" (I tried to make it short, appropriate for a title) and, of course, change it in the comment and in the test file to match.
The crash happened with an indexed virtual column whose value is evaluated using a function that has a different meaning in sql_mode='' vs sql_mode=ORACLE:
diff --git a/mysql-test/suite/compat/oracle/r/func_concat.result b/mysql-test/suite/compat/oracle/r/func_concat.result index 392d579707a..17ca4be078a 100644 --- a/mysql-test/suite/compat/oracle/r/func_concat.result +++ b/mysql-test/suite/compat/oracle/r/func_concat.result @@ -211,14 +211,14 @@ SET sql_mode=ORACLE; CREATE VIEW v1 AS SELECT 'foo'||NULL||'bar' AS test; SHOW CREATE VIEW v1; View Create View character_set_client collation_connection -v1 CREATE VIEW "v1" AS select concat_operator_oracle(concat_operator_oracle('foo',NULL),'bar') AS "test" latin1 latin1_swedish_ci +v1 CREATE VIEW "v1" AS select concat(concat('foo',NULL),'bar') AS "test" latin1 latin1_swedish_ci SELECT * FROM v1; test foobar SET sql_mode=DEFAULT; SHOW CREATE VIEW v1; View Create View character_set_client collation_connection -v1 CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select concat_operator_oracle(concat_operator_oracle('foo',NULL),'bar') AS `test` latin1 latin1_swedish_ci +v1 CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select oracle_schema.concat(oracle_schema.concat('foo',NULL),'bar') AS `test` latin1 latin1_swedish_ci
please, add a mysqldump test. Like, create a table with virtual columns, check, defaults in the default sql mode. and create a table in oracle mode. Also, a view in the default mode and a view in oracle mode. and then mysqldump, to see that it dumps and restores everything correctly may be stored routines/triggers/etc, if you'd like, but they aren't directly relevant to this MDEV, as far as I understand.
SELECT * FROM v1; test foobar diff --git a/mysql-test/suite/compat/oracle/r/func_decode.result b/mysql-test/suite/compat/oracle/r/func_decode.result index 2809e971be3..1870a1ec0d5 100644 --- a/mysql-test/suite/compat/oracle/r/func_decode.result +++ b/mysql-test/suite/compat/oracle/r/func_decode.result @@ -1,8 +1,8 @@ SET sql_mode=ORACLE; SELECT DECODE(10); -ERROR 42000: Incorrect parameter count in the call to native function 'DECODE' +ERROR 42000: Incorrect parameter count in the call to native function 'oracle_schema.DECODE'
Hmm, may be this should say DECODE as before? you know, falls under the case "if you don't change sql_mode back and forth, you won't see schema-qualified names"
SELECT DECODE(10,10); -ERROR 42000: Incorrect parameter count in the call to native function 'DECODE' +ERROR 42000: Incorrect parameter count in the call to native function 'oracle_schema.DECODE' SELECT DECODE(10,10,'x10'); DECODE(10,10,'x10') x10 diff --git a/mysql-test/suite/compat/oracle/r/vcol_innodb.result b/mysql-test/suite/compat/oracle/r/vcol_innodb.result new file mode 100644 index 00000000000..9fa97c75c10 --- /dev/null +++ b/mysql-test/suite/compat/oracle/r/vcol_innodb.result @@ -0,0 +1,51 @@ +SET @table_open_cache=@@GLOBAL.table_open_cache;
why do you need to manipulate table_open_cache? to trigger a reopen? Just do flush tables, it's explicit, more readable and more... controllable.
+SET sql_mode=''; +CREATE TABLE t (d INT,b VARCHAR(1),c CHAR(1),g CHAR(1) GENERATED ALWAYS AS (SUBSTR(b,0,0)) VIRTUAL,PRIMARY KEY(b),KEY g(g)) ENGINE=InnoDB; +INSERT INTO t VALUES (0); +ERROR 21S01: Column count doesn't match value count at row 1 +SET sql_mode='ORACLE'; +INSERT INTO t SET c=REPEAT (1,0); +Warnings: +Warning 1364 Field 'b' doesn't have a default value +ALTER TABLE t CHANGE COLUMN a b INT; diff --git a/sql/item_func.h b/sql/item_func.h index 76a997c33fb..cdbefb82541 100644 --- a/sql/item_func.h +++ b/sql/item_func.h @@ -56,8 +56,40 @@ class Item_func :public Item_func_or_sum, 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_cast_temporal(String *str, enum_query_type query_type); + + void print_schema_qualified_name(String *to, + const LEX_CSTRING &schema_name, + const char *function_name) const
I don't see why you'd need this helper. is it something that was used in earlier versions of the patch?
+ { + // e.g. oracle_schema.func() + to->append(schema_name); + to->append('.'); + to->append(function_name); + } + + void print_sql_mode_qualified_name(String *to, + enum_query_type query_type, + const char *function_name) const + { + const Schema *func_schema= schema(); + if (!func_schema || func_schema == Schema::find_implied(current_thd)) + to->append(function_name); + else + print_schema_qualified_name(to, func_schema->name(), function_name); + } + + void print_sql_mode_qualified_name(String *to, enum_query_type query_type) + const + { + return print_sql_mode_qualified_name(to, query_type, func_name()); + }
I don't see why you need this helper either, you never use print_sql_mode_qualified_name with the last argument being not func_name(). So you can remove this helper and the third argument of print_sql_mode_qualified_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); + 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 ae078dbb22f..92d5e196da4 100644 --- a/sql/item_strfunc.cc +++ b/sql/item_strfunc.cc @@ -2170,13 +2170,31 @@ bool Item_func_trim::fix_length_and_dec()
void Item_func_trim::print(String *str, enum_query_type query_type) { + LEX_CSTRING suffix= {STRING_WITH_LEN("_oracle")}; if (arg_count == 1) { - Item_func::print(str, query_type); + if (query_type & QT_FOR_FRM) + { + // 10.3 downgrade compatibility for FRM + str->append(func_name()); + if (schema() == &oracle_schema_ref) + str->append(suffix); + } + else + print_sql_mode_qualified_name(str, query_type, func_name()); + print_args_parenthesized(str, query_type); return; } - str->append(Item_func_trim::func_name()); - str->append(func_name_ext()); + + if (query_type & QT_FOR_FRM) + { + // 10.3 downgrade compatibility for FRM + str->append(Item_func_trim::func_name()); + if (schema() == &oracle_schema_ref) + str->append(suffix); + } + else + print_sql_mode_qualified_name(str, query_type, Item_func_trim::func_name());
it'd be simpler if you move the above block that prints the function name before if (arg_count == 1) also you won't need suffix, but can do like in all other functions str->append(STRING_WITH_LEN("trim_oracle");
str->append('('); str->append(mode_name()); str->append(' '); diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc index 71f592a3852..bb53d1a510a 100644 --- a/sql/sql_lex.cc +++ b/sql/sql_lex.cc @@ -2084,7 +2100,64 @@ bool Lex_input_stream::get_7bit_or_8bit_ident(THD *thd, uchar *last_char) }
-int Lex_input_stream::scan_ident_sysvar(THD *thd, Lex_ident_cli_st *str) +/* + Resolve special SQL functions that have a qualified syntax in sql_yacc.yy. + These functions are not listed in the native function registry + because of a special syntax, or a reserved keyword: + + mariadb_schema.SUBSTRING('a' FROM 1 FOR 2) -- Special syntax
I didn't find it in Oracle's manual, by the way
+ mariadb_schema.TRIM(BOTH ' ' FROM 'a') -- Special syntax + mariadb_schema.REPLACE('a','b','c') -- Verb keyword +*/ + +int Lex_input_stream::find_keyword_qualified_special_func(Lex_ident_cli_st *str, + uint length) const +{ + /* + There are many other special functions, see the following grammar rules: + function_call_keyword + function_call_nonkeyword + Here we resolve only those that have a qualified syntax to handle + different behavior in different @@sql_mode settings. + + Other special functions do not work in qualified context: + SELECT mariadb_schema.year(now()); -- Function year is not defined + SELECT mariadb_schema.now(); -- Function now is not defined + + We don't resolve TRIM_ORACLE here, because it does not have + a qualified syntax yet. Search for "trim_operands" in sql_yacc.yy + to find more comments. + */ diff --git a/sql/sql_schema.h b/sql/sql_schema.h index 1174bc7a83f..2c52646f2ea 100644 --- a/sql/sql_schema.h +++ b/sql/sql_schema.h @@ -77,5 +98,6 @@ class Schema
extern Schema mariadb_schema; +extern const Schema &oracle_schema_ref;
What's the difference between these two definitions. Do you expect someone will need to change mariadb_schema?
#endif // SQL_SCHEMA_H_INCLUDED
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
Hello Sergei, On 10/27/23 6:44 PM, Sergei Golubchik wrote:
Hi, Alexander,
Few questions and suggestions below. Nothing major.
Thanks for the review. A new version is here: https://github.com/MariaDB/server/commit/dd92459b0b16fdd431b216d2787e24d57e1... Please find comments inline below:
On Oct 27, Alexander Barkov wrote:
commit 2d937b62c33 Author: Alexander Barkov <bar@mariadb.com> Date: Mon Apr 4 14:50:21 2022 +0400
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
would it make sense to change the MDEV title in Jira to better describe the problem? I sometimes do it with my bugs.
Like "LPAD in vcol created in ORACLE mode makes table corrupted in non-ORACLE"
(I tried to make it short, appropriate for a title)
and, of course, change it in the comment and in the test file to match.
I agree. Done.
The crash happened with an indexed virtual column whose value is evaluated using a function that has a different meaning in sql_mode='' vs sql_mode=ORACLE:
diff --git a/mysql-test/suite/compat/oracle/r/func_concat.result b/mysql-test/suite/compat/oracle/r/func_concat.result index 392d579707a..17ca4be078a 100644 --- a/mysql-test/suite/compat/oracle/r/func_concat.result +++ b/mysql-test/suite/compat/oracle/r/func_concat.result @@ -211,14 +211,14 @@ SET sql_mode=ORACLE; CREATE VIEW v1 AS SELECT 'foo'||NULL||'bar' AS test; SHOW CREATE VIEW v1; View Create View character_set_client collation_connection -v1 CREATE VIEW "v1" AS select concat_operator_oracle(concat_operator_oracle('foo',NULL),'bar') AS "test" latin1 latin1_swedish_ci +v1 CREATE VIEW "v1" AS select concat(concat('foo',NULL),'bar') AS "test" latin1 latin1_swedish_ci SELECT * FROM v1; test foobar SET sql_mode=DEFAULT; SHOW CREATE VIEW v1; View Create View character_set_client collation_connection -v1 CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select concat_operator_oracle(concat_operator_oracle('foo',NULL),'bar') AS `test` latin1 latin1_swedish_ci +v1 CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select oracle_schema.concat(oracle_schema.concat('foo',NULL),'bar') AS `test` latin1 latin1_swedish_ci
please, add a mysqldump test. Like, create a table with virtual columns, check, defaults in the default sql mode. and create a table in oracle mode. Also, a view in the default mode and a view in oracle mode. and then mysqldump, to see that it dumps and restores everything correctly
Added this test: mysqldump_restore_func_qualified.test
may be stored routines/triggers/etc, if you'd like, but they aren't directly relevant to this MDEV, as far as I understand.
SELECT * FROM v1; test foobar diff --git a/mysql-test/suite/compat/oracle/r/func_decode.result b/mysql-test/suite/compat/oracle/r/func_decode.result index 2809e971be3..1870a1ec0d5 100644 --- a/mysql-test/suite/compat/oracle/r/func_decode.result +++ b/mysql-test/suite/compat/oracle/r/func_decode.result @@ -1,8 +1,8 @@ SET sql_mode=ORACLE; SELECT DECODE(10); -ERROR 42000: Incorrect parameter count in the call to native function 'DECODE' +ERROR 42000: Incorrect parameter count in the call to native function 'oracle_schema.DECODE'
Hmm, may be this should say DECODE as before? you know, falls under the case "if you don't change sql_mode back and forth, you won't see schema-qualified names"
Fixed.
SELECT DECODE(10,10); -ERROR 42000: Incorrect parameter count in the call to native function 'DECODE' +ERROR 42000: Incorrect parameter count in the call to native function 'oracle_schema.DECODE' SELECT DECODE(10,10,'x10'); DECODE(10,10,'x10') x10 diff --git a/mysql-test/suite/compat/oracle/r/vcol_innodb.result b/mysql-test/suite/compat/oracle/r/vcol_innodb.result new file mode 100644 index 00000000000..9fa97c75c10 --- /dev/null +++ b/mysql-test/suite/compat/oracle/r/vcol_innodb.result @@ -0,0 +1,51 @@ +SET @table_open_cache=@@GLOBAL.table_open_cache;
why do you need to manipulate table_open_cache? to trigger a reopen? Just do flush tables, it's explicit, more readable and more... controllable.
I copied this test from the original bug report, to prove that the exact reported problem is gone.
+SET sql_mode=''; +CREATE TABLE t (d INT,b VARCHAR(1),c CHAR(1),g CHAR(1) GENERATED ALWAYS AS (SUBSTR(b,0,0)) VIRTUAL,PRIMARY KEY(b),KEY g(g)) ENGINE=InnoDB; +INSERT INTO t VALUES (0); +ERROR 21S01: Column count doesn't match value count at row 1 +SET sql_mode='ORACLE'; +INSERT INTO t SET c=REPEAT (1,0); +Warnings: +Warning 1364 Field 'b' doesn't have a default value +ALTER TABLE t CHANGE COLUMN a b INT; diff --git a/sql/item_func.h b/sql/item_func.h index 76a997c33fb..cdbefb82541 100644 --- a/sql/item_func.h +++ b/sql/item_func.h @@ -56,8 +56,40 @@ class Item_func :public Item_func_or_sum, 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_cast_temporal(String *str, enum_query_type query_type); + + void print_schema_qualified_name(String *to, + const LEX_CSTRING &schema_name, + const char *function_name) const
I don't see why you'd need this helper. is it something that was used in earlier versions of the patch?
+ { + // e.g. oracle_schema.func() + to->append(schema_name); + to->append('.'); + to->append(function_name); + } + + void print_sql_mode_qualified_name(String *to, + enum_query_type query_type, + const char *function_name) const + { + const Schema *func_schema= schema(); + if (!func_schema || func_schema == Schema::find_implied(current_thd)) + to->append(function_name); + else + print_schema_qualified_name(to, func_schema->name(), function_name); + } + + void print_sql_mode_qualified_name(String *to, enum_query_type query_type) + const + { + return print_sql_mode_qualified_name(to, query_type, func_name()); + }
I don't see why you need this helper either, you never use print_sql_mode_qualified_name with the last argument being not func_name().
It's used with Item_func_trim::func_name() as well, in Item_func_trim::print(). Item_func_trim::print() is quite complex. It handles these classes: Item_func_trim Item_func_trim_oracle Item_func_ltrim Item_func_ltrim_oracle Item_func_rtrim Item_func_rtrim_oracle I'd like to avoid major refactoring in here.
So you can remove this helper and the third argument of print_sql_mode_qualified_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); + 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 ae078dbb22f..92d5e196da4 100644 --- a/sql/item_strfunc.cc +++ b/sql/item_strfunc.cc @@ -2170,13 +2170,31 @@ bool Item_func_trim::fix_length_and_dec()
void Item_func_trim::print(String *str, enum_query_type query_type) { + LEX_CSTRING suffix= {STRING_WITH_LEN("_oracle")}; if (arg_count == 1) { - Item_func::print(str, query_type); + if (query_type & QT_FOR_FRM) + { + // 10.3 downgrade compatibility for FRM + str->append(func_name()); + if (schema() == &oracle_schema_ref) + str->append(suffix); + } + else + print_sql_mode_qualified_name(str, query_type, func_name()); + print_args_parenthesized(str, query_type); return; } - str->append(Item_func_trim::func_name()); - str->append(func_name_ext()); + + if (query_type & QT_FOR_FRM) + { + // 10.3 downgrade compatibility for FRM + str->append(Item_func_trim::func_name()); + if (schema() == &oracle_schema_ref) + str->append(suffix); + } + else + print_sql_mode_qualified_name(str, query_type, Item_func_trim::func_name());
it'd be simpler if you move the above block that prints the function name before if (arg_count == 1)
also you won't need suffix, but can do like in all other functions
str->append(STRING_WITH_LEN("trim_oracle");
str->append('('); str->append(mode_name()); str->append(' '); diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc index 71f592a3852..bb53d1a510a 100644 --- a/sql/sql_lex.cc +++ b/sql/sql_lex.cc @@ -2084,7 +2100,64 @@ bool Lex_input_stream::get_7bit_or_8bit_ident(THD *thd, uchar *last_char) }
-int Lex_input_stream::scan_ident_sysvar(THD *thd, Lex_ident_cli_st *str) +/* + Resolve special SQL functions that have a qualified syntax in sql_yacc.yy. + These functions are not listed in the native function registry + because of a special syntax, or a reserved keyword: + + mariadb_schema.SUBSTRING('a' FROM 1 FOR 2) -- Special syntax
I didn't find it in Oracle's manual, by the way
I'm not sure I misunderstood your question. Do you mean Oracle supports only SUBSTR, and does not support SUBSTRING? But MariaDB supports both SUBSTR and SUBSTRING as synonyms. We need to handle both.
+ mariadb_schema.TRIM(BOTH ' ' FROM 'a') -- Special syntax + mariadb_schema.REPLACE('a','b','c') -- Verb keyword +*/ + +int Lex_input_stream::find_keyword_qualified_special_func(Lex_ident_cli_st *str, + uint length) const +{ + /* + There are many other special functions, see the following grammar rules: + function_call_keyword + function_call_nonkeyword + Here we resolve only those that have a qualified syntax to handle + different behavior in different @@sql_mode settings. + + Other special functions do not work in qualified context: + SELECT mariadb_schema.year(now()); -- Function year is not defined + SELECT mariadb_schema.now(); -- Function now is not defined + + We don't resolve TRIM_ORACLE here, because it does not have + a qualified syntax yet. Search for "trim_operands" in sql_yacc.yy + to find more comments. + */ diff --git a/sql/sql_schema.h b/sql/sql_schema.h index 1174bc7a83f..2c52646f2ea 100644 --- a/sql/sql_schema.h +++ b/sql/sql_schema.h @@ -77,5 +98,6 @@ class Schema
extern Schema mariadb_schema; +extern const Schema &oracle_schema_ref;
What's the difference between these two definitions.
The difference it that the data type of "oracle_schema" is Schema_oracle. But it's defined inside sql_schema.cc and it not seen outside. So here in sql_schema.h I use oracle_schema_ref as a reference of the "Schema" data type to something of the "Schema_oracle" data type. Another option would be to: - put the definition of the "class Schema_oracle" inside sql_schema.h - have "extern const Schema_oracle schema_oracle" inside sql_schema.h
Do you expect someone will need to change mariadb_schema?
It seems in earlier commits mariadb_schema was not marked as "const". Most likely it can be.
#endif // SQL_SCHEMA_H_INCLUDED
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
Hi, Alexander, On Oct 31, Alexander Barkov wrote:
+++ b/mysql-test/suite/compat/oracle/r/vcol_innodb.result @@ -0,0 +1,51 @@ +SET @table_open_cache=@@GLOBAL.table_open_cache;
why do you need to manipulate table_open_cache? to trigger a reopen? Just do flush tables, it's explicit, more readable and more... controllable.
I copied this test from the original bug report, to prove that the exact reported problem is gone.
generally it's a good idea. but in case of pquery found bugs, the "test case" is constructed by randomly reordering existsing lines from other test cases. In that case, I think, it wouldn't hurt to rewrite it into something more robust and meaningful. or, think of it that way - if you don't rewrite it, you don't increase the number of different lines in test cases, so pquery will only test permutations of very same lines over and over :) Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
participants (2)
-
Alexander Barkov -
Sergei Golubchik