Re: 14a12f98eb8: MDEV-15935 Clean up string sys_var types.
Hi, Yuchen, ok to push, after making one change (const) below. On Sep 13, Yuchen Pei wrote:
revision-id: 14a12f98eb8 (mariadb-11.0.1-232-g14a12f98eb8) parent(s): e987b9350cb author: Yuchen Pei committer: Yuchen Pei timestamp: 2023-09-13 11:50:41 +1000 message:
MDEV-15935 Clean up string sys_var types.
Merge sys_var_charptr with sys_var_charptr_base, as well as merge Sys_var_session_lexstring into Sys_var_lexstring. Also refactored update methods of sys_var_charptr accordingly.
Because the class is more generic, session_update() calls sys_var_charptr::session_update() which does not assume a buffer field associated with THD, but instead call strdup/free, we get rid of THD::default_master_connection_buff accordingly
You could've mentioned here "this also makes THD smaller by 192 bytes, and there can be many thousands of concurrent THDs" I mean, it's a good result, one of the benefits of your cleanup.
diff --git a/sql/sys_vars.inl b/sql/sys_vars.inl index 31ebbae01ca..5bac91a0db3 100644 --- a/sql/sys_vars.inl +++ b/sql/sys_vars.inl @@ -492,14 +492,18 @@ class Sys_var_mybool: public Sys_var_typelib Backing store: char*
@note - This class supports only GLOBAL variables, because THD on destruction - does not destroy individual members of SV, there's no way to free - allocated string variables for every thread. + + Note that the memory management for SESSION_VAR's is manual, the + value must be strdup'ed in THD::init() and freed in + plugin_thdvar_cleanup(). TODO: it should be done automatically when + we'll have more session string variables to justify it. Maybe some + kind of a loop over all variables, like sys_var_end() in set_var.cc? */ -class Sys_var_charptr_base: public sys_var +class Sys_var_charptr: public sys_var { + size_t max_length= 2000;
Should be const, right? No need to store max_length in every Sys_var_charptr instance, if you aren't going to modify it per instance.
public: - Sys_var_charptr_base(const char *name_arg, + Sys_var_charptr(const char *name_arg, const char *comment, int flag_args, ptrdiff_t off, size_t size, CMD_LINE getopt, const char *def_val, PolyLock *lock=0,
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei, Thanks for the review. Pushed to preview-11.3-preview, see jira for the updated commits. On Wed 2023-09-13 22:53:37 +0200, Sergei Golubchik wrote:
[... 12 lines elided]
MDEV-15935 Clean up string sys_var types.
Merge sys_var_charptr with sys_var_charptr_base, as well as merge Sys_var_session_lexstring into Sys_var_lexstring. Also refactored update methods of sys_var_charptr accordingly.
Because the class is more generic, session_update() calls sys_var_charptr::session_update() which does not assume a buffer field associated with THD, but instead call strdup/free, we get rid of THD::default_master_connection_buff accordingly
You could've mentioned here "this also makes THD smaller by 192 bytes, and there can be many thousands of concurrent THDs"
Done.
I mean, it's a good result, one of the benefits of your cleanup.
Given the cleanup has some benefits, what do you say we also push it to the earliest applicable version? It will also make it less likely for people to create new string sys_var classes or THD fields to hold string buffers, thus less likely to cause conflicts when merging to 11.3. I can create a new ticket for this backport task.
[... 7 lines elided]
@note - This class supports only GLOBAL variables, because THD on destruction - does not destroy individual members of SV, there's no way to free - allocated string variables for every thread. + + Note that the memory management for SESSION_VAR's is manual, the + value must be strdup'ed in THD::init() and freed in + plugin_thdvar_cleanup(). TODO: it should be done automatically when + we'll have more session string variables to justify it. Maybe some + kind of a loop over all variables, like sys_var_end() in set_var.cc? */ -class Sys_var_charptr_base: public sys_var +class Sys_var_charptr: public sys_var { + size_t max_length= 2000;
Should be const, right? No need to store max_length in every Sys_var_charptr instance, if you aren't going to modify it per instance.
Done.
[... 11 lines elided]
Best, Yuchen
participants (2)
-
Sergei Golubchik -
Yuchen Pei