maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Jan!

On Mar 08, Jan Lindström wrote:
> revision-id: a1c4620646d43eaff979c132a57ee2101070fa5e
> parent(s): 1b74f32b1d94465f6d6e14e4aa7ba1f94c32e39e
> committer: Jan Lindström
> branch nick: 10.1-innodb
> timestamp: 2015-03-08 17:23:05 +0200
> message:
> 
> MDEV-6858: enforce_storage_engine option
> 
> Merge from Percona Server enforced use of a specific storage engine
> authored by Stewart Smith.
> 
> Modified to be session variable and modifiable only by SUPER. Use
> similar implementation as default_storage_engine.
> 
> diff --git a/mysql-test/t/enforce_storage_engine.test b/mysql-test/t/enforce_storage_engine.test
> new file mode 100644
> index 0000000..36231e6
> --- /dev/null
> +++ b/mysql-test/t/enforce_storage_engine.test
> @@ -0,0 +1,35 @@
...
> +--error ER_UNKNOWN_STORAGE_ENGINE
> +CREATE TABLE t2 (c1 INT PRIMARY KEY AUTO_INCREMENT, c2 VARCHAR(10)) ENGINE=MyISAM;
> +
> +SET SESSION enforce_storage_engine=MyISAM;
> +select @@session.enforce_storage_engine;
> +select * from t1;
> +drop table t1;
> +
> +--error 1286
> +SET SESSION enforce_storage_engine=FooBar;
> +
> +select @@session.enforce_storage_engine;

Consider adding tests for "only modifiable by SUPER"
(create a new user, connect with it, see that enforce_storage_engine
cannot be changed), and for modified enforce_storage_engine
(you tested that it can be changed, but didn't do any CREATE TABLE with
the new value).

> diff --git a/sql/handler.cc b/sql/handler.cc
> index 12d7ffb..5d5ac30 100644
> --- a/sql/handler.cc
> +++ b/sql/handler.cc
> @@ -117,6 +117,15 @@ static plugin_ref ha_default_tmp_plugin(THD *thd)
>    return ha_default_plugin(thd);
>  }
>  
> +
> +static plugin_ref ha_enforced_plugin(THD *thd)
> +{
> +  if (thd->variables.enforced_table_plugin)
> +    return thd->variables.enforced_table_plugin;
> +  return NULL;
> +}
> +
> +
>  /** @brief
>    Return the default storage engine handlerton for thread
>  
> @@ -148,6 +157,25 @@ handlerton *ha_default_tmp_handlerton(THD *thd)
>  
>  
>  /** @brief
> +  Return the enforced storage engine handlerton for thread
> +
> +  SYNOPSIS
> +    ha_enforce_handlerton(thd)
> +    thd         current thread
> +
> +  RETURN
> +    pointer to handlerton OR NULL
> +
> +*/
> +handlerton *ha_enforced_handlerton(THD *thd)
> +{
> + plugin_ref plugin= ha_enforced_plugin(thd);
> + if (plugin)
> +   return plugin_hton(plugin);
> + return NULL;
> +}
> +
> +/** @brief
>    Return the storage engine handlerton for the supplied name
>    
>    SYNOPSIS
> diff --git a/sql/mysqld.cc b/sql/mysqld.cc
> index b97742d..6966fd9 100644
> --- a/sql/mysqld.cc
> +++ b/sql/mysqld.cc
> @@ -4738,8 +4738,8 @@ static void add_file_to_crash_report(char *file)
>  #define init_default_storage_engine(X,Y) \
>    init_default_storage_engine_impl(#X, X, &global_system_variables.Y)
>  
> -static int init_default_storage_engine_impl(const char *opt_name,
> -                                            char *engine_name, plugin_ref *res)
> +int init_default_storage_engine_impl(const char *opt_name,
> +                                     char *engine_name, plugin_ref *res)

You've made init_default_storage_engine_impl() extern, but you never
use it anywhere.

And you've forgot to create a command line option for
enforced_table_plugin.

>  {
>    if (!engine_name)
>    {
> diff --git a/sql/sql_table.cc b/sql/sql_table.cc
> index e9a1ae9..184c073 100644
> --- a/sql/sql_table.cc
> +++ b/sql/sql_table.cc
> @@ -9745,12 +9745,24 @@ static bool check_engine(THD *thd, const char *db_name,
>    DBUG_ENTER("check_engine");
>    handlerton **new_engine= &create_info->db_type;
>    handlerton *req_engine= *new_engine;
> +  handlerton *enf_engine= ha_enforced_handlerton(thd);

I'd write it directly as

  thd->variables.enforced_table_plugin ?  plugin_hton(thd->variables.enforced_table_plugin) : NULL;

Or as an inlined function here, in sql_table.cc.
Not as two small but non-inlined functions in handler.cc, that are only
used once here.

>    bool no_substitution= thd->variables.sql_mode & MODE_NO_ENGINE_SUBSTITUTION;
>    *new_engine= ha_checktype(thd, req_engine, no_substitution);
>    DBUG_ASSERT(*new_engine);
>    if (!*new_engine)
>      DBUG_RETURN(true);
>  
> +  if (enf_engine)
> +  {
> +    if (enf_engine != *new_engine && no_substitution)
> +    {
> +      const char *engine_name= ha_resolve_storage_engine_name(req_engine);
> +      my_error(ER_UNKNOWN_STORAGE_ENGINE, MYF(0), engine_name, engine_name);
> +      DBUG_RETURN(TRUE);
> +    }
> +    *new_engine= enf_engine;
> +  }
> +
>    if (req_engine && req_engine != *new_engine)
>    {
>      push_warning_printf(thd, Sql_condition::WARN_LEVEL_NOTE,
> diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc
> index 8371df0..cd53714 100644
> --- a/sql/sys_vars.cc
> +++ b/sql/sys_vars.cc
> @@ -3437,6 +3437,28 @@ static Sys_var_plugin Sys_default_tmp_storage_engine(
>         SESSION_VAR(tmp_table_plugin), NO_CMD_LINE,
>         MYSQL_STORAGE_ENGINE_PLUGIN, DEFAULT(&default_tmp_storage_engine));
>  
> +extern int init_default_storage_engine_impl(const char* opt_name, char *engine_name, plugin_ref *plugin);

Not used below.

> +
> +
> +static bool check_super_and_not_null(sys_var *self, THD*thd, set_var *var)

I think that NULL value should be allowed, it means "don't enforce a
storage engine".

> +{
> +#ifndef NO_EMBEDDED_ACCESS_CHECKS
> +  if (!(thd->security_ctx->master_access & SUPER_ACL))
> +  {
> +    my_error(ER_SPECIFIC_ACCESS_DENIED_ERROR, MYF(0), "SUPER");
> +    return true;
> +  }
> +#endif
> +  return var->value && var->value->is_null();
> +}
> +
> +static Sys_var_plugin Sys_enforce_storage_engine(
> +       "enforce_storage_engine", "Force the use of a storage engine for new "
> +       "tables",
> +       SESSION_VAR(enforced_table_plugin),
> +       NO_CMD_LINE, MYSQL_STORAGE_ENGINE_PLUGIN,
> +       DEFAULT(0), NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(check_super_and_not_null));
> +
>  #if defined(ENABLED_DEBUG_SYNC)
>  /*
>    Variable can be set for the session only.
Regards,
Sergei