Hi Sergei,
Thanks for the comments. Please see below my replies and the jira
ticket for the updated commits.
On Tue 2023-09-12 12:06:40 +0200, Sergei Golubchik wrote:
> [... 22 lines elided]
>> > 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.
I see, thanks for explaining.
> [... 10 lines elided]
>> > 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?
Done. Also added a test on a sys_var_charptr var size.
> 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."
Done, with some minor editing. I could not find my_getopt and mention
handle_options() instead, and I use sys_var_end as an example of looping
over all variables.
> * 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."
Done.
> [... 15 lines elided]
>> > 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.
Ah I see, done. Got rid of global_update_prepare() too.
>> > 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.
Yup.
> [... 5 lines elided]
Best,
Yuchen
_______________________________________________
developers mailing list -- developers@lists.mariadb.org
To unsubscribe send an email to developers-le...@lists.mariadb.org
