[Commits] MDEV-19653 Add class Sql_cmd_create_table

Sergey Vojtovich svoj at mariadb.org
Fri May 31 10:46:36 EEST 2019


Hi Alexander,

> 
> commit d070c68861670c0a556e3f29cb0b3d5fdd87a8c8
> Author: Alexander Barkov <bar at 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


More information about the commits mailing list