[Maria-developers] Review of MDEV-7112 Split HA_CREATE_INFO
Hi. It think the split is good, I liked it. I didn't like that you've tried to put the refactoring of sql_db.cc and sql_lang.h in the same commit, please move it to a separate patch. MDEV-5359 should not depend on that. Anyway, see all my comments below:
diff --git a/sql/structs.h b/sql/structs.h index 99561c5..b38685c 100644 --- a/sql/structs.h +++ b/sql/structs.h @@ -471,4 +471,67 @@ class Discrete_intervals_list { Discrete_interval* get_current() const { return current; }; };
+ +struct DDL_options_st +{ +public: + enum Options + { + OPT_NONE= 0, + OPT_IF_NOT_EXISTS= 2, // CREATE TABLE IF NOT EXISTS + OPT_LIKE= 4, // CREATE TABLE LIKE + OPT_OR_REPLACE= 16, // CREATE OR REPLACE TABLE + OPT_OR_REPLACE_SLAVE_GENERATED= 32,// REPLACE was added on slave, it was + // not in the original query on master. + OPT_IF_EXISTS= 64 + }; + +private: + Options m_options; + +protected: + +public: + Options create_like_options() const + { + return (DDL_options_st::Options) + (((uint) m_options) & (OPT_IF_NOT_EXISTS | OPT_OR_REPLACE)); + } + void init() { m_options= OPT_NONE; } + void init(Options options) { m_options= options; } + void set(Options other) + { + m_options= other; + } + void set(const DDL_options_st other) + { + m_options= other.m_options; + } + bool if_not_exists() const { return m_options & OPT_IF_NOT_EXISTS; } + bool or_replace() const { return m_options & OPT_OR_REPLACE; } + bool or_replace_slave_generated() const + { return m_options & OPT_OR_REPLACE_SLAVE_GENERATED; } + bool like() const { return m_options & OPT_LIKE; } + bool if_exists() const { return m_options & OPT_IF_EXISTS; } + void add(const DDL_options_st::Options other) + { + m_options= (Options) ((uint) m_options | (uint) other); + } + void add(const DDL_options_st &other) + { + add(other.m_options); + } + DDL_options_st operator|(const DDL_options_st &other) + { + add(other.m_options); + return *this; + } + DDL_options_st operator|=(DDL_options_st::Options other) + { + add(other); + return *this; + } +};
I don't particularly like this style of C++ing... It's way too verbose. but ok
+ + #endif /* STRUCTS_INCLUDED */ diff --git a/sql/handler.h b/sql/handler.h index 0044556..7b6a5f4 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -376,10 +376,7 @@ enum enum_alter_inplace_result { #define HA_KEY_BLOB_LENGTH 2
#define HA_LEX_CREATE_TMP_TABLE 1 -#define HA_LEX_CREATE_IF_NOT_EXISTS 2 -#define HA_LEX_CREATE_TABLE_LIKE 4 #define HA_CREATE_TMP_ALTER 8 -#define HA_LEX_CREATE_REPLACE 16
One empty line here, please, HA_MAX_REC_LENGTH is not a flag
#define HA_MAX_REC_LENGTH 65535
/* Table caching type */ @@ -1571,9 +1568,9 @@ enum enum_stats_auto_recalc { HA_STATS_AUTO_RECALC_DEFAULT= 0, HA_STATS_AUTO_RECALC_ON, HA_STATS_AUTO_RECALC_OFF };
Could you add structure comments here? Like what you have in MDEV, "This will be passed to mysql_create_db, mysql_drop_db, mysqld_show_create_db", etc To explain what every structure is for, what kind of data it should store, and expected users (internal DB DDL, internal table DDL, handler, etc)
-struct HA_CREATE_INFO +struct Table_contents_source_st { - CHARSET_INFO *table_charset, *default_table_charset; + CHARSET_INFO *table_charset; LEX_CUSTRING tabledef_version; LEX_STRING connect_string; const char *password, *tablespace; @@ -1635,6 +1635,55 @@ struct HA_CREATE_INFO };
comments for all new structures, please
+struct Schema_specification_st +{ + CHARSET_INFO *default_table_charset; + void init() + { + bzero(this, sizeof(*this)); + } +}; + + +struct HA_CREATE_INFO: public Table_contents_source_st, + public Schema_specification_st +{ + void init() + { + Table_contents_source_st::init(); + Schema_specification_st::init(); + } +}; + + +struct Table_specification_st: public HA_CREATE_INFO, + public DDL_options_st +{ + // Deep initialization + void init() + { + HA_CREATE_INFO::init(); + DDL_options_st::init(); + } + void init(DDL_options_st::Options options) + { + HA_CREATE_INFO::init(); + DDL_options_st::init(options); + } + /* + Quick initialization, for parser. + Most of the HA_CREATE_INFO is left uninitialized. + It gets fully initialized in sql_yacc.yy, only when the parser + scans a related keyword (e.g. CREATE, ALTER). + */ + void lex_start() + { + HA_CREATE_INFO::options= 0; + DDL_options_st::init(); + } +}; + + /** In-place alter handler context.
diff --git a/sql/sql_lang.h b/sql/sql_lang.h new file mode 100644 index 0000000..76a1340 --- /dev/null +++ b/sql/sql_lang.h @@ -0,0 +1,79 @@ +/* Copyright (c) 2014, MariaDB Foundation + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; version 2 of the License. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ + +#ifndef SQL_LANG_INCLUDED +#define SQL_LANG_INCLUDED + +namespace SQL { + +class Identifier: protected LEX_CSTRING +{ + void set(const LEX_STRING &other) + { + str= other.str; + length= other.length; + DBUG_ASSERT(str[length] == 0); + } +public: + Identifier(const LEX_STRING &other) + { + set(other); + } + Identifier(const LEX *); + const char *name() const { return str; } + const size_t name_length() const { return length; } +};
Eh? What all this has to do with MDEV-7112 "Split HA_CREATE_INFO" ? it's not mentioned there. If it's unrelated refactoring, please, put it in a separate patch.
+ + +class Identifier_normalized: public Identifier +{ + char m_buf[NAME_LEN + 1 + MYSQL50_TABLE_NAME_PREFIX_LENGTH + 10]; + void normalize(); +public: + Identifier_normalized(const LEX_STRING &other) + :Identifier(other) { normalize(); } + Identifier_normalized(const LEX *lex) + :Identifier(lex) { normalize(); } +}; + + +namespace Statement { + + +/* + All SQL statements that can be executed directly. + Statement instances are created using the data in thd->lex, + previously populated by the SQL parser. +*/ +class Directly_executable +{ +public: + /* + Perform all stages of a direct SQL execution: + - Check for validity (e.g. name in "CREATE SCHEMA name" is valid). + - Check access + - Apply filters (e.g. Rpl_filters on slave) + - Perform WSREP_TO_ISOLATION + - Execute the actual action + */ + virtual int exec_direct(THD *thd) const = 0; +}; + +} // End of namescpace Statement + + +} // End of namespace SQL + +#endif /* SQL_LANG_INCLUDED */ diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 0bbcca5..0edda65 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -1923,7 +1923,9 @@ bool MDL_deadlock_handler::handle_condition(THD *, */
static bool -open_table_get_mdl_lock(THD *thd, Open_table_context *ot_ctx, +open_table_get_mdl_lock(THD *thd, + const DDL_options_st &options, + Open_table_context *ot_ctx,
What's the point - you don't seem to be using options in this function? And that recursively means that you don't need 'options' in all other functions that only pass 'options' argument down to open_table_get_mdl_lock() but otherwise don't use it.
MDL_request *mdl_request, uint flags, MDL_ticket **mdl_ticket) @@ -4176,8 +4182,8 @@ thr_lock_type read_lock_type_for_table(THD *thd, DBUG_RETURN(FALSE);
/* Check if CREATE TABLE without REPLACE was used */ - create_table= (thd->lex->sql_command == SQLCOM_CREATE_TABLE && - !(thd->lex->create_info.options & HA_LEX_CREATE_REPLACE)); + create_table= thd->lex->sql_command == SQLCOM_CREATE_TABLE && + !options.or_replace();
why, options are still in thd->lex, aren't they?
if (!(flags & MYSQL_OPEN_SKIP_SCOPED_MDL_LOCK)) { diff --git a/sql/sql_db.h b/sql/sql_db.h index 62d379c..dc62e6b 100644 --- a/sql/sql_db.h +++ b/sql/sql_db.h @@ -35,13 +33,178 @@ bool mysql_opt_change_db(THD *thd, bool my_dboptions_cache_init(void); void my_dboptions_cache_free(void); bool check_db_dir_existence(const char *db_name); -bool load_db_opt(THD *thd, const char *path, HA_CREATE_INFO *create); +bool load_db_opt(THD *thd, const char *path, Schema_specification_st *create); bool load_db_opt_by_name(THD *thd, const char *db_name, - HA_CREATE_INFO *db_create_info); + Schema_specification_st *db_create_info); CHARSET_INFO *get_default_db_collation(THD *thd, const char *db_name); bool my_dbopt_init(void); void my_dbopt_cleanup(void);
+ +namespace SQL { + + +namespace Statement { + + +namespace Schema { + + +// Schema name +class Name: public Identifier_normalized +{ +public: + Name(const LEX_STRING &other) :Identifier_normalized(other) { } + Name(const LEX *lex) :Identifier_normalized(lex) { } +#ifdef HAVE_REPLICATION + bool check_if_slave_ignored(THD *thd) const; +#endif + bool check() const; + bool check_with_error() const; +}; + + +// Schema definition options: IF EXISTS, IF NOT EXISTS +class DDL_options: public DDL_options_st +{ +public: + DDL_options() { init(); } + DDL_options(Options options) + { + set(options); + } + DDL_options(const DDL_options_st options) + { + set(options); + } + DDL_options(const LEX *lex); +}; + + +// Schema specification, e.g. CHARACTER SET xxx +class Specification: public Schema_specification_st +{ +public: + Specification(const Schema_specification_st &info) + :Schema_specification_st(info) + { } + Specification(const LEX *lex); +}; + + +/** + A common class for all Schema DDL options: + - CREATE SCHEMA + - DROP SCHEMA + - ALTER SCHEMA + - ALTER SCHEMA UPGRADE DATA DIRECTORY NAME +*/ +class DDL: public Directly_executable +{ +protected: + bool rm_db(THD *thd, const Name &name, bool if_exists, bool silent) const; + + int create_db(THD *thd, + const Name &name, + const DDL_options_st options, + const Schema_specification_st &info, + bool silent) const; +}; + + +class DDL_with_name: public DDL, public Name +{ + /* + Execute a DDL statement. + Assumes that Name has already been checked for validity. + */ + virtual bool exec(THD *thd) const = 0; + virtual bool check_access(THD *) const = 0; +protected: + DDL_with_name(const Name &name): Name(name) { } + DDL_with_name(const LEX *lex): Name(lex) { } +public: + int exec_direct(THD *thd) const; +}; + + +class DDL_with_name_and_options: public DDL_with_name, public DDL_options +{ +protected: + DDL_with_name_and_options(const LEX *lex) + :DDL_with_name(lex), DDL_options(lex) { } + DDL_with_name_and_options(const Name &name) + :DDL_with_name(name), DDL_options() { } +}; + + +class Create: public DDL_with_name_and_options, public Specification +{ + bool exec(THD *thd) const + { + return DDL::create_db(thd, *this, *this, *this, false); + } + bool check_access(THD *thd) const + { + return ::check_access(thd, CREATE_ACL, name(), NULL, NULL, 1, 0); + } +public: + Create(const LEX *lex) + :DDL_with_name_and_options(lex), Specification(lex) + { } + Create(const Name &name, const Schema_specification_st &info) + :DDL_with_name_and_options(name), Specification(info) + { } +}; + + +class Drop: public DDL_with_name_and_options +{ + bool exec(THD *thd) const + { + return rm_db(thd, *this, if_exists(), false); + } + bool check_access(THD *thd) const + { + return ::check_access(thd, DROP_ACL, name(), NULL, NULL, 1, 0); + } +public: + Drop(const Name &name) :DDL_with_name_and_options(name) { } + Drop(const LEX *lex) :DDL_with_name_and_options(lex) { } +}; + + +class Alter: public DDL_with_name, public Specification +{ + bool exec(THD *thd) const; + bool check_access(THD *thd) const + { + return ::check_access(thd, ALTER_ACL, name(), NULL, NULL, 1, 0); + } +public: + Alter(const LEX *lex) :DDL_with_name(lex), Specification(lex) { } +}; + + +class Upgrade: public DDL_with_name +{ + bool exec(THD *thd) const; + bool check_access(THD *thd) const + { + return ::check_access(thd, ALTER_ACL, name(), NULL, NULL, 1, 0) || + ::check_access(thd, DROP_ACL, name(), NULL, NULL, 1, 0) || + ::check_access(thd, CREATE_ACL, name(), NULL, NULL, 1, 0); + } +public: + Upgrade(const LEX *lex) :DDL_with_name(lex) { } + int exec_direct(THD *thd) const; +}; + + +} // End of namespace SQL::Statement::Schema +} // End of namespace SQL::Statement +} // End of namespace SQL
Please, no. This and sql_lang.h - make it a separate patch This one should be *only* about splitting HA_CREATE_INFO
+ #define MY_DB_OPT_FILE "db.opt"
#endif /* SQL_DB_INCLUDED */ diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index b905d8b..c59332e 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -4181,143 +4176,21 @@ static bool do_execute_sp(THD *thd, sp_head *sp) } break; case SQLCOM_CREATE_DB: - { - /* - As mysql_create_db() may modify HA_CREATE_INFO structure passed to - it, we need to use a copy of LEX::create_info to make execution - prepared statement- safe. - */ - HA_CREATE_INFO create_info(lex->create_info); - if (check_db_name(&lex->name)) - { - my_error(ER_WRONG_DB_NAME, MYF(0), lex->name.str); - break; - } - /* - If in a slave thread : - CREATE DATABASE DB was certainly not preceded by USE DB. - For that reason, db_ok() in sql/slave.cc did not check the - do_db/ignore_db. And as this query involves no tables, tables_ok() - above was not called. So we have to check rules again here. - */ -#ifdef HAVE_REPLICATION - if (thd->slave_thread) - { - rpl_filter= thd->system_thread_info.rpl_sql_info->rpl_filter; - if (!rpl_filter->db_ok(lex->name.str) || - !rpl_filter->db_ok_with_wild_table(lex->name.str)) - { - my_message(ER_SLAVE_IGNORED_TABLE, ER(ER_SLAVE_IGNORED_TABLE), MYF(0)); - break; - } - } -#endif - if (check_access(thd, CREATE_ACL, lex->name.str, NULL, NULL, 1, 0)) - break; - WSREP_TO_ISOLATION_BEGIN(lex->name.str, NULL, NULL) - res= mysql_create_db(thd, lex->name.str, &create_info, 0);
separate refactoring patch, please
+ if ((res= SQL::Statement::Schema::Create(lex).exec_direct(thd)) < 0) + goto error; break; - } case SQLCOM_DROP_DB: - { - if (check_db_name(&lex->name)) - { - my_error(ER_WRONG_DB_NAME, MYF(0), lex->name.str); - break; - } - /* - If in a slave thread : - DROP DATABASE DB may not be preceded by USE DB. - For that reason, maybe db_ok() in sql/slave.cc did not check the - do_db/ignore_db. And as this query involves no tables, tables_ok() - above was not called. So we have to check rules again here. - */ -#ifdef HAVE_REPLICATION - if (thd->slave_thread) - { - rpl_filter= thd->system_thread_info.rpl_sql_info->rpl_filter; - if (!rpl_filter->db_ok(lex->name.str) || - !rpl_filter->db_ok_with_wild_table(lex->name.str)) - { - my_message(ER_SLAVE_IGNORED_TABLE, ER(ER_SLAVE_IGNORED_TABLE), MYF(0)); - break; - } - } -#endif - if (check_access(thd, DROP_ACL, lex->name.str, NULL, NULL, 1, 0)) - break; - WSREP_TO_ISOLATION_BEGIN(lex->name.str, NULL, NULL) - res= mysql_rm_db(thd, lex->name.str, lex->check_exists, 0); + if ((res= SQL::Statement::Schema::Drop(lex).exec_direct(thd)) < 0) + goto error; break; - } case SQLCOM_ALTER_DB_UPGRADE: - { - LEX_STRING *db= & lex->name; -#ifdef HAVE_REPLICATION - if (thd->slave_thread) - { - rpl_filter= thd->system_thread_info.rpl_sql_info->rpl_filter; - if (!rpl_filter->db_ok(db->str) || - !rpl_filter->db_ok_with_wild_table(db->str)) - { - res= 1; - my_message(ER_SLAVE_IGNORED_TABLE, ER(ER_SLAVE_IGNORED_TABLE), MYF(0)); - break; - } - } -#endif - if (check_db_name(db)) - { - my_error(ER_WRONG_DB_NAME, MYF(0), db->str); - break; - } - if (check_access(thd, ALTER_ACL, db->str, NULL, NULL, 1, 0) || - check_access(thd, DROP_ACL, db->str, NULL, NULL, 1, 0) || - check_access(thd, CREATE_ACL, db->str, NULL, NULL, 1, 0)) - { - res= 1; - break; - } - WSREP_TO_ISOLATION_BEGIN(db->str, NULL, NULL) - res= mysql_upgrade_db(thd, db); - if (!res) - my_ok(thd); + if ((res= SQL::Statement::Schema::Upgrade(lex).exec_direct(thd)) < 0) + goto error; break; - } case SQLCOM_ALTER_DB: - { - LEX_STRING *db= &lex->name; - HA_CREATE_INFO create_info(lex->create_info); - if (check_db_name(db)) - { - my_error(ER_WRONG_DB_NAME, MYF(0), db->str); - break; - } - /* - If in a slave thread : - ALTER DATABASE DB may not be preceded by USE DB. - For that reason, maybe db_ok() in sql/slave.cc did not check the - do_db/ignore_db. And as this query involves no tables, tables_ok() - above was not called. So we have to check rules again here. - */ -#ifdef HAVE_REPLICATION - if (thd->slave_thread) - { - rpl_filter= thd->system_thread_info.rpl_sql_info->rpl_filter; - if (!rpl_filter->db_ok(db->str) || - !rpl_filter->db_ok_with_wild_table(db->str)) - { - my_message(ER_SLAVE_IGNORED_TABLE, ER(ER_SLAVE_IGNORED_TABLE), MYF(0)); - break; - } - } -#endif - if (check_access(thd, ALTER_ACL, db->str, NULL, NULL, 1, 0)) - break; - WSREP_TO_ISOLATION_BEGIN(db->str, NULL, NULL) - res= mysql_alter_db(thd, db->str, &create_info); + if ((res= SQL::Statement::Schema::Alter(lex).exec_direct(thd)) < 0) + goto error; break; - } case SQLCOM_SHOW_CREATE_DB: { char db_name_buff[NAME_LEN+1]; diff --git a/sql/sql_show.cc b/sql/sql_show.cc index 0541cbc..2536255 100644 --- a/sql/sql_show.cc +++ b/sql/sql_show.cc @@ -1696,14 +1695,14 @@ int show_create_table(THD *thd, TABLE_LIST *table_list, String *packet,
packet->append(STRING_WITH_LEN("CREATE ")); if (create_info_arg && - (create_info_arg->org_options & HA_LEX_CREATE_REPLACE || + ((create_info_arg->or_replace() && + !create_info_arg->or_replace_slave_generated()) ||
that's a bit strange. Could you explain this? why org_options & HA_LEX_CREATE_REPLACE is translated to or_replace() && !or_replace_slave_generated()
create_info_arg->table_was_deleted)) packet->append(STRING_WITH_LEN("OR REPLACE ")); if (share->tmp_table) packet->append(STRING_WITH_LEN("TEMPORARY ")); packet->append(STRING_WITH_LEN("TABLE ")); - if (create_info_arg && - (create_info_arg->options & HA_LEX_CREATE_IF_NOT_EXISTS)) + if (create_info_arg && create_info_arg->if_not_exists()) packet->append(STRING_WITH_LEN("IF NOT EXISTS ")); if (table_list->schema_table) alias= table_list->schema_table->table_name; diff --git a/sql/sql_table.cc b/sql/sql_table.cc index b991215..7bb293b 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -4628,7 +4628,8 @@ int create_table_impl(THD *thd, const char *orig_db, const char *orig_table_name, const char *db, const char *table_name, const char *path, - HA_CREATE_INFO *create_info, + const DDL_options_st options, + HA_CREATE_INFO &create_info,
why? I generally prefer pointers over C++ references - one can immediately see what variables can be modified by the function. is there a compelling reason to use references here?
Alter_info *alter_info, int create_table_mode, bool *is_trans, @@ -5273,9 +5275,10 @@ bool mysql_create_like_table(THD* thd, TABLE_LIST* table, */
/* Copy temporarily the statement flags to thd for lock_table_names() */ + // QQ: is this really needed???
may be not anymore, as you pass flags separately
uint save_thd_create_info_options= thd->lex->create_info.options; - thd->lex->create_info.options|= create_info->options; - res= open_tables(thd, &thd->lex->query_tables, ¬_used, 0); + thd->lex->create_info.options|= create_info.options; + res= open_tables(thd, create_info, &thd->lex->query_tables, ¬_used, 0); thd->lex->create_info.options= save_thd_create_info_options;
if (res) diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index 632bdd2..0f2c0e0 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -1621,7 +1623,8 @@ bool my_yyoverflow(short **a, YYSTYPE **b, ulong *yystacksize); IDENT_sys TEXT_STRING_sys TEXT_STRING_literal NCHAR_STRING opt_component key_cache_name sp_opt_label BIN_NUM label_ident TEXT_STRING_filesystem ident_or_empty - opt_constraint constraint opt_ident opt_if_not_exists_ident + opt_constraint constraint opt_ident + opt_if_not_exists_opt_table_element_name
can you please try to come up with shorter identifier names? 40 characters - that's a bit too long :)
%type <lex_str_ptr> opt_table_alias diff --git a/storage/xtradb/handler/ha_innodb.cc b/storage/xtradb/handler/ha_innodb.cc index f3b1167..6b1942e 100644 --- a/storage/xtradb/handler/ha_innodb.cc +++ b/storage/xtradb/handler/ha_innodb.cc @@ -11349,7 +11349,7 @@ static __attribute__((nonnull, warn_unused_result))
/* Do not use DATA DIRECTORY with TEMPORARY TABLE. */ if (create_info->data_file_name - && create_info->options & HA_LEX_CREATE_TMP_TABLE) { + && create_info->tmp_table()) {
nope, you should not change storage engine API here. tmp_table() was a convenience wrapper, old way should still work
push_warning( thd, Sql_condition::WARN_LEVEL_WARN, ER_ILLEGAL_HA_CREATE_OPTION,
Regards, Sergei
Hi Sergei, Thanks for review. I addressed most of your suggestions. A fixed version is attached. See details inline: On 12/03/2014 01:13 AM, Sergei Golubchik wrote:
Hi.
It think the split is good, I liked it.
I didn't like that you've tried to put the refactoring of sql_db.cc and sql_lang.h in the same commit, please move it to a separate patch. MDEV-5359 should not depend on that.
When I had to change the interface for mysql_create_db(), mysql_rm_db(), mysql_alter_db(), I thought why not to do it in the right way at once... And it was very easy to do in case of the db routines. Anyway, I agree to make a separate task for this, It's easier to review this way :)
Anyway, see all my comments below:
diff --git a/sql/structs.h b/sql/structs.h index 99561c5..b38685c 100644 --- a/sql/structs.h +++ b/sql/structs.h
<skip>
+ DDL_options_st operator|=(DDL_options_st::Options other) + { + add(other); + return *this; + } +};
I don't particularly like this style of C++ing... It's way too verbose. but ok
This is to make the code on sql_yacc.yy shorter: lex->set_command_with_check(SQLCOM_CREATE_TABLE, $2, $1 | $4) I find "$1 | $4" very clearly readable, and short.
+ + #endif /* STRUCTS_INCLUDED */ diff --git a/sql/handler.h b/sql/handler.h index 0044556..7b6a5f4 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -376,10 +376,7 @@ enum enum_alter_inplace_result { #define HA_KEY_BLOB_LENGTH 2
#define HA_LEX_CREATE_TMP_TABLE 1 -#define HA_LEX_CREATE_IF_NOT_EXISTS 2 -#define HA_LEX_CREATE_TABLE_LIKE 4 #define HA_CREATE_TMP_ALTER 8 -#define HA_LEX_CREATE_REPLACE 16
One empty line here, please, HA_MAX_REC_LENGTH is not a flag
Done.
#define HA_MAX_REC_LENGTH 65535
/* Table caching type */ @@ -1571,9 +1568,9 @@ enum enum_stats_auto_recalc { HA_STATS_AUTO_RECALC_DEFAULT= 0, HA_STATS_AUTO_RECALC_ON, HA_STATS_AUTO_RECALC_OFF };
Could you add structure comments here? Like what you have in MDEV, "This will be passed to mysql_create_db, mysql_drop_db, mysqld_show_create_db", etc
To explain what every structure is for, what kind of data it should store, and expected users (internal DB DDL, internal table DDL, handler, etc)
Done. <skip>
diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 0bbcca5..0edda65 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -1923,7 +1923,9 @@ bool MDL_deadlock_handler::handle_condition(THD *, */
static bool -open_table_get_mdl_lock(THD *thd, Open_table_context *ot_ctx, +open_table_get_mdl_lock(THD *thd, + const DDL_options_st &options, + Open_table_context *ot_ctx,
What's the point - you don't seem to be using options in this function?
And that recursively means that you don't need 'options' in all other functions that only pass 'options' argument down to open_table_get_mdl_lock() but otherwise don't use it.
Thanks for noticing this. I fixed this function and the callers that do not use options otherwise.
MDL_request *mdl_request, uint flags, MDL_ticket **mdl_ticket) @@ -4176,8 +4182,8 @@ thr_lock_type read_lock_type_for_table(THD *thd, DBUG_RETURN(FALSE);
/* Check if CREATE TABLE without REPLACE was used */ - create_table= (thd->lex->sql_command == SQLCOM_CREATE_TABLE && - !(thd->lex->create_info.options & HA_LEX_CREATE_REPLACE)); + create_table= thd->lex->sql_command == SQLCOM_CREATE_TABLE && + !options.or_replace();
why, options are still in thd->lex, aren't they?
Not really always. sql_parse.cc does the following: Table_specification_st create_info(lex->create_info); ... if (thd->slave_thread && slave_ddl_exec_mode_options == SLAVE_EXEC_MODE_IDEMPOTENT && !lex->create_info.if_not_exists()) { create_info.add(DDL_options_st::OPT_OR_REPLACE); create_info.add(DDL_options_st::OPT_OR_REPLACE_SLAVE_GENERATED); } So the "options" parameter can have extra flags comparing to thd->lex->create_info. Btw, it would be nice to make somehow thd->lex->create_info inaccessible inside lock_table_names() and some other routines, to avoid this ambiguity. But I have no idea how to do so in a small change. <skip>
diff --git a/sql/sql_show.cc b/sql/sql_show.cc index 0541cbc..2536255 100644 --- a/sql/sql_show.cc +++ b/sql/sql_show.cc @@ -1696,14 +1695,14 @@ int show_create_table(THD *thd, TABLE_LIST *table_list, String *packet,
packet->append(STRING_WITH_LEN("CREATE ")); if (create_info_arg && - (create_info_arg->org_options & HA_LEX_CREATE_REPLACE || + ((create_info_arg->or_replace() && + !create_info_arg->or_replace_slave_generated()) ||
that's a bit strange. Could you explain this? why org_options & HA_LEX_CREATE_REPLACE is translated to or_replace() && !or_replace_slave_generated()
Notice, I removed "org_options". Now everything is passed through a single set of flags: the DDL_options_st part of *create_info_arg. And the related flag is set in sql_parse.cc, here: create_info.add(DDL_options_st::OPT_OR_REPLACE_SLAVE_GENERATED); I find this logic to be much easier to understand.
create_info_arg->table_was_deleted)) packet->append(STRING_WITH_LEN("OR REPLACE ")); if (share->tmp_table) packet->append(STRING_WITH_LEN("TEMPORARY ")); packet->append(STRING_WITH_LEN("TABLE ")); - if (create_info_arg && - (create_info_arg->options & HA_LEX_CREATE_IF_NOT_EXISTS)) + if (create_info_arg && create_info_arg->if_not_exists()) packet->append(STRING_WITH_LEN("IF NOT EXISTS ")); if (table_list->schema_table) alias= table_list->schema_table->table_name; diff --git a/sql/sql_table.cc b/sql/sql_table.cc index b991215..7bb293b 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -4628,7 +4628,8 @@ int create_table_impl(THD *thd, const char *orig_db, const char *orig_table_name, const char *db, const char *table_name, const char *path, - HA_CREATE_INFO *create_info, + const DDL_options_st options, + HA_CREATE_INFO &create_info,
why? I generally prefer pointers over C++ references - one can immediately see what variables can be modified by the function. is there a compelling reason to use references here?
Well, I had a hope I'd manage to do it a const: const HA_CREATE_INFO &create_info And in case of "const", I really prefer "const XXX &name", instead of "const XXX *name". It gives a lot of advantages. But could not make it a const without more changes with splitting of the structures into parts. Reverted back to "HA_CREATE_INFO *create_info".
Alter_info *alter_info, int create_table_mode, bool *is_trans, @@ -5273,9 +5275,10 @@ bool mysql_create_like_table(THD* thd, TABLE_LIST* table, */
/* Copy temporarily the statement flags to thd for lock_table_names() */ + // QQ: is this really needed???
may be not anymore, as you pass flags separately
I'm not 100% sure. I only pass DDL_options_st separately. But the handler flags are still in thd->lex->create_info.options. And I don't know if the callers do not change them..,
uint save_thd_create_info_options= thd->lex->create_info.options; - thd->lex->create_info.options|= create_info->options; - res= open_tables(thd, &thd->lex->query_tables, ¬_used, 0); + thd->lex->create_info.options|= create_info.options; + res= open_tables(thd, create_info, &thd->lex->query_tables, ¬_used, 0); thd->lex->create_info.options= save_thd_create_info_options;
if (res) diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index 632bdd2..0f2c0e0 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -1621,7 +1623,8 @@ bool my_yyoverflow(short **a, YYSTYPE **b, ulong *yystacksize); IDENT_sys TEXT_STRING_sys TEXT_STRING_literal NCHAR_STRING opt_component key_cache_name sp_opt_label BIN_NUM label_ident TEXT_STRING_filesystem ident_or_empty - opt_constraint constraint opt_ident opt_if_not_exists_ident + opt_constraint constraint opt_ident + opt_if_not_exists_opt_table_element_name
can you please try to come up with shorter identifier names? 40 characters - that's a bit too long :)
I renamed it because it did not reflect what it was used for: 1. I read "opt_if_not_exists_ident" as "IF NOT EXISTS ident, or empty". But in fact it's more complex. 2. It's used only in key name or constraint name context. So "ident" was also confusing (too general). I thought my name was very self explanatory. Can we keep it? Or, suggest a shorter one :)
%type <lex_str_ptr> opt_table_alias diff --git a/storage/xtradb/handler/ha_innodb.cc b/storage/xtradb/handler/ha_innodb.cc index f3b1167..6b1942e 100644 --- a/storage/xtradb/handler/ha_innodb.cc +++ b/storage/xtradb/handler/ha_innodb.cc @@ -11349,7 +11349,7 @@ static __attribute__((nonnull, warn_unused_result))
/* Do not use DATA DIRECTORY with TEMPORARY TABLE. */ if (create_info->data_file_name - && create_info->options & HA_LEX_CREATE_TMP_TABLE) { + && create_info->tmp_table()) {
nope, you should not change storage engine API here. tmp_table() was a convenience wrapper, old way should still work
I haven't changed the API at this point, so the old way still works. But I don't want any newly created engines to copy the old way. There is a chance that we'll move the table scope part into a separate structure, and Table_scope_and_contents_source_st will then inherit it. Now table scope is only responsible for TEMPORARY or not TEMPORARY, but in the SQL standard it's something more complex: http://savage.net.au/SQL/sql-2003-2.bnf.html#table%20scope So if we ever decide to support more table scope options, the HA_LEX_CREATE_TMP_TABLE flag most likely won't be stored in create_info->options any more. In any cases, encapsulation is generally better in many aspects than direct field access. tmp_table() was not just a convenience wrapper, it is indeed a step towards the right direction :) And I'd like to propagate it, to close direct access eventually.
push_warning( thd, Sql_condition::WARN_LEVEL_WARN, ER_ILLEGAL_HA_CREATE_OPTION,
Regards, Sergei
Hi, Alexander! On Dec 03, Alexander Barkov wrote:
On 12/03/2014 01:13 AM, Sergei Golubchik wrote:
Hi.
It think the split is good, I liked it.
I didn't like that you've tried to put the refactoring of sql_db.cc and sql_lang.h in the same commit, please move it to a separate patch. MDEV-5359 should not depend on that.
When I had to change the interface for mysql_create_db(), mysql_rm_db(), mysql_alter_db(), I thought why not to do it in the right way at once... And it was very easy to do in case of the db routines.
Anyway, I agree to make a separate task for this, It's easier to review this way :)
Thanks!
Anyway, see all my comments below:
diff --git a/sql/structs.h b/sql/structs.h index 99561c5..b38685c 100644 --- a/sql/structs.h +++ b/sql/structs.h <skip> + DDL_options_st operator|=(DDL_options_st::Options other) + { + add(other); + return *this; + } +};
I don't particularly like this style of C++ing... It's way too verbose. but ok
This is to make the code on sql_yacc.yy shorter:
lex->set_command_with_check(SQLCOM_CREATE_TABLE, $2, $1 | $4)
I find "$1 | $4" very clearly readable, and short.
Yes, I didn't mean operator| in particular, but the whole class with accessors for every single bit, etc. C++ is very verbose language and the more code you write, more code someone will have to read. So I generally like it when refactoring makes the codebase smaller, not larger.
diff --git a/sql/sql_show.cc b/sql/sql_show.cc index 0541cbc..2536255 100644 --- a/sql/sql_show.cc +++ b/sql/sql_show.cc @@ -1696,14 +1695,14 @@ int show_create_table(THD *thd, TABLE_LIST *table_list, String *packet,
packet->append(STRING_WITH_LEN("CREATE ")); if (create_info_arg && - (create_info_arg->org_options & HA_LEX_CREATE_REPLACE || + ((create_info_arg->or_replace() && + !create_info_arg->or_replace_slave_generated()) ||
that's a bit strange. Could you explain this? why org_options & HA_LEX_CREATE_REPLACE is translated to or_replace() && !or_replace_slave_generated()
Notice, I removed "org_options".
Now everything is passed through a single set of flags: the DDL_options_st part of *create_info_arg.
And the related flag is set in sql_parse.cc, here:
create_info.add(DDL_options_st::OPT_OR_REPLACE_SLAVE_GENERATED);
I find this logic to be much easier to understand.
Yes, of course, I've noticed that. Still, old condition was org_options & HA_LEX_CREATE_REPLACE and not org_options & HA_LEX_CREATE_REPLACE && !(options & HA_LEX_CREATE_REPLACE) So, why your new condition is or_replace() && !or_replace_slave_generated() ?
diff --git a/sql/sql_table.cc b/sql/sql_table.cc index b991215..7bb293b 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -4628,7 +4628,8 @@ int create_table_impl(THD *thd, const char *orig_db, const char *orig_table_name, const char *db, const char *table_name, const char *path, - HA_CREATE_INFO *create_info, + const DDL_options_st options, + HA_CREATE_INFO &create_info,
why? I generally prefer pointers over C++ references - one can immediately see what variables can be modified by the function. is there a compelling reason to use references here?
Well, I had a hope I'd manage to do it a const:
const HA_CREATE_INFO &create_info
And in case of "const", I really prefer "const XXX &name", instead of "const XXX *name". It gives a lot of advantages.
Okay, with 'const' I could agree, right.
diff --git a/storage/xtradb/handler/ha_innodb.cc b/storage/xtradb/handler/ha_innodb.cc index f3b1167..6b1942e 100644 --- a/storage/xtradb/handler/ha_innodb.cc +++ b/storage/xtradb/handler/ha_innodb.cc @@ -11349,7 +11349,7 @@ static __attribute__((nonnull, warn_unused_result))
/* Do not use DATA DIRECTORY with TEMPORARY TABLE. */ if (create_info->data_file_name - && create_info->options & HA_LEX_CREATE_TMP_TABLE) { + && create_info->tmp_table()) {
nope, you should not change storage engine API here. tmp_table() was a convenience wrapper, old way should still work
I haven't changed the API at this point, so the old way still works. But I don't want any newly created engines to copy the old way.
Then only change engines that we develop (MyISAM, Aria, etc) and engines where MariaDB repositories are kind of the authoritative source of the engine source code (Connect, OQGraph). But don't change engines that are developed elsewhere and then merged into MariaDB - those engines we should try to keep as close to the upstream as possible and only change when absolutely necessary.
There is a chance that we'll move the table scope part into a separate structure, and Table_scope_and_contents_source_st will then inherit it. Now table scope is only responsible for TEMPORARY or not TEMPORARY, but in the SQL standard it's something more complex:
So if we ever decide to support more table scope options, the HA_LEX_CREATE_TMP_TABLE flag most likely won't be stored in create_info->options any more.
Yes, and then it will be absolutely necessary to change all engines, because they'll stop working otherwise.
diff --git a/sql/handler.h b/sql/handler.h index 0044556..002a22e 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -1571,9 +1569,41 @@ enum enum_stats_auto_recalc { HA_STATS_AUTO_RECALC_DEFAULT= 0, HA_STATS_AUTO_RECALC_ON, HA_STATS_AUTO_RECALC_OFF };
-struct HA_CREATE_INFO +/** + A helper struct for schema DDL statements: + CREATE SCHEMA [IF NOT EXISTS] name [ schema_specification... ] + ALTER SCHEMA name [ schema_specification... ] + + It stores the "schema_specification" part of the CREATE/ALTER statements and + is passed to mysql_create_db() and mysql_alter_db(). + Currently consists only of the schema default character set and collation.
by the way, have you seen http://www.tocker.ca/2014/12/02/proposal-to-deprecate-collation_database-and... ? Regards, Sergei
Hi Sergei, On 12/03/2014 04:08 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Dec 03, Alexander Barkov wrote:
On 12/03/2014 01:13 AM, Sergei Golubchik wrote:
Hi.
It think the split is good, I liked it.
I didn't like that you've tried to put the refactoring of sql_db.cc and sql_lang.h in the same commit, please move it to a separate patch. MDEV-5359 should not depend on that.
When I had to change the interface for mysql_create_db(), mysql_rm_db(), mysql_alter_db(), I thought why not to do it in the right way at once... And it was very easy to do in case of the db routines.
Anyway, I agree to make a separate task for this, It's easier to review this way :)
Thanks!
Anyway, see all my comments below:
diff --git a/sql/structs.h b/sql/structs.h index 99561c5..b38685c 100644 --- a/sql/structs.h +++ b/sql/structs.h <skip> + DDL_options_st operator|=(DDL_options_st::Options other) + { + add(other); + return *this; + } +};
I don't particularly like this style of C++ing... It's way too verbose. but ok
This is to make the code on sql_yacc.yy shorter:
lex->set_command_with_check(SQLCOM_CREATE_TABLE, $2, $1 | $4)
I find "$1 | $4" very clearly readable, and short.
Yes, I didn't mean operator| in particular, but the whole class with accessors for every single bit, etc. C++ is very verbose language and the more code you write, more code someone will have to read. So I generally like it when refactoring makes the codebase smaller, not larger.
Let me try to advertise a different approach :) I like more encapsulation. I'd encapsulate every single member in every single class, really. - It allows stricter control on what the callers can do - It makes the code shorter for the callers - It's easier to modify the code in the future: you can move the members across structs/classes, change inheritance, make methods virtual, move methods between private, protected and public sections. The caller side code still stays exactly the same! In case of direct member access, any of the above operation will most likely need changes on the caller side. In many cases, sooner or later you have to wrap members into methods anyway. - It's easier to set breakpoints having methods. - For exotic cases, polymorphic classes can share code using templates. For example, Field and Item have some duplicate code. Had they a set of similar properties wrapped into methods, they could share some code using templates. But they are different: Field->decimals() vs Item->dec, etc. The price is a few lines of code in the struct/class declaration, which make the code grow a little bit in the beginning. But in the long terms this can even reduce the amount of code.
diff --git a/sql/sql_show.cc b/sql/sql_show.cc index 0541cbc..2536255 100644 --- a/sql/sql_show.cc +++ b/sql/sql_show.cc @@ -1696,14 +1695,14 @@ int show_create_table(THD *thd, TABLE_LIST *table_list, String *packet,
packet->append(STRING_WITH_LEN("CREATE ")); if (create_info_arg && - (create_info_arg->org_options & HA_LEX_CREATE_REPLACE || + ((create_info_arg->or_replace() && + !create_info_arg->or_replace_slave_generated()) ||
that's a bit strange. Could you explain this? why org_options & HA_LEX_CREATE_REPLACE is translated to or_replace() && !or_replace_slave_generated()
Notice, I removed "org_options".
Now everything is passed through a single set of flags: the DDL_options_st part of *create_info_arg.
And the related flag is set in sql_parse.cc, here:
create_info.add(DDL_options_st::OPT_OR_REPLACE_SLAVE_GENERATED);
I find this logic to be much easier to understand.
Yes, of course, I've noticed that. Still, old condition was
org_options & HA_LEX_CREATE_REPLACE
and not
org_options & HA_LEX_CREATE_REPLACE && !(options & HA_LEX_CREATE_REPLACE)
So, why your new condition is
or_replace() && !or_replace_slave_generated()
On slave "CREATE TABLE" is translated to "CREATE TABLE IF NOT EXISTS". But the slave itself can do binary logging again. It writes "IF NOT EXISTS" only if it was in the original query. mysql_create_like_table() calls show_create_table() for binlogging purposes. mysql_create_like_table() needs or_replace() not to fail on error. show_create_table() needs to write "OR REPLACE" into the binlog only if "OR REPLACE" was in the original query. In the old code the caller made sure to set create_info.org_options and create_info.options properly, to make this logic work. Now there is no org_options any more, so everything is done in the single set of options (the DDL_options_st part of create_info). is_replace() makes mysql_create_line_table() not to fail. is_replace_slave_generated() prevents adding of "IF NOT EXISTS" in show_create_table() in the cases when is_replace() was set on the slave side, not in the original query.
?
diff --git a/sql/sql_table.cc b/sql/sql_table.cc index b991215..7bb293b 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -4628,7 +4628,8 @@ int create_table_impl(THD *thd, const char *orig_db, const char *orig_table_name, const char *db, const char *table_name, const char *path, - HA_CREATE_INFO *create_info, + const DDL_options_st options, + HA_CREATE_INFO &create_info,
why? I generally prefer pointers over C++ references - one can immediately see what variables can be modified by the function. is there a compelling reason to use references here?
Well, I had a hope I'd manage to do it a const:
const HA_CREATE_INFO &create_info
And in case of "const", I really prefer "const XXX &name", instead of "const XXX *name". It gives a lot of advantages.
Okay, with 'const' I could agree, right.
diff --git a/storage/xtradb/handler/ha_innodb.cc b/storage/xtradb/handler/ha_innodb.cc index f3b1167..6b1942e 100644 --- a/storage/xtradb/handler/ha_innodb.cc +++ b/storage/xtradb/handler/ha_innodb.cc @@ -11349,7 +11349,7 @@ static __attribute__((nonnull, warn_unused_result))
/* Do not use DATA DIRECTORY with TEMPORARY TABLE. */ if (create_info->data_file_name - && create_info->options & HA_LEX_CREATE_TMP_TABLE) { + && create_info->tmp_table()) {
nope, you should not change storage engine API here. tmp_table() was a convenience wrapper, old way should still work
I haven't changed the API at this point, so the old way still works. But I don't want any newly created engines to copy the old way.
Then only change engines that we develop (MyISAM, Aria, etc) and engines where MariaDB repositories are kind of the authoritative source of the engine source code (Connect, OQGraph). But don't change engines that are developed elsewhere and then merged into MariaDB - those engines we should try to keep as close to the upstream as possible and only change when absolutely necessary.
There is a chance that we'll move the table scope part into a separate structure, and Table_scope_and_contents_source_st will then inherit it. Now table scope is only responsible for TEMPORARY or not TEMPORARY, but in the SQL standard it's something more complex:
So if we ever decide to support more table scope options, the HA_LEX_CREATE_TMP_TABLE flag most likely won't be stored in create_info->options any more.
Yes, and then it will be absolutely necessary to change all engines, because they'll stop working otherwise.
I see, this is for easier merge purposes. Let's keep InnoDB/XTraDB use direct member access.
diff --git a/sql/handler.h b/sql/handler.h index 0044556..002a22e 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -1571,9 +1569,41 @@ enum enum_stats_auto_recalc { HA_STATS_AUTO_RECALC_DEFAULT= 0, HA_STATS_AUTO_RECALC_ON, HA_STATS_AUTO_RECALC_OFF };
-struct HA_CREATE_INFO +/** + A helper struct for schema DDL statements: + CREATE SCHEMA [IF NOT EXISTS] name [ schema_specification... ] + ALTER SCHEMA name [ schema_specification... ] + + It stores the "schema_specification" part of the CREATE/ALTER statements and + is passed to mysql_create_db() and mysql_alter_db(). + Currently consists only of the schema default character set and collation.
by the way, have you seen http://www.tocker.ca/2014/12/02/proposal-to-deprecate-collation_database-and... ?
Yes, Sanja sent me this link yesterday. I think this is going to be a good change. From what I remember, changing character_set_database manually was originally (in 4.1?) the recommended way to do LOAD DATA INFILE and SELECT INTO OUTFILE using an alternative character set. Later, these statements were extended to have their own CHARACTER SET clauses for this purposes. So it should be fine to make character_set_database read-only.
Regards, Sergei
Hi, Alexander! On Dec 03, Alexander Barkov wrote:
Let me try to advertise a different approach :) I like more encapsulation. I'd encapsulate every single member in every single class, really.
I know :) I've heard these arguments many times too, and they aren't wrong. It's about balance, perhaps.
Yes, and then it will be absolutely necessary to change all engines, because they'll stop working otherwise.
I see, this is for easier merge purposes. Let's keep InnoDB/XTraDB use direct member access.
InnoDB, XtraDB, TokuDB, sphinx, spider, mroonga. And perfschema, perhaps. All other engines could be changed at once to use the "proper" approach. Regards, Sergei
participants (2)
-
Alexander Barkov -
Sergei Golubchik