- developers - lists.mariadb.org

Debugging MDEV-28776, the rare rpl.rpl_mark_optimize_tbl_ddl test failure
by Kristian Nielsen 21 Jul '23

21 Jul '23

Hi Brandon, The test failure in https://jira.mariadb.org/browse/MDEV-28776 (the one with 10 failed retries after deadlock, not the freebsd failures) seems like it could be something serious. But it has been very difficult to track down. I believe you have tried different things, and I also tried hard to reproduce it, so far without success. But it's still failing occasionally in actual buildbot runs. What do you think about pushing the below to the different branches? It makes the test case … [View More]

3 6

Re: 534031b77e0: MDEV-31400 Simple plugin dependency resolution
by Sergei Golubchik 20 Jul '23

20 Jul '23

Hi, Yuchen, Thanks. Conceptually all good, the approach is correct. See comments below - about the state and about user visible warnings, nothing major. On Jul 18, Yuchen Pei wrote: > revision-id: 534031b77e0 (mariadb-10.9.5-17-g534031b77e0) > parent(s): d8997f875e2 > author: Yuchen Pei > committer: Yuchen Pei > timestamp: 2023-07-03 17:54:56 +1000 > message: > > MDEV-31400 Simple plugin dependency resolution > > diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.… [View More]cc > index 5a077a934ac..15717352040 100644 > --- a/sql/sql_plugin.cc > +++ b/sql/sql_plugin.cc > @@ -1435,6 +1435,47 @@ void plugin_unlock_list(THD *thd, plugin_ref *list, size_t count) > DBUG_VOID_RETURN; > } > > +static int plugin_do_initialize(struct st_plugin_int *plugin, uint &state) > +{ > + DBUG_ENTER("plugin_do_initialize"); > + mysql_mutex_assert_not_owner(&LOCK_plugin); > + plugin_type_init init= plugin_type_initialize[plugin->plugin->type]; > + if (!init) > + init= (plugin_type_init) plugin->plugin->init; > + if (init) > + if (int ret= init(plugin)) > + { > + /* Plugin init failed but requested a retry if possible */ > + if (unlikely(ret== HA_ERR_RETRY_INIT)) > + sql_print_warning("Plugin '%s' registration as a %s failed.", > + plugin->name.str, plugin_type_names[plugin->plugin->type].str); > + else > + sql_print_error("Plugin '%s' registration as a %s failed.", > + plugin->name.str, plugin_type_names[plugin->plugin->type].str); > + DBUG_RETURN(ret); > + } > + state= PLUGIN_IS_READY; // plugin->init() succeeded > + > + if (plugin->plugin->status_vars) > + { > + /* > + historical ndb behavior caused MySQL plugins to specify > + status var names in full, with the plugin name prefix. > + this was never fixed in MySQL. > + MariaDB fixes that but supports MySQL style too. > + */ > + SHOW_VAR *show_vars= plugin->plugin->status_vars; > + SHOW_VAR tmp_array[2]= {{plugin->plugin->name, > + (char *) plugin->plugin->status_vars, SHOW_ARRAY}, > + {0, 0, SHOW_UNDEF}}; > + if (strncasecmp(show_vars->name, plugin->name.str, plugin->name.length)) > + show_vars= tmp_array; > + > + if (add_status_vars(show_vars)) > + DBUG_RETURN(1); > + } > + DBUG_RETURN(0); > +} > > static int plugin_initialize(MEM_ROOT *tmp_root, struct st_plugin_int *plugin, > int *argc, char **argv, bool options_only) > @@ -1595,7 +1594,7 @@ int plugin_init(int *argc, char **argv, int flags) > size_t i; > struct st_maria_plugin **builtins; > struct st_maria_plugin *plugin; > - struct st_plugin_int tmp, *plugin_ptr, **reap; > + struct st_plugin_int tmp, *plugin_ptr, **reap, **retry_end, **retry_start; > MEM_ROOT tmp_root; > bool reaped_mandatory_plugin= false; > bool mandatory= true; > @@ -1737,11 +1736,16 @@ int plugin_init(int *argc, char **argv, int flags) > */ > > mysql_mutex_lock(&LOCK_plugin); > + /* List of plugins to reap */ > reap= (st_plugin_int **) my_alloca((plugin_array.elements+1) * sizeof(void*)); > *(reap++)= NULL; > + /* List of plugins to retry */ > + retry_start= retry_end= > + (st_plugin_int **) my_alloca((plugin_array.elements+1) * sizeof(void*)); > > for(;;) > { > + int error; > for (i=0; i < MYSQL_MAX_PLUGIN_TYPE_NUM; i++) > { > HASH *hash= plugin_hash + plugin_type_initialization_order[i]; > @@ -1753,16 +1757,46 @@ int plugin_init(int *argc, char **argv, int flags) > bool plugin_table_engine= lex_string_eq(&plugin_table_engine_name, > &plugin_ptr->name); > bool opts_only= flags & PLUGIN_INIT_SKIP_INITIALIZATION && > - (flags & PLUGIN_INIT_SKIP_PLUGIN_TABLE || > - !plugin_table_engine); > - if (plugin_initialize(&tmp_root, plugin_ptr, argc, argv, opts_only)) > + (flags & PLUGIN_INIT_SKIP_PLUGIN_TABLE || !plugin_table_engine); > + error= plugin_initialize(&tmp_root, plugin_ptr, argc, argv, > + opts_only); > + if (error) > { > plugin_ptr->state= PLUGIN_IS_DYING; > - *(reap++)= plugin_ptr; > + /* The plugin wants a retry of the initialisation, > + possibly due to dependency on other plugins */ > + if (unlikely(error == HA_ERR_RETRY_INIT)) > + *(retry_end++)= plugin_ptr; > + else > + *(reap++)= plugin_ptr; > } > } > } > } > + /* Retry plugins that asked for it */ > + while (retry_start < retry_end) > + { > + st_plugin_int **to_re_retry, **retrying; > + for (to_re_retry= retrying= retry_start; retrying < retry_end; retrying++) > + { > + plugin_ptr= *retrying; > + uint state= plugin_ptr->state; > + mysql_mutex_unlock(&LOCK_plugin); you might want to add here sql_print_information("plugin XXX registration is retried") otherwise it would be confusing to see "plugin XXX registration failed" and then see the plugin perferctly initialized and usable. Alternatively (preferable, even) don't print "plugin registration failed" above, print it when adding a plugin to the reap list. So that the user won't see fail/retry loop at all, this is the inner working of the plugin subsystem, no user serviceable parts inside. > + error= plugin_do_initialize(plugin_ptr, state); > + mysql_mutex_lock(&LOCK_plugin); > + plugin_ptr->state= state; why do you need to manipulate (save/restore) the state here? Is that even correct? You set the state to PLUGIN_IS_DYING earlier and plugin_do_initialize() an change it to PLUGIN_IS_READY on success. You wouldn't want to change PLUGIN_IS_READY to PLUGIN_IS_DYING, would you? If this is the case, then you don't need to save the state at all, instead you need something like if (error) plugin_ptr->state= PLUGIN_IS_DYING; > + if (error == HA_ERR_RETRY_INIT) > + *(to_re_retry++)= plugin_ptr; > + else if (error) > + *(reap++)= plugin_ptr; > + } > + /* If the retry list has not changed, i.e. if all retry attempts > + result in another retry request, empty the retry list */ > + if (to_re_retry == retry_end) > + while (to_re_retry > retry_start) > + *(reap++)= *(--to_re_retry); > + retry_end= to_re_retry; > + } > > /* load and init plugins from the plugin table (unless done already) */ > if (flags & PLUGIN_INIT_SKIP_PLUGIN_TABLE) > @@ -1775,7 +1809,8 @@ int plugin_init(int *argc, char **argv, int flags) > } > > /* > - Check if any plugins have to be reaped > + Merge the retry list to the reap list, then reap the failed > + plugins. Note that during the merge we reverse the order in retry seems like this comment is obsolete > */ > while ((plugin_ptr= *(--reap))) > { > @@ -1788,6 +1823,7 @@ int plugin_init(int *argc, char **argv, int flags) > } > > mysql_mutex_unlock(&LOCK_plugin); > + my_afree(retry_start); > my_afree(reap); > if (reaped_mandatory_plugin && !opt_help) > goto err; > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org [View Less]

2 1

Re: Client-server protocol improvement proposal: Connection redirection
by Andrey Sidorov 19 Jul '23

19 Jul '23

Hi Otto, thanks for bringing this to my attention. Happy to add redirect support to mysql2, looks like its relatively straightforward and sits on top of the existing protocol Andrey On Wed, 19 Jul 2023 at 15:44, Otto Kekäläinen <otto(a)kekalainen.net> wrote: > Hi Andrey! > > I am reaching out to you because you maintain > https://github.com/sidorares/node-mysql2 > > Have you heard about this proposal on the MariaDB developers list? > > It suggests adding a new … [View More]feature to the MariaDB/MySQL protocol where > the server would be able to tell clients "No accepting connections - > connect to this server instead". The implementation would be work by > sending a new error type ERR_REDIRECT along with a client the > IP/hostname of a new server, similar to how HTTP 301 redirect works- > > Full thread visible at > > https://lists.mariadb.org/hyperkitty/list/developers@lists.mariadb.org/thre… > > If you want to participate in discussion, just reply to this message > and change recipient to developers(a)lists.mariadb.org. > > Thanks! > > > ---------- Forwarded message --------- > From: Lenski, Daniel via developers <developers(a)lists.mariadb.org> > Date: Wed, 28 Jun 2023 at 15:24 > Subject: [MariaDB developers] Client-server protocol improvement > proposal: Connection redirection > To: developers(a)lists.mariadb.org <developers(a)lists.mariadb.org> > > > Hi! > > I would like to propose a new feature in the MariaDB client-server > protocol: application-layer redirection of client connections. > > We want the MariaDB server to be able to tell clients connecting to > it, “Sorry, this server is unavailable. Connect to an alternate server > instead.” This mechanism is inspired by HTTP 302 (“temporary > redirect”) mechanism familiar to all developers of web applications, > and is intended to have similar semantics and security properties, > since these have now been widely deployed and tested for decades. > > I have submitted a minimal but viable implementation of this at: > https://github.com/MariaDB/server/pull/2681, server-side implementation > https://github.com/mariadb-corporation/mariadb-connector-c/pull/226, > MariaDB Connector/C implementation > > > I am seeking advice and opinions on these, and will proceed to > finalize this in the following 1-2 months. > > In its current form, this implementation allows the MariaDB server > administrator to set two variables: > > SET GLOBAL SERVER_REDIRECT_MODE={ON,OFF} (the default is OFF) > SET GLOBAL SERVER_REDIRECT_TARGET='my-new-server.example.com' (or > 192.168.0.123:3307, or new-server.com:3308, etc) > > When SERVER_REDIRECT_MODE is set to on, the server will stop accepting > new TCP-based connections (unless they are to the extra port, which is > intended for emergency administrative use), and it will instead > respond to all clients with an error packet: > > $ mariadb --host my-server.example.com > ERROR 4189 (HY000): |Server is redirecting clients to > 'my-new-server.example.com:3307'|my-new-server.example.com:3307 > > When an appropriately-updated client receives this error packet (error > code 4196, message formatted as |Human readable message|<value of > SERVER_REDIRECT_TARGET>), it will parse the redirection target out > from the error message and attempt a new connection: > > $ updated-mariadb --host my-server.example.com -e 'select @@hostname' > Got server redirect to 'my-new-server.example.com' (port 3307) > +---------------------------+ > | @@hostname | > +---------------------------+ > | my-new-server.example.com | > +---------------------------+ > > The feature is gracefully backwards-compatible. Old clients that do > not support redirection will fail and show the message in a form from > which humans can deduce what happened. New clients that do support it > will follow it to the new server. > > [...] > [View Less]

1 0

Re: 845fbe9ce01: MDEV-30820 slow log Rows_examined out of range
by Sergei Golubchik 18 Jul '23

18 Jul '23

Hi, Sanja, The patch is ok, but the test case is rather weird. What does it do? How long does it run? On Jul 17, Oleksandr Byelkin wrote: > revision-id: 845fbe9ce01 (mariadb-10.4.30-30-g845fbe9ce01) > parent(s): 02cd3675c4d > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2023-07-17 10:22:54 +0200 > message: > > MDEV-30820 slow log Rows_examined out of range > > Fix row counters to be able to get any possible value. > > diff --git … [View More]a/mysql-test/main/log_slow.test b/mysql-test/main/log_slow.test > --- a/mysql-test/main/log_slow.test > +++ b/mysql-test/main/log_slow.test > @@ -119,3 +119,98 @@ drop table t; > --echo # > --echo # End of 10.3 tests > --echo # > + > + > +--echo # > +--echo # MDEV-30820: slow log Rows_examined out of range > +--echo # > + > +set @log_output.save= @@global.log_output, @slow_log.save= @@global.slow_query_log; > +set global log_output= 'TABLE', slow_query_log= ON; > +set long_query_time= 0.000001; > +create database db; > +use db; > + > +--delimiter // > +CREATE OR REPLACE FUNCTION `get_id`(`INPUT_STRING` VARCHAR(50), `DECRYPT_KEY` CHAR(20)) RETURNS int(11) > +BEGIN > + DECLARE > + REQUEST_ID INT ; > + SET > + REQUEST_ID = > + IF( > + ( > + LOCATE('#$#$#$ LHP_ID_', INPUT_STRING) > + ) > 0 AND > + ( > + INPUT_STRING REGEXP DECRYPT_KEY > + )>0, > + ( > + SPLIT_STRING( > + SUBSTRING_INDEX( > + SUBSTRING_INDEX(INPUT_STRING, '#$#$#$ LHP_ID_', -1), > + '#$#$#$', > + 1 > + ), > + '-', > + 2 > + ) > + ), > + NULL > + ) ; > + SET > + REQUEST_ID=IF( > + ISNULL(REQUEST_ID)OR(REQUEST_ID='')OR(REQUEST_ID=0), > + NULL, > + REQUEST_ID > + > + ); > + RETURN(REQUEST_ID) ; > +END > +// > + > +CREATE OR REPLACE FUNCTION `SPLIT_STRING`(`str` VARCHAR(255), `delim` VARCHAR(12), `pos` INT) RETURNS varchar(255) CHARSET utf8mb4 > +RETURN REPLACE( > + SUBSTRING( > + SUBSTRING_INDEX(str , delim , pos) , > + CHAR_LENGTH( > + SUBSTRING_INDEX(str , delim , pos - 1) > + ) + 1 > + ) , > + delim , > + '' > +) > +// > + > +CREATE TABLE `tab1` ( > +`ID` int(11) NOT NULL AUTO_INCREMENT, > +`NAME_F` varchar(50) DEFAULT NULL, > + PRIMARY KEY (`ID`) > +) DEFAULT CHARSET=utf8mb4 // > + > + CREATE TABLE `tab2` ( > + `ID` int(11) NOT NULL AUTO_INCREMENT, > + `TAB1_ID` int(11) DEFAULT NULL, > + PRIMARY KEY (`id`) > +) DEFAULT CHARSET=utf8mb4 // > + > +for i in 1..100 do insert into tab1 values (null,'qwerty'); end for ; // > +for i in 1..1000 do insert into tab2 values (null,round(rand()*10000)); end for ; // > + > +--delimiter ; > + > +SELECT > + get_id(CAST(aes_decrypt(tab1.NAME_F,'V41iNM0n4') AS char),'KM_ID_PL') as get_string, > + (CASE WHEN (SELECT ID FROM tab2 where tab2.TAB1_ID = tab1.ID LIMIT 1) IS NULL THEN 0 ELSE 1 END) AS IS_ATT > +FROM > + tab1 > +ORDER BY 2 DESC > +LIMIT 2; > + > +set global log_output= @log_output.save, slow_query_log= @slow_log.save; > +drop database db; > + > +--echo # > +--echo # End of 10.4 tests > +--echo # Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org [View Less]

2 5

Re: 5102d86beae: MDEV-30164 System variable for default collations
by Sergei Golubchik 17 Jul '23

17 Jul '23

Hi, Alexander, Looks good, thanks. ok to push. a couple of comments below - but really, on remnants of the previous version of the commit. On Jul 17, Alexander Barkov wrote: > revision-id: 5102d86beae (mariadb-10.11.2-47-g5102d86beae) > parent(s): 3430767e006 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2023-07-10 13:17:45 +0400 > message: > > MDEV-30164 System variable for default collations > > This patch adds a way to override … [View More]default collations > (or "character set collations") for desired character sets. > > diff --git a/mysql-test/main/mysqld--help.result b/mysql-test/main/mysqld--help.result > index 77df8536fa2..5c361e3da88 100644 > --- a/mysql-test/main/mysqld--help.result > +++ b/mysql-test/main/mysqld--help.result > @@ -144,6 +144,8 @@ The following specify which files/extra groups are read (specified before remain > Don't ignore client side character set value sent during > handshake. > (Defaults to on; use --skip-character-set-client-handshake to disable.) > + --character-set-collations=name > + Set default collations for character sets. shouldn't it say "overrides"? > --character-set-filesystem=name > Set the filesystem character set. > -C, --character-set-server=name > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 607877dfb83..d35869e7e39 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -6061,6 +6061,32 @@ mysql_execute_command(THD *thd, bool is_called_from_prepared_stmt) > } > thd->reset_kill_query(); > } > + > + /* > + Binary logging is now done. Unset the "used" flags to avoid > + flags leaking to the next event (and to the COMMIT statement > + in the end of the current event). > + > + Example: > + > + Suppose a non-default collation (in @@character_set_collations) > + was used during the statement, the mysqlbinlog output for > + the current statement will contain a sequence like this: > + > + SET character_set_collations='utf8mb3=utf8mb3_bin'; > + INSERT INTO t1 VALUES (_utf8mb3'test'); > + COMMIT; > + > + The statment (INSERT in this example) is already in binlog at this point, and the > + and the "SET character_set_collations" is written inside a > + Q_CHARACTER_SET_COLLATIONS chunk in its log entry header. > + The flag CHARACTER_SET_COLLATIONS_USED is not needed any more. > + The COMMIT can be printed without "SET character_set_collations". > + > + The same logic applies to the other _USED flags. That's a bit too verbose in my opinion. The example was good in the previous version of the patch, but now the first paragraph of the comment pretty much explains it all. but do it as you prefer. > + */ > + thd->used= 0; > + > if (unlikely(thd->is_error()) || > (thd->variables.option_bits & OPTION_MASTER_SQL_ERROR)) > { Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org [View Less]

1 0

Re: 8e9c68b49dd: MDEV-25237 Assertion `global_system_variables.session_track_system_variables' failed in Session_sysvars_tracker::init | SIGSEGV's in __strlen_avx2 | UBSAN: runtime error: null pointer passed as argument 1, which is declared to never be null in my_strdup
by Sergei Golubchik 15 Jul '23

15 Jul '23

Hi, Sanja, ok to push. but see comments below: On Jul 15, Oleksandr Byelkin wrote: > revision-id: 8e9c68b49dd (mariadb-10.4.30-9-g8e9c68b49dd) > parent(s): 423c28f0aa4 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2023-07-10 13:40:46 +0200 > message: > > MDEV-25237 Assertion `global_system_variables.session_track_system_variables' failed in Session_sysvars_tracker::init | SIGSEGV's in __strlen_avx2 | UBSAN: runtime error: null pointer passed … [View More]as argument 1, which is declared to never be null in my_strdup again, if you see a bug with a very long summary, particularly if it basically makes no sense to a user - in that case, please, don't hesitate to make it shorter and more meaningful. For example "crash after setting session_track_system_variables to an invalid value" > Fix of typo in checking variable list corectness. > > Fix of error handling in case of variable list parse error > > diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc > index 2f6c5e29bf2..3e1558f6759 100644 > --- a/sql/session_tracker.cc > +++ b/sql/session_tracker.cc > @@ -221,7 +221,7 @@ bool sysvartrack_validate_value(THD *thd, const char *str, size_t len) > /* Remove leading/trailing whitespace. */ > trim_whitespace(system_charset_info, &var); > > - if (!strcmp(var.str, "*") && !find_sys_var(thd, var.str, var.length)) > + if (strcmp(var.str, "*") && !find_sys_var(thd, var.str, var.length)) > return true; > > if (lasts) > @@ -331,9 +331,10 @@ void Session_sysvars_tracker::init(THD *thd) > mysql_mutex_assert_owner(&LOCK_global_system_variables); > DBUG_ASSERT(thd->variables.session_track_system_variables == > global_system_variables.session_track_system_variables); > - DBUG_ASSERT(global_system_variables.session_track_system_variables); > thd->variables.session_track_system_variables= > - my_strdup(global_system_variables.session_track_system_variables, > + my_strdup((global_system_variables.session_track_system_variables? > + global_system_variables.session_track_system_variables: > + ""), this is such a common pattern that we have a helper for that, use my_strdup(safe_str(global_system_variables.session_track_system_variables)) > MYF(MY_WME | MY_THREAD_SPECIFIC)); > } > > @@ -572,6 +573,12 @@ bool sysvartrack_global_update(THD *thd, char *str, size_t len) > { > LEX_STRING tmp= { str, len }; > Session_sysvars_tracker::vars_list dummy; > + DBUG_EXECUTE_IF("dbug_session_tracker_parse_error", > + { > + my_error(ER_OUTOFMEMORY, MYF(0), 1); > + return true; > + }); > + > if (!dummy.parse_var_list(thd, tmp, false, system_charset_info)) > { > dummy.construct_var_list(str, len + 1); > diff --git a/sql/sys_vars.inl b/sql/sys_vars.inl > index 84d1cd6b331..3e282de439a 100644 > --- a/sql/sys_vars.inl > +++ b/sql/sys_vars.inl > @@ -620,7 +620,11 @@ class Sys_var_sesvartrack: public Sys_var_charptr_base > { > if (sysvartrack_global_update(thd, new_val, > var->save_result.string_value.length)) > + { > + if (new_val) > + my_free(new_val); it's ok, as you like. But technically you don't need an `if` here, my_free() just as free() works with NULL pointers fine. > new_val= 0; > + } > } > global_update_finish(new_val); > return (new_val == 0 && var->save_result.string_value.str != 0); > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org [View Less]

1 0

Re: 8d51c6d234b: MDEV-30164 System variable for default collations
by Sergei Golubchik 10 Jul '23

10 Jul '23

Hi, Alexander, Please, see comments below. Sorry, it took a while. On Jun 23, Alexander Barkov wrote: > revision-id: 8d51c6d234b (mariadb-10.11.2-13-g8d51c6d234b) > parent(s): ceb0e7f944b > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2023-03-21 10:07:57 +0400 > message: > > MDEV-30164 System variable for default collations > > diff --git a/mysql-test/main/ctype_collate_implicit.test b/mysql-test/main/ctype_collate_implicit.test > --- /… [View More]dev/null > +++ b/mysql-test/main/ctype_collate_implicit.test > @@ -0,0 +1,209 @@ > +--source include/have_utf8.inc > +--source include/have_utf8mb4.inc > + > +--echo # > +--echo # MDEV-30164 System variable for default collations > +--echo # > + > +SET @@character_set_collations= ' utf8mb3 = utf8mb3_bin , LATIN1 = LATIN1_BIN '; also, please, test that parsing of commas is relaxed too. Like set @@character_set_collations= ',,,utf8mb3 = utf8mb3_bin,,latin1 = latin1_bin,,,'; it makes it easier for the user to edit the value. > +SELECT @@character_set_collations; > +SET @@character_set_collations=''; > +SELECT @@character_set_collations; > + > +SET @@character_set_collations='utf8mb3=utf8mb3_bin'; > +SELECT @@character_set_collations; > +SET @@character_set_collations=''; > + > +--error ER_COLLATION_CHARSET_MISMATCH > +SET @@character_set_collations='utf8mb3=utf8mb4_general_ci'; > +SELECT @@character_set_collations; > + > +--error ER_COLLATION_CHARSET_MISMATCH > +SET @@character_set_collations='utf8mb4=utf8mb3_general_ci'; > +SELECT @@character_set_collations; > + > +SET @@character_set_collations='utf8mb3=utf8mb3_general_ci'; > +SELECT @@character_set_collations; > + > +SET @@character_set_collations='utf8mb4=utf8mb4_general_ci,latin1=latin1_bin'; > +SELECT @@character_set_collations; > + > +--error ER_COLLATION_CHARSET_MISMATCH > +SET @@character_set_collations='utf8mb4=uca1400_ai_ci,latin1=uca1400_ai_ci'; > + > +# All or nothing is set. "Nothing" in this case because of the error on latin1. > +# The "uca1400_ai_ci FOR utf8mb4" part was ignored. > +SELECT @@character_set_collations; > +SELECT @@character_set_collations RLIKE 'utf8mb4=utf8mb4_general_ci' AS expect_true; > + > + > +SET @@character_set_collations='utf8mb4=uca1400_ai_ci'; > +SELECT @@character_set_collations; > + > +SET NAMES utf8mb4; > +SELECT @@collation_connection; > + > +# We have to disable --view-protocol for the following statement. > +# 'mtr --view-protocol' creates a separate connection for these statements: > +# CREATE VIEW mysqltest_tmp_sp AS ...; > +# DROP VIEW mysqltest_tmp_sp; > +# The current @@character_set_collations does not affect this connection. > +# So --view-protocol would return the hard-coded character set collation here, > +# instead of utf8mb4_uca1400_ai_ci > + > +--disable_view_protocol > +SELECT collation('literal'); > +--enable_view_protocol > +EXECUTE IMMEDIATE 'SELECT COLLATION(?)' USING 'literal'; > + > +CREATE VIEW v1 AS SELECT 'literal', collation('literal') as cl; > +SHOW CREATE VIEW v1; > +SELECT * FROM v1; > +DROP VIEW v1; > + > + > +# Override @@collation_connection to utf8mb4_general_ci. > +# Make sure that CREATE statements does not use @@collation_connection. > +# to detect implicit collations. > +# Implicit collations are detected using @@character_set_collations! > + > +SET NAMES utf8mb4 COLLATE utf8mb4_general_ci; > + > +CREATE TABLE t1 (a TEXT CHARACTER SET utf8mb4); > +SHOW CREATE TABLE t1; > +DROP TABLE t1; > + > +CREATE TABLE t1 (a TEXT CHARACTER SET utf8mb4 COLLATE DEFAULT); > +SHOW CREATE TABLE t1; > +DROP TABLE t1; > + > +CREATE TABLE t1 (a TEXT COLLATE DEFAULT) CHARACTER SET utf8mb4; > +SHOW CREATE TABLE t1; > +DROP TABLE t1; > + > +CREATE TABLE t1 (a TEXT) CHARACTER SET utf8mb4; > +SHOW CREATE TABLE t1; > +DROP TABLE t1; > + > +CREATE DATABASE db1 CHARACTER SET utf8mb4; > +SHOW CREATE DATABASE db1; > +DROP DATABASE db1; > + > + > +# Test how @@character_set_collations affects various expressions > +# with implicit collations. > + > + > +let query=SELECT > + @@collation_connection AS conn, > + COLLATION('a') AS lit, > + COLLATION(CONCAT(1)) AS num, > + COLLATION(CAST(123 AS CHAR)) AS casti, > + COLLATION(_utf8mb4'a') AS litu, > + COLLATION(_utf8mb4 0x62) AS lituh, > + COLLATION(_utf8mb4 X'63') AS lituhs, > + COLLATION(CAST(123 AS CHAR CHARACTER SET utf8mb4)) AS castic, > + COLLATION(CHAR(0x61 USING utf8mb4)) AS chr, > + COLLATION(CONVERT('a' USING utf8mb4)) AS conv; > + > +# The below SET NAMES sets @@collation_connection to utf8mb4_general_ci. > +# But @@character_set_collations still contains utf8mb4=uca1400_ai_ci. > + > +SET NAMES utf8mb4 COLLATE utf8mb4_general_ci; > + > +# Columns expected to print utf8mb4_general_ci > +# because they use @@collation_connection: > +# - String literals without introducers > +# - Automatic number-to-string conversions > +# - CAST(AS CHAR) - without USING > +# > +# Columns expected to print utf8mb4_uca1400_ai_ci > +# because they use the current session default collation > +# for the character set (as specified in @@collation_connection) > +# - String literals with introducers > +# - CAST(AS CHAR USING cs) > +# - CHAR() > +# - CONVERT() > + > +--vertical_results > +--eval $query; > +--horizontal_results > + > +# This sets collation_connection to utf8mb4_uca1400_ai_ci > +# according to @@character_set_collations. > +SET NAMES utf8mb4; > + > +# Now all columns are expected to print utf8mb4_uca1400_ai_ci: > +# - Some columns because @@collation_connection says so > +# - Some columns because @@character_set_collations says so. > + > +--vertical_results > +--eval $query; > +--horizontal_results > + > + > +# > +# INFORMATION_SCHEMA > +# > + > +SET character_set_collations='latin1=latin1_bin,utf8mb4=uca1400_ai_ci'; > +SHOW CHARACTER SET LIKE 'latin1'; > +SELECT * FROM INFORMATION_SCHEMA.CHARACTER_SETS > +WHERE CHARACTER_SET_NAME='latin1'; > + > +SHOW COLLATION LIKE 'latin1%'; > +SELECT COLLATION_NAME, IS_DEFAULT > +FROM INFORMATION_SCHEMA.COLLATIONS > +WHERE CHARACTER_SET_NAME LIKE 'latin1%'; > +SELECT COLLATION_NAME, FULL_COLLATION_NAME, IS_DEFAULT > +FROM INFORMATION_SCHEMA.COLLATION_CHARACTER_SET_APPLICABILITY > +WHERE COLLATION_NAME LIKE 'latin1%'; > + > +SHOW CHARACTER SET LIKE 'utf8mb4'; > +SELECT * FROM INFORMATION_SCHEMA.CHARACTER_SETS > +WHERE CHARACTER_SET_NAME='utf8mb4'; > + > +SHOW COLLATION LIKE '%uca1400_ai_ci%'; > +SELECT COLLATION_NAME, IS_DEFAULT > +FROM INFORMATION_SCHEMA.COLLATIONS > +WHERE COLLATION_NAME LIKE '%uca1400_ai_ci%'; > +SELECT COLLATION_NAME, FULL_COLLATION_NAME, IS_DEFAULT > +FROM INFORMATION_SCHEMA.COLLATION_CHARACTER_SET_APPLICABILITY > +WHERE COLLATION_NAME LIKE '%uca1400_ai_ci%'; > + > +# > +# Prepared statements: reprepare on @@character_set_collations change. > +# > + > +SET @@character_set_collations=''; > +PREPARE stmt FROM 'SELECT ' > + 'COLLATION(CAST("x" AS CHAR CHARACTER SET utf8mb3)) AS a, ' > + 'COLLATION(_utf8mb3"x") AS b'; > +EXECUTE stmt; > +SET @@character_set_collations='utf8mb3=utf8mb3_bin'; > +EXECUTE stmt; > + > +SET @@character_set_collations='utf8mb3=utf8mb3_bin'; > +PREPARE stmt FROM 'SELECT ' > + 'COLLATION(CAST("x" AS CHAR CHARACTER SET utf8mb3)) AS a, ' > + 'COLLATION(_utf8mb3"x") AS b'; > +EXECUTE stmt; > +SET @@character_set_collations=DEFAULT; > +EXECUTE stmt; > + > +SET NAMES utf8mb3; > +SET @@character_set_collations=''; > +PREPARE stmt FROM 'CREATE TABLE t1 ' > + '(a TEXT CHARACTER SET utf8mb3 COLLATE DEFAULT COLLATE utf8mb3_general_ci)'; > +EXECUTE stmt; > +SHOW CREATE TABLE t1; > +DROP TABLE t1; > + > +SET @@character_set_collations='utf8mb3=utf8mb3_bin'; > +--error ER_CONFLICTING_DECLARATIONS > +EXECUTE stmt; > + > +SET @@character_set_collations=''; > +EXECUTE stmt; > +SHOW CREATE TABLE t1; > +DROP TABLE t1; > diff --git a/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result b/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result > --- a/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result > +++ b/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result > @@ -502,6 +502,16 @@ NUMERIC_BLOCK_SIZE NULL > ENUM_VALUE_LIST NULL > READ_ONLY NO > COMMAND_LINE_ARGUMENT NULL > +VARIABLE_NAME CHARACTER_SET_COLLATIONS > +VARIABLE_SCOPE SESSION > +VARIABLE_TYPE VARCHAR > +VARIABLE_COMMENT Default collations for character sets Better to emphasize somehow that it's not a complete list of default collations, but adjustments to the compiled-in list of default collations. If this variable is empty, it doesn't mean that character sets have no default collations. I'm not sure how to put this succintly in the help text though :) > +NUMERIC_MIN_VALUE NULL > +NUMERIC_MAX_VALUE NULL > +NUMERIC_BLOCK_SIZE NULL > +ENUM_VALUE_LIST NULL > +READ_ONLY NO > +COMMAND_LINE_ARGUMENT NULL > VARIABLE_NAME CHARACTER_SET_CONNECTION > VARIABLE_SCOPE SESSION > VARIABLE_TYPE ENUM > diff --git a/sql/lex_charset.cc b/sql/lex_charset.cc > --- a/sql/lex_charset.cc > +++ b/sql/lex_charset.cc > @@ -447,6 +451,17 @@ Lex_exact_charset_opt_extended_collate::find_default_collation() const > } > > > +CHARSET_INFO * > +Lex_exact_charset_opt_extended_collate:: > + find_mapped_default_collation(Charset_collation_map_st::Used *used, > + const Charset_collation_map_st &map) const > +{ > + CHARSET_INFO *cs= find_compiled_default_collation(); > + if (!cs) > + return nullptr; > + return map.get_collation_for_charset(used, cs); This seems a bit redundant. find_compiled_default_collation() will do get_charset_by_csname(m_ci, MY_CS_PRIMARY) and only then you'll be able to search, because you map default compiled collation id to use specified collation id. but conceptually you need to map a charset to a collation, not a collation to a collation. If you'd map, say, cs->cs_name.str (just the pointer, not the string value) or, say, cs->cset, then you'd be mapping a charset to a collation. and here you'd be able to do just return map.get_collation_for_charset(used, m_ci) with find_compiled_default_collation() being done inside get_collation_for_charset() and only when Elem_st wasn't found > +} > + > /* > Resolve an empty or a contextually typed collation according to the > upper level default character set (and optionally a collation), e.g.: > diff --git a/sql/lex_charset.h b/sql/lex_charset.h > --- a/sql/lex_charset.h > +++ b/sql/lex_charset.h > @@ -544,6 +561,21 @@ struct Lex_exact_charset_extended_collation_attrs_st > { > return m_ci; > } > + CHARSET_INFO *charset_info(Charset_collation_map_st::Used *used, > + const Charset_collation_map_st &map) const > + { > + switch (m_type) > + { > + case TYPE_CHARACTER_SET: > + return map.get_collation_for_charset(used, m_ci); Why do you need this special charset_info(used,map) that differs from charset_info() when m_type==TYPE_CHARACTER_SET, and is used in set_charset_collation_attrs() only? > + case TYPE_EMPTY: > + case TYPE_CHARACTER_SET_COLLATE_EXACT: > + case TYPE_COLLATE_CONTEXTUALLY_TYPED: > + case TYPE_COLLATE_EXACT: > + break; > + } > + return m_ci; > + } > Type type() const > { > return m_type; > diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc > --- a/sql/log_event_server.cc > +++ b/sql/log_event_server.cc > @@ -1989,6 +1997,16 @@ int Query_log_event::do_apply_event(rpl_group_info *rgi, > thd->variables.sql_mode= > (sql_mode_t) ((thd->variables.sql_mode & MODE_NO_DIR_IN_CREATE) | > (sql_mode & ~(sql_mode_t) MODE_NO_DIR_IN_CREATE)); > + > + size_t cslen= thd->variables.character_set_collations.from_binary( > + character_set_collations.str, > + character_set_collations.length); > + if (cslen != character_set_collations.length) > + { > + thd->variables.character_set_collations.init(); > + goto compare_errors; // QQ: report an error here? is it corrupted event or unknown collation? > + } > + > if (charset_inited) > { > rpl_sql_thread_info *sql_info= thd->system_thread_info.rpl_sql_info; > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -6049,6 +6049,27 @@ mysql_execute_command(THD *thd, bool is_called_from_prepared_stmt) > } > thd->reset_kill_query(); > } > + > + /* > + If a non-default collation (in @@character_set_collations) > + was used during the statement, the mysqlbinlog output for > + the current statement will contain a sequence like this: > + > + SET character_set_collations='utf8mb3=utf8mb3_bin'; > + INSERT INTO t1 VALUES (_utf8mb3'test'); > + COMMIT; > + > + The statment (INSERT in this example) is already in binlog at this point, and the > + and the "SET character_set_collations" is written inside a > + Q_CHARACTER_SET_COLLATIONS chunk in its log entry header. > + The flag CHARACTER_SET_COLLATIONS_USED is not needed any more. > + > + Let's suppress the flag to avoid a Q_CHARACTER_SET_COLLATIONS chunk > + inside the COMMIT log entry header - it would be useless and would > + only waste space in the binary log. > + */ > + thd->used&= ~THD::CHARACTER_SET_COLLATIONS_USED; Wouldn't that logic apply to other THD::xxx_USED flags? May be you should reset thd->used=0 here? > + > if (unlikely(thd->is_error()) || > (thd->variables.option_bits & OPTION_MASTER_SQL_ERROR)) > { > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -12457,8 +12459,10 @@ bool HA_CREATE_INFO:: I'd prefer you didn't put a line break after :: see how useful it makes the hunk header? > { > // Make sure we don't do double resolution in direct SQL execution > DBUG_ASSERT(!default_table_charset || thd->stmt_arena->is_stmt_execute()); > + Character_set_collations_used used(thd); > if (!(default_table_charset= > - default_cscl.resolved_to_context(ctx))) > + default_cscl.resolved_to_context(&used, > + thd->variables.character_set_collations, ctx))) > return true; > } > > diff --git a/sql/sql_class.h b/sql/sql_class.h > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -5620,6 +5624,29 @@ class THD: public THD_count, /* this must be first */ > }; > > > +class Character_set_collations_used: public Charset_collation_map_st::Used > +{ > + THD *m_thd; > +public: > + Character_set_collations_used(THD *thd) > + :m_thd(thd) > + { } > + ~Character_set_collations_used() > + { > + /* > + Mark THD that the collation map was used, > + no matter if a compiled or a mapped collation was why no matter? as far as I understand, you didn't plan to change compiled-in defaults > + found during charset->collation resolution. > + Even if the map was empty, we still need to print > + SET @@session.character_set_collations=''; > + in mariadb-binlog output. > + */ > + if (m_used) > + m_thd->used|= THD::CHARACTER_SET_COLLATIONS_USED; > + } > +}; > + > + > /* > Start a new independent transaction for the THD. > The old one is stored in this object and restored when calling > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc > --- a/sql/sql_prepare.cc > +++ b/sql/sql_prepare.cc > @@ -3538,6 +3544,13 @@ static void mysql_stmt_execute_common(THD *thd, > DBUG_VOID_RETURN; > } > > + if (stmt->prepare_time_charset_collation_map_version() != > + thd->variables.character_set_collations.version()) > + { > + if (stmt->reprepare()) > + DBUG_VOID_RETURN; > + } why do you reprepare here and now where reprepare() is normally happens, in the Prepared_statement::execute_loop()? > + > /* > In case of direct execution application decides how many parameters > to send. > @@ -5220,6 +5252,13 @@ bool Prepared_statement::execute(String *expanded_query, bool open_cursor) > MYSQL_QUERY_EXEC_START(thd->query(), thd->thread_id, thd->get_db(), > &thd->security_ctx->priv_user[0], > (char *) thd->security_ctx->host_or_ip, 1); > + /* > + If PREPARE used @@character_set_collations, > + then we need to make sure binary log writes > + the map in the event header. > + */ why is it different from other THD::xx_USED flags? > + thd->used|= m_prepare_time_thd_used_flags & > + THD::CHARACTER_SET_COLLATIONS_USED; > error= mysql_execute_command(thd, true); > MYSQL_QUERY_EXEC_DONE(error); > thd->update_server_status(); > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -436,6 +436,115 @@ static bool update_auto_increment_increment (sys_var *self, THD *thd, enum_var_t > > #endif /* WITH_WSREP */ > > + > +class Sys_var_charset_collation_map: public sys_var in sys_var.inl, please > +{ > +public: > + Sys_var_charset_collation_map(const char *name_arg, const char *comment, > + int flag_args, ptrdiff_t off, size_t size, > + CMD_LINE getopt, > + enum binlog_status_enum binlog_status_arg) > + :sys_var(&all_sys_vars, name_arg, comment, > + flag_args, off, getopt.id, getopt.arg_type, > + SHOW_CHAR, > + DEFAULT(0), nullptr, binlog_status_arg, > + nullptr, nullptr, nullptr) > + { > + option.var_type|= GET_STR; > + } > + > +private: > + > + static bool charset_collation_map_from_item(Charset_collation_map_st *map, > + Item *item, > + myf utf8_flag) > + { > + String *value, buffer; > + if (!(value= item->val_str_ascii(&buffer))) > + return true; > + return map->from_text(value->to_lex_cstring(), utf8_flag); > + } > + > + static const uchar *make_value_ptr(THD *thd, > + const Charset_collation_map_st &map) > + { > + size_t nbytes= map.text_format_nbytes_needed(); > + char *buf= (char *) thd->alloc(nbytes); > + size_t length= map.print(buf, nbytes); > + return (uchar *) thd->strmake(buf, length); Eh, sorry. I don't understand. You allocate memory twice? for two copies of the string? > + } > + > + > +private: > + > + bool do_check(THD *thd, set_var *var) override > + { > + Charset_collation_map_st map; > + return charset_collation_map_from_item(&map, var->value, > + thd->get_utf8_flag()); > + } > + > + void session_save_default(THD *thd, set_var *var) override > + { > + thd->variables.character_set_collations.set( > + global_system_variables.character_set_collations, 1); > + } > + > + void global_save_default(THD *thd, set_var *var) override > + { > + global_system_variables.character_set_collations.init(); > + } > + > + bool session_update(THD *thd, set_var *var) override > + { > + Charset_collation_map_st map; > + if (!var->value) > + { > + session_save_default(thd, var); > + return false; > + } > + if (charset_collation_map_from_item(&map, var->value, thd->get_utf8_flag())) The idea is that var->value is only evaluated in check and is not reevaluated in update. check is supposed to store the value in set_var *var for update to use. > + return true; > + thd->variables.character_set_collations.set(map, 1); > + return false; > + } > + > + bool global_update(THD *thd, set_var *var) override > + { > + Charset_collation_map_st map; > + if (!var->value) > + { > + global_save_default(thd, var); > + return false; > + } > + if (charset_collation_map_from_item(&map, var->value, thd->get_utf8_flag())) > + return true; > + global_system_variables.character_set_collations= map; > + return false; > + } > + > + const uchar * > + session_value_ptr(THD *thd, const LEX_CSTRING *base) const override > + { > + return make_value_ptr(thd, thd->variables.character_set_collations); > + } > + > + const uchar * > + global_value_ptr(THD *thd, const LEX_CSTRING *base) const override > + { > + return make_value_ptr(thd, global_system_variables. > + character_set_collations); > + } > +}; > + > + > +static Sys_var_charset_collation_map Sys_character_set_collations( > + "character_set_collations", > + "Default collations for character sets", > + SESSION_VAR(character_set_collations), > + NO_CMD_LINE, NOT_IN_BINLOG); > + > + > static Sys_var_double Sys_analyze_sample_percentage( > "analyze_sample_percentage", > "Percentage of rows from the table ANALYZE TABLE will sample " > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -9686,6 +9713,9 @@ column_default_non_parenthesized_expr: > } > | CONVERT_SYM '(' expr USING charset_name ')' > { > + Character_set_collations_used used(thd); > + $5= thd->variables.character_set_collations. > + get_collation_for_charset(&used, $5); this is rather awkward pattern that you use everywhere, with Character_set_collations_used used(thd); why not to pass thd as an argument instead? - Character_set_collations_used used(thd); $5= thd->variables.character_set_collations. - get_collation_for_charset(&used, $5); + get_collation_for_charset(thd, $5); > $$= new (thd->mem_root) Item_func_conv_charset(thd, $3, $5); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > diff --git a/sql/simple_tokenizer.h b/sql/simple_tokenizer.h > --- /dev/null > +++ b/sql/simple_tokenizer.h > @@ -0,0 +1,83 @@ > +/* Copyright (c) 2022, MariaDB Corporation. 2023 > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; version 2 of the License. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program; if not, write to the Free Software > + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1335 USA */ > + > +#ifndef SIMPLE_TOKENIZER_INCLUDED > +#define SIMPLE_TOKENIZER_INCLUDED > + > + > +class Simple_tokenizer > +{ > + const char *m_ptr; > + const char *m_end; > +public: > + Simple_tokenizer(const char *str, size_t length) > + :m_ptr(str), m_end(str + length) > + { } > + const char *ptr() const > + { > + return m_ptr; > + } > + bool eof() const > + { > + return m_ptr >= m_end; > + } > + void get_spaces() > + { > + for ( ; !eof(); m_ptr++) > + { > + if (m_ptr[0] != ' ') > + break; > + } > + } > + bool is_ident_start(char ch) const > + { > + return (ch >= 'a' && ch <= 'z') || > + (ch >= 'A' && ch <= 'Z') || > + ch == '_'; > + } > + bool is_ident_body(char ch) const > + { > + return is_ident_start(ch) || > + (ch >= '0' && ch <= '9'); > + } > + bool is_ident_start() const > + { > + return !eof() && is_ident_start(*m_ptr); > + } > + bool is_ident_body() const > + { > + return !eof() && is_ident_body(*m_ptr); > + } > + LEX_CSTRING get_ident() > + { > + if (!is_ident_start()) > + return {m_ptr,0}; > + const char *start= m_ptr++; > + for ( ; is_ident_body(); m_ptr++) > + { } > + LEX_CSTRING res= {start, (size_t) (m_ptr - start)}; > + return res; > + } > + bool get_char(char ch) > + { > + if (eof() || *m_ptr != ch) > + return true; > + m_ptr++; > + return false; > + } > +}; that's too big and complex, but also not complex enough. note that we have quite a few of those already, in patricular in typelib.c, in strfunc.cc, item_func.cc (find_in_set) etc. and this one could've been 2-3 times smaller for @@character_set_collations only, and it still cannot replace any other parsers from above. I'd keep just two methods: get_token() - which skips whitespaces and returns the next substring of non-white-space and not ',' or '=' characters. multibyte-safe. And get_char, which returns the next non-white-space character. this parser could be used in all use cases from above. > + > + > +#endif // SIMPLE_TOKENIZER_INCLUDED > diff --git a/sql/charset_collations.h b/sql/charset_collations.h > --- /dev/null > +++ b/sql/charset_collations.h > @@ -0,0 +1,265 @@ > +/* Copyright (c) 2023, MariaDB Corporation. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; version 2 of the License. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program; if not, write to the Free Software > + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1335 USA */ > + > +#ifndef LEX_CHARSET_COLLATIONS_INCLUDED > +#define LEX_CHARSET_COLLATIONS_INCLUDED > + > +struct Charset_collation_map_st > +{ > +public: > + > + class Used > + { > + public: > + enum map_used_t > + { > + USED_NONE= 0, > + USED_COMPILED_COLLATION= 1 << 0, > + USED_MAPPED_COLLATION= 1 << 1 > + }; > + protected: > + map_used_t m_used; > + public: > + Used() > + :m_used(USED_NONE) > + { } > + void add(map_used_t flag) > + { > + m_used= (map_used_t) ((uint) m_used | (uint) flag); > + } > + }; > + > + struct Elem_st > + { > + protected: > + CHARSET_INFO *m_charset; > + CHARSET_INFO *m_collation; What is the difference between charset and collation? I was under impression that any CHARSET_INFO describes a collation, there is no special structure just for a charset. Am I wrong? > + static size_t print_lex_string(char *dst, const LEX_CSTRING &str) > + { > + memcpy(dst, str.str, str.length); > + return str.length; > + } > + public: > + /* > + Size in text format: 'utf8mb4=utf8mb4_unicode_ai_ci' > + */ > + static constexpr size_t text_size_max() > + { > + return MY_CS_CHARACTER_SET_NAME_SIZE + 1 + > + MY_CS_COLLATION_NAME_SIZE; > + } > + CHARSET_INFO *charset() const > + { > + return m_charset; > + } > + CHARSET_INFO *collation() const > + { > + return m_collation; > + } > + void set_collation(CHARSET_INFO *cl) > + { > + m_collation= cl; > + } > + size_t print(char *dst) const > + { > + const char *dst0= dst; > + dst+= print_lex_string(dst, m_charset->cs_name); > + *dst++= '='; > + dst+= print_lex_string(dst, m_collation->coll_name); > + return (size_t) (dst - dst0); > + } > + int cmp_by_charset_id(const Elem_st &rhs) const > + { > + return m_charset->number < rhs.m_charset->number ? -1 : > + m_charset->number > rhs.m_charset->number ? +1 : 0; > + } > + }; > + class Elem: public Elem_st > + { > + public: > + Elem(CHARSET_INFO *charset, CHARSET_INFO *collation) > + { > + m_charset= charset; > + m_collation= collation; > + } > + }; > +protected: > + Elem_st m_element[8]; // Should be enough for now > + uint m_count; > + uint m_version; > + > + static int cmp_by_charset_id(const void *a, const void *b) > + { > + return static_cast<const Elem_st*>(a)-> > + cmp_by_charset_id(*static_cast<const Elem_st*>(b)); > + } > + > + void sort() > + { > + qsort(m_element, m_count, sizeof(Elem_st), cmp_by_charset_id); > + } > + > + const Elem_st *find_elem_by_charset_id(uint id) const > + { > + if (!m_count) > + return NULL; > + int first= 0, last= ((int) m_count) - 1; > + for ( ; first <= last; ) > + { > + const int middle= (first + last) / 2; > + DBUG_ASSERT(middle >= 0); > + DBUG_ASSERT(middle < (int) m_count); > + const uint middle_id= m_element[middle].charset()->number; > + if (middle_id == id) > + return &m_element[middle]; > + if (middle_id < id) > + first= middle + 1; > + else > + last= middle - 1; > + } > + return NULL; > + } > + > + bool insert(const Elem_st &elem) > + { > + DBUG_ASSERT(elem.charset()->state & MY_CS_PRIMARY); > + if (m_count >= array_elements(m_element)) > + return true; > + m_element[m_count]= elem; > + m_count++; > + sort(); > + return false; > + } > + > + bool insert_or_replace(const Elem_st &elem) > + { > + DBUG_ASSERT(elem.charset()->state & MY_CS_PRIMARY); > + const Elem_st *found= find_elem_by_charset_id(elem.charset()->number); > + if (found) > + { > + const_cast<Elem_st*>(found)->set_collation(elem.collation()); > + return false; > + } > + return insert(elem); > + } > + > +public: > + void init() > + { > + m_count= 0; > + m_version= 0; > + } > + uint count() const > + { > + return m_count; > + } > + uint version() const > + { > + return m_version; > + } > + void set(const Charset_collation_map_st &rhs, uint version_increment) > + { > + uint version= m_version; > + *this= rhs; > + m_version= version + version_increment; > + } > + const Elem_st & operator[](uint pos) const > + { > + DBUG_ASSERT(pos < m_count); > + return m_element[pos]; > + } > + bool insert_or_replace(const class Lex_exact_charset &cs, > + const class Lex_extended_collation &cl, > + bool error_on_conflicting_duplicate); > + CHARSET_INFO *get_collation_for_charset(Used *used, > + CHARSET_INFO *cs) const > + { > + DBUG_ASSERT(cs->state & MY_CS_PRIMARY); > + const Elem_st *elem= find_elem_by_charset_id(cs->number); > + if (elem) > + { > + used->add(Used::USED_MAPPED_COLLATION); > + return elem->collation(); > + } > + used->add(Used::USED_COMPILED_COLLATION); > + return cs; > + } > + size_t text_format_nbytes_needed() const > + { > + return (Elem_st::text_size_max() + 1/* for ',' */) * m_count; > + } > + size_t print(char *dst, size_t nbytes_available) const > + { > + const char *dst0= dst; > + const char *end= dst + nbytes_available; > + for (uint i= 0; i < m_count; i++) > + { > + if (Elem_st::text_size_max() + 1/* for ',' */ > (size_t) (end - dst)) > + break; > + if (i > 0) > + *dst++= ','; > + dst+= m_element[i].print(dst); > + } > + return dst - dst0; > + } > + static constexpr size_t binary_size_max() > + { > + return 1/*count*/ + 4 * array_elements(m_element); > + } > + size_t to_binary(char *dst) const > + { > + const char *dst0= dst; > + *dst++= (char) (uchar) m_count; > + for (uint i= 0; i < m_count; i++) > + { > + int2store(dst, (uint16) m_element[i].charset()->number); > + dst+= 2; > + int2store(dst, (uint16) m_element[i].collation()->number); > + dst+= 2; > + } > + return (size_t) (dst - dst0); > + } > + size_t from_binary(const char *src, size_t srclen) > + { > + const char *src0= src; > + init(); > + if (!srclen) > + return 0; // Empty > + uint count= (uchar) *src++; > + if (srclen < 1 + 4 * count) > + return 0; > + for (uint i= 0; i < count; i++, src+= 4) > + { > + CHARSET_INFO *cs, *cl; > + if (!(cs= get_charset(uint2korr(src), MYF(0))) || > + !(cl= get_charset(uint2korr(src + 2), MYF(0)))) > + { > + /* > + Unpacking from binary format happens on the slave side. > + If for some reasons the slave does not know about a > + character set or a collation, just skip the pair here. > + This pair might not even be needed. > + */ > + continue; > + } > + insert_or_replace(Elem(cs, cl)); > + } > + return src - src0; > + } > + bool from_text(const LEX_CSTRING &str, myf utf8_flag); > +}; > + > + > +#endif // LEX_CHARSET_COLLATIONS_INCLUDED Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org [View Less]

2 3

Re: d8e04ef367b: MDEV-25237 Assertion `global_system_variables.session_track_system_variables' failed in Session_sysvars_tracker::init | SIGSEGV's in __strlen_avx2 | UBSAN: runtime error: null pointer passed as argument 1, which is declared to never be null in my_strdup
by Sergei Golubchik 07 Jul '23

07 Jul '23

Hi, Oleksandr, On Jul 07, Oleksandr Byelkin wrote: > revision-id: d8e04ef367b (mariadb-10.4.30-9-gd8e04ef367b) > parent(s): 423c28f0aa4 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2023-07-03 17:50:49 +0200 > message: > > MDEV-25237 Assertion `global_system_variables.session_track_system_variables' failed in Session_sysvars_tracker::init | SIGSEGV's in __strlen_avx2 | UBSAN: runtime error: null pointer passed as argument 1, which is declared … [View More]

1 0

Re: d23555b1920: MDEV-30984 Online ALTER table is denied with non-informative error messages
by Sergei Golubchik 06 Jul '23

06 Jul '23

Hi, Nikita, On Jun 24, Nikita Malyavin wrote: > revision-id: d23555b1920 (mariadb-11.0.1-138-gd23555b1920) > parent(s): df90bf34644 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-06-20 16:10:48 +0300 > message: > > MDEV-30984 Online ALTER table is denied with non-informative error messages > > diff --git a/mysql-test/main/alter_table_online.result b/mysql-test/main/alter_table_online.result > --- a/mysql-test/main/alter_table_online.… [View More]result > +++ b/mysql-test/main/alter_table_online.result > @@ -179,25 +179,27 @@ t CREATE TABLE `t` ( > UNIQUE KEY `b` (`b`) > ) ENGINE=InnoDB DEFAULT CHARSET=latin1 COLLATE=latin1_swedish_ci > alter table t drop b, change c c serial, algorithm=copy, lock=none; > -ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > +ERROR 0A000: LOCK=NONE is not supported. Reason: CHANGE COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED > # Check existed unique keys. > create or replace table t(a int, b int not null, c int not null, d int); > # No unique in the old table; > alter table t add unique(b, c), modify d int auto_increment, add key(d), > algorithm=copy, lock=none; > -ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > +ERROR 0A000: LOCK=NONE is not supported. Reason: CHANGE COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED > alter table t add unique(a, b); > # Unique in the old table has nulls; > alter table t modify d int auto_increment, add key(d), > algorithm=copy, lock=none; > -ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > +ERROR 0A000: LOCK=NONE is not supported. Reason: CHANGE COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED > alter table t add unique(b, c); > # Change unique'scolumn; Typo > -alter table t change b x int, modify d int auto_increment, add key(d), > +alter table t change b x bigint, modify d int auto_increment, add key(d), > algorithm=copy, lock=none; > -ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > -# Finally good. > -alter table t modify d int auto_increment, add key(d), > +ERROR 0A000: LOCK=NONE is not supported. Reason: CHANGE COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED > +# Finally good. Simple renames with a type unchenged will not affect typo s/unchenged/unchanged/ > +# the result. Also NOT NULL -> NULL transform is fine. > +alter table t modify d int auto_increment, add key(d), > +change b x int null, > algorithm=copy, lock=none; > drop table t; > # MDEV-31172 Server crash or ASAN errors in online_alter_check_autoinc > diff --git a/sql/sql_class.h b/sql/sql_class.h > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -1228,6 +1228,15 @@ class Query_arena > { return strdup_root(mem_root,str); } > inline char *strmake(const char *str, size_t size) const > { return strmake_root(mem_root,str,size); } > + inline LEX_CSTRING strcat(const LEX_CSTRING &a, const LEX_CSTRING &b) const > + { > + char *buf= (char*)alloc(a.length + b.length + 1); > + if (unlikely(!buf)) > + return null_clex_str; > + strncpy(buf, a.str, a.length); memcpy both ↑ and ↓ (and then you might need to append \0 explicitly) > + strncpy(buf + a.length, b.str, b.length + 1); > + return {buf, a.length + b.length}; > + } > inline void *memdup(const void *str, size_t size) const > { return memdup_root(mem_root,str,size); } > inline void *memdup_w_gap(const void *str, size_t size, size_t gap) const > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -9869,26 +9869,21 @@ bool online_alter_check_autoinc(const THD *thd, const Alter_info *alter_info, > */ > for (uint k= 0; k < table->s->keys; k++) > { > - const KEY &key= table->s->key_info[k]; > + const KEY &key= table->key_info[k]; This is generally incorrect. User specified keys are in table->s->key_info[] Keys in table->key_info[] can be modified to match what indexes are actually created, in particular, for long uniques table->key_info[] differs from table->s->key_info[] > if ((key.flags & HA_NOSAME) == 0 || key.flags & HA_NULL_PART_KEY) > continue; > bool key_parts_good= true; > for (uint kp= 0; kp < key.user_defined_key_parts && key_parts_good; kp++) > { > - const char *field_name= key.key_part[kp].field->field_name.str; > - for (const auto &c: alter_info->drop_list) > - if (c.type == Alter_drop::COLUMN > - && my_strcasecmp(system_charset_info, c.name, field_name) == 0) > - { > - key_parts_good= false; > - break; > - } > + const Field *f= key.key_part[kp].field; > + // tmp_set contains dropped fields after mysql_prepare_alter_table > + key_parts_good= !bitmap_is_set(&table->tmp_set, f->field_index); > + > if (key_parts_good) > for (const auto &c: alter_info->create_list) > - if (c.change.str && my_strcasecmp(system_charset_info, c.change.str, > - field_name) == 0) > + if (c.field == f) > { > - key_parts_good= false; > + key_parts_good= f->is_equal(c); > break; > } > } > @@ -9896,25 +9891,63 @@ bool online_alter_check_autoinc(const THD *thd, const Alter_info *alter_info, > return true; > } > > - const char *old_autoinc= table->found_next_number_field > - ? table->found_next_number_field->field_name.str > - : ""; > - bool online= true; > for (const auto &c: alter_info->create_list) > { > - if (c.change.str && c.flags & AUTO_INCREMENT_FLAG) > + if (c.flags & AUTO_INCREMENT_FLAG) > { > - if (my_strcasecmp(system_charset_info, c.change.str, old_autoinc) != 0) > + if (c.field && !(c.field->flags & AUTO_INCREMENT_FLAG)) > + return false; > + break; > + } > + } > + return true; > +} > + > +static > +const char *online_alter_check_supported(const THD *thd, > + const Alter_info *alter_info, > + const TABLE *table, bool *online) > { > - if (c.create_if_not_exists // check IF EXISTS option > - && table->find_field_by_name(&c.change) == NULL) > - continue; > - online= false; > + DBUG_ASSERT(!table->s->tmp_table); that's confusing. *online can be false here already, then return "DROP SYSTEM VERSIONING" will be wrong and you use some other condition to decide whether "DROP SYSTEM VERSIONING" is correct or not. it'd be much clearer not to call online_alter_check_supported in this case. and add here DBUG_ASSERT(*online); > + > + *online= *online && (alter_info->flags & ALTER_DROP_SYSTEM_VERSIONING) == 0; and then you won't need to use && above, just as you don't use it below > + if (!*online) > + return "DROP SYSTEM VERSIONING"; > + > + *online= !thd->lex->ignore; > + if (!*online) > + return "ALTER IGNORE TABLE"; > + > + *online= !table->versioned(VERS_TRX_ID); > + if (!*online) > + return "BIGINT GENERATED ALWAYS AS ROW_START"; > + > + List<FOREIGN_KEY_INFO> fk_list; > + table->file->get_foreign_key_list(thd, &fk_list); > + for (auto &fk: fk_list) > + { > + if (fk_modifies_child(fk.delete_method) || > + fk_modifies_child(fk.update_method)) > + { > + *online= false; > + // Don't fall to a common unsupported case to avoid heavy string ops. > + if (alter_info->requested_lock == Alter_info::ALTER_TABLE_LOCK_NONE) > + { > + return fk_modifies_child(fk.delete_method) > + ? thd->strcat({STRING_WITH_LEN("ON DELETE ")}, > + *fk_option_name(fk.delete_method)).str > + : thd->strcat({STRING_WITH_LEN("ON UPDATE ")}, > + *fk_option_name(fk.update_method)).str; > } > - break; > + return NULL; > } > } > - return online; > + > + *online= online_alter_check_autoinc(thd, alter_info, table); > + if (!*online) > + return "CHANGE COLUMN ... AUTO_INCREMENT"; > + > + return NULL; > } > > > @@ -10979,14 +10994,14 @@ do_continue:; > > if (!table->s->tmp_table) > { > - // COPY algorithm doesn't work with concurrent writes. > + auto *reason= online_alter_check_supported(thd, alter_info, table, &online); > + // COPY algorithm works with concurrent writes only when online is true. as I wrote above, better put it inside if (online) { > if (!online && > alter_info->requested_lock == Alter_info::ALTER_TABLE_LOCK_NONE) > { > + DBUG_ASSERT(reason); > my_error(ER_ALTER_OPERATION_NOT_SUPPORTED_REASON, MYF(0), > - "LOCK=NONE", > - ER_THD(thd, ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_COPY), > - "LOCK=SHARED"); > + "LOCK=NONE", reason, "LOCK=SHARED"); > goto err_new_table_cleanup; > } > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org [View Less]

3 7

[MariaDB developers]Re: a2e71bc8e89: MDEV-22979 MDEV-31400 Fixing spider init bugs
by Yuchen Pei 03 Jul '23

03 Jul '23

Hi Sergei, Thanks for the comments. I have addressed them and the url of the new patch can be found in the new request for review comment in the ticket. On Mon 2023-06-12 18:07:03 +0200, Sergei Golubchik wrote: > Hi, Yuchen, > > See comments inline. Here I'm only reviewing MDEV-31400, > please, tell me if you'd like me to review spider part too > Given that Alexey (cc'd) has already been working on the review of the spider init bug fix, I think it makes sense to ask Alexey to … [View More]continue on the spider part, which I just did on MDEV-22979. Given that the spider part requires the patch for MDEV-31400 to work, I have the spider part on top of the init dependency part. > On Jun 12, Yuchen Pei wrote: >> revision-id: a2e71bc8e89 (mariadb-10.9.5-19-ga2e71bc8e89) >> parent(s): cb0e0c915f6 >> author: Yuchen Pei >> committer: Yuchen Pei >> timestamp: 2023-06-08 16:13:56 +1000 >> message: >> >> MDEV-22979 MDEV-31400 Fixing spider init bugs > > please, make a separate commit for MDEV-31400 Done. > >> diff --git a/sql/handler.cc b/sql/handler.cc >> index a12e9ea18f5..60080f1da6a 100644 >> --- a/sql/handler.cc >> +++ b/sql/handler.cc >> @@ -646,10 +648,14 @@ int ha_initialize_handlerton(st_plugin_int *plugin) >> hton->slot= HA_SLOT_UNDEF; >> /* Historical Requirement */ >> plugin->data= hton; // shortcut for the future >> - if (plugin->plugin->init && plugin->plugin->init(hton)) >> + if (plugin->plugin->init && (ret= plugin->plugin->init(hton))) >> { >> - sql_print_error("Plugin '%s' init function returned error.", >> - plugin->name.str); >> + if (unlikely(ret == -1)) >> + sql_print_warning("Plugin '%s' init function returned error but >> may be retried.", >> + plugin->name.str); >> + else >> + sql_print_error("Plugin '%s' init function returned error.", >> + plugin->name.str); > > let's treat all plugins identically. Meaning, in particular, no warnings > or errors in ha_initialize_handlerton(). They belong to sql_plugin.cc > Done. >> goto err; >> } >> >> diff --git a/sql/mysqld.cc b/sql/mysqld.cc >> index 32392ab882e..caf85736770 100644 >> --- a/sql/mysqld.cc >> +++ b/sql/mysqld.cc >> @@ -365,6 +365,7 @@ bool opt_disable_networking=0, opt_skip_show_db=0; >> bool opt_skip_name_resolve=0; >> my_bool opt_character_set_client_handshake= 1; >> bool opt_endinfo, using_udf_functions; >> +bool udf_initialized= 0; > > I don't think you need that. You can use exising mysqld_server_started. > That won't work, because mysqld_server_started will only be turned on very late, towards the end of mysqld_main(). If spider is initialised between udf_init() and mysqld_server_started is 1 (for example in an init-file), then the udf will fail. See https://github.com/MariaDB/server/commit/91176a7be7c where the test udf_mysql_func_early fails with "query 'SELECT SPIDER_DIRECT_SQL('select * from tbl_a', 'results', 'srv "s_2_1", database "auto_test_remote"')' failed: ER_SP_DOES_NOT_EXIST (1305): FUNCTION auto_test_local.SPIDER_DIRECT_SQL does not exist" because of this. Now, I assume MDEV-31401, the task to include the udf insertion trick in early calls CREATE FUNCTION, will fix this issue without introduce any new global booleans because it should has access to the internal variable `initialized`, though I do not know how complex or doable this task is. So I guess there's a trade-off here between fixing the spider init bugs now and investigating MDEV-31401. Would you like me to explore MDEV-31401 first? >> my_bool locked_in_memory; >> bool opt_using_transactions; >> bool volatile abort_loop; >> diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc >> index 5a077a934ac..e564ab5a38d 100644 >> --- a/sql/sql_plugin.cc >> +++ b/sql/sql_plugin.cc >> @@ -1462,10 +1462,16 @@ static int plugin_initialize(MEM_ROOT *tmp_root, struct st_plugin_int *plugin, >> >> if (plugin_type_initialize[plugin->plugin->type]) >> { >> - if ((*plugin_type_initialize[plugin->plugin->type])(plugin)) >> + ret= (*plugin_type_initialize[plugin->plugin->type])(plugin); >> + if (ret) >> { >> - sql_print_error("Plugin '%s' registration as a %s failed.", >> - plugin->name.str, plugin_type_names[plugin->plugin->type].str); >> + /* Plugin init failed but requested a retry if possible */ >> + if (unlikely(ret == -1)) >> + sql_print_warning("Plugin '%s' registration as a %s failed but may be retried.", >> + plugin->name.str, plugin_type_names[plugin->plugin->type].str); >> + else >> + sql_print_error("Plugin '%s' registration as a %s failed.", >> + plugin->name.str, plugin_type_names[plugin->plugin->type].str); > > 1. "may be" is confusing, we should be less vague in the error > messages Done. I have expanded it a bit more to the following but we still really use "will be" here without either losing accuracy or being overly verbose. Even if it fails and returns the retry code, the retry will not happen if no new plugins are successfully initialised this round. --8<---------------cut here---------------start------------->8--- sql_print_warning("Plugin '%s' registration as a %s failed and may " "be retried provided it is being loaded with " "plugin-load[-add]", plugin->name.str, plugin_type_names[plugin->plugin->type].str); --8<---------------cut here---------------end--------------->8--- > > 2. about (ret == -1). I looked at what plugin init functions return now: > 33 plugins don't have an init function > 76 plugins always return 0 > 13 plugins return 0 or 1 > 3 plugins return 0 or -1 > 3 plugins return 0 or HA_ERR_INITIALIZATION, HA_ERR_OUT_OF_MEM > and innodb inconsistently return 0, HA_ERR_xxx as above, or 1 > > so, I suggest to introduce, like, HA_ERR_DEPENDENCIES (or whatever) > and only retry plugins that return that specific error code. > Done. The HA_ERR_ namespace is tightly packed and bounded by HA_ERR_FIRST (120) and HA_ERR_LAST (198), and I crammed in a #define HA_ERR_RETRY_INIT 129. >> goto err; >> } >> } >> @@ -1462,8 +1462,14 @@ static int plugin_initialize(MEM_ROOT *tmp_root, struct st_plugin_int *plugin, >> >> if (plugin_type_initialize[plugin->plugin->type]) >> { >> - if ((*plugin_type_initialize[plugin->plugin->type])(plugin)) >> + ret= (*plugin_type_initialize[plugin->plugin->type])(plugin); > > there's a second branch below, with plugin->plugin->init(), > please, handle it too. May be, do as with deinit? > > plugin_type_init deinit= plugin_type_deinitialize[plugin->plugin->type]; > if (!deinit) > deinit= (plugin_type_init)(plugin->plugin->deinit); > > if (deinit && deinit(plugin)) > > Less code duplication here. > Done. >> + if (ret) >> { >> + /* Plugin init failed but requested a retry if possible */ >> + if (unlikely(ret == -1)) >> + sql_print_warning("Plugin '%s' registration as a %s failed but may be retried.", >> + plugin->name.str, plugin_type_names[plugin->plugin->type].str); >> + else >> sql_print_error("Plugin '%s' registration as a %s failed.", >> plugin->name.str, plugin_type_names[plugin->plugin->type].str); >> goto err; >> @@ -1737,11 +1743,34 @@ int plugin_init(int *argc, char **argv, int flags) >> */ >> >> mysql_mutex_lock(&LOCK_plugin); >> + /* List of plugins to reap */ >> reap= (st_plugin_int **) my_alloca((plugin_array.elements+1) * sizeof(void*)); >> *(reap++)= NULL; >> + /* List of plugins to retry */ >> + retry= (st_plugin_int **) my_alloca((plugin_array.elements+1) * sizeof(void*)); >> + *(retry++)= NULL; >> >> for(;;) >> { >> + /* Number of plugins that is successfully initialised in a round */ >> + int num_initialized; >> + do >> + { >> + num_initialized= 0; >> + /* If any plugins failed and requested a retry, clean up before >> + retry */ >> + while ((plugin_ptr= *(--retry))) >> + { >> + mysql_mutex_unlock(&LOCK_plugin); >> + plugin_deinitialize(plugin_ptr, true); >> + mysql_mutex_lock(&LOCK_plugin); >> + /** Needed to satisfy assertions in `test_plugin_options()` */ > > Hmm, I don't think it'll work. I mean, test_plugin_options() - as far as > I remember - will remove processed options from the command line array, > meaning you cannot call it twice for the same plugin. > > May be it'd be better to repeat only plugin init function call and not > the complete initialize/deinitialize cycle? Done. I separated out a plugin_do_initialize() function that skips the test_options() part and largely mirrors the plugin_deinitialize() function, and calls this function instead of the full plugin_initialize() during a retry. I also made sure the code path outside of plugin_init() does not change, i.e. if plugin_initialize() is called from an INSTALL statement. The contract of init and deinit is not clear, but I suppose it is a reasonable expectation that if a plugin init wants to be retried, it will clean up before returning init instead of relying the server to call the deinit before retrying. > >> + my_afree(plugin_ptr->ptr_backup); >> + plugin_ptr->ptr_backup= NULL; >> + plugin_ptr->nbackups= 0; >> + plugin_ptr->state= PLUGIN_IS_UNINITIALIZED; >> + } >> + retry++; >> for (i=0; i < MYSQL_MAX_PLUGIN_TYPE_NUM; i++) >> { >> HASH *hash= plugin_hash + plugin_type_initialization_order[i]; > > Regards, > Sergei > VP of MariaDB Server Engineering > and security(a)mariadb.org Best, Yuchen [View Less]

2 3