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