Hi, Yuchen, On Sep 08, Yuchen Pei wrote:
revision-id: 365faf7229f (mariadb-11.0.1-103-g365faf7229f) parent(s): 4f39ae75606 author: Yuchen Pei committer: Yuchen Pei timestamp: 2023-09-08 13:12:22 +1000 message:
MDEV-15935 Clean up string sys_var types.
Merge sys_var_charptr with sys_var_charptr_base, as well as merge Sys_var_session_lexstring into Sys_var_lexstring. Also refactored update methods of sys_var_charptr accordingly.
Because the class is more generic, session_update() calls sys_var_charptr::session_update() which does not assume a buffer field associated with THD, but instead call strdup/free, we get rid of THD::default_master_connection_buff accordingly
diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 18e796655bd..52fa6c27112 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -1233,10 +1233,12 @@ void THD::init() avoid temporary tables replication failure. */ variables.pseudo_thread_id= thread_id; - variables.default_master_connection.str= default_master_connection_buff; - ::strmake(default_master_connection_buff, - global_system_variables.default_master_connection.str, - variables.default_master_connection.length); + variables.default_master_connection.str= + my_strndup(PSI_INSTRUMENT_ME,
may be key_memory_Sys_var_charptr_value instead of PSI_INSTRUMENT_ME?
+ global_system_variables.default_master_connection.str, + global_system_variables.default_master_connection.length, + MYF(MY_WME | MY_THREAD_SPECIFIC)); + mysql_mutex_unlock(&LOCK_global_system_variables);
user_time.val= start_time= start_time_sec_part= 0; diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc index d39589ed1ab..4724e2200e6 100644 --- a/sql/sql_plugin.cc +++ b/sql/sql_plugin.cc @@ -3279,6 +3279,9 @@ void plugin_thdvar_init(THD *thd) #ifndef EMBEDDED_LIBRARY thd->session_tracker.sysvars.deinit(thd); #endif + my_free((char*) thd->variables.default_master_connection.str); + thd->variables.default_master_connection.str= 0; + thd->variables.default_master_connection.length= 0;
shouldn't this go inside cleanup_variables() ?
thd->variables= global_system_variables;
diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc index 4843450352e..064c60ab2b0 100644 --- a/sql/sys_vars.cc +++ b/sql/sys_vars.cc @@ -1355,12 +1355,11 @@ static bool check_master_connection(sys_var *self, THD *thd, set_var *var) return false; }
-static Sys_var_session_lexstring Sys_default_master_connection( +static Sys_var_lexstring Sys_default_master_connection( "default_master_connection", "Master connection to use for all slave variables and slave commands", - SESSION_ONLY(default_master_connection), - NO_CMD_LINE, - DEFAULT(""), MAX_CONNECTION_NAME, ON_CHECK(check_master_connection)); + SESSION_ONLY(default_master_connection), NO_CMD_LINE, DEFAULT(""), + NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(check_master_connection));
some length limit would still be nice, I'd say. Doesn't have to be per variable, may be all Sys_var_lexstring could have some common limit, like, arbitrarily, 1K or 10K.
#endif
static Sys_var_charptr_fscs Sys_init_file( diff --git a/sql/sys_vars.inl b/sql/sys_vars.inl index 31ebbae01ca..0f2a07d92fc 100644 --- a/sql/sys_vars.inl +++ b/sql/sys_vars.inl @@ -576,6 +571,21 @@ class Sys_var_charptr_base: public sys_var new_val= 0; return new_val; } + char *session_update_prepare(set_var *var)
do you need this session_update_prepare method? global_update_prepare() and global_update_finish() are used by Sys_var_sesvartrack. Getting rid of them is a task for another day :)
+ { + return update_prepare(var, MYF(MY_WME | MY_THREAD_SPECIFIC)); + } + bool session_update(THD *thd, set_var *var) override + { + char *new_val= session_update_prepare(var); + my_free(session_var(thd, char*)); + session_var(thd, char*)= new_val; + return (new_val == 0 && var->save_result.string_value.str != 0); + } + char *global_update_prepare(set_var *var) + { + return update_prepare(var, MYF(MY_WME)); + } void global_update_finish(char *new_val) { if (flags & ALLOCATED) @@ -583,50 +593,28 @@ class Sys_var_charptr_base: public sys_var flags|= ALLOCATED; global_var(char*)= new_val; } - bool global_update(THD *thd, set_var *var) + bool global_update(THD *thd, set_var *var) override { - char *new_val= global_update_prepare(thd, var); + char *new_val= global_update_prepare(var); global_update_finish(new_val); return (new_val == 0 && var->save_result.string_value.str != 0); } - void session_save_default(THD *thd, set_var *var)= 0; - void global_save_default(THD *thd, set_var *var) + void save_default(set_var *var) { char *ptr= (char*)(intptr)option.def_value; var->save_result.string_value.str= ptr; var->save_result.string_value.length= ptr ? strlen(ptr) : 0; } -}; - -class Sys_var_charptr: public Sys_var_charptr_base -{ -public: - Sys_var_charptr(const char *name_arg, - const char *comment, int flag_args, ptrdiff_t off, size_t size, - CMD_LINE getopt, - const char *def_val, PolyLock *lock=0, - enum binlog_status_enum binlog_status_arg=VARIABLE_NOT_IN_BINLOG, - on_check_function on_check_func=0, - on_update_function on_update_func=0, - const char *substitute=0) : - Sys_var_charptr_base(name_arg, comment, flag_args, off, size, getopt, - def_val, lock, binlog_status_arg, - on_check_func, on_update_func, substitute) + void session_save_default(THD *, set_var *var) override { - SYSVAR_ASSERT(scope() == GLOBAL); - SYSVAR_ASSERT(size == sizeof(char *)); + return save_default(var);
no, session's default value is global's value. See any other Sys_var_xxx class, e.g. for Sys_var_mybool: void session_save_default(THD *thd, set_var *var) { var->save_result.ulonglong_value= (ulonglong)*(my_bool *)global_value_ptr(thd, 0); } You cannot have a test for that now, but let's have one in the redirect_url commit.
} - - bool session_update(THD *thd, set_var *var) + void global_save_default(THD *, set_var *var) override { - DBUG_ASSERT(FALSE); - return true; + return save_default(var); } - void session_save_default(THD *thd, set_var *var) - { DBUG_ASSERT(FALSE); } };
- class Sys_var_charptr_fscs: public Sys_var_charptr { using Sys_var_charptr::Sys_var_charptr; diff --git a/mysql-test/suite/sys_vars/r/mdev_15935.result b/mysql-test/suite/sys_vars/r/mdev_15935.result new file mode 100644 index 00000000000..d20c2ad2f19 --- /dev/null +++ b/mysql-test/suite/sys_vars/r/mdev_15935.result @@ -0,0 +1,12 @@ +# +# test that the global/session-only enforcement is still in place despite the merge of string sys_var classes +# +set session init_connect="whatever"; +ERROR HY000: Variable 'init_connect' is a GLOBAL variable and should be set with SET GLOBAL +set session wsrep_auto_increment_control="whatever"; +ERROR HY000: Variable 'wsrep_auto_increment_control' is a GLOBAL variable and should be set with SET GLOBAL +set global default_master_connection="whatever"; +ERROR HY000: Variable 'default_master_connection' is a SESSION variable and can't be used with SET GLOBAL
not sure you need that, it's checked in the very base class, you could've have affected that I'd say it's enough if existing tests for Sys_var_session_lexstring and Sys_var_sesvartrack variables succeed and ASAN/MSAN are happy
+# +# end of test mdev_15935 +#
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org