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