Hi, Yuchen, On Sep 12, Yuchen Pei 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?
It's performance schema instrumentation. Memory allocations tagged with key_memory_Sys_var_charptr_value will be visible in various performance schema memory-related tables (e.g. in memory_summary_global_by_event_name) with the event name being "Sys_var_charptr::value" (see mysqld.cc). PSI_INSTRUMENT_ME means the memory allocation event is not instrumented and won't be visible in performance schema. It appears to me that allocations related to Sys_var_charptr are filed under key_memory_Sys_var_charptr_value event.
-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?
Yes, good idea. Better put the limit in Sys_var_charptr and Sys_var_lexstring inherits from it, so it should get it automatically, right? Also, a couple of things I didn't really like, but which aren't worth fixing now. Could you please add comments like: * before class Sys_var_lexstring, something like "Note that my_getopt only sets the pointer to the value, the length must be updated manually to match. It's done in mysqld.cc see opt_init_connect. TODO: it should be done automatically (some kind of a loop over all variables?) when we'll have more Sys_var_lexstring variables to justify it." * before class Sys_var_charptr, something like "Note that memory management for SESSION_VAR's is manual, the value must be strdup'ed in THD::init() and freed in THD::cleanup(), see e.g. redirect_url. TODO: it should be done automatically (some kind of a loop over all variables?) when we'll have more session string variables to justify it." I'm not sure I got THD methods right, but you know what they are.
#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?
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()).
I mean, instead of char *session_update_prepare(set_var *var) { 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); you can write directly bool session_update(THD *thd, set_var *var) override { char *new_val= update_prepare(var, MYF(MY_WME | MY_THREAD_SPECIFIC)); it's not like session_update_prepare() is used in many places and you want to make sure that flags are the same everywhere.
no, session's default value is global's value. ... You cannot have a test for that now, but let's have one in the redirect_url commit.
And later I found that you already had a test for that in the redirect_url commit. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org