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