September 2023 - developers - lists.mariadb.org

[Maria-developers] MariaDB // Temporal tables
by Robert Palm 02 Oct '24

02 Oct '24

2 2

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

Re: [MariaDB commits] [PATCH] MDEV-31872: Add --old_mode=COMPAT_DISCOURAGED
by Sergei Golubchik 12 Sep '23

12 Sep '23

Hi, Kristian, On Sep 12, Kristian Nielsen wrote: > > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index e25d293bbb7..18d08996aa8 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -3863,12 +3863,13 @@ static const char *old_mode_names[]= > "IGNORE_INDEX_ONLY_FOR_JOIN", // deprecated since 11.3 > "COMPAT_5_1_CHECKSUM", // deprecated since 11.3 > "LOCK_ALTER_TABLE_COPY", // deprecated since 11.3 > + "COMPAT_DISCOURAGED", > 0 > }; > > void old_mode_deprecated_warnings(THD *thd, ulonglong v) > { > - v &= ~OLD_MODE_DEFAULT_VALUE; > + v &= ~(OLD_MODE_DEFAULT_VALUE | OLD_MODE_COMPAT_DISCOURAGED); No, this is wrong. old-mode *by definition* is something that is a temporary workaround for something new, old-mode values are by design deprecated from the day one. So, OLD_MODE_COMPAT_DISCOURAGED shouldn't be on the line above, and thus it'll itself be deprecated and will cause a deprecation warning. Which is rather silly, as I wrote in a previous email, and that's why an old-mode is not a good solution for turning deprecated warnings away. > for (uint i=0; old_mode_names[i]; i++) > if ((1ULL<<i) & v) > { > diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc > index 4b96271c331..339430a2794 100644 > --- a/sql/item_strfunc.cc > +++ b/sql/item_strfunc.cc > @@ -2720,7 +2720,16 @@ bool Item_func_encode::seed() > hash_password(rand_nr, key->ptr(), key->length()); > sql_crypt.init(rand_nr); > > - warn_deprecated<1103>(current_thd, func_name_cstring().str); > + if (!(current_thd->variables.old_behavior & OLD_MODE_COMPAT_DISCOURAGED)) > + { > + /* > + This function should not be removed despite the deprecation warning. > + That would cause problems for users who can then no longer access > + columns in their database that might have been inserted as > + ENCODE(str,pass_str). > + */ > + warn_deprecated<1103>(current_thd, func_name_cstring().str); > + } No, this is exactly what I wanted to prevent by adding a warn_deprecated<>() wrapper. If something is giving a warning "... is deprecated and will be removed" - it *has to be removed*. We don't want to lie to users to simply scare them. If we promise to do something, we should do it. If we aren't going to remove ENCODE/DECODE functions, we shouldn't deprecate them at all. Which is the result I'm leaning to right now. > > return FALSE; > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: 75ea482c4fc: MDEV-30973 GTID in deadlock output
by Sergei Golubchik 11 Sep '23

11 Sep '23

Hi, Aleksey, On Sep 11, Aleksey Midenkov wrote: > revision-id: 75ea482c4fc (mariadb-11.0.1-230-g75ea482c4fc) > parent(s): fe62f70fb4b > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2023-09-01 20:30:24 +0300 > message: > > MDEV-30973 GTID in deadlock output > > Example: > > TRANSACTION 40, ACTIVE 0 sec starting index read > mysql tables in use 1, locked 1 > LOCK WAIT 2 lock struct(s), heap size 1152, 1 row lock(s) > MySQL thread id 23, OS thread handle 140201491547840, query id 310, GTID 3-1-1 Updating > delete from tu where a = 9999 and b = 'xxxx' > 2023-04-12 19:53:58 22 [Note] InnoDB: *** (1) WAITING FOR THIS LOCK TO BE GRANTED: > > Deterministic slave_deadlock.test > > Instead of "MDEV-10962 Deadlock with 3 concurrent DELETEs by unique key" > this one uses "MDEV-22698 Deadlock on concurrent acquisition from multiple indexes" > > --- > mysql-test/suite/rpl/t/slave_deadlock.test | 93 ++++++++++++++++++++++++++++++ > sql/sql_show.cc | 10 ++++ please move slave_deadlock.result from the following commit to this one. otherwise ok Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Implementing Binlog GTID Indexes (MDEV-4991)
by Kristian Nielsen 10 Sep '23

10 Sep '23

I am implementing Binlog GTID Indexes, to fix an old performance regression for GTID when slaves connect to the master. I now have a solid design and a working prototype, so I wanted to describe the work to encourage early comments and suggestions. I'm hoping this will make it to the next release (where is the place to actively follow/participate in the release planning?) Ever since I implemented GTID, connecting slaves needed to sequentially scan the leading portion of one binlog file to locate the GTID position to start at. This can be slow as binlog files default to 1GB in size, even worse so when binlog files can now be encrypted. I files MDEV-4991 for this 10 years ago, and it is embarrassing that this has somehow remained unfixed for so long :-/ But now I'm fixing it. The current implementation is in the work-in-progress branch knielsen_mdev4991: https://github.com/MariaDB/server/tree/knielsen_mdev4991 The code is already functional, with the testsuite passing. Still missing is binlog purge, index crash recovery, async path, and general testing and cleanup. The basic idea is for each binlog file master-bin.000001 to write an index file master-bin.000001.idx. The index contains a B+-Tree in which the keys are pairs of (GTID state, binlog offset). A connecting slave's GTID position can be looked up quickly in the tree to find the corresponding binlog offset (and gtid_binlog_state) to start replicating at. Similarly, a non-GTID connecting slave can quickly look up its starting offset (BINLOG_GTID_POS()) to obtain the GTID position corresponding to its starting offset. The index is written out to disk concurrently with the writing of the binlog file. Since the tree is written in sorted order and append-only, the implementation is significantly simplified over a general B+-Tree, and can also be described as a Log-structed Merge Tree with a Btree search structure on top. All GTID state keys (except the first) in the index are delta-compressed, storing only the GTIDs that changed since the last record (which typically will be only one). Additionally the index is sparse, storing only say every 1-in-10 GTIDs in the index (this will be configurable of course). Using a sparse index, the disk space is reduced at the cost only of scanning a few extra events of the binlog file to find the exact position requested. Actual benchmarking of space usage is TBO, but we can expect a typical binlog file of 1 GB containing say 3 million GTIDs to require something like 10 MB of index disk space (1% increase). The file format is page-based (unlike the binlog file), so that it can be written efficiently and easily read using random-access. The index is written in parallel with the binlog file, but no extra fsync()s are needed (except one at the end of the file when the index is closed and synced to disk). The writing of the index will happen asynchroneously from the binlog background thread; this way it will have minimal impact on the performance and scalability of the binlog and the contested LOCK_log. Connecting slaves can read from the currently-being-written ("hot") GTID index by accessing internal memory buffers of pages not yet written to disk. If the server crashes, the existing binlog recovery and binlog checkpoint mechanism will be used to re-create any incomplete indexes as part of the normal recovery scan of binlog files. If somehow a binlog index should be found corrupt or missing (could be eg. old server upgrade or backup script that omits the new .idx files), the code will gracefully fall back to the old (slower) way of sequentially scanning the binlog file to locate the slave starting position. I think this will be a great (if rather late) improvement to one remaining corner of the GTID implementation that currently has sub-optimal performance, and I'm exited to get this completed and reviewed/tested and added to hopefully the next MariaDB release. - Kristian.

1 0

Re: 365faf7229f: MDEV-15935 Clean up string sys_var types.
by Sergei Golubchik 08 Sep '23

08 Sep '23

Hi, Yuchen, On Sep 08, Yuchen Pei wrote: > revision-id: 365faf7229f (mariadb-11.0.1-103-g365faf7229f) > parent(s): 4f39ae75606 > author: Yuchen Pei > committer: Yuchen Pei > timestamp: 2023-09-08 13:12:22 +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 > 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? > + global_system_variables.default_master_connection.str, > + global_system_variables.default_master_connection.length, > + MYF(MY_WME | MY_THREAD_SPECIFIC)); > + > mysql_mutex_unlock(&LOCK_global_system_variables); > > 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() ? > > thd->variables= global_system_variables; > > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index 4843450352e..064c60ab2b0 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -1355,12 +1355,11 @@ static bool check_master_connection(sys_var *self, THD *thd, set_var *var) > return false; > } > > -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. > #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 :) > + { > + 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); > + my_free(session_var(thd, char*)); > + session_var(thd, char*)= new_val; > + return (new_val == 0 && var->save_result.string_value.str != 0); > + } > + char *global_update_prepare(set_var *var) > + { > + return update_prepare(var, MYF(MY_WME)); > + } > void global_update_finish(char *new_val) > { > if (flags & ALLOCATED) > @@ -583,50 +593,28 @@ class Sys_var_charptr_base: public sys_var > flags|= ALLOCATED; > global_var(char*)= new_val; > } > - bool global_update(THD *thd, set_var *var) > + bool global_update(THD *thd, set_var *var) override > { > - char *new_val= global_update_prepare(thd, var); > + char *new_val= global_update_prepare(var); > global_update_finish(new_val); > return (new_val == 0 && var->save_result.string_value.str != 0); > } > - void session_save_default(THD *thd, set_var *var)= 0; > - void global_save_default(THD *thd, set_var *var) > + void save_default(set_var *var) > { > char *ptr= (char*)(intptr)option.def_value; > var->save_result.string_value.str= ptr; > var->save_result.string_value.length= ptr ? strlen(ptr) : 0; > } > -}; > - > -class Sys_var_charptr: public Sys_var_charptr_base > -{ > -public: > - 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, > - enum binlog_status_enum binlog_status_arg=VARIABLE_NOT_IN_BINLOG, > - on_check_function on_check_func=0, > - on_update_function on_update_func=0, > - const char *substitute=0) : > - Sys_var_charptr_base(name_arg, comment, flag_args, off, size, getopt, > - def_val, lock, binlog_status_arg, > - on_check_func, on_update_func, substitute) > + 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. > } > - > - bool session_update(THD *thd, set_var *var) > + void global_save_default(THD *, set_var *var) override > { > - DBUG_ASSERT(FALSE); > - return true; > + return save_default(var); > } > - void session_save_default(THD *thd, set_var *var) > - { DBUG_ASSERT(FALSE); } > }; > > - > class Sys_var_charptr_fscs: public Sys_var_charptr > { > using Sys_var_charptr::Sys_var_charptr; > diff --git a/mysql-test/suite/sys_vars/r/mdev_15935.result b/mysql-test/suite/sys_vars/r/mdev_15935.result > 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 > +# > +# end of test mdev_15935 > +# Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: 04b99a30a3f: MDEV-15935 Adding global/session var redirect_url
by Sergei Golubchik 08 Sep '23

08 Sep '23

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

2 1

Re: 44e692d0dbc: MDEV-29167 new db-level SHOW CREATE ROUTINE privilege
by Sergei Golubchik 06 Sep '23

06 Sep '23

Hi, Oleksandr, On Sep 04, Oleksandr Byelkin wrote: > revision-id: 44e692d0dbc (mariadb-11.0.1-228-g44e692d0dbc) > parent(s): 9cb75f333fa > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2023-09-04 14:12:29 +0200 > message: > > MDEV-29167 new db-level SHOW CREATE ROUTINE privilege > > -- Create general_log if CSV is enabled. > diff --git a/scripts/mariadb_system_tables_fix.sql b/scripts/mariadb_system_tables_fix.sql > --- a/scripts/mariadb_system_tables_fix.sql > +++ b/scripts/mariadb_system_tables_fix.sql > @@ -706,6 +706,9 @@ ALTER TABLE db modify Delete_history_priv enum('N','Y') COLLATE utf8mb3_general_ > > UPDATE user SET Delete_history_priv = Super_priv WHERE @had_user_delete_history_priv = 0; > > +ALTER TABLE db ADD Show_create_routine_priv enum('N','Y') COLLATE utf8mb3_general_ci NOT NULL DEFAULT 'N' AFTER Delete_history_priv; > +ALTER TABLE db modify Show_create_routine_priv enum('N','Y') COLLATE utf8mb3_general_ci NOT NULL DEFAULT 'N'; Why two statements? > + > ALTER TABLE user ADD plugin char(64) CHARACTER SET latin1 DEFAULT '' NOT NULL AFTER max_user_connections, > ADD authentication_string TEXT NOT NULL AFTER plugin; > ALTER TABLE user CHANGE auth_string authentication_string TEXT NOT NULL; > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -9003,7 +9008,7 @@ bool check_routine_level_acl(THD *thd, const char *db, const char *name, > NULL, db, > sctx->priv_role, > name, sph, 0))) > - no_routine_acl= !(grant_proc->privs & SHOW_PROC_ACLS); > + no_routine_acl= !(grant_proc->privs & SHOW_PROC_NO_DEF_ACLS); Oh, now I get it. All the time up until this line I thought that SHOW_PROC_NO_DEF_ACLS means "privileges that *not a definer* of the routine needs to have" And now I see that it means to "show procedure without definition". Please, rename to something, like, SHOW_PROC_EXISTANCE or SHOW_PROC_NO_BODY or even SHOW_PROC_WITHOUT_DEFINITION. But SHOW_PROC_NO_DEF is rather misleading. > } > mysql_rwlock_unlock(&LOCK_grant); > return no_routine_acl; > @@ -9237,7 +9242,7 @@ static const char *command_array[]= > "CREATE USER", "EVENT", "TRIGGER", "CREATE TABLESPACE", "DELETE HISTORY", > "SET USER", "FEDERATED ADMIN", "CONNECTION ADMIN", "READ_ONLY ADMIN", > "REPLICATION SLAVE ADMIN", "REPLICATION MASTER ADMIN", "BINLOG ADMIN", > - "BINLOG REPLAY", "SLAVE MONITOR" > + "BINLOG REPLAY", "SLAVE MONITOR", "SHOW CREATE ROUTINE" > }; > > static uint command_lengths[]= > @@ -9250,7 +9255,7 @@ static uint command_lengths[]= > 11, 5, 7, 17, 14, > 8, 15, 16, 15, > 23, 24, 12, > - 13, 13 > + 13, 13, 19 May be you'll change these two arrays into one of LEX_CSTRING's ? in a separate commit, of course. > }; > > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -6676,7 +6677,8 @@ int store_schema_params(THD *thd, TABLE *table, TABLE *proc_table, > DBUG_RETURN(0); > > if (!full_access) > - full_access= !strcmp(sp_user, definer.str); > + full_access= !check_db_routine_access(thd, SHOW_CREATE_ROUTINE_ACL, > + db.str, name.str, sph, TRUE); Wait, I thought we agreed that to see a routine body one has to be either the definer or granted SHOW CREATE ROUTINE. As far as I can see check_db_routine_access doesn't check for the user being the definer. > if (!full_access && > check_some_routine_access(thd, db.str, name.str, sph)) > DBUG_RETURN(0); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

3 3

Re: 82a49812507: MDEV-22597 Add views for periods in information_schema
by Sergei Golubchik 05 Sep '23

05 Sep '23

Hi, Nikita, On Aug 31, Nikita Malyavin wrote: > revision-id: 82a49812507 (mariadb-11.0.1-228-g82a49812507) > parent(s): 23b15d24eb3 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-08-30 22:49:36 +0400 > message: > > MDEV-22597 Add views for periods in information_schema > > Two new information_schema views are added: > * PERIOD table -- columns TABLE_CATALOG, TABLE_SCHEMA, TABLE_NAME, > PERIOD_NAME, START_COLUMN_NAME, END_COLUMN_NAME. > * KEY_PERIOD_USAGE -- works similar to KEY_COLUMN_USAGE, but for periods. > Columns CONSTRAINT_CATALOG, CONSTRAINT_SCHEMA, CONSTRAINT_NAME, > TABLE_CATALOG, TABLE_SCHEMA, TABLE_NAME, PERIOD_NAME > > Two new columns are added to the COLUMNS view: > IS_SYSTEM_TIME_PERIOD_START, IS_SYSTEM_TIME_PERIOD_END - contain YES/NO. > > diff --git a/mysql-test/main/information_schema-big.result b/mysql-test/main/information_schema-big.result > index 94c8d274fad..ad0451293c9 100644 > --- a/mysql-test/main/information_schema-big.result > +++ b/mysql-test/main/information_schema-big.result good, you didn't forget to update big and embedded tests > diff --git a/mysql-test/suite/period/r/i_s_notembedded.result b/mysql-test/suite/period/r/i_s_notembedded.result > new file mode 100644 > index 00000000000..879376cc8b4 > --- /dev/null > +++ b/mysql-test/suite/period/r/i_s_notembedded.result > @@ -0,0 +1,52 @@ > +select * from information_schema.periods; > +TABLE_CATALOG TABLE_SCHEMA TABLE_NAME PERIOD START_COLUMN_NAME END_COLUMN_NAME > +create or replace table t1 (id int primary key, s timestamp(6), e timestamp(6), > +period for mytime(s,e)); > +create or replace table t2 (id int primary key, s timestamp(6), e timestamp(6), > +period for mytime(s,e)) with system versioning; > +show columns from t1; > +Field Type Null Key Default Extra > +id int(11) NO PRI NULL > +s timestamp(6) NO NULL > +e timestamp(6) NO NULL > +select * from information_schema.periods where table_schema = 'test'; > +TABLE_CATALOG TABLE_SCHEMA TABLE_NAME PERIOD START_COLUMN_NAME END_COLUMN_NAME > +def test t2 SYSTEM_TIME row_start row_end I don't think so, they're INVISIBLE_SYSTEM. for all practical purposes they don't exist as columns in the table. Shouldn't be shown either. But don't remove this test, let's keep it to show that row_start/row_end are *not* shown here. And add table t3 with full standard definition of period for system_time. it should be shown all right. > +def test t2 mytime s e > +def test t1 mytime s e > +create user periods_hidden@localhost; ... > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index 1dc60a01f38..876d6c6f244 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -6165,11 +6166,83 @@ static void store_variable_type(THD *thd, const sp_variable *spvar, > } > } > > +static int store_schema_period_record(THD *thd, TABLE_LIST *tl, > + TABLE *schema_table, > + const LEX_CSTRING *db_name, > + const LEX_CSTRING *table_name, > + const TABLE_SHARE::period_info_t &period) > +{ > + TABLE_SHARE *s= tl->table->s; > + const CHARSET_INFO *cs= system_charset_info; > + > + int err= schema_table->field[0]->store(STRING_WITH_LEN("def"), cs); > + if(!err) err= schema_table->field[1]->store(db_name, cs); > + if(!err) err= schema_table->field[2]->store(table_name, cs); > + if(!err) err= schema_table->field[3]->store(period.name, cs); well, ok. but as you've seen elsewhere these go without checks, as these calls generally cannot fail, they don't even allocate any memory. > + > + if (err) return err; > + > + int period_field= 4; > + for (auto *field: {period.start_field(s), period.end_field(s)}) > + { > +#ifndef NO_EMBEDDED_ACCESS_CHECKS > + bool col_granted= get_column_grant(thd, &tl->grant, > + db_name->str, table_name->str, > + field->field_name.str) & COL_ACLS; > + if (col_granted) > +#endif > + { > + schema_table->field[period_field]->set_notnull(); > + err= schema_table->field[period_field]->store(field->field_name, cs); > + } > + > + if (err) > + return err; > + period_field++; > + } > + > + return schema_table_store_record(thd, schema_table); > +} > + > + > +static int > +get_schema_period_records(THD *thd, TABLE_LIST *tl, > + TABLE *schema_table, //!< @ref > + //!< Show::periods_fields_info > + bool res, > + const LEX_CSTRING *db_name, > + const LEX_CSTRING *table_name) > +{ > + TABLE *table= tl->table; > + > + if (!table || (!table->s->period.name && !table->versioned())) > + return 0; > + > +#ifndef NO_EMBEDDED_ACCESS_CHECKS > + check_access(thd, SELECT_ACL, db_name->str, > + &tl->grant.privilege, 0, 0, MY_TEST(tl->schema_table)); > + if (is_temporary_table(tl)) > + { > + tl->grant.privilege|= TMP_TABLE_ACLS; > + } > +#endif > + int err= 0; > + if (table->versioned()) > + err= store_schema_period_record(thd, tl, schema_table, db_name, table_name, > + table->s->vers); > + if (!err && table->s->period.name) > + err= store_schema_period_record(thd, tl, schema_table, db_name, table_name, > + table->s->period); > + > + return err; > +} > > -static int get_schema_column_record(THD *thd, TABLE_LIST *tables, > - TABLE *table, bool res, > - const LEX_CSTRING *db_name, > - const LEX_CSTRING *table_name) > +static > +int get_schema_column_record(THD *thd, TABLE_LIST *tables, > + TABLE *table, //!< @ref Show::columns_fields_info > + bool res, > + const LEX_CSTRING *db_name, > + const LEX_CSTRING *table_name) > { > LEX *lex= thd->lex; > const char *wild= lex->wild ? lex->wild->ptr() : NullS; > @@ -7513,30 +7592,41 @@ static int get_schema_triggers_record(THD *thd, TABLE_LIST *tables, > DBUG_RETURN(0); > } > > +static int > +store_key_column_usage(TABLE *table, const LEX_CSTRING &db_name, > + const LEX_CSTRING &table_name, > + const LEX_CSTRING &key_name, > + const LEX_CSTRING &col_name) > +{ > + CHARSET_INFO *cs= system_charset_info; > + static const LEX_CSTRING def{STRING_WITH_LEN("def")}; > + const LEX_CSTRING *values[] { &def, &db_name, &key_name, > + &def, &db_name, &table_name, &col_name }; > + int err = 0; > + for (uint i = 0; i < array_elements(values) && !err; i++) > + table->field[i]->store(values[i], cs); you forgot `err=` here :) but as I wrote above, it's not needed here anyway > + return 0; > +} > > static void > store_key_column_usage(TABLE *table, const LEX_CSTRING *db_name, > const LEX_CSTRING *table_name, const char *key_name, > - size_t key_len, const char *con_type, size_t con_len, > + size_t key_len, const char *col_name, size_t col_len, > longlong idx) > { > - CHARSET_INFO *cs= system_charset_info; > - table->field[0]->store(STRING_WITH_LEN("def"), cs); > - table->field[1]->store(db_name->str, db_name->length, cs); > - table->field[2]->store(key_name, key_len, cs); > - table->field[3]->store(STRING_WITH_LEN("def"), cs); > - table->field[4]->store(db_name->str, db_name->length, cs); > - table->field[5]->store(table_name->str, table_name->length, cs); > - table->field[6]->store(con_type, con_len, cs); > + store_key_column_usage(table, *db_name, *table_name, {key_name, key_len}, > + {col_name, col_len}); pretty, but better, please, put it back. You can use the old store_key_column_usage from your get_schema_key_period_usage_record() just the same. There's no need to have two store_key_column_usage() functions. > table->field[7]->store((longlong) idx, TRUE); > } > > > -static int get_schema_key_column_usage_record(THD *thd, > - TABLE_LIST *tables, > - TABLE *table, bool res, > - const LEX_CSTRING *db_name, > - const LEX_CSTRING *table_name) > +static int > +get_schema_key_column_usage_record(THD *thd, TABLE_LIST *tables, > + TABLE *table, > + //!< @ref Show::key_column_usage_fields_info > + bool res, > + const LEX_CSTRING *db_name, > + const LEX_CSTRING *table_name) > { > DBUG_ENTER("get_schema_key_column_usage_record"); > if (res) > @@ -9703,6 +9824,18 @@ ST_FIELD_INFO key_column_usage_fields_info[]= > CEnd() > }; > > +ST_FIELD_INFO key_period_usage_fields_info[]= > +{ > + Column("CONSTRAINT_CATALOG", Catalog(), NOT_NULL, OPEN_FULL_TABLE), > + Column("CONSTRAINT_SCHEMA", Name(), NOT_NULL, OPEN_FULL_TABLE), > + Column("CONSTRAINT_NAME", Name(), NOT_NULL, OPEN_FULL_TABLE), > + Column("TABLE_CATALOG", Catalog(), NOT_NULL, OPEN_FULL_TABLE), > + Column("TABLE_SCHEMA", Name(), NOT_NULL, OPEN_FULL_TABLE), > + Column("TABLE_NAME", Name(), NOT_NULL, OPEN_FULL_TABLE), > + Column("PERIOD_NAME", Name(), NOT_NULL, OPEN_FULL_TABLE), Cannot they all be OPEN_FRM_ONLY? > + CEnd() > +}; > + > > ST_FIELD_INFO table_names_fields_info[]= > { > @@ -10208,6 +10358,8 @@ ST_SCHEMA_TABLE schema_tables[]= > {0, 0, 0, 0, 0, 0, 0, 0, 0, 0} > }; > > +static_assert(array_elements(schema_tables) == SCH_ENUM_SIZE + 1, > + "Update enum_schema_tables as well."); +1 > > int initialize_schema_table(st_plugin_int *plugin) > { Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 6

Re: 8114fb63bbb: MDEV-31684: Add timezone information to DATE_FORMAT
by Sergei Golubchik 04 Sep '23

04 Sep '23

Hi, Rucha, Few comments, after I saw the commit in the branch: On Sep 04, Rucha Deodhar wrote: > revision-id: 8114fb63bbb (mariadb-11.0.1-229-g8114fb63bbb) > parent(s): 7ba9c7fb84b > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2023-09-01 11:01:09 +0530 > message: > > MDEV-31684: Add timezone information to DATE_FORMAT > > Before starting to go over the format string, prepare the current time > zone information incase '%z' or '%Z' is encountered. > This information can be obtained as given below: > > A) If timezone is not set ( meaning we are working with system timezone): > Get the MYSQL_TIME representation for current time and GMT time using > current thread variable for timezone and timezone variable for UTC > respectively. This MYSQL_TIME variable will be used to calculate time > difference. Also convert current time in second to tm structure to > get system timezone information. > > B) If timezone is set as offset: > Get timezone information using current timezone information and store > in appropriate variable. > > C) If timezone is set as some place (example: Europe/Berlin) > Get timezone information by searching the timezone. During internal > timezone search, information like timeoffset from UTC and abbrevation > is stored in another relevant structure. Hence use the same information. > > diff --git a/mysql-test/main/date_formats.result b/mysql-test/main/date_formats.result > index 463cce39520..23aeffedef7 100644 > --- a/mysql-test/main/date_formats.result > +++ b/mysql-test/main/date_formats.result > @@ -573,3 +573,67 @@ time_format('01 02:02:02', '%T') > select time_format('2001-01-01 02:02:02', '%T'); > time_format('2001-01-01 02:02:02', '%T') > 02:02:02 > +# > +# Beginning of 11.3 test > +# > +# MDEV-31684: Add timezone information to DATE_FORMAT > +# > +SET @old_timezone= @@time_zone; > +SET TIME_ZONE='Japan'; > +SELECT DATE_FORMAT('2009-10-04 22:23:00', '%z %Z') AS current_timezone; > +current_timezone > ++0900 CJT > +SELECT DATE_FORMAT(CURDATE(), '%z %Z') AS current_tz; 1) please, don't use CURDATE/CURRENT_TIMESTAMP/etc. The time zone abbreviation depends on the date: $ TZ=Europe/Berlin date -d '2020-01-01 1:2:3' Wed Jan 1 01:02:03 CET 2020 $ TZ=Europe/Berlin date -d '2020-06-01 1:2:3' Mon Jun 1 01:02:03 CEST 2020 See? Could be CET or CEST. Japan doesn't seem to have daylight saving, so it'll be the same abbreviation throughout the year, but it can still change at some point. So always use a fixed date, like in your first test. 2) why is it CJT at all? $ TZ=Japan date -d '2009-10-04 22:23:00' +'%z %Z' +0900 JST it looks like it should be JST? ... 3) also, please test that CET/CEST example. Or, rather, MET/MEST/MST/MMT example - the data for these abbreviations are already present in mariadb_test_data_timezone.sql That is, add lines like SET @@time_zone='Europe/Moscow'; SELECT DATE_FORMAT('....', '%z %Z'); SELECT DATE_FORMAT('....', '%z %Z'); SELECT DATE_FORMAT('....', '%z %Z'); and with different dates it should return different abbreviations for the same time zone. > +# > +# output depends on system time information. Hence replacing > +# to avoid result diff test failures. > +# > +SET @@time_zone= default; > +SELECT DATE_FORMAT('2009-10+|-HH:MM ABC22:23:00', '%z %Z') AS current_timezone; 1) I don't understand what you're testing here :( 2) Anyway, note that you can put, say, --timezone=Europe/Berlin into the date_formats.opt file and the server will be restarted with this value in TZ. That is, you can force the system time zone to be a specific fixed value. > diff --git a/scripts/mariadb_test_data_timezone.sql b/scripts/mariadb_test_data_timezone.sql > index 8d07d413cef..83bb548478d 100644 > --- a/scripts/mariadb_test_data_timezone.sql > +++ b/scripts/mariadb_test_data_timezone.sql > @@ -14,8 +14,8 @@ > -- along with this program; if not, write to the Free Software > -- Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335 USA > > -INSERT INTO time_zone_name (Name, Time_Zone_id) ... > -INSERT INTO time_zone (Time_zone_id, Use_leap_seconds) ... > -INSERT INTO time_zone_transition (Time_zone_id, Transition_time, Transition_type_id) ... > -INSERT INTO time_zone_transition_type (Time_zone_id, Transition_type_id, `Offset`, Is_DST, Abbreviation) ... > -INSERT INTO time_zone_leap_second (Transition_time, Correction) ... > +INSERT INTO time_zone_name (Name, Time_Zone_id) VALUES ... ('India/Kolkata', 6); > +INSERT INTO time_zone (Time_zone_id, Use_leap_seconds) ... > +INSERT INTO time_zone_transition (Time_zone_id, Transition_time, Transition_type_id) ... > +INSERT INTO time_zone_transition_type (Time_zone_id, Transition_type_id, `Offset`, Is_DST, Abbreviation) ... > +INSERT INTO time_zone_leap_second (Transition_time, Correction) ... 1. Why? I didn't see where you use this new value in tests. 2. Is it really India/Kolkata? The official https://www.iana.org/time-zones seems to have Asia/Kolkata. > diff --git a/sql/item_timefunc.cc b/sql/item_timefunc.cc > index 26adc4eddaa..f8d3fe7dfe8 100644 > --- a/sql/item_timefunc.cc > +++ b/sql/item_timefunc.cc > @@ -468,20 +468,111 @@ static bool extract_date_time(THD *thd, DATE_TIME_FORMAT *format, > DBUG_RETURN(1); > } > > +/* > + > + Gets local time in MYSQL_TIME structure and tm struct. > + > +*/ > +void get_timezone_offset_information(THD *thd, Time_zone *tz, int &hr, > + int &min, char* abbrevation, > + bool &is_neg) > +{ > + if (!tz) > + return; > + > + if (tz->get_name()->length() == tz_SYSTEM_name.length() && > + !strncmp((const char*)(tz->get_name()->ptr()), > + (const char*)(tz_SYSTEM_name.ptr()), 6)) > + { > + struct tm tm_local_time; > + time_t time_sec= my_time(0); > + MYSQL_TIME local_TIME, gmt_TIME; > + ulonglong seconds_diff; > + ulong microsec_diff; > + > + /* Get local time in MYSQL_TIME struct. */ > + thd->variables.time_zone->gmt_sec_to_TIME(&local_TIME, > + thd->query_start()); > + /* Get GMT time in MYSQL_TIME struct. */ > + my_tz_UTC->gmt_sec_to_TIME(&gmt_TIME, thd->query_start()); > + > + /* We need above two MYSQL_TIME to get time difference. */ > + is_neg= calc_time_diff(&local_TIME, &gmt_TIME, 1, > + &seconds_diff, &microsec_diff); > + > + /* > + once we have time difference in seconds_diff, > + convert it into hr and min. > + */ > + hr= (int)(seconds_diff/3600L); > + int temp= (int)(seconds_diff%3600L); > + min= temp/60L; > + > + /* > + Get timezone abbrevation. It is stored in tm structure, > + so convert time to tm struct and copy it. > + */ 1) tm structure also has the offset, you don't need to calculate it manually. 2) you should use not my_time(0) but the value of the first DATE_FORMAT() argument > + localtime_r(&time_sec, &tm_local_time); > +# ifdef __USE_MISC > + int len= strlen((tm_local_time.tm_zone)); > + strncpy(abbrevation, (tm_local_time.tm_zone), len); > + abbrevation[len]= '\0'; > +# endif > + } > + else > + { > + /* > + Information for "named" timezone and offset can be obtained from their > + relevant class. So get the timezone information first. > + */ > + struct tz tmp; > + tz->get_timezone_information(&tmp); > + is_neg= tmp.is_behind; > + > + /* Get time in hour and min. */ > + time_t time_in_sec= abs(tmp.seconds_offset); > + hr= (int)(time_in_sec/3600L); > + int temp= (int)(time_in_sec%3600L); > + min= temp/60L; > + > + /* Copy the abbrevation. */ > + strncpy(abbrevation, (const char*)(tmp.abbrevation), > + strlen((const char*)(tmp.abbrevation))); > + abbrevation[strlen((const char*)(tmp.abbrevation))+1]='\0'; > + } > + > + return; > +} > + > > /** > Create a formatted date/time value in a string. > */ > > -static bool make_date_time(const String *format, const MYSQL_TIME *l_time, > - timestamp_type type, const MY_LOCALE *locale, > - String *str) > +static bool make_date_time(THD *thd, const String *format, > + const MYSQL_TIME *l_time, timestamp_type type, > + const MY_LOCALE *locale, String *str) > { > char intbuff[15]; > uint hours_i; > uint weekday; > ulong length; > const char *ptr, *end; > + int diff_hr=0, diff_min=0; > + bool curr_time_behind_utc= false; > + char abbrevation[8]; > + > + Time_zone* curr_timezone= my_tz_find(thd, > + thd->variables.time_zone->get_name()); > + > + /* > + precalculate local time and timezone information because > + %z or %Z maybe present in the format. > + */ > + memset(abbrevation, 0, sizeof(abbrevation)); > + get_timezone_offset_information(thd, curr_timezone, diff_hr, > + diff_min, abbrevation, > + curr_time_behind_utc); Just a thought. get_timezone_offset_information() doesn't seem to be very cheap. May be do it only when %z or %Z is requested? Like case 'z': if (!have_tz_info) { memset(abbrevation, 0, sizeof(abbrevation)); get_timezone_offset_information(thd, curr_timezone, diff_hr, diff_min, abbrevation, curr_time_behind_utc); have_tz_info= true; } Or better something with less code duplication :) > str->length(0); > > @@ -699,6 +790,31 @@ static bool make_date_time(const String *format, const MYSQL_TIME *l_time, > str->append_with_prefill(intbuff, length, 1, '0'); > break; > > + case 'z': > + { > + if (curr_time_behind_utc) > + str->append("-", 1, system_charset_info); > + else > + str->append("+", 1, system_charset_info); > + > + if (diff_hr/10 == 0) > + str->append("0", 1, system_charset_info); > + length= (uint) (int10_to_str(diff_hr, intbuff, 10) - intbuff); > + str->append_with_prefill(intbuff, length, 1, '0'); > + if (diff_min/10 == 0) > + str->append("0", 1, system_charset_info); > + length= (uint) (int10_to_str(diff_min, intbuff, 10) - intbuff); > + str->append_with_prefill(intbuff, length, 1, '0'); why do you append "0" manually, if you use append_with_prefill(), which, I suppose, does it automatically? or, if you add "0" manually, you should use simple append(). > + } > + break; > + > + case 'Z': > + { > + str->append(abbrevation, > + (uint) strlen(abbrevation), > + system_charset_info); > + } > + break; > default: > str->append(*ptr); > break; > @@ -1912,7 +2028,7 @@ String *Item_func_date_format::val_str(String *str) > > /* Create the result string */ > str->set_charset(collation.collation); > - if (!make_date_time(format, &l_time, > + if (!make_date_time(thd, format, &l_time, > is_time_format ? MYSQL_TIMESTAMP_TIME : > MYSQL_TIMESTAMP_DATE, > lc, str)) > diff --git a/sql/tztime.cc b/sql/tztime.cc > index e77a7529332..4662fa082e0 100644 > --- a/sql/tztime.cc > +++ b/sql/tztime.cc > @@ -55,15 +55,10 @@ > > /* > Now we don't use abbreviations in server but we will do this in future. > + Edit: Startd needing abbrevation in server as part of task MDEV-31684. typo > */ > -#if defined(TZINFO2SQL) || defined(TESTTIME) > -#define ABBR_ARE_USED > -#else > -#if !defined(DBUG_OFF) > -/* Let use abbreviations for debug purposes */ > -#undef ABBR_ARE_USED > -#define ABBR_ARE_USED > -#endif /* !defined(DBUG_OFF) */ > +#ifndef ABBR_ARE_USED > +#define ABBR_ARE_USED > #endif /* defined(TZINFO2SQL) || defined(TESTTIME) */ > > /* Structure describing local time type (e.g. Moscow summer time (MSD)) */ > @@ -1008,11 +1003,6 @@ TIME_to_gmt_sec(const MYSQL_TIME *t, const TIME_ZONE_INFO *sp, uint *error_code) > > #if !defined(TESTTIME) && !defined(TZINFO2SQL) > > -/* > - String with names of SYSTEM time zone. > -*/ > -static const String tz_SYSTEM_name("SYSTEM", 6, &my_charset_latin1); > - > > /* > Instance of this class represents local time zone used on this system > @@ -1031,6 +1021,7 @@ class Time_zone_system : public Time_zone > virtual my_time_t TIME_to_gmt_sec(const MYSQL_TIME *t, uint *error_code) const; > virtual void gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const; > virtual const String * get_name() const; > + virtual void get_timezone_information(struct tz* curr_tz) const; > }; > > > @@ -1097,6 +1088,12 @@ Time_zone_system::gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const > adjust_leap_second(tmp); > } > > +void > +Time_zone_system::get_timezone_information(struct tz* curr_tz) const > +{ this is weird. You have a special if (tz->get_name()->length() == tz_SYSTEM_name.length() && !strncmp((const char*)(tz->get_name()->ptr()), (const char*)(tz_SYSTEM_name.ptr()), 6)) to detect SYSTEM time zone by name in get_timezone_offset_information(). why not to put that logic here, in the dedicated method, like you did for other time zones? > + return; > +} > + > > /* > Get name of time zone > @@ -1128,6 +1125,7 @@ class Time_zone_utc : public Time_zone > uint *error_code) const; > virtual void gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const; > virtual const String * get_name() const; > + virtual void get_timezone_information(struct tz* curr_tz) const; > }; > > > @@ -1175,6 +1173,12 @@ Time_zone_utc::gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const > adjust_leap_second(tmp); > } > > +void > +Time_zone_utc::get_timezone_information(struct tz* curr_tz) const > +{ what is printed for UTC? Please, add a test for that. > + return; > +} > + > > /* > Get name of time zone > @@ -1210,6 +1214,7 @@ class Time_zone_db : public Time_zone > virtual my_time_t TIME_to_gmt_sec(const MYSQL_TIME *t, uint *error_code) const; > virtual void gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const; > virtual const String * get_name() const; > + virtual void get_timezone_information(struct tz* curr_tz) const; > private: > TIME_ZONE_INFO *tz_info; > const String *tz_name; > @@ -1280,6 +1285,16 @@ Time_zone_db::gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const > adjust_leap_second(tmp); > } > > +void > +Time_zone_db::get_timezone_information(struct tz* curr_tz) const > +{ > + curr_tz->seconds_offset= tz_info->revtis->rt_offset; > + curr_tz->is_behind= tz_info->revtis->rt_offset < 0 ? true : false; > + strncpy((char*)(curr_tz->abbrevation), (const char*)(tz_info->chars), > + strlen((const char*)(tz_info->chars))+1); > + curr_tz->abbrevation[strlen((const char*)(tz_info->chars))+1]='\0'; here and below (in Time_zone_offset) 1. why casts? 2. you use strncpy incorrectly, the third argument specifies the size of the buffer, like strncpy(curr_tz->abbrevation, tz_info->chars, sizeof(curr_tz->abbrevation)); but better use strmake, it'll do the zero-termination for you strmake(curr_tz->abbrevation, tz_info->chars, sizeof(curr_tz->abbrevation)-1); > +} > + > > /* > Get name of time zone > @@ -1309,6 +1324,7 @@ class Time_zone_offset : public Time_zone > uint *error_code) const; > virtual void gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const; > virtual const String * get_name() const; > + virtual void get_timezone_information(struct tz* curr_tz) const; > /* > This have to be public because we want to be able to access it from > my_offset_tzs_get_key() function > @@ -1434,6 +1450,16 @@ Time_zone_offset::get_name() const > return &name; > } > > +void > +Time_zone_offset::get_timezone_information(struct tz* curr_tz) const > +{ > + curr_tz->seconds_offset= offset; > + const char *name= get_name()->ptr(); > + curr_tz->is_behind= name[0] == '+' ? false : true; > + strncpy((char*)(curr_tz->abbrevation), name, this->get_name()->length()); > + curr_tz->abbrevation[this->get_name()->length()]= '\0'; > +} > + > > static Time_zone_utc tz_UTC; > static Time_zone_system tz_SYSTEM; > diff --git a/sql/tztime.h b/sql/tztime.h > index 6d8af62ecd4..57e4aa2021b 100644 > --- a/sql/tztime.h > +++ b/sql/tztime.h > @@ -26,6 +26,11 @@ > #include "sql_list.h" /* Sql_alloc */ > #include "sql_string.h" /* String */ > > +/* > + String with names of SYSTEM time zone. > +*/ > +static const String tz_SYSTEM_name("SYSTEM", 6, &my_charset_latin1); No, sorry. It's a .h file included in many .cc files. You'll have a separate `static const String` in every single source file that includes this header. > + > class THD; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0