Re: 04b99a30a3f: MDEV-15935 Adding global/session var redirect_url
Hi, Yuchen, On Sep 01, Yuchen Pei wrote:
revision-id: 04b99a30a3f (mariadb-11.0.1-103-g04b99a30a3f) parent(s): 4f39ae75606 author: Yuchen Pei committer: Yuchen Pei timestamp: 2023-08-16 11:31:16 +1000 message:
MDEV-15935 Adding global/session 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. 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.
diff --git a/mysql-test/main/redirect.opt b/mysql-test/main/redirect.opt
move this to the sys_vars suite, please
--- /dev/null +++ b/mysql-test/main/redirect.opt @@ -0,0 +1 @@ +--redirect_url=mysql://foobar
ok, as long as this isn't a complete feature, but just one piece of it. but when client support will be implemented (CONC-599) and the server will actually send this to the connecting clients (MDEV-31609), this test will need to be fixed, as foobar is not something a client can be redirected to. Also, it'd be more interesting to try something like --init-connect='set @@redirect_url="mysql://foobar"' instead of the simple --redirect_url=mysql://foobar because init-connect is much more flexible, it can generate the url on the fly, it can send different users to different locations. So it'd make sense to test init-connect approach.
diff --git a/mysql-test/main/redirect.test b/mysql-test/main/redirect.test new file mode 100644 --- /dev/null +++ b/mysql-test/main/redirect.test @@ -0,0 +1,56 @@ +--echo # +--echo # MDEV-15935 Connection Redirection Mechanism in MariaDB Client/Server Protocol +--echo # + +set @old_redirect_url=@@redirect_url; +select @@global.redirect_url; +set global redirect_url=default; +select @@global.redirect_url; +--error ER_WRONG_VALUE_FOR_VAR +set global redirect_url="mariadb.org"; +--error ER_WRONG_VALUE_FOR_VAR +set global redirect_url="https://mariadb.org";
before doing all this fun, please remove redirect_url from the session_track_system_variables, or just set it to the empty string.
diff --git a/sql/sql_class.h b/sql/sql_class.h --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -3585,6 +3587,7 @@ class THD: public THD_count, /* this must be first */ */ LEX_CSTRING connection_name; char default_master_connection_buff[MAX_CONNECTION_NAME+1]; + char redirect_url_buff[MAX_CONNECTION_NAME+1];
This isn't such a good idea (I'm afraid, I missed it when default_master_connection_buff was added). A couple of years ago Monty have spent significant efforts trying to make THD smaller as it has grown to be too large. Now THD keeps growing again. Let's try not to encourage it, if we can help it.
uint8 password; /* 0, 1 or 2 */ uint8 failed_com_change_user; bool slave_thread; diff --git a/sql/sys_vars.inl b/sql/sys_vars.inl --- a/sql/sys_vars.inl +++ b/sql/sys_vars.inl @@ -637,6 +637,129 @@ class Sys_var_charptr_fscs: public Sys_var_charptr ... +class Sys_var_redirect_url: public Sys_var_charptr_base
please, no. new classes are needed only for variables with very special, unlike anything else, behavior. redirect_url is just a session string variable. See the comment at the top of sys_vars.cc But (!) there are no other session string variables yet. So I'd recomment to fix Sys_var_charptr to support session variables. And then create redirect_url out of it. Practically it might need no changes at all, if you put explicit strmake (or strdup/free) into THD::init like you did anyway. After that, I suspect, you might be able to remove Sys_var_charptr_base, Sys_var_sesvartrack, and Sys_var_session_lexstring, because Sys_var_charptr/Sys_var_lexstring will be able to serve all those use cases.
+{ + static const size_t max_length = MAX_CONNECTION_NAME; +public: + Sys_var_redirect_url(const char *name_arg, + const char *comment, + CMD_LINE getopt, + const char *def_val, PolyLock *lock= 0) : + Sys_var_charptr_base(name_arg, comment, + SESSION_VAR(redirect_url), + getopt, def_val, lock, + VARIABLE_NOT_IN_BINLOG, 0, 0, 0) + { + global_var(LEX_CSTRING).length= strlen(def_val); + option.var_type|= GET_STR; + *const_cast<SHOW_TYPE*>(&show_val_type)= SHOW_LEX_STRING; + }
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei, Thanks for the comments. Please find the updated commits in the latest comment in the ticket. On Fri 2023-09-01 19:22:02 +0200, Sergei Golubchik wrote:
Hi, Yuchen,
On Sep 01, Yuchen Pei wrote:
revision-id: 04b99a30a3f (mariadb-11.0.1-103-g04b99a30a3f) parent(s): 4f39ae75606 author: Yuchen Pei committer: Yuchen Pei timestamp: 2023-08-16 11:31:16 +1000 message:
MDEV-15935 Adding global/session 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. 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.
diff --git a/mysql-test/main/redirect.opt b/mysql-test/main/redirect.opt
move this to the sys_vars suite, please
Done.
--- /dev/null +++ b/mysql-test/main/redirect.opt @@ -0,0 +1 @@ +--redirect_url=mysql://foobar
ok, as long as this isn't a complete feature, but just one piece of it.
but when client support will be implemented (CONC-599) and the server will actually send this to the connecting clients (MDEV-31609), this test will need to be fixed, as foobar is not something a client can be redirected to.
Ack.
Also, it'd be more interesting to try something like
--init-connect='set @@redirect_url="mysql://foobar"'
instead of the simple --redirect_url=mysql://foobar
because init-connect is much more flexible, it can generate the url on the fly, it can send different users to different locations. So it'd make sense to test init-connect approach.
Done.
diff --git a/mysql-test/main/redirect.test b/mysql-test/main/redirect.test new file mode 100644 --- /dev/null +++ b/mysql-test/main/redirect.test @@ -0,0 +1,56 @@ +--echo # +--echo # MDEV-15935 Connection Redirection Mechanism in MariaDB Client/Server Protocol +--echo # + +set @old_redirect_url=@@redirect_url; +select @@global.redirect_url; +set global redirect_url=default; +select @@global.redirect_url; +--error ER_WRONG_VALUE_FOR_VAR +set global redirect_url="mariadb.org"; +--error ER_WRONG_VALUE_FOR_VAR +set global redirect_url="https://mariadb.org";
before doing all this fun, please remove redirect_url from the session_track_system_variables, or just set it to the empty string.
Done. Setting it to empty for simplicity, as removing requires considering 3 cases of whether redirect_url is the only, first, or non-first item in the var list. I had also added a couple of lines before the statement setting it to empty, to test that it prints changes in redirect_url, but it did not work with --ps-protocol, as it printed out extra SESSION_TRACK_SCHEMA changes, even with --disable_ps_protocol, and with `set session_track_system_variables="redirect_url";': https://buildbot.mariadb.org/#/builders/534/builds/9207
diff --git a/sql/sql_class.h b/sql/sql_class.h --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -3585,6 +3587,7 @@ class THD: public THD_count, /* this must be first */ */ LEX_CSTRING connection_name; char default_master_connection_buff[MAX_CONNECTION_NAME+1]; + char redirect_url_buff[MAX_CONNECTION_NAME+1];
This isn't such a good idea (I'm afraid, I missed it when default_master_connection_buff was added). A couple of years ago Monty have spent significant efforts trying to make THD smaller as it has grown to be too large. Now THD keeps growing again. Let's try not to encourage it, if we can help it.
Done. Without a THD field to store the string buffer, we'll have to resort to strdup/free, as THD::strmake() puts the space in the temporary runtime memroot which gets freed after a statement. In any case, I removed default_master_connection_buff too, as part of the cleanup of Sys_var_* classes requested below.
uint8 password; /* 0, 1 or 2 */ uint8 failed_com_change_user; bool slave_thread; diff --git a/sql/sys_vars.inl b/sql/sys_vars.inl --- a/sql/sys_vars.inl +++ b/sql/sys_vars.inl @@ -637,6 +637,129 @@ class Sys_var_charptr_fscs: public Sys_var_charptr ... +class Sys_var_redirect_url: public Sys_var_charptr_base
please, no. new classes are needed only for variables with very special, unlike anything else, behavior.
redirect_url is just a session string variable. See the comment at the top of sys_vars.cc
But (!) there are no other session string variables yet.
So I'd recomment to fix Sys_var_charptr to support session variables. And then create redirect_url out of it.
Done. Shall we add a new session_cleanup() method to sys_var that gets called in ~THD(), so that we don't have to do it manually in say plugin_thdvar_cleanup()?
Practically it might need no changes at all, if you put explicit strmake (or strdup/free) into THD::init like you did anyway.
After that, I suspect, you might be able to remove Sys_var_charptr_base, Sys_var_sesvartrack, and Sys_var_session_lexstring, because Sys_var_charptr/Sys_var_lexstring will be able to serve all those use cases.
Done except Sys_var_sesvartrack, which has its own custom update methods and is a bit more complex to remove. Because of all these merges that removed extra checks on global-only/session-only, I added a regression test mdev_15935 on enforcement of such checks. Now that there are two commits for this ticket, does it make sense to push the cleanup commit to an earlier version? Best, Yuchen
participants (2)
-
Sergei Golubchik -
Yuchen Pei