September 2023 - developers - lists.mariadb.org

Re: 217a7a7ceff: online alter: rework savepoints
by Sergei Golubchik 09 Oct '23

09 Oct '23

Hi, Nikita, On Sep 29, Nikita Malyavin wrote: > revision-id: 217a7a7ceff (mariadb-11.2.1-10-g217a7a7ceff) > parent(s): ad6a305d84d > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-09-23 11:46:36 +0400 > message: > > online alter: rework savepoints > > Use standard handlerton functions for savepoint add/rollback. > > To identify the savepoint, the pointer passed is used. > > Every table that has online alter in progress maintains a list of > savepoints independently. > > Also this removes setting a value to a global variable savepoint_alloc_size > without any protection, which was a race condition bug. > > --- a/sql/online_alter.cc > +++ b/sql/online_alter.cc > @@ -6,7 +6,19 @@ > > static handlerton *online_alter_hton; > > -using Online_alter_cache_list= ilist<online_alter_cache_data>; > +using sv_id_t= void *; typedef, please > + > +struct Online_alter_cache_list: ilist<online_alter_cache_data> > +{ > + sv_id_t savepoint_id= 0; > +}; > @@ -281,13 +308,11 @@ static int online_alter_log_init(void *p) > { > online_alter_hton= (handlerton *)p; > online_alter_hton->db_type= DB_TYPE_ONLINE_ALTER; > - online_alter_hton->savepoint_offset= sizeof(my_off_t); > + online_alter_hton->savepoint_offset= 1; you can even use 0, I guess. Otherwise it'll still be 8, as the server will need to align allocation, I suppose. But I suggest to try to swap the implementation. Make online_alter_hton->savepoint_offset=sizeof(void*); and in savepoint_set create a list or an array of all offsets and store it in the savepoint. Something like this: struct { uint savepoints; my_off_t log_prev_pos[1]; } oasv; int online_alter_savepoint_set(handlerton *hton, THD *thd, oasv **sv) { uint count= 0, i=0; for (auto &cache: cache_list) count++; DBUG_ASSERT(count); *sv= malloc(sizeof(oasv) + sizeof(my_off_t)*(count - 1)); (*sv)->savepoints= count; for (auto &cache: cache_list) (*sv)->log_prev_pos[i]= cache.get_byte_position(); } and so on. You just need to make sure you always use push_back and not push_front for adding caches to the cache_list. instead of a structure with a counter you can use an array with MY_OFF_T_UNDEF (not 0!) at the end. > online_alter_hton->close_connection= online_alter_close_connection; > > - online_alter_hton->savepoint_set= // Done by online_alter_savepoint_set > - [](handlerton *, THD *, void *){ return 0; }; > - online_alter_hton->savepoint_rollback= // Done by online_alter_savepoint_rollback > - [](handlerton *, THD *, void *){ return 0; }; > + online_alter_hton->savepoint_set= online_alter_savepoint_set; > + online_alter_hton->savepoint_rollback= online_alter_savepoint_rollback; > online_alter_hton->savepoint_rollback_can_release_mdl= > [](handlerton *hton, THD *thd){ return true; }; Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 2

Re: ad6a305d84d: online alter: use thd->ha_data to store cache_list
by Sergei Golubchik 09 Oct '23

09 Oct '23

Hi, Nikita, On Sep 29, Nikita Malyavin wrote: > revision-id: ad6a305d84d (mariadb-11.2.1-9-gad6a305d84d) > parent(s): 132c56f1325 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-09-23 11:46:36 +0400 > message: > > online alter: use thd->ha_data to store cache_list nice > diff --git a/sql/online_alter.cc b/sql/online_alter.cc > index 3255fc06c3e..5ad2156cd21 100644 > --- a/sql/online_alter.cc > +++ b/sql/online_alter.cc > @@ -6,6 +6,7 @@ > > static handlerton *online_alter_hton; > > +using Online_alter_cache_list= ilist<online_alter_cache_data>; please, use typedef for that > class online_alter_cache_data: public Sql_alloc, public ilist_node<>, > public binlog_cache_data > @@ -246,7 +268,11 @@ int online_alter_savepoint_rollback(THD *thd, LEX_CSTRING name) > > static int online_alter_close_connection(handlerton *hton, THD *thd) > { > - DBUG_ASSERT(thd->online_alter_cache_list.empty()); > + Ha_data &ha= thd->ha_data[online_alter_hton->slot]; > + auto *cache_list= (Online_alter_cache_list*)ha.ha_ptr; why not thd_get_ha_data()? and don't forget thd_set_ha_data(thd, online_alter_hton, 0); THD can be reused, I suppose, so let's play safe. > + > + DBUG_ASSERT(!cache_list || cache_list->empty()); > + delete cache_list; > return 0; > } Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 3

Re: 132c56f1325: online alter: extract the source to a separate file
by Sergei Golubchik 29 Sep '23

29 Sep '23

Hi, Nikita, On Sep 29, Nikita Malyavin wrote: > revision-id: 132c56f1325 (mariadb-11.2.1-8-g132c56f1325) > parent(s): a71b13433f9 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-09-23 11:46:36 +0400 > message: > > online alter: extract the source to a separate file > > Move all the functions dedicated to online alter to a newly created > online_alter.cc. Copyright/GPL header in both new files, please. Also one comment below. > With that, make many functions static and simplify the static functions > naming. > > Also, rename binlog_log_row_online_alter -> online_alter_log_row. > > diff --git a/sql/log_event.h b/sql/log_event.h > index 848ad0cead9..35142e034f3 100644 > --- a/sql/log_event.h > +++ b/sql/log_event.h > @@ -729,19 +729,6 @@ enum Log_event_type > }; > > > -/* > - Bit flags for what has been writing to cache. Used to > - discard logs with table map events but not row events and > - nothing else important. This is stored by cache. > -*/ > - > -enum enum_logged_status > -{ > - LOGGED_TABLE_MAP= 1, > - LOGGED_ROW_EVENT= 2, > - LOGGED_NO_DATA= 4, > - LOGGED_CRITICAL= 8 > -}; > > static inline bool LOG_EVENT_IS_QUERY(enum Log_event_type type) > { > @@ -979,6 +966,19 @@ typedef struct st_print_event_info > } PRINT_EVENT_INFO; > #endif > > +/** > + Bit flags for what has been writing to cache. Used to > + discard logs with table map events but not row events and > + nothing else important. This is stored by cache. > +*/ > +enum enum_logged_status > +{ > + LOGGED_TABLE_MAP= 1, > + LOGGED_ROW_EVENT= 2, > + LOGGED_NO_DATA= 4, > + LOGGED_CRITICAL= 8 > +}; > + why did you move it? looks unintentional. Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: a71b13433f9: MDEV-32126 Assertion fails upon online ALTER and binary log enabled
by Sergei Golubchik 29 Sep '23

29 Sep '23

Hi, Nikita, On Sep 29, Nikita Malyavin wrote: > revision-id: a71b13433f9 (mariadb-11.2.1-7-ga71b13433f9) > parent(s): 883e7e5f7e7 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-09-22 17:55:14 +0400 > message: > > MDEV-32126 Assertion fails upon online ALTER and binary log enabled > > Assertion `!writer.checksum_len || writer.remains == 0' fails upon > concurrent online ALTER and transactions with failing statements and binary > log enabled. > Also another assertion, `pos != (~(my_off_t) 0)', fails in my_seek, upon > reinit_io_cache, on a simplified test. This means that IO_CACHE wasn't > properly initialized, or had an error before. > > The overall problem is a deep interference with the effect of an installed > binlog_hton. As I wrote in Jira, please, provide a better explanation here. I had to debug the issue myself and as far as I can see, the problem happens because you register binlog_hton for a transaction, but doesn't prepare the cache manager for it. This can be fixed simply with @@ -2279,6 +2279,12 @@ int binlog_log_row_online_alter(TABLE* table, const ucha> trans_register_ha(thd, false, binlog_hton, 0); if (thd->in_multi_stmt_transaction_mode()) trans_register_ha(thd, true, binlog_hton, 0); + if (mysql_bin_log.is_open()) + { + binlog_cache_mngr *cache_mngr= thd->binlog_get_cache_mngr(); + if (cache_mngr && cache_mngr->trx_cache.get_prev_position() == MY_OFF_T_UNDEF) + thd->binlog_set_stmt_begin(); + } } which solves all cases from the bug report. Don't get me wrong, I don't suggest this fix, it's kind of hackish, and I liked how other pieces of the puzzle fell into a place after a separate handlerton, like savepoints and online_alter_cache_list. I'm just saying that "magical interference with the binlog_hton" doesn't explain the problem. At all. > Solution: > Extract online alter operations into a separate handlerton. > > --- a/sql/CMakeLists.txt > +++ b/sql/CMakeLists.txt > @@ -218,6 +218,8 @@ MYSQL_ADD_PLUGIN(partition ha_partition.cc STORAGE_ENGINE DEFAULT STATIC_ONLY > RECOMPILE_FOR_EMBEDDED) > MYSQL_ADD_PLUGIN(sql_sequence ha_sequence.cc STORAGE_ENGINE MANDATORY STATIC_ONLY > RECOMPILE_FOR_EMBEDDED) > +MYSQL_ADD_PLUGIN(online_alter_log log.cc STORAGE_ENGINE MANDATORY STATIC_ONLY > +NOT_EMBEDDED) better add it next to builtin_maria_binlog_plugin, instead of via MYSQL_ADD_PLUGIN. > ADD_LIBRARY(sql STATIC ${SQL_SOURCE}) > MAYBE_DISABLE_IPO(sql) > diff --git a/sql/log.cc b/sql/log.cc > index ee78844464e..132ec254adf 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -12522,3 +12512,61 @@ void wsrep_register_binlog_handler(THD *thd, bool trx) > } > > #endif /* WITH_WSREP */ > + > +static int online_alter_close_connection(handlerton *hton, THD *thd) > +{ > + DBUG_ASSERT(thd->online_alter_cache_list.empty()); > + return 0; > +} > + > +handlerton *online_alter_hton; > + > +int online_alter_log_init(void *p) > +{ > + online_alter_hton= (handlerton *)p; > + online_alter_hton->db_type= DB_TYPE_ONLINE_ALTER; > + online_alter_hton->savepoint_offset= sizeof(my_off_t); > + online_alter_hton->close_connection= online_alter_close_connection; > + > + online_alter_hton->savepoint_set= // Done by online_alter_savepoint_set > + [](handlerton *, THD *, void *){ return 0; }; > + online_alter_hton->savepoint_rollback= // Done by online_alter_savepoint_rollback > + [](handlerton *, THD *, void *){ return 0; }; > + online_alter_hton->savepoint_rollback_can_release_mdl= > + [](handlerton *hton, THD *thd){ return true; }; > + > + online_alter_hton->commit= [](handlerton *, THD *thd, bool all) > + { return binlog_online_alter_end_trans(thd, all, true); }; > + online_alter_hton->rollback= [](handlerton *, THD *thd, bool all) > + { return binlog_online_alter_end_trans(thd, all, false); }; > + online_alter_hton->commit_by_xid= [](handlerton *hton, XID *xid) > + { return binlog_online_alter_end_trans(current_thd, true, true); }; > + online_alter_hton->rollback_by_xid= [](handlerton *hton, XID *xid) > + { return binlog_online_alter_end_trans(current_thd, true, false); }; should it be so? commit_by_xid/rollback_by_xid take XID as an argument ad you ignore it. If commit_by_xid/rollback_by_xid shouldn't be called for online alter, don't set them at all. Or set to something that always fails (and add an assert too). > + > + online_alter_hton->drop_table= [](handlerton *, const char*) { return -1; }; yeah. Like this. > + online_alter_hton->flags= HTON_NOT_USER_SELECTABLE | HTON_HIDDEN > + | HTON_NO_ROLLBACK; better remove HTON_NO_ROLLBACK. As far as I can see it doesn't do anything here anyway, so the only effect it has - everyone reading the code will stop and thing "why HTON_NO_ROLLBACK is here?" Takes at least 5-10 minutes to look through all places where HTON_NO_ROLLBACK is tested to see that online_alter_hton cannot be there. > + return 0; > +} > + > +struct st_mysql_storage_engine online_alter_storage_engine= > +{ MYSQL_HANDLERTON_INTERFACE_VERSION }; > + > +maria_declare_plugin(online_alter_log) > +{ > + MYSQL_STORAGE_ENGINE_PLUGIN, > + &online_alter_storage_engine, > + "online_alter_log", > + "MariaDB PLC", "plc" lowercase > + "This is a pseudo storage engine to represent the online alter log in a transaction", > + PLUGIN_LICENSE_GPL, > + online_alter_log_init, > + NULL, > + 0x0100, // 1.0 > + NULL, // no status vars > + NULL, // no sysvars > + "1.0", > + MariaDB_PLUGIN_MATURITY_STABLE > +} > +maria_declare_plugin_end; Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: f9c1546bd7d: MDEV-32123: require_secure_transport doesn't allow TCP connections
by Sergei Golubchik 28 Sep '23

28 Sep '23

Hi, Dmitry, On Sep 28, Dmitry Shulga wrote: > revision-id: f9c1546bd7d (mariadb-11.2.1-6-gf9c1546bd7d) > parent(s): c4cbafe0c4c > author: Dmitry Shulga > committer: Dmitry Shulga > timestamp: 2023-09-28 16:20:36 +0700 > message: > > MDEV-32123: require_secure_transport doesn't allow TCP connections > > diff --git a/mysql-test/main/require_secure_transport.test b/mysql-test/main/require_secure_transport.test > index e238e732423..4ad18afb96d 100644 > --- a/mysql-test/main/require_secure_transport.test > +++ b/mysql-test/main/require_secure_transport.test > @@ -5,11 +5,16 @@ SET GLOBAL require_secure_transport=ON; > --error ER_SECURE_TRANSPORT_REQUIRED > connect without_ssl,localhost,root,,,,,TCP NOSSL; > --enable_query_log > + > +connect with_ssl,localhost,root,,,,,TCP SSL; where's a check that a connection was established successfully and it indeed uses SSL with require_secure_transport enabled? also, you didn't add any tests for --require-secure-transport=1 > +disconnect with_ssl; > + > connection default; > SET GLOBAL require_secure_transport=OFF; > --disable_query_log > connect without_ssl,localhost,root,,,,,TCP NOSSL; > --enable_query_log > disconnect without_ssl; > + > connection default; > DROP TABLE t1; > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 1f00a8670a6..3ce1c363c86 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -14519,6 +14546,22 @@ bool acl_authenticate(THD *thd, uint com_change_user_pkt_len) > > if (initialized) // if not --skip-grant-tables > { > + /* > + Check whether the option require_secure_transport is on and in case > + it is true that the secured connection type is used, that is either > + unix socket or named pipe or ssl is in use. > + */ > + if(check_require_secured_transport(thd)) > + { > + Host_errors errors; > + errors.m_ssl= 1; > + inc_host_errors(mpvio.auth_info.thd->security_ctx->ip, &errors); > + status_var_increment(thd->status_var.access_denied_errors); > + my_error(ER_SECURE_TRANSPORT_REQUIRED, MYF(0)); > + > + DBUG_RETURN(1); > + } No, no. This is wrong. The point of "require_secure_transport" is to make sure that *everything* is sent over the secure transport. In particular, passwords must never be sent if the transport is not secure. You cannot check for it after the authentication, you must abort the connection is soon as possible. (technically, the server can do nothing to prevent the client from sending passwords in plain-text over the insecure connection. but practically clients use mysql_native_password by default, so the password won't leak) Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: [PATCH] MDEV-31872 Deprecate ENCODE()/DECODE()
by Kristian Nielsen 26 Sep '23

26 Sep '23

Kristian Nielsen <knielsen(a)knielsen-hq.org> writes: > From: Sergei Golubchik <serg(a)mariadb.org> > @@ -2724,6 +2725,10 @@ bool Item_func_encode::seed() > hash_password(rand_nr, key->ptr(), key->length()); > sql_crypt.init(rand_nr); > > + push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, ER_WARN_DEPRECATED_SYNTAX, > + ER_THD(thd, ER_WARN_DEPRECATED_SYNTAX_NO_REPLACEMENT), > + func_name_cstring().str); // since 11.3 > + NAK. As I already explained in the MDEV-31872, deprecating these functions is not appropriate and will hurt users, not help them. Don't do this. You need to see this from the user's point of view. She is a DBA, she has an application with columns encoded with ENCODE(). What do you expect her to do, rewrite the application because you or someone does not like these functions? That's very disrespectful. If you want to give a warning for these functions, then you need to create an SQL mode or some other handle for the DBA to disable the warning and keep their application running. And the warning should explain why it's there, and what should be done to avoid it. And you need to carefully justify how burdening the DBA with this task of tracking down the source of the new warning and the way to remove it, is worth the benefit. And deprecation suggests eventual removal, which is even worse, please don't do that. You haven't even updated the documentation https://mariadb.com/kb/en/encode/ to eg. say "encode" or "scramble" instead of encrypt. "base64_encode" also does not do encryption, nor is rot13 cryptographically safe, but that doesn't justify removing them either. Most columns in mariadb databases are not encrypted or encoded at all, how is ENCODE() any less secure that that? - Kristian.

3 10

Re: 025ca90e5df: MDEV-32101 CREATE PACKAGE [BODY] for sql_mode=DEFAULT
by Sergei Golubchik 22 Sep '23

22 Sep '23

Hi, Alexander, On Sep 15, Alexander Barkov wrote: > revision-id: 025ca90e5df (mariadb-11.0.1-231-g025ca90e5df) > parent(s): 8ad1e26b1ba > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2023-09-05 17:33:27 +0400 > message: > > MDEV-32101 CREATE PACKAGE [BODY] for sql_mode=DEFAULT > > This patch adds PACKAGE support with SQL/PSM dialect for sql_mode=DEFAULT: > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 7452f6a083a..22c60bd4277 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -18454,6 +18518,22 @@ drop_routine: > if (Lex->stmt_drop_procedure($3, $4)) > MYSQL_YYABORT; > } > + | DROP PACKAGE_MARIADB_SYM opt_if_exists sp_name > + { > + LEX *lex= Lex; > + lex->set_command(SQLCOM_DROP_PACKAGE, $3); > + if (unlikely(lex->sphead)) > + my_yyabort_error((ER_SP_NO_DROP_SP, MYF(0), "PACKAGE")); > + lex->spname= $4; > + } > + | DROP PACKAGE_MARIADB_SYM BODY_MARIADB_SYM opt_if_exists sp_name > + { > + LEX *lex= Lex; > + lex->set_command(SQLCOM_DROP_PACKAGE_BODY, $4); > + if (unlikely(lex->sphead)) > + my_yyabort_error((ER_SP_NO_DROP_SP, MYF(0), "PACKAGE BODY")); > + lex->spname= $5; > + } > ; at least here and in SHOW you can use package: PACKAGE_MARIADB_SYM | PACKAGE_ORACLE_SYM ; and then move drop_routine out of ifdef > > > @@ -18498,6 +18578,248 @@ create_routine: > (Item_result) $8, $10)) > MYSQL_YYABORT; > } > + > + | create_or_replace definer_opt PACKAGE_MARIADB_SYM > + opt_if_not_exists sp_name opt_create_package_chistics_init > + remember_name > + { > + sp_package *pkg; > + if (unlikely(!(pkg= Lex-> > + create_package_start(thd, > + SQLCOM_CREATE_PACKAGE, > + &sp_handler_package_spec, > + $5, $1 | $4)))) > + MYSQL_YYABORT; > + pkg->set_c_chistics(Lex->sp_chistics); > + } > + opt_sqlpsm_package_specification_element_list END > + remember_end_opt > + { > + if (unlikely(Lex->create_package_finalize(thd, $5, NULL, $7, $11))) > + MYSQL_YYABORT; > + } > + | create_or_replace definer_opt PACKAGE_MARIADB_SYM BODY_MARIADB_SYM > + opt_if_not_exists sp_name opt_create_package_chistics_init > + remember_name > + { > + sp_package *pkg; > + if (unlikely(!(pkg= Lex-> > + create_package_start(thd, > + SQLCOM_CREATE_PACKAGE_BODY, > + &sp_handler_package_body, > + $6, $1 | $5)))) > + MYSQL_YYABORT; > + pkg->set_c_chistics(Lex->sp_chistics); > + Lex->sp_block_init(thd); > + } > + sqlpsm_package_implementation_declare_section > + { > + if (unlikely(Lex->sp_block_with_exceptions_finalize_declarations(thd))) > + MYSQL_YYABORT; > + } > + sqlpsm_package_implementation_executable_section > + { > + $10.hndlrs+= $12.hndlrs; > + if (unlikely(Lex->sp_block_finalize(thd, $10))) > + MYSQL_YYABORT; > + } > + remember_end_opt > + { > + if (unlikely(Lex->create_package_finalize(thd, $6, NULL, $8, $14))) > + MYSQL_YYABORT; > + } this can also use `package` rule as above and be moved outside of %ifdef. you'll only need %ifdef MARIADB sp_tail_is: _empty ; opt_sp_name: _empty { $$= NULL; } ; %else sp_tail_is: IS | AS ; opt_sp_name: _empty { $$= NULL; } | sp_name { $$= $1; } ; %endif > + ; > + > + > +opt_sqlpsm_package_specification_element_list: > + _empty > + | sqlpsm_package_specification_element_list > + ; and so on, this can be called simply opt_package_specification_element_list if placed under %ifdef MARIADB > + > +sqlpsm_package_specification_element_list: > + sqlpsm_package_specification_element > + | sqlpsm_package_specification_element_list > + sqlpsm_package_specification_element > + ; ... Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: 14a12f98eb8: MDEV-15935 Clean up string sys_var types.
by Sergei Golubchik 14 Sep '23

14 Sep '23

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(a)mariadb.org

2 1

Re: 9db04b3dc73: MDEV-15935 Adding global/session system var redirect_url
by Sergei Golubchik 13 Sep '23

13 Sep '23

Hi, Yuchen, Thanks. Ok to push both commits into preview-11.3-preview branch On Sep 13, Yuchen Pei wrote: > revision-id: 9db04b3dc73 (mariadb-11.0.1-233-g9db04b3dc73) > parent(s): 14a12f98eb8 > author: Yuchen Pei > committer: Yuchen Pei > timestamp: 2023-09-13 11:50:42 +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. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: 8ac17273b14: MDEV-15935 Adding global/session system var redirect_url
by Sergei Golubchik 13 Sep '23

13 Sep '23

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(a)mariadb.org

2 4