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
_______________________________________________
developers mailing list -- developers@lists.mariadb.org
To unsubscribe send an email to developers-le...@lists.mariadb.org
