Hi Alexander,
commit d070c68861670c0a556e3f29cb0b3d5fdd87a8c8 Author: Alexander Barkov <bar@mariadb.com> Date: Fri May 31 09:29:35 2019 +0400
MDEV-19653 Add class Sql_cmd_create_table
diff --git a/sql/sql_alter.cc b/sql/sql_alter.cc index e37d547..dfd421f 100644 --- a/sql/sql_alter.cc +++ b/sql/sql_alter.cc @@ -193,6 +193,20 @@ bool Sql_cmd_alter_table::execute(THD *thd) SELECT_LEX *select_lex= &lex->select_lex; /* first table of first SELECT_LEX */ TABLE_LIST *first_table= (TABLE_LIST*) select_lex->table_list.first; + + bool used_engine= lex->create_info.used_fields & HA_CREATE_USED_ENGINE; + DBUG_ASSERT(m_storage_engine_name.str || !used_engine);
I think it is possible to specify ALTER TABLE t1 ENGINE='' We should not crash in this case.
+ if (used_engine && + thd->resolve_storage_engine(&lex->create_info.db_type, + m_storage_engine_name, + lex->create_info.tmp_table())) + return true; + + if ((lex->create_info.used_fields & HA_CREATE_USED_ENGINE) && + !lex->create_info.db_type) + lex->create_info.used_fields&= ~HA_CREATE_USED_ENGINE; + + Why not
if (lex->create_info.used_fields & HA_CREATE_USED_ENGINE) { DBUG_ASSERT(m_storage_engine_name.str); if (thd->resolve_storage_engine(&lex->create_info.db_type, m_storage_engine_name, lex->create_info.tmp_table())) return true; if (!lex->create_info.db_type) lex->create_info.used_fields&= ~HA_CREATE_USED_ENGINE; }
/* Code in mysql_alter_table() may modify its HA_CREATE_INFO argument, so we have to use a copy of this structure to make execution diff --git a/sql/sql_alter.h b/sql/sql_alter.h index a503837..66c20fc 100644 --- a/sql/sql_alter.h +++ b/sql/sql_alter.h @@ -385,7 +385,8 @@ class Sql_cmd_common_alter_table : public Sql_cmd Sql_cmd_alter_table represents the generic ALTER TABLE statement. @todo move Alter_info and other ALTER specific structures from Lex here. */ -class Sql_cmd_alter_table : public Sql_cmd_common_alter_table +class Sql_cmd_alter_table : public Sql_cmd_common_alter_table, + public Sql_cmd_options_table_dml s/dml/ddl?
{ public: /** @@ -397,6 +398,8 @@ class Sql_cmd_alter_table : public Sql_cmd_common_alter_table ~Sql_cmd_alter_table() {}
+ Sql_cmd_options_table_dml *options_table_dml() { return this; } + bool execute(THD *thd); };
@@ -423,4 +426,27 @@ class Sql_cmd_discard_import_tablespace : public Sql_cmd_common_alter_table const enum_tablespace_op_type m_tablespace_op; };
+ +class Sql_cmd_create_table: public Sql_cmd, + public Sql_cmd_options_table_dml +{ +public: + Sql_cmd_create_table() + {} + + ~Sql_cmd_create_table() + { } + + enum_sql_command sql_command_code() const + { + return SQLCOM_ALTER_TABLE; + } Did you mean SQLCOM_CREATE_TABLE?
+ + Sql_cmd_options_table_dml *options_table_dml() { return this; } + + bool execute(THD *thd); + +}; + + #endif It doesn't feels like a proper place for Sql_cmd_create_table. May be move either to sql_table.h or sql_cmd.h? Why do you need empty constructor/destructor?
diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 8fabcd5..e107f57 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -6151,6 +6151,32 @@ int THD::decide_logging_format(TABLE_LIST *tables)
#ifndef MYSQL_CLIENT
+bool THD::resolve_storage_engine(handlerton **ha, const LEX_CSTRING &name, + bool tmp_table) I don't feel like THD is the right home for this. Can we have it as a regular function in handler.cc. Like ha_resolve_by_name_with_error().
+{ + LEX_STRING tmp_name= {(char*) name.str, name.length}; const_cast<char*>(name.str) precisely.
+ plugin_ref plugin= ha_resolve_by_name(this, &tmp_name, tmp_table); + + if (plugin) May be
if (plugin_ref plugin= ha_resolve_by_name(this, &tmp_name, tmp_table))
+ { + *ha= plugin_hton(plugin); + return false; + } + + *ha= NULL; + if (variables.sql_mode & MODE_NO_ENGINE_SUBSTITUTION) + { + my_error(ER_UNKNOWN_STORAGE_ENGINE, MYF(0), name.str); + return true; + } + push_warning_printf(this, Sql_condition::WARN_LEVEL_WARN, + ER_UNKNOWN_STORAGE_ENGINE, + ER_THD(this, ER_UNKNOWN_STORAGE_ENGINE), + name.str); + return false; +} + + /* Template member function for ensuring that there is an rows log event of the apropriate type before proceeding. diff --git a/sql/sql_cmd.h b/sql/sql_cmd.h index 9583e01..3f6aa4d 100644 --- a/sql/sql_cmd.h +++ b/sql/sql_cmd.h @@ -102,6 +102,23 @@ enum enum_sql_command { SQLCOM_END };
+ +// We should eventually move LEX::create_info here +class Sql_cmd_options_table_dml +{ +protected: + LEX_CSTRING m_storage_engine_name; +public: + Sql_cmd_options_table_dml() + :m_storage_engine_name({0,0}) + { } Not sure if we need initialize this. In fact it'd be nice to let ASAN catch bad accesses.
+ void set_storage_engine_name(const LEX_CSTRING &name) + { + m_storage_engine_name= name; + } +}; + + /** @class Sql_cmd - Representation of an SQL command.
@@ -145,6 +162,8 @@ class Sql_cmd : public Sql_alloc */ virtual bool execute(THD *thd) = 0;
+ virtual Sql_cmd_options_table_dml *options_table_dml() { return NULL; } + protected: Sql_cmd() {} diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 9cb65e8..c8b9219 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -5699,6 +5431,7 @@ mysql_execute_command(THD *thd) case SQLCOM_OPTIMIZE: case SQLCOM_REPAIR: case SQLCOM_TRUNCATE: + case SQLCOM_CREATE_TABLE: case SQLCOM_ALTER_TABLE: DBUG_ASSERT(first_table == all_tables && first_table != 0); /* fall through */ This assertion is already duplicated by Sql_cmd_create_table::execute(). I wonder if it'd make sense to move SQLCOM_CREATE_TABLE forward?
diff --git a/sql/sql_table.cc b/sql/sql_table.cc index ca78c01..c4c3ef8 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -10116,3 +10116,304 @@ bool check_engine(THD *thd, const char *db_name,
DBUG_RETURN(false); } + + +bool Sql_cmd_create_table::execute(THD *thd) +{ + DBUG_ENTER("Sql_cmd_create_table::execute"); + LEX *lex= thd->lex; + TABLE_LIST *all_tables= lex->query_tables; + SELECT_LEX *select_lex= &lex->select_lex; + TABLE_LIST *first_table= select_lex->table_list.first; + DBUG_ASSERT(first_table == all_tables && first_table != 0); + bool link_to_local; + TABLE_LIST *create_table= first_table; + TABLE_LIST *select_tables= lex->create_last_non_select_table->next_global; + /* most outer SELECT_LEX_UNIT of query */ + SELECT_LEX_UNIT *unit= &lex->unit; + int res= 0; + + bool used_engine= lex->create_info.used_fields & HA_CREATE_USED_ENGINE; + DBUG_ASSERT(m_storage_engine_name.length || !used_engine); + if (used_engine) + { + if (thd->resolve_storage_engine(&lex->create_info.db_type, + m_storage_engine_name, + lex->create_info.tmp_table())) + DBUG_RETURN(true); // Engine not found, substitution is not allowed + + if (!lex->create_info.db_type) + { + lex->create_info.use_default_db_type(thd); + push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, + ER_WARN_USING_OTHER_HANDLER, + ER_THD(thd, ER_WARN_USING_OTHER_HANDLER), + hton_name(lex->create_info.db_type)->str, + create_table->table_name); + } + } It should probably be joined with "If no engine type was given..." condition. Then you won't need used_engine variable.
diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index 59cc73d..7de5bef 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -2454,6 +2454,8 @@ create: create_or_replace opt_temporary TABLE_SYM opt_if_not_exists table_ident { LEX *lex= thd->lex; + if (!(lex->m_sql_cmd= new (thd->mem_root) Sql_cmd_create_table())) + MYSQL_YYABORT; C++ allows to omit parenthesis if constructor has no parameters.
@@ -5515,10 +5507,19 @@ create_table_options: ;
create_table_option: - ENGINE_SYM opt_equal storage_engines + ENGINE_SYM opt_equal ident_or_text { - Lex->create_info.db_type= $3; - Lex->create_info.used_fields|= HA_CREATE_USED_ENGINE; + LEX *lex= Lex; + if (!lex->m_sql_cmd) + { + DBUG_ASSERT(lex->sql_command == SQLCOM_ALTER_TABLE); + if (!(lex->m_sql_cmd= new (thd->mem_root) Sql_cmd_alter_table())) + MYSQL_YYABORT; + } This looks ugly, but I guess we cannot fix it with low effort.
+ Sql_cmd_options_table_dml *opt= lex->m_sql_cmd->options_table_dml(); + DBUG_ASSERT(opt); // Expect a proper Sql_cmd Are there any other options to get Sql_cmd_options_table_dml here?
+ opt->set_storage_engine_name({$3.str, $3.length}); + lex->create_info.used_fields|= HA_CREATE_USED_ENGINE; } | MAX_ROWS opt_equal ulonglong_num { @@ -5783,21 +5784,10 @@ default_collation: storage_engines: ident_or_text { - plugin_ref plugin= ha_resolve_by_name(thd, &$1, - thd->lex->create_info.tmp_table()); - - if (plugin) - $$= plugin_hton(plugin); - else - { - if (thd->variables.sql_mode & MODE_NO_ENGINE_SUBSTITUTION) - my_yyabort_error((ER_UNKNOWN_STORAGE_ENGINE, MYF(0), $1.str)); - $$= 0; - push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, - ER_UNKNOWN_STORAGE_ENGINE, - ER_THD(thd, ER_UNKNOWN_STORAGE_ENGINE), - $1.str); - } + LEX_CSTRING tmp= {$1.str, $1.length}; + if (thd->resolve_storage_engine(&$$, tmp, + thd->lex->create_info.tmp_table())) + MYSQL_YYABORT; I guess you could avoid temporary variable here.
Regards, Sergey