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/dd92459b0b16fdd431b216d2787e24d57e1b2b58
Please find comments inline below:
On Oct 27, Alexander Barkov wrote:
commit 2d937b62c33
Author: Alexander Barkov <b...@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 secur...@mariadb.org
_______________________________________________
developers mailing list -- developers@lists.mariadb.org
To unsubscribe send an email to developers-le...@lists.mariadb.org
