Hi Sergei, Sorry for duplicate message - I forgot to cc the list... Thanks for the comments, please see below my reply and the JIRA ticket for the updated commits. On Fri 2023-09-08 20:48:43 +0200, Sergei Golubchik wrote:
Hi, Yuchen,
[... 19 lines elided]
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?
Done. What is the difference btw?
[... 6 lines elided]
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() ?
I don't think so. AFAICT cleanup_variables() is meant to clean up actual plugin variables, and apart from plugin_thdvar_init() is also called in plugin_thdvar_init() and plugin_shutdown(), but default_master_connection is not a plugin variable. Ideally it should not be in plugin_thdvar_init() either, but instead there should be a say session_cleanup() method in the sys_var interface called at ~THD destructor. In the absense of this method plugin_thdvar_init() seems to be the best place, which is also where session_track_system_variables is cleaned up.
[... 10 lines elided]
-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.
Done. Also added a test. How about Sys_var_charptr?
#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 :)
Well I do need to call my_memdup() to allocate space for the string. And it would be good to reuse existing routines that does the same thing, which is why I reuse existing code from in update_prepare() (formerly global_update_prepare()).
[... 39 lines elided]
-class Sys_var_charptr: public Sys_var_charptr_base -{
[... 12 lines elided]
+ 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.
Done.
[... 18 lines elided]
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
Done. Removed these tests.
[... 8 lines elided]
Best, Yuchen