Hi Sergei, Thanks for review. Please find comments inline: On 7/28/20 6:02 PM, Sergei Golubchik wrote:

Hi, Alexander!

On Jul 28, Alexander Barkov wrote:

...

revision-id: ca7e6f8d572 (mariadb-10.3.21-178-gca7e6f8d572) parent(s): f3f23b5c4bd author: Alexander Barkov committer: Alexander Barkov timestamp: 2020-07-08 12:37:17 +0400 message:

MDEV-19632 Replication aborts with ER_SLAVE_CONVERSION_FAILED upon CREATE ... SELECT in ORACLE mode

diff --git a/mysql-test/main/lowercase_fs_off.result b/mysql-test/main/lowercase_fs_off.result index f2a8ec14641..3f0b08a78c4 100644 --- a/mysql-test/main/lowercase_fs_off.result +++ b/mysql-test/main/lowercase_fs_off.result @@ -158,3 +158,13 @@ show triggers like '%T1%'; Trigger Event Table Statement Timing Created sql_mode Definer character_set_client collation_connection Database Collation drop table t1; set GLOBAL sql_mode=default; +# +# MDEV-19632 Replication aborts with ER_SLAVE_CONVERSION_FAILED upon CREATE ... SELECT in ORACLE mode +# +# Compatibility schema names respect the filesystem case sensitivity

I agree, it should. But this is not consistent with information_schema. So I'd suggest a comment in eq_name method, explaining why compatibility schema names respect the filesystem case sensitivity. (because, e.g., we eventually turn them into real schemas, on disk).

Good idea. Will do.

...

+CREATE TABLE t1 (a MARIADB_SCHEMA.date); +ERROR HY000: Unknown data type: 'MARIADB_SCHEMA.date' +CREATE TABLE t1 (a Mariadb_schema.date); +ERROR HY000: Unknown data type: 'Mariadb_schema.date' +CREATE TABLE t1 (a mariadb_schema.date); +DROP TABLE t1; diff --git a/mysql-test/suite/compat/oracle/t/type_date.test b/mysql-test/suite/compat/oracle/t/type_date.test index 61f7aa53944..34adc5889b4 100644 --- a/mysql-test/suite/compat/oracle/t/type_date.test +++ b/mysql-test/suite/compat/oracle/t/type_date.test @@ -2,3 +2,101 @@ SET sql_mode=ORACLE; ... +--echo # +--echo # Qualified syntax is not supported yet in SP +--echo #

an MDEV for that?

There is no one yet. Would you like me to create one and mentions it number here?

...

diff --git a/sql/field.cc b/sql/field.cc index 8accfb35b0b..4ed38f2c06f 100644 --- a/sql/field.cc +++ b/sql/field.cc @@ -10407,6 +10407,19 @@ void Column_definition::set_attributes(const Lex_field_type_st &type, set_handler(type.type_handler()); charset= cs;

+#if MYSQL_VERSION_ID > 100500 +#error When merging to 10.5, please move the code below to +#error Type_handler_timestamp_common::Column_definition_set_attributes()

good

...

+#else + /* + Unlike other types TIMESTAMP fields are NOT NULL by default. + Unless --explicit-defaults-for-timestamp is given. + */ + if (!opt_explicit_defaults_for_timestamp && + type.type_handler()->field_type() == MYSQL_TYPE_TIMESTAMP) + flags|= NOT_NULL_FLAG; +#endif + if (type.length()) { int err; diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc index 0b423355170..3fb07601f99 100644 --- a/sql/sql_lex.cc +++ b/sql/sql_lex.cc @@ -2080,6 +2081,13 @@ int Lex_input_stream::scan_ident_start(THD *thd, Lex_ident_cli_st *str)

uint length= yyLength(); yyUnget(); // ptr points now after last token char + + if (thd->lex->parsing_options.lookup_keywords_after_qualifier) + { + if (int tokval= find_keyword(str, length, 0)) + return tokval; + }

why is that?

This is because our parser has a hack to avoid looking up keywords near a dot. Here we need to lookup to make all field_type_xxx rules work with the qualified syntax. Otherwise mariadb_schema.datetime(6) won't parse because in this rule: | DATETIME opt_field_length the token will be IDENT instead of DATETIME. So in this patch I allow looking up keywords after the dot inside the data type grammar. Btw, I suggest to remove this hack eventually. So keywords are always looked up.

...

+ str->set_ident(m_tok_start, length, is_8bit); m_cpp_text_start= m_cpp_tok_start; m_cpp_text_end= m_cpp_text_start + length; @@ -8284,3 +8292,32 @@ bool LEX::tvc_finalize_derived() current_select->linkage= DERIVED_TABLE_TYPE; return tvc_finalize(); } + + +bool LEX::map_data_type(const Lex_ident_sys_st &schema_name, + Lex_field_type_st *type) const +{ + const Schema *schema= schema_name.str ? + Schema::find_by_name(schema_name) : + Schema::find_implied(thd); + if (!schema) + { + char buf[128]; + const Name type_name= type->type_handler()->name(); + my_snprintf(buf, sizeof(buf), "%.*s.%.*s", + (int) schema_name.length, schema_name.str, + (int) type_name.length(), type_name.ptr()); +#if MYSQL_VERSION_ID > 100500 +#error Please remove the old code + my_error(ER_UNKNOWN_DATA_TYPE, MYF(0), buf); +#else + my_printf_error(ER_UNKNOWN_ERROR, "Unknown data type: '%-.64s'", + MYF(0), buf); +#endif + return true; + } + const Type_handler *mapped= schema->map_data_type(thd, type->type_handler()); + if (mapped != type->type_handler()) + type->set_handler(mapped);

why do you need an if() here? type->set_handler(mapped) is not that expensive.

Will fix.

...

+ return false; +} diff --git a/sql/sql_show.cc b/sql/sql_show.cc index 3dbc7724928..fcc2d44fdd6 100644 --- a/sql/sql_show.cc +++ b/sql/sql_show.cc @@ -2224,6 +2224,13 @@ int show_create_table(THD *thd, TABLE_LIST *table_list, String *packet, append_identifier(thd, packet, &field->field_name); packet->append(' ');

+ const Type_handler *th= field->type_handler(); + const Schema *implied_schema= Schema::find_implied(thd); + if (th != implied_schema->map_data_type(thd, th)) + { + packet->append(th->schema()->name(), system_charset_info);

sorry, I don't see it. How can th->schema() return not mariadb_schema?

Currently th->schema() cannot return not "mariadb_schema". But the intent is to have schema specific data types in the future. For example, Oracle's DATE may become a real separate data type (with Type_handler_date_oracle, Field_date_oracle, all the other stuff) in the future, instead of just an alias for MariaDB's DATETIME. So this code just takes this into account.

...

+ packet->append(STRING_WITH_LEN("."), system_charset_info); + } type.set(tmp, sizeof(tmp), system_charset_info); field->sql_type(type); packet->append(type.ptr(), type.length(), system_charset_info); diff --git a/storage/test_sql_discovery/mysql-test/sql_discovery/simple.result b/storage/test_sql_discovery/mysql-test/sql_discovery/simple.result index 23b7804638f..1feea5e47ee 100644 --- a/storage/test_sql_discovery/mysql-test/sql_discovery/simple.result +++ b/storage/test_sql_discovery/mysql-test/sql_discovery/simple.result @@ -82,7 +82,7 @@ select * from t1; ERROR HY000: Engine TEST_SQL_DISCOVERY failed to discover table `test`.`t1` with 'create table t1 (a uint)' show warnings; Level Code Message -Error 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'uint)' at line 1 +Error 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near ')' at line 1

this is not ideal :( can you try to keep the old error message?

I'm afraid this is not easy. The error happens inside bison. 'uint' is now a known data type, so it tries to parse it as a schema qualified data type, but there is no a dot after the schema name 'uint'. So the error is now quite correct. Note, starting from 10.5 it works as follows: MariaDB [test]> create table t1 (a uint); ERROR 4161 (HY000): Unknown data type: 'uint' So probably not worthy efforts. What do you think? Thanks.

Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org