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
_______________________________________________
developers mailing list -- developers@lists.mariadb.org
To unsubscribe send an email to developers-le...@lists.mariadb.org
