Re: 8ac17273b14: MDEV-15935 Adding global/session system var redirect_url
Hi, Yuchen, Right. This is pretty much the whole feature. Mostly ok, few minor comments below. On Sep 08, Yuchen Pei wrote:
revision-id: 8ac17273b14 (mariadb-11.0.1-104-g8ac17273b14) parent(s): 365faf7229f author: Yuchen Pei committer: Yuchen Pei timestamp: 2023-09-08 14:46:50 +1000 message:
MDEV-15935 Adding global/session system var redirect_url
Adding a global/session var `redirect_url' of string type. The initial value is empty. Can be supplied in mysqld with --redirect-url or set in --init-connect. A valid redirect_url should be of the format
{mysql,mariadb}://host[:port]
where <host> is an arbitrary string not containing colons, and <port> is a number between 0 and 65535 inclusive.
The variable will be used by the server to notify clients that they should connect to another server, specified by the value of the variable, if not empty.
The notification is done by the inclusion of the variable in session_track_system_variable.
diff --git a/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result b/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result index fa3bf6b4354..85dc924fe5f 100644 --- a/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result +++ b/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result @@ -3232,6 +3232,16 @@ NUMERIC_BLOCK_SIZE 1 ENUM_VALUE_LIST NULL READ_ONLY NO COMMAND_LINE_ARGUMENT REQUIRED +VARIABLE_NAME REDIRECT_URL +VARIABLE_SCOPE SESSION +VARIABLE_TYPE VARCHAR +VARIABLE_COMMENT URL to redirect to, if not empty
see if you can make it a bit more descriptive. if not, ok as is. hmm, and a second thought. it's in the sysvars_server_embedded test. it doesn't make sense there (embedded doesn't accept connections, and doesn't have session tracking, iirc), so, may be hide it under #ifndef EMBEDDED_LIBRARY ?
+NUMERIC_MIN_VALUE NULL +NUMERIC_MAX_VALUE NULL +NUMERIC_BLOCK_SIZE NULL +ENUM_VALUE_LIST NULL +READ_ONLY NO +COMMAND_LINE_ARGUMENT REQUIRED VARIABLE_NAME REQUIRE_SECURE_TRANSPORT VARIABLE_SCOPE GLOBAL VARIABLE_TYPE BOOLEAN diff --git a/sql/sql_class.h b/sql/sql_class.h index cf2b5b96a22..c86a76f6a08 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -878,6 +878,7 @@ typedef struct system_variables
Time_zone *time_zone; char *session_track_system_variables; + char *redirect_url;
/* Some wsrep variables */ ulonglong wsrep_trx_fragment_size; diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc index 4724e2200e6..70aba9cc4d5 100644 --- a/sql/sql_plugin.cc +++ b/sql/sql_plugin.cc @@ -3307,6 +3309,10 @@ void plugin_thdvar_init(THD *thd) #ifndef EMBEDDED_LIBRARY thd->session_tracker.sysvars.init(thd); #endif + thd->variables.redirect_url= + my_strdup(PSI_INSTRUMENT_ME, global_system_variables.redirect_url,
and here, key_memory_Sys_var_charptr_value
+ MYF(MY_WME | MY_THREAD_SPECIFIC)); + DBUG_VOID_RETURN; }
@@ -3375,6 +3381,8 @@ void plugin_thdvar_cleanup(THD *thd) #ifndef EMBEDDED_LIBRARY thd->session_tracker.sysvars.deinit(thd); #endif + my_free(thd->variables.redirect_url); + thd->variables.redirect_url= 0;
also, perhaps in cleanup_variables() ?
my_free((char*) thd->variables.default_master_connection.str); thd->variables.default_master_connection.str= 0; thd->variables.default_master_connection.length= 0; diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc index 064c60ab2b0..a93e68f40d7 100644 --- a/sql/sys_vars.cc +++ b/sql/sys_vars.cc @@ -1343,6 +1343,61 @@ Sys_init_connect( DEFAULT(""), &PLock_sys_init_connect, NOT_IN_BINLOG, ON_CHECK(check_init_string));
+/** + Validate a redirect_url string. + + A valid string is either empty, or of the format mysql://host[:port], + where host is an arbitrary string without any colon ':'. + + @param str A string to validate + @param len Length of the string + @retval false The string is valid + @retval true The string is invalid +*/ +static bool sysvar_validate_redirect_url(sys_var *, THD *, set_var *var) +{ + char *str= var->save_result.string_value.str; + size_t len= var->save_result.string_value.length; + /* Empty string is valid */ + if (len == 0) + return false; + const char* end= str + len; + if (!strncmp(str, "mysql://", strlen("mysql://")))
use !strncmp(str, STRING_WITH_LEN("mysql://")) avoids no strlen and no need to write the string twice. same below.
+ str+= strlen("mysql://");
ah. Then, I'd suggest instead LEX_CSTRING mysql_prefix={STRING_WITH_LEN("mysql://")}; if (!strncmp(str, mysql_prefix.str, mysql_prefix.length)) str+= mysql_prefix.length;
+ else if (!strncmp(str, "mariadb://", strlen("mariadb://"))) + str+= strlen("mariadb://"); + else + return true; + /* Host name cannot be empty */ + if (str == end) + return true; + /* Find the colon, if any */ + while (str < end && *str != ':') + str++; + /* Found colon */ + if (str < end) + { + /* Should have at least one number after the colon */ + if (str + 1 == end) + return true; + int p= 0; + while (str < end && isdigit(*++str)) + p= p * 10 + (*str - '0'); + /* Should be all numbers after the colon */ + if (str < end) + return true; + /* The port number should be between 0 and 65535 */ + if (p > 65535) + return true;
check for overflow too. you can either put the p>65535 check inside the loop or at the end check that, like, (str - colon_ptr) < 7.
+ } + return false; +} + +static Sys_var_charptr Sys_redirect_url( + "redirect_url", "URL to redirect to, if not empty", + SESSION_VAR(redirect_url), CMD_LINE(REQUIRED_ARG), DEFAULT(""), + NO_MUTEX_GUARD, NOT_IN_BINLOG, sysvar_validate_redirect_url, 0, 0);
By convention, write ON_CHECK(sysvar_validate_redirect_url). And you don't need 0's at the end, so just NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(sysvar_validate_redirect_url)); Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
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
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 security@mariadb.org
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
Hi Sergei, Thanks for the comments. Please see below my replies, and the JIRA ticket for the updated commits. On Fri 2023-09-08 21:15:47 +0200, Sergei Golubchik wrote:
[... 31 lines elided]
diff --git a/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result b/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result index fa3bf6b4354..85dc924fe5f 100644 --- a/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result +++ b/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result @@ -3232,6 +3232,16 @@ NUMERIC_BLOCK_SIZE 1 ENUM_VALUE_LIST NULL READ_ONLY NO COMMAND_LINE_ARGUMENT REQUIRED +VARIABLE_NAME REDIRECT_URL +VARIABLE_SCOPE SESSION +VARIABLE_TYPE VARCHAR +VARIABLE_COMMENT URL to redirect to, if not empty
see if you can make it a bit more descriptive. if not, ok as is.
Done.
hmm, and a second thought. it's in the sysvars_server_embedded test. it doesn't make sense there (embedded doesn't accept connections, and doesn't have session tracking, iirc), so, may be hide it under #ifndef EMBEDDED_LIBRARY ?
Done. Moved it to before the session track group.
[... 19 lines elided]
/* Some wsrep variables */ ulonglong wsrep_trx_fragment_size; diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc index 4724e2200e6..70aba9cc4d5 100644 --- a/sql/sql_plugin.cc +++ b/sql/sql_plugin.cc @@ -3307,6 +3309,10 @@ void plugin_thdvar_init(THD *thd) #ifndef EMBEDDED_LIBRARY thd->session_tracker.sysvars.init(thd); #endif + thd->variables.redirect_url= + my_strdup(PSI_INSTRUMENT_ME, global_system_variables.redirect_url,
and here, key_memory_Sys_var_charptr_value
Done.
[... 5 lines elided]
@@ -3375,6 +3381,8 @@ void plugin_thdvar_cleanup(THD *thd) #ifndef EMBEDDED_LIBRARY thd->session_tracker.sysvars.deinit(thd); #endif + my_free(thd->variables.redirect_url); + thd->variables.redirect_url= 0;
also, perhaps in cleanup_variables() ?
See my reply to the same comment in the review of the other commit.
[... 23 lines elided]
+static bool sysvar_validate_redirect_url(sys_var *, THD *, set_var *var) +{ + char *str= var->save_result.string_value.str; + size_t len= var->save_result.string_value.length; + /* Empty string is valid */ + if (len == 0) + return false; + const char* end= str + len; + if (!strncmp(str, "mysql://", strlen("mysql://")))
use !strncmp(str, STRING_WITH_LEN("mysql://")) avoids no strlen and no need to write the string twice. same below.
+ str+= strlen("mysql://");
ah. Then, I'd suggest instead
LEX_CSTRING mysql_prefix={STRING_WITH_LEN("mysql://")};
if (!strncmp(str, mysql_prefix.str, mysql_prefix.length)) str+= mysql_prefix.length;
Done.
+ else if (!strncmp(str, "mariadb://", strlen("mariadb://"))) + str+= strlen("mariadb://"); + else + return true; + /* Host name cannot be empty */ + if (str == end) + return true; + /* Find the colon, if any */ + while (str < end && *str != ':') + str++; + /* Found colon */ + if (str < end) + { + /* Should have at least one number after the colon */ + if (str + 1 == end) + return true; + int p= 0; + while (str < end && isdigit(*++str)) + p= p * 10 + (*str - '0'); + /* Should be all numbers after the colon */ + if (str < end) + return true; + /* The port number should be between 0 and 65535 */ + if (p > 65535) + return true;
check for overflow too. you can either put the p>65535 check inside the loop or at the end check that, like, (str - colon_ptr) < 7.
Done.
+ } + return false; +} + +static Sys_var_charptr Sys_redirect_url( + "redirect_url", "URL to redirect to, if not empty", + SESSION_VAR(redirect_url), CMD_LINE(REQUIRED_ARG), DEFAULT(""), + NO_MUTEX_GUARD, NOT_IN_BINLOG, sysvar_validate_redirect_url, 0, 0);
By convention, write ON_CHECK(sysvar_validate_redirect_url). And you don't need 0's at the end, so just
NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(sysvar_validate_redirect_url));
Done.
[... 5 lines elided]
Best, Yuchen
participants (2)
-
Sergei Golubchik -
Yuchen Pei