- 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 log InnoDB information about deadlocks encountered. This way, when it fails the next time in buildbot we can see 1) what kind of InnoDB deadlock is triggering the problem, if any; and 2) if it actually gets a deadlock inside InnoDB 10 times in a row, or if it is something else. - Kristian. ----------------------------------------------------------------------- Author: Kristian Nielsen <knielsen(a)knielsen-hq.org> Date: Sun Jul 9 15:18:03 2023 +0200 MDEV-28776: rpl.rpl_mark_optimize_tbl_ddl fails with timeout on sync_with_master This commit just extends the testcase to include some more information when the test failure occurs. The failure has so far only been reproducible in buildbot runs. Signed-off-by: Kristian Nielsen <knielsen(a)knielsen-hq.org> ----------------------------------------------------------------------- diff --git a/mysql-test/suite/rpl/t/rpl_mark_optimize_tbl_ddl.opt b/mysql-test/suite/rpl/t/rpl_mark_optimize_tbl_ddl.opt new file mode 100644 index 00000000000..3c53d5257b4 --- /dev/null +++ b/mysql-test/suite/rpl/t/rpl_mark_optimize_tbl_ddl.opt @@ -0,0 +1 @@ +--innodb-print-all-deadlocks=1

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.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

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 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. > > [...] >

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 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

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 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

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 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

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 > --- /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

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 to never be null in my_strdup first, if you see a bug with a very long summary, particularly if it's 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" > --- a/sql/sys_vars.inl > +++ b/sql/sys_vars.inl > @@ -620,7 +620,12 @@ class Sys_var_sesvartrack: public Sys_var_charptr_base > { > if (sysvartrack_global_update(thd, new_val, > var->save_result.string_value.length)) > - new_val= 0; > + { > + if (new_val) > + my_free(new_val); > + // keep the old value in case of error > + new_val= (char*)my_strdup(global_var(const char*), MYF(MY_WME)); > + } No, this is wrong. Well, not wrong, but an incorrect bug fix. update should not fail, this is the contract. check() is supposed to verify that the value is valid, so basically update must succeed. technically it can fail because of OOM, I suppose, so restoring the value here might be needed still. But in the case of OOM, strdup() might be not such a great idea. Perhaps you can make sure that OOM is impossible and update truly cannot fail? Anyway, the actual fix for MDEV-25237 is to make check fail on an invalid value. In fact, it was a simple typo: @@ -221,7 +221,7 @@ bool sysvartrack_validate_value(THD *thd, const char *str, > /* 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; > } > 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

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.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

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 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

2 3

Re: 8b5de389ab1: MDEV-31400 Simple plugin dependency resolution
by Sergei Golubchik 03 Jul '23

03 Jul '23

Hi, Yuchen, On Jul 01, Yuchen Pei wrote: > revision-id: 8b5de389ab1 (mariadb-10.9.5-17-g8b5de389ab1) > parent(s): d8997f875e2 > author: Yuchen Pei > committer: Yuchen Pei > timestamp: 2023-06-13 20:08:28 +1000 > message: > > MDEV-31400 Simple plugin dependency resolution > > We introduce simple plugin dependency. A plugin init function may > return HA_ERR_RETRY_INIT. If this happens during server startup when > the server is trying to initialise all plugins, the failed plugins > will be retried, until no more plugins succeed in initialisation or > want to be retried. > > This will fix spider init bugs which is caused in part by its > dependency on Aria for initialisation. > > The reason we need a new return code, instead of treating every > failure as a request for retry, is that it may be impossible to clean > up after a failed plugin initialisation. Take InnoDB for example, it > has a global variable `buf_page_cleaner_is_active`, which may not > satisfy an assertion during a second initialisation try, probably > because InnoDB does not expect the initialisation to be called > twice. A test that may fail because of this is > `encryption.corrupted_during_recovery`, see for example[1], which is > tested at 73835f64b7fc245d38812380685aca03bef72bb5, a previous commit again, commits that you are going to push into the main branche should not refer to commits that you aren't going to push. these are dangling references leading nowhere. > where we retry every failed plugin. > > [1] https://buildbot.mariadb.org/#/builders/369/builds/10107/steps/7/logs/stdio nor references to buildbot logs, they're very short lived, while I can see commits in the server git repo going to 2000. > > diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc > index 5a077a934ac..92d74aa51e8 100644 > --- a/sql/sql_plugin.cc > +++ b/sql/sql_plugin.cc > @@ -1435,6 +1435,49 @@ 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 and may " please, don't write "may". It's saying "eh, I don't know, you, user, figure it out". But you should know better than the user. So either say "will be retried" or "won't be retried" (in which case, you can just use the error message below and not mention retry at all). > + "be retried provided it is being loaded with " > + "plugin-load[-add]", > + 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) > @@ -1737,32 +1738,71 @@ 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))) > + plugin_ptr->state= PLUGIN_IS_TO_BE_RETRIED; Why do you need a special state for that? You haven't fixed the SHOW PLUGINS command to show it correctly. And I didn't check other conditions, may be some of them need to take the new state into account too, I don't know. Wouldn't it be easier to have a special retry look after the main for() without all its complexity and without a new state. Like (pseudocode, and note that here I've changed the creative trick of walking retry[] backwards until NULL, so now it iterated forward until end): while (retry_start < retry_end) { retry_ptr= retry= retry_start; while ((plugin_ptr= *(retry++))) { mysql_mutex_unlock(&LOCK_plugin); error= plugin_do_initialize(plugin_ptr, state); mysql_mutex_lock(&LOCK_plugin); if (error == HA_ERR_RETRY_INIT) *(retry_ptr++)= plugin_ptr; else if (error) *(reap++)= plugin_ptr; } if (retry == retry_ptr) while (retry_ptr > retry_start) *(reap++)= *(--retry_ptr); retry_end= retry_ptr; } > + retry++; > for (i=0; i < MYSQL_MAX_PLUGIN_TYPE_NUM; i++) > { > HASH *hash= plugin_hash + plugin_type_initialization_order[i]; > for (uint idx= 0; idx < hash->records; idx++) > { > plugin_ptr= (struct st_plugin_int *) my_hash_element(hash, idx); > + if (plugin_ptr->state == PLUGIN_IS_UNINITIALIZED || > + plugin_ptr->state == PLUGIN_IS_TO_BE_RETRIED) > + { > + int error; > if (plugin_ptr->state == PLUGIN_IS_UNINITIALIZED) > { > 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)) > + error= plugin_initialize(&tmp_root, plugin_ptr, argc, argv, > + opts_only); > + } else > { > + uint state= plugin_ptr->state; > + mysql_mutex_unlock(&LOCK_plugin); > + error= plugin_do_initialize(plugin_ptr, state); > + mysql_mutex_lock(&LOCK_plugin); > + plugin_ptr->state= state; > + } > + if (error) > + { > plugin_ptr->state= PLUGIN_IS_DYING; > + /* The plugin wants a retry of the initialisation, > + possibly due to dependency on other plugins */ > + if (unlikely(error == HA_ERR_RETRY_INIT)) > + *(retry++)= plugin_ptr; > + else > *(reap++)= plugin_ptr; > } > + else > + num_initialized++; > } > } > } > + /* Only retry if at least one plugin has been initialised > + successfully and at least one has requested a retry during this > + round */ > + } while (num_initialized > 0 && *(retry - 1)); > > /* load and init plugins from the plugin table (unless done already) */ > if (flags & PLUGIN_INIT_SKIP_PLUGIN_TABLE) > @@ -1775,8 +1815,11 @@ 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 > */ > + while ((plugin_ptr= *(--retry))) > + *(reap++) = plugin_ptr; I already have that in my pseudocode above > while ((plugin_ptr= *(--reap))) > { > mysql_mutex_unlock(&LOCK_plugin); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Numeric datatype with large decimal values
by brajmohan saxena 23 Jun '23

23 Jun '23

Hi Folks, Do we have Or someone already working on creating a Numeric (decimal) data type plugin in MariaDB, Which supports large decimal digits and may be upto infinite. It would be great if someone already has some pointers on it. And let me know the best approach to achieve this type of data type implementation in MariaDB. Thanks in advance. Thanks Braj mohan saxena

1 0

Re: f3aa4d0a215: MDEV-29447 MDEV-26285 MDEV-31338 Refactor spider_db_mbase_util::open_item_func
by Sergei Golubchik 22 Jun '23

22 Jun '23

Hi, Yuchen, On Jun 20, Yuchen Pei wrote: > revision-id: f3aa4d0a215 (mariadb-10.4.30-4-gf3aa4d0a215) > parent(s): f5dceafd0ba > author: Yuchen Pei > committer: Yuchen Pei > timestamp: 2023-06-14 15:02:38 +1000 > message: > > MDEV-29447 MDEV-26285 MDEV-31338 Refactor spider_db_mbase_util::open_item_func > > spider_db_mbase_util::open_item_func() is a monster function. > It is difficult to maintain while it is expected that we need to > modify it when a new SQL function or a new func_type is added. > > We split the function into two distinct functions: one handles the > case of str != NULL and the other handles the case of str == NULL. > > This refactoring was done in a conservative way because we do not > have comprehensive tests on the function. > > It also fixes MDEV-29447 and MDEV-31338 where field items that are > arguments of a func item may be used before created / initialised. > > Note this commit is a port of > 3836098c29ef1b7ff9d5fbde99b690eab73a0df1 (MDEV-26285) to current > versions 10.3+. As I wrote in the first review, please, don't mention transient commit hashes in a permanent commit comment. This commit will, supposedly, be pushed into the main branch. While 3836098c29ef1 will, on the other hand, disappear, as it's not in any of the main branches. You'll have a dangling pointer > Signed-off-by: Yuchen Pei <yuchen.pei(a)mariadb.com> also, it looks strange to sign-off your own commits > > diff --git a/storage/spider/spd_db_mysql.cc b/storage/spider/spd_db_mysql.cc > index a51c1497b02..1ed733105c6 100644 > --- a/storage/spider/spd_db_mysql.cc > +++ b/storage/spider/spd_db_mysql.cc > @@ -5848,94 +5992,12 @@ int spider_db_mbase_util::open_item_func( > { > if (!strncasecmp("utc_timestamp", func_name, func_name_length)) > { > - if (str) > str->length(str->length() - SPIDER_SQL_OPEN_PAREN_LEN); > DBUG_RETURN(spider_db_open_item_string(item_func, NULL, spider, str, > alias, alias_length, dbton_id, use_fields, fields)); > } else if (!strncasecmp("timestampdiff", func_name, func_name_length)) > { > -#ifdef ITEM_FUNC_TIMESTAMPDIFF_ARE_PUBLIC Why did you removed this and not the #else part? > - Item_func_timestamp_diff *item_func_timestamp_diff = > - (Item_func_timestamp_diff *) item_func; > - if (str) > - { > - const char *interval_str; > - uint interval_len; > - switch (item_func_timestamp_diff->int_type) > - { > - case INTERVAL_YEAR: > - interval_str = SPIDER_SQL_YEAR_STR; > - interval_len = SPIDER_SQL_YEAR_LEN; > - break; > - case INTERVAL_QUARTER: > - interval_str = SPIDER_SQL_QUARTER_STR; > - interval_len = SPIDER_SQL_QUARTER_LEN; > - break; > - case INTERVAL_MONTH: > - interval_str = SPIDER_SQL_MONTH_STR; > - interval_len = SPIDER_SQL_MONTH_LEN; > - break; > - case INTERVAL_WEEK: > - interval_str = SPIDER_SQL_WEEK_STR; > - interval_len = SPIDER_SQL_WEEK_LEN; > - break; > - case INTERVAL_DAY: > - interval_str = SPIDER_SQL_DAY_STR; > - interval_len = SPIDER_SQL_DAY_LEN; > - break; > - case INTERVAL_HOUR: > - interval_str = SPIDER_SQL_HOUR_STR; > - interval_len = SPIDER_SQL_HOUR_LEN; > - break; > - case INTERVAL_MINUTE: > - interval_str = SPIDER_SQL_MINUTE_STR; > - interval_len = SPIDER_SQL_MINUTE_LEN; > - break; > - case INTERVAL_SECOND: > - interval_str = SPIDER_SQL_SECOND_STR; > - interval_len = SPIDER_SQL_SECOND_LEN; > - break; > - case INTERVAL_MICROSECOND: > - interval_str = SPIDER_SQL_MICROSECOND_STR; > - interval_len = SPIDER_SQL_MICROSECOND_LEN; > - break; > - default: > - interval_str = ""; > - interval_len = 0; > - break; > - } > - str->length(str->length() - SPIDER_SQL_OPEN_PAREN_LEN); > - if (str->reserve(func_name_length + SPIDER_SQL_OPEN_PAREN_LEN + > - interval_len + SPIDER_SQL_COMMA_LEN)) > - DBUG_RETURN(HA_ERR_OUT_OF_MEM); > - str->q_append(func_name, func_name_length); > - str->q_append(SPIDER_SQL_OPEN_PAREN_STR, SPIDER_SQL_OPEN_PAREN_LEN); > - str->q_append(interval_str, interval_len); > - str->q_append(SPIDER_SQL_COMMA_STR, SPIDER_SQL_COMMA_LEN); > - } > - if ((error_num = spider_db_print_item_type(item_list[0], NULL, spider, > - str, alias, alias_length, dbton_id, use_fields, fields))) > - DBUG_RETURN(error_num); > - if (str) > - { > - if (str->reserve(SPIDER_SQL_COMMA_LEN)) > - DBUG_RETURN(HA_ERR_OUT_OF_MEM); > - str->q_append(SPIDER_SQL_COMMA_STR, SPIDER_SQL_COMMA_LEN); > - } > - if ((error_num = spider_db_print_item_type(item_list[1], NULL, spider, > - str, alias, alias_length, dbton_id, use_fields, fields))) > - DBUG_RETURN(error_num); > - if (str) > - { > - if (str->reserve(SPIDER_SQL_CLOSE_PAREN_LEN)) > - DBUG_RETURN(HA_ERR_OUT_OF_MEM); > - str->q_append(SPIDER_SQL_CLOSE_PAREN_STR, > - SPIDER_SQL_CLOSE_PAREN_LEN); > - } > - DBUG_RETURN(0); > -#else > DBUG_RETURN(ER_SPIDER_COND_SKIP_NUM); > -#endif > } > } else if (func_name_length == 14) > { Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

DBA in Tokyo
by James Tobin 20 Jun '23

20 Jun '23

Hello, I'm working with an employer that is looking to hire a DBA in Tokyo. Consequently, I had hoped some members of this list may like to discuss. I can be reached using "JamesBTobin (at) Gmail (dot) Com". Kind regards, James

1 0

[Maria-developers] Review input for: MDEV-22534 Decorrelate IN subquery
by Sergey Petrunia 01 Jun '23

01 Jun '23

Hi Yuchen, Please find below review input for > commit ba9cf8efeb0b1e1c132d565adb79c98156783f15 > Author: Yuchen Pei <yuchen.pei(a)mariadb.com> > Date: Fri May 19 20:06:31 2023 +1000 > > MDEV-22534 Decorrelate IN subquery > First, general questions: Q1: It seems there is some overlap between Item_in_subselect::exists2in_processor() and Item_exists_subselect::exists2in_processor() Did you consider factoring out common code rather than copying it? (if yes, I'd like to know why it couldn't be done). Q2: Where is the coe that removes the correlated equalities from the subquery's WHERE? (I assume Item_exists_subselect code does it?) > diff --git a/sql/item_subselect.cc b/sql/item_subselect.cc > index 9e6c205ca76..8d893794da3 100644 > --- a/sql/item_subselect.cc > +++ b/sql/item_subselect.cc > @@ -3099,6 +3099,132 @@ static bool find_inner_outer_equalities(Item **conds, > return TRUE; > } > > +/* Checks whether item tree intersects with the free list */ > +static bool intersects_free_list(Item *item, THD *thd) > +{ > + for (const Item *to_find= thd->free_list; to_find; to_find= to_find->next) > + if (item->walk(&Item::find_item_processor, 1, (void *) to_find)) > + return true; > + return false; > +} > + Please try moving this function into a separate file. First name that came to my mind: opt_subq_decorrelate.cc Feel free to suggest better names. The idea is that we should aim for better structure. (This request is only valid if the factoring out suggested above doesn't work out). > +/* De-correlate where conditions in an IN subquery */ This needs a bigger comment describing what the rewrite does. > +bool Item_in_subselect::exists2in_processor(void *opt_arg) > +{ > + THD *thd= (THD *)opt_arg; > + SELECT_LEX *first_select= unit->first_select(); > + JOIN *join= first_select->join; > + bool will_be_correlated; > + Dynamic_array<EQ_FIELD_OUTER> eqs(PSI_INSTRUMENT_MEM, 5, 5); > + List<Item> outer; > + Query_arena *arena= NULL, backup; > + int res= FALSE; > + DBUG_ENTER("Item_in_subselect::exists2in_processor"); > + Please move comments out of the if statement and number the conditions. For examples, see best_access_path(), large if-statement starting with "Don't test table scan if it can't be better." or "EXISTS-to-IN coversion and ORDER BY ... LIMIT clause" in Item_exists_subselect::exists2in_processor(). > + if (!optimizer_flag(thd, OPTIMIZER_SWITCH_EXISTS_TO_IN) || > + /* proceed only if I'm a toplevel IN or a toplevel NOT IN */ > + !(is_top_level_item() || > + (upper_not && upper_not->is_top_level_item())) || what is the reason behind this condition? If we're able to handle top-level NOT IN, why can't we handle subquery in any context? Can we really handle the NOT IN? (I suspect not, it will suffer from the semantics of IN subqueries with NULLs.. I can't find the name for this, it's described e.g. here: https://petrunia.net/2006/11/16/inany-subqueries-null-woes/ ) > + first_select->is_part_of_union() || /* skip if part of a union */ > + first_select->group_list.elements || /* skip if with group by */ > + first_select->with_sum_func || /* skip if aggregation */ > + join->having || /* skip if with having */ > + !join->conds || /* skip if no conds */ > + /* skip if left expr is a single nonscalar subselect */ > + (left_expr->type() == Item::SUBSELECT_ITEM && > + !left_expr->type_handler()->is_scalar_type())) The above seems to be some exotic SQL. Is it something like (select col1,lol2 from t1) IN (select col3,col4 from t2 where ..) and does MariaDB really support this? Please add a comment about this. > + DBUG_RETURN(FALSE); > + /* iterate over conditions, and check whether they can be moved out. */ > + if (find_inner_outer_equalities(&join->conds, eqs)) > + DBUG_RETURN(FALSE); > + /* If we are in the execution of a prepared statement, check for > + intersection with the free list to avoid segfault. Note that the > + check for prepared statement execution is necessary, otherwise it > + will likely always find intersection thus abort the > + transformation. > + > + fixme: this only works when HAVE_PSI_STATEMENT_INTERFACE is > + defined. It causes CI's like amd64-debian-10-debug-embedded to > + fail. Are there other ways to find out we are in the execution of a > + prepared statement? */ I'm looking at the code of activate_stmt_arena_if_needed(). It seems "stmt_arena->is_conventional()" is the check you're looking for. > + if (thd->m_statement_state.m_parent_prepared_stmt) > + { > + for (uint i= 0; i < (uint) eqs.elements(); i++) > + { > + if (intersects_free_list(*eqs.at(i).eq_ref, thd)) > + DBUG_RETURN(FALSE); This check doesn't look good. As far as I understand, it is a temporary measure until related re-execution bug(s) are fixed by Igor (and/or Sanja)? > + } > + } > + /* Determines whether the result will be correlated */ > + { > + List<Item> unused; > + Collect_deps_prm prm= {&unused, // parameters > + unit->first_select()->nest_level_base, // nest_level_base > + 0, // count > + unit->first_select()->nest_level, // nest_level > + FALSE // collect > + }; > + walk(&Item::collect_outer_ref_processor, TRUE, &prm); > + DBUG_ASSERT(prm.count > 0); > + DBUG_ASSERT(prm.count >= (uint)eqs.elements()); > + will_be_correlated= prm.count > (uint)eqs.elements(); See the example pasted in the MDEV: https://jira.mariadb.org/browse/MDEV-22534?focusedCommentId=259705&page=com… Somehow, I get here will_be_correlated==FALSE but Materialization is still not considered for the subquery? > + } > + /* Back up the free list */ > + arena= thd->activate_stmt_arena_if_needed(&backup); > + /* Construct the items for left_expr */ > + if (left_expr->type() == Item::ROW_ITEM) > + for (uint i= 0; i < left_expr->cols(); i++) > + outer.push_back(left_expr->element_index(i)); > + else > + outer.push_back(left_expr); > + const uint offset= first_select->item_list.elements; > + /* Move items to outer and select item list */ > + for (uint i= 0; i < (uint)eqs.elements(); i++) > + { > + Item **eq_ref= eqs.at(i).eq_ref; > + Item_ident *local_field= eqs.at(i).local_field; > + Item *outer_exp= eqs.at(i).outer_exp; > + first_select->item_list.push_back(local_field, thd->mem_root); > + first_select->ref_pointer_array[offset + i]= (Item *)local_field; How do we know that ref_pointer_array has enough room for the new elements? (We can discuss this with Sanja, he's an expert on the topic). I see this check in Item_exists_subselect::exists2in_processor if ((uint)eqs.elements() > (first_select->item_list.elements + first_select->select_n_reserved)) Is it how Item_exists_subselect handles this issue? > + outer.push_back(outer_exp); > + *eq_ref= new (thd->mem_root) Item_int(thd, 1); > + if((*eq_ref)->fix_fields(thd, (Item **)eq_ref)) > + { > + res= TRUE; > + goto out; > + } > + } > + /* Update the left_expr */ > + left_expr= new (thd->mem_root) Item_row(thd, outer); > + if (left_expr->fix_fields(thd, &left_expr)) > + { > + res= TRUE; > + goto out; > + } > + left_expr_orig= left_expr; > + is_correlated= will_be_correlated; > + /* Update any Item_in_optimizer wrapper if exists */ > + if (optimizer) > + { > + optimizer->reset_cache(); > + if (optimizer->fix_left(thd)) > + { > + res= TRUE; > + goto out; > + } > + } > + { > + OPT_TRACE_TRANSFORM(thd, trace_wrapper, trace_transform, > + get_select_lex()->select_number, "IN (SELECT)", > + "decorrelation"); > + } > +out: > + /* Restores the free list */ > + if (arena) > + thd->restore_active_arena(arena, &backup); > + DBUG_RETURN(res); > +} On Mon, Dec 20, 2021 at 09:29:47PM +0300, Sergey Petrunia wrote: > Hello Igor, > > I'm trying examples with this patch: > > create table t1 (a int not null, key(a)); > insert into t1 select seq from seq_1_to_1000; > > First, I debug this example: > > explain select * from t1 where a <10 or a>=10; > > The execution enters key_or(), passes through the new code this patch has added > and key_or() returns NULL. Good. > > But if I modify the example slightly: > > explain select * from t1 where a <20 or a>=10 ; > > Here, key_or() still returns a SEL_ARG object that represents an infinite > range: > > (gdb) fini > Run till exit from #0 key_or (...) > 0x0000555555f1fca2 in tree_or (...) > Value returned is $24 = (SEL_ARG *) 0x7fff70064d88 > (gdb) p dbug_print_sel_arg($24) > $26 = 0x7fff70024390 "SEL_ARG(-inf<=a<=+inf)" > > Should this case be fixed as well? > > On Fri, Dec 17, 2021 at 02:11:40PM -0800, IgorBabaev wrote: > > revision-id: 677643a80986107491b7886441f2828384f0494b (mariadb-10.2.31-1286-g677643a) > > parent(s): 8bb55633699612279744c055e22eeca8d4058273 > > author: Igor Babaev > > committer: Igor Babaev > > timestamp: 2021-12-17 14:11:39 -0800 > > message: > > > > MDEV-27262 Unexpected index intersection with full index scan for an index > > > > If when extracting a range condition foran index from the WHERE condition > > I think the comment wording could be improved also. > > > Range Optimizer sees that the range condition covers the whole index then > > such condition should be discarded because cannot it be used in any range > > scan. In some cases Range Optimizer really does it, but there remained some > > conditions for which it was not done. As a result the optimizer could > > produce index merge plans with the full index scan for one of the indexes > > participating in the index merge. > > This could be observed in one of the test cases from index_merge1.inc > > where a plan with index_merge_sort_union was produced and in the test case > > reported for this bug where a plan with index_merge_sort_intersect was > > produced. In both cases one of two index scans participating in index merge > > ran over the whole index. > > The patch slightly changes the original above mentioned test case from > > index_merge1.inc to be able to produce an intended plan employing > > index_merge_sort_union. The original query was left to show that index > > merge is not used for it anymore. > > It should be noted that for the plan with index_merge_sort_intersect could > > be chosen for execution only due to a defect in the InnoDB code that > > returns wrong estimates for the cardinality of big ranges. > > > > This bug led to serious problems in 10.4+ where the optimization using > > Rowid filters is employed (see mdev-26446). > > > > Approved by Oleksandr Byelkin <sanja(a)mariadb.com> > > > > --- > > mysql-test/include/index_merge1.inc | 8 +++- > > mysql-test/r/index_merge_myisam.result | 12 +++-- > > mysql-test/r/range_innodb.result | 81 ++++++++++++++++++++++++++++++++++ > > mysql-test/t/range_innodb.test | 78 ++++++++++++++++++++++++++++++++ > > sql/opt_range.cc | 7 +++ > > 5 files changed, 181 insertions(+), 5 deletions(-) > > > > diff --git a/mysql-test/include/index_merge1.inc b/mysql-test/include/index_merge1.inc > > index b168a76..440f1f7 100644 > > --- a/mysql-test/include/index_merge1.inc > > +++ b/mysql-test/include/index_merge1.inc > > @@ -150,15 +150,19 @@ explain select * from t0 where > > (((key3 <7 and key7 < 6) or key5 < 2) and (key5 < 5 or key6 < 6)); > > > > explain select * from t0 where > > - ((key3 <5 or key5 < 4) and (key1 < 4 or key2 < 4)) > > + ((key3 < 4 or key5 < 4) and (key1 < 4 or key2 < 4)) > > or > > ((key3 >=5 or key5 < 2) and (key5 < 5 or key6 < 6)); > > > > explain select * from t0 force index(i1, i2, i3, i4, i5, i6 ) where > > - ((key3 <5 or key5 < 4) and (key1 < 4 or key2 < 4)) > > + ((key3 < 4 or key5 < 4) and (key1 < 4 or key2 < 4)) > > or > > ((key3 >=5 or key5 < 2) and (key5 < 5 or key6 < 6)); > > > > +explain select * from t0 force index(i1, i2, i3, i4, i5, i6 ) where > > + ((key3 < 5 or key5 < 4) and (key1 < 4 or key2 < 4)) > > + or > > + ((key3 >=5 or key5 < 2) and (key5 < 5 or key6 < 6)); > > # 8. Verify that "order by" after index merge uses filesort > > select * from t0 where key1 < 5 or key8 < 4 order by key1; > > > > diff --git a/mysql-test/r/index_merge_myisam.result b/mysql-test/r/index_merge_myisam.result > > index 5a23092..a096c34 100644 > > --- a/mysql-test/r/index_merge_myisam.result > > +++ b/mysql-test/r/index_merge_myisam.result > > @@ -173,17 +173,23 @@ or > > id select_type table type possible_keys key key_len ref rows Extra > > 1 SIMPLE t0 index_merge i1,i2,i3,i5,i6,i7 i3,i5 4,4 NULL 11 Using sort_union(i3,i5); Using where > > explain select * from t0 where > > -((key3 <5 or key5 < 4) and (key1 < 4 or key2 < 4)) > > +((key3 < 4 or key5 < 4) and (key1 < 4 or key2 < 4)) > > or > > ((key3 >=5 or key5 < 2) and (key5 < 5 or key6 < 6)); > > id select_type table type possible_keys key key_len ref rows Extra > > 1 SIMPLE t0 ALL i1,i2,i3,i5,i6 NULL NULL NULL 1024 Using where > > explain select * from t0 force index(i1, i2, i3, i4, i5, i6 ) where > > -((key3 <5 or key5 < 4) and (key1 < 4 or key2 < 4)) > > +((key3 < 4 or key5 < 4) and (key1 < 4 or key2 < 4)) > > or > > ((key3 >=5 or key5 < 2) and (key5 < 5 or key6 < 6)); > > id select_type table type possible_keys key key_len ref rows Extra > > -1 SIMPLE t0 index_merge i1,i2,i3,i5,i6 i3,i5 0,4 NULL 1024 Using sort_union(i3,i5); Using where > > +1 SIMPLE t0 index_merge i1,i2,i3,i5,i6 i3,i5 4,4 NULL 1024 Using sort_union(i3,i5); Using where > > +explain select * from t0 force index(i1, i2, i3, i4, i5, i6 ) where > > +((key3 < 5 or key5 < 4) and (key1 < 4 or key2 < 4)) > > +or > > +((key3 >=5 or key5 < 2) and (key5 < 5 or key6 < 6)); > > +id select_type table type possible_keys key key_len ref rows Extra > > +1 SIMPLE t0 ALL i1,i2,i3,i5,i6 NULL NULL NULL 1024 Using where > > select * from t0 where key1 < 5 or key8 < 4 order by key1; > > key1 key2 key3 key4 key5 key6 key7 key8 > > 1 1 1 1 1 1 1 1023 > > diff --git a/mysql-test/r/range_innodb.result b/mysql-test/r/range_innodb.result > > index f2349f2..ccb6da3 100644 > > --- a/mysql-test/r/range_innodb.result > > +++ b/mysql-test/r/range_innodb.result > > @@ -108,3 +108,84 @@ DROP TABLE t0,t1; > > SET @@GLOBAL.debug_dbug = @saved_dbug; > > set @@optimizer_switch= @optimizer_switch_save; > > # End of 10.1 tests > > +# > > +# MDEV-27262: Index intersection with full scan over an index > > +# > > +CREATE TABLE t1 ( > > +id int(10) unsigned NOT NULL AUTO_INCREMENT, > > +p char(32) DEFAULT NULL, > > +es tinyint(3) unsigned NOT NULL DEFAULT 0, > > +er tinyint(3) unsigned NOT NULL DEFAULT 0, > > +x mediumint(8) unsigned NOT NULL DEFAULT 0, > > +PRIMARY KEY (id), > > +INDEX es (es), > > +INDEX x (x), > > +INDEX er (er,x), > > +INDEX p (p) > > +) ENGINE=InnoDB DEFAULT CHARSET=latin1; > > +insert into t1(es,er) select 0, 1 from seq_1_to_45; > > +insert into t1(es,er) select 0, 2 from seq_1_to_49; > > +insert into t1(es,er) select 0, 3 from seq_1_to_951; > > +insert into t1(es,er) select 0, 3 from seq_1_to_1054; > > +insert into t1(es,er) select 0, 6 from seq_1_to_25; > > +insert into t1(es,er) select 0, 11 from seq_1_to_1; > > +insert into t1(es,er) select 1, 1 from seq_1_to_45; > > +insert into t1(es,er) select 1, 2 from seq_1_to_16; > > +insert into t1(es,er) select 1, 3 from seq_1_to_511; > > +insert into t1(es,er) select 1, 4 from seq_1_to_687; > > +insert into t1(es,er) select 1, 6 from seq_1_to_50; > > +insert into t1(es,er) select 1, 7 from seq_1_to_4; > > +insert into t1(es,er) select 1, 11 from seq_1_to_1; > > +insert into t1(es,er) select 2, 1 from seq_1_to_82; > > +insert into t1(es,er) select 2, 2 from seq_1_to_82; > > +insert into t1(es,er) select 2, 3 from seq_1_to_1626; > > +insert into t1(es,er) select 2, 4 from seq_1_to_977; > > +insert into t1(es,er) select 2, 6 from seq_1_to_33; > > +insert into t1(es,er) select 2, 11 from seq_1_to_1; > > +insert into t1(es,er) select 3, 1 from seq_1_to_245; > > +insert into t1(es,er) select 3, 2 from seq_1_to_81; > > +insert into t1(es,er) select 3, 3 from seq_1_to_852; > > +insert into t1(es,er) select 3, 4 from seq_1_to_2243; > > +insert into t1(es,er) select 3, 6 from seq_1_to_44; > > +insert into t1(es,er) select 3, 11 from seq_1_to_1; > > +insert into t1(es,er) select 4, 1 from seq_1_to_91; > > +insert into t1(es,er) select 4, 2 from seq_1_to_83; > > +insert into t1(es,er) select 4, 3 from seq_1_to_297; > > +insert into t1(es,er) select 4, 4 from seq_1_to_2456; > > +insert into t1(es,er) select 4, 6 from seq_1_to_19; > > +insert into t1(es,er) select 4, 11 from seq_1_to_1; > > +update t1 set p='foobar'; > > +update t1 set x=0; > > +set @save_isp=@@innodb_stats_persistent; > > +set global innodb_stats_persistent= 1; > > +analyze table t1; > > +Table Op Msg_type Msg_text > > +test.t1 analyze status OK > > +set optimizer_switch='index_merge_sort_intersection=on'; > > +SELECT * FROM t1 > > +WHERE ((p = 'foo' AND er != 4) OR er = 4 ) AND (es >= 4) LIMIT 2; > > +id p es er x > > +14645 foobar 4 4 0 > > +14646 foobar 4 4 0 > > +EXPLAIN EXTENDED SELECT * FROM t1 > > +WHERE ((p = 'foo' AND er != 4) OR er = 4 ) AND (es >= 4) LIMIT 2; > > +id select_type table type possible_keys key key_len ref rows filtered Extra > > +1 SIMPLE t1 range es,er,p es 1 NULL # 100.00 Using index condition; Using where > > +Warnings: > > +Note 1003 select `test`.`t1`.`id` AS `id`,`test`.`t1`.`p` AS `p`,`test`.`t1`.`es` AS `es`,`test`.`t1`.`er` AS `er`,`test`.`t1`.`x` AS `x` from `test`.`t1` where (`test`.`t1`.`p` = 'foo' and `test`.`t1`.`er` <> 4 or `test`.`t1`.`er` = 4) and `test`.`t1`.`es` >= 4 limit 2 > > +set optimizer_switch='index_merge_sort_intersection=off'; > > +SELECT * FROM t1 > > +WHERE ((p = 'foo' AND er != 4) OR er = 4 ) AND (es >= 4) LIMIT 2; > > +id p es er x > > +14645 foobar 4 4 0 > > +14646 foobar 4 4 0 > > +EXPLAIN EXTENDED SELECT * FROM t1 > > +WHERE ((p = 'foo' AND er != 4) OR er = 4 ) AND (es >= 4) LIMIT 2; > > +id select_type table type possible_keys key key_len ref rows filtered Extra > > +1 SIMPLE t1 range es,er,p es 1 NULL # 100.00 Using index condition; Using where > > +Warnings: > > +Note 1003 select `test`.`t1`.`id` AS `id`,`test`.`t1`.`p` AS `p`,`test`.`t1`.`es` AS `es`,`test`.`t1`.`er` AS `er`,`test`.`t1`.`x` AS `x` from `test`.`t1` where (`test`.`t1`.`p` = 'foo' and `test`.`t1`.`er` <> 4 or `test`.`t1`.`er` = 4) and `test`.`t1`.`es` >= 4 limit 2 > > +set optimizer_switch='index_merge_sort_intersection=default'; > > +set global innodb_stats_persistent= @save_isp; > > +DROP TABLE t1; > > +# End of 10.2 tests > > diff --git a/mysql-test/t/range_innodb.test b/mysql-test/t/range_innodb.test > > index 428e5c2..7420e72 100644 > > --- a/mysql-test/t/range_innodb.test > > +++ b/mysql-test/t/range_innodb.test > > @@ -116,3 +116,81 @@ SET @@GLOBAL.debug_dbug = @saved_dbug; > > set @@optimizer_switch= @optimizer_switch_save; > > > > --echo # End of 10.1 tests > > + > > +--echo # > > +--echo # MDEV-27262: Index intersection with full scan over an index > > +--echo # > > + > > +--source include/have_sequence.inc > > + > > +CREATE TABLE t1 ( > > + id int(10) unsigned NOT NULL AUTO_INCREMENT, > > + p char(32) DEFAULT NULL, > > + es tinyint(3) unsigned NOT NULL DEFAULT 0, > > + er tinyint(3) unsigned NOT NULL DEFAULT 0, > > + x mediumint(8) unsigned NOT NULL DEFAULT 0, > > + PRIMARY KEY (id), > > + INDEX es (es), > > + INDEX x (x), > > + INDEX er (er,x), > > + INDEX p (p) > > +) ENGINE=InnoDB DEFAULT CHARSET=latin1; > > + > > +insert into t1(es,er) select 0, 1 from seq_1_to_45; > > +insert into t1(es,er) select 0, 2 from seq_1_to_49; > > +insert into t1(es,er) select 0, 3 from seq_1_to_951; > > +insert into t1(es,er) select 0, 3 from seq_1_to_1054; > > +insert into t1(es,er) select 0, 6 from seq_1_to_25; > > +insert into t1(es,er) select 0, 11 from seq_1_to_1; > > +insert into t1(es,er) select 1, 1 from seq_1_to_45; > > +insert into t1(es,er) select 1, 2 from seq_1_to_16; > > +insert into t1(es,er) select 1, 3 from seq_1_to_511; > > +insert into t1(es,er) select 1, 4 from seq_1_to_687; > > +insert into t1(es,er) select 1, 6 from seq_1_to_50; > > +insert into t1(es,er) select 1, 7 from seq_1_to_4; > > +insert into t1(es,er) select 1, 11 from seq_1_to_1; > > +insert into t1(es,er) select 2, 1 from seq_1_to_82; > > +insert into t1(es,er) select 2, 2 from seq_1_to_82; > > +insert into t1(es,er) select 2, 3 from seq_1_to_1626; > > +insert into t1(es,er) select 2, 4 from seq_1_to_977; > > +insert into t1(es,er) select 2, 6 from seq_1_to_33; > > +insert into t1(es,er) select 2, 11 from seq_1_to_1; > > +insert into t1(es,er) select 3, 1 from seq_1_to_245; > > +insert into t1(es,er) select 3, 2 from seq_1_to_81; > > +insert into t1(es,er) select 3, 3 from seq_1_to_852; > > +insert into t1(es,er) select 3, 4 from seq_1_to_2243; > > +insert into t1(es,er) select 3, 6 from seq_1_to_44; > > +insert into t1(es,er) select 3, 11 from seq_1_to_1; > > +insert into t1(es,er) select 4, 1 from seq_1_to_91; > > +insert into t1(es,er) select 4, 2 from seq_1_to_83; > > +insert into t1(es,er) select 4, 3 from seq_1_to_297; > > +insert into t1(es,er) select 4, 4 from seq_1_to_2456; > > +insert into t1(es,er) select 4, 6 from seq_1_to_19; > > +insert into t1(es,er) select 4, 11 from seq_1_to_1; > > +update t1 set p='foobar'; > > +update t1 set x=0; > > +set @save_isp=@@innodb_stats_persistent; > > +set global innodb_stats_persistent= 1; > > +analyze table t1; > > + > > +let $q= > > +SELECT * FROM t1 > > + WHERE ((p = 'foo' AND er != 4) OR er = 4 ) AND (es >= 4) LIMIT 2; > > + > > +set optimizer_switch='index_merge_sort_intersection=on'; > > +eval $q; > > +--replace_column 9 # > > +eval EXPLAIN EXTENDED $q; > > + > > +set optimizer_switch='index_merge_sort_intersection=off'; > > +# execution of $q and explain for it led to an assertion failure in 10.4 > > +# (with the optimizer switch rowid_filter set to 'on') > > +eval $q; > > +--replace_column 9 # > > +eval EXPLAIN EXTENDED $q; > > +set optimizer_switch='index_merge_sort_intersection=default'; > > + > > +set global innodb_stats_persistent= @save_isp; > > +DROP TABLE t1; > > + > > +--echo # End of 10.2 tests > > diff --git a/sql/opt_range.cc b/sql/opt_range.cc > > index f3f1843..2e05b88 100644 > > --- a/sql/opt_range.cc > > +++ b/sql/opt_range.cc > > @@ -9413,6 +9413,13 @@ key_or(RANGE_OPT_PARAM *param, SEL_ARG *key1,SEL_ARG *key2) > > key2->copy_min(tmp); > > if (!(key1=key1->tree_delete(tmp))) > > { // Only one key in tree > > + if (key2->min_flag & NO_MIN_RANGE && > > + key2->max_flag & NO_MAX_RANGE) > > + { > > + if (key2->maybe_flag) > > + return new SEL_ARG(SEL_ARG::MAYBE_KEY); > > + return 0; // Always true OR > > + } > > key1=key2; > > key1->make_root(); > > key2=key2_next; > > BR > Sergei > -- > Sergei Petrunia, Software Developer > MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net > > -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

2 3

Re: [Maria-developers] c457f237511: MDEV-30984 Online ALTER table is denied with non-informative error messages
by Sergei Golubchik 29 May '23

29 May '23

Hi, Nikita, comments below refer to a combined diff of both commits On May 23, Nikita Malyavin wrote: > revision-id: c457f237511 (mariadb-11.0.1-124-gc457f237511) > parent(s): f4b04ec534d > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-05-15 21:07:50 +0300 > message: > > MDEV-30984 Online ALTER table is denied with non-informative error messages Two main thoughts. First, users will generally don't do show warnings after an error - mariadb already puts some useful info into a warning after an error in some (few) cases, and it's always a surprise when I write in a bug report "use SHOW WARNINGS". It's not very intuitive. And in your case it's even unnecessary, because your error message is LOCK=NONE is not supported. Reason: %s. Try LOCK=SHARED and "Reason" part is perfectly suited for an explanation that you generate, instead of a misleading "COPY algorithm requires a lock" (because it doesn't). Second thought, your messages are hard-coded and won't be translated even if the user will configure the server to use, say, spanish or Japanese error messages. See how it was solved for "COPY algorithm requires a lock", ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_COPY. But I think the message will still be comprehensible if you'll simply print the offending sql clause as the reason, this won't need any localization. See below where I've shown what your error messages would look like if you do that. If you think that's too cryptic, then, please, use the ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_COPY approach. Two more comments at the very end but I'll repeat them here, because they're easy to overlook: AUTO_INCREMENT instead of AUTOINC, and a test for DROP SYSTEM VERSIONING. > diff --git a/mysql-test/main/alter_table_online.result b/mysql-test/main/alter_table_online.result > index 8b726ccb591..bcf7afea9df 100644 > --- a/mysql-test/main/alter_table_online.result > +++ b/mysql-test/main/alter_table_online.result > @@ -4,6 +4,10 @@ > create table t (a int); > alter ignore table t add primary key (a), 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: ALTER IGNORE. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 ALTER IGNORE TABLE is incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > drop table t; > # > # MDEV-28771 Assertion `table->in_use&&tdc->flushed' failed after ALTER > @@ -92,12 +96,20 @@ on update cascade) engine=InnoDB; > insert into t2 values (1),(2),(3); > alter table t2 add c int, 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: FOREIGN KEY ... ON UPDATE CASCADE. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 Tables with CASCADE/SET NULL foreign keys are incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > alter table t2 add c int, algorithm=inplace, lock=none; > create or replace table t2 (b int, foreign key (b) > references t1 (a) > on delete set null) engine=InnoDB; > alter table t2 add c int, 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: FOREIGN KEY ... ON DELETE SET NULL. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 Tables with CASCADE/SET NULL foreign keys are incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > alter table t2 add c int, algorithm=inplace, lock=none; > create or replace table t2 (b int, foreign key (b) > references t1 (a) > @@ -117,6 +129,10 @@ b int references t1 (b) on update cascade) engine=InnoDB; > insert into t2 values (1, 1),(2, 2); > alter table t2 add c int, 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: FOREIGN KEY ... ON DELETE SET NULL. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 Tables with CASCADE/SET NULL foreign keys are incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > alter table t2 add c int, algorithm=copy; > alter table t2 add d int, algorithm=inplace; > drop table t2, t1; > @@ -132,6 +148,10 @@ period for system_time (row_start, row_end)) > engine=innodb with system versioning; > alter table t1 add c int, 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: BIGINT GENERATED ALWAYS AS ROW_START. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 Transaction-versioned tables are incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > alter table t1 add c int, algorithm=inplace; > alter table t1 add d int, lock=none; > set system_versioning_alter_history= default; > @@ -143,6 +163,10 @@ drop table t1; > create table t (a serial, b int) engine=innodb; > alter table t drop a, modify b 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: MODIFY COLUMN ... SERIAL. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > set statement sql_mode= NO_AUTO_VALUE_ON_ZERO for > alter table t drop a, modify b serial, algorithm=copy, lock=none; > create or replace table t (a serial, b int) engine=innodb; > @@ -157,6 +181,10 @@ t CREATE TABLE `t` ( > # Only unsafe approach is fine because of possible collisions. > alter table t modify a int, modify b 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: MODIFY COLUMN ... SERIAL. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > # > # Check that we treat autoinc columns correctly modify old autoinc is > # fine, adding new autoinc for existed column is unsafe. > @@ -180,22 +208,38 @@ t CREATE TABLE `t` ( > ) 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: MODIFY COLUMN ... SERIAL. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. 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: MODIFY COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. 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: MODIFY COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > alter table t add unique(b, c); > # Change unique'scolumn; > alter table t change b x int, 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: MODIFY COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 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), > algorithm=copy, lock=none; > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 99570008ddb..22447db42af 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -9916,6 +9916,67 @@ bool online_alter_check_autoinc(const THD *thd, const Alter_info *alter_info, > return online; > } > > +bool online_alter_is_supported(THD *thd, const Alter_info *alter_info, > + const TABLE *table) > +{ > + const char *reason= NULL; > + List<FOREIGN_KEY_INFO> fk_list; > + > + if (alter_info->flags & ALTER_DROP_SYSTEM_VERSIONING) > + { > + reason= "DROP SYSTEM VERSIONING is"; no test for that > + goto unsupported; > + } > + > + if (thd->lex->ignore) > + { > + reason= "ALTER IGNORE TABLE is"; > + goto unsupported; > + } > + > + if (table->s->tmp_table) > + { > + reason= "Temporary tables are"; > + goto unsupported; > + } > + > + if (table->versioned(VERS_TRX_ID)) > + { > + reason= "Transaction-versioned tables are"; > + goto unsupported; > + } > + > + 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)) > + { > + reason= "Tables with CASCADE/SET NULL foreign keys are"; > + goto unsupported; > + } > + } > + > + if (!online_alter_check_autoinc(thd, alter_info, table)) > + { > + reason= "Adding AUTOINC to an existing column for a table without a " AUTO_INCREMENT, please, don't abbrev user visible messages > + "primary key is"; > + goto unsupported; > + } > + > + return true; > + > +unsupported: > + if (alter_info->requested_lock == Alter_info::ALTER_TABLE_LOCK_NONE > + && alter_info->algorithm(thd) == Alter_info::ALTER_TABLE_ALGORITHM_COPY) > + push_warning_printf(thd, Sql_condition::WARN_LEVEL_NOTE, > + ER_ALTER_OPERATION_NOT_SUPPORTED_REASON, > + "%s incompatible with " > + "LOCK=NONE, ALGORITHM=COPY", > + reason); > + return false; > +} > + > > /** > Alter table Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 4

Re: [Maria-developers] 56e2cd20ed3: MDEV-31033 ER_KEY_NOT_FOUND upon online COPY ALTER on a partitioned table
by Sergei Golubchik 29 May '23

29 May '23

Hi, Nikita, On May 24, Nikita Malyavin wrote: > revision-id: 56e2cd20ed3 (mariadb-11.0.1-114-g56e2cd20ed3) > parent(s): 47f5c7ae7e4 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-05-10 01:49:42 +0300 > message: > > MDEV-31033 ER_KEY_NOT_FOUND upon online COPY ALTER on a partitioned table > > The row events were applied "twice": once for the ha_partition, and one > more time for the underlying storage engine. > > There's no such problem in binlog/rpl, because ha_partiton::row_logging > is normally set to false. > > The fix makes the events replicate only when the handler is a root handler. > We will try to *guess* this by comparing it to table->file. The same > approach is used in the MDEV-21540 fix, 231feabd. The assumption is made, > that the row methods are only called for table->file (and never for a > cloned handler), hence the assertions are added in ha_innobase and > ha_myisam to make sure that this is true at least for those engines > > Also closes MDEV-31040, however the test is not included, since we have no > convenient way to construct a deterministic version. > > diff --git a/sql/handler.cc b/sql/handler.cc > index 3fcbb5171d6..d8aca5bc80b 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -7280,7 +7280,7 @@ int handler::binlog_log_row(const uchar *before_record, > log_func, row_logging_has_trans); > > #ifdef HAVE_REPLICATION > - if (unlikely(!error && table->s->online_alter_binlog)) > + if (unlikely(!error && table->s->online_alter_binlog && is_root_handler())) > error= binlog_log_row_online_alter(table, before_record, after_record, > log_func); > #endif // HAVE_REPLICATION > @@ -7800,7 +7800,7 @@ int handler::ha_write_row(const uchar *buf) > DBUG_RETURN(error); > > /* > - NOTE: this != table->file is true in 3 cases: > + NOTE: is_root_handler() is false in 3 cases: may be move this comment closer to is_root_handler() ? > > 1. under copy_partitions() (REORGANIZE PARTITION): that does not > require long unique check as it does not introduce new rows or new index. > @@ -7810,7 +7810,7 @@ int handler::ha_write_row(const uchar *buf) > 3. under ha_mroonga::wrapper_write_row() > */ > > - if (table->s->long_unique_table && this == table->file) > + if (table->s->long_unique_table && is_root_handler()) > { > DBUG_ASSERT(inited == NONE || lookup_handler != this); > if ((error= check_duplicate_long_entries(buf))) > @@ -7862,13 +7862,13 @@ int handler::ha_update_row(const uchar *old_data, const uchar *new_data) > error= ha_check_overlaps(old_data, new_data); > > /* > - NOTE: this != table->file is true under partition's ha_update_row(): > + NOTE: is_root_handler() is false under partition's ha_update_row(): > check_duplicate_long_entries_update() was already done by > ha_partition::ha_update_row(), no need to check it again for each single > partition. Same applies to ha_mroonga wrapper. > */ > > - if (!error && table->s->long_unique_table && this == table->file) > + if (!error && table->s->long_unique_table && is_root_handler()) > error= check_duplicate_long_entries_update(new_data); > table->status= saved_status; > > @@ -7913,6 +7913,12 @@ int handler::ha_update_row(const uchar *old_data, const uchar *new_data) > return error; > } > > + > +bool handler::is_root_handler() const > +{ > + return this == table->file; > +} better make it inline in handler.h > + > /* > Update first row. Only used by sequence tables > */ > diff --git a/sql/handler.h b/sql/handler.h > index fb0eba83c38..9b903e7ebce 100644 > --- a/sql/handler.h > +++ b/sql/handler.h > @@ -3502,6 +3502,10 @@ class handler :public Sql_alloc > return extra(HA_EXTRA_NO_KEYREAD); > } > > +protected: > + bool is_root_handler() const; > + > +public: > int check_collation_compatibility(); > int check_long_hash_compatibility() const; > int ha_check_for_upgrade(HA_CHECK_OPT *check_opt); > diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc > index f035437b1d7..94d9da3486b 100644 > --- a/storage/innobase/handler/ha_innodb.cc > +++ b/storage/innobase/handler/ha_innodb.cc > @@ -7750,6 +7750,8 @@ ha_innobase::write_row( > > DBUG_ENTER("ha_innobase::write_row"); > > + DBUG_ASSERT(is_root_handler() || table->file->ht != innodb_hton_ptr); instead of putting this assert into every handler's implementation and in many methods, why not to have it in handler ha_ methods or may be even in the is_root_handler itself? You can compare this->ht with table->file->ht. For example, like DBUG_ASSERT(this == table->file || this->ht != table->file->ht); > + > trx_t* trx = thd_to_trx(m_user_thd); > > /* Validation checks before we commence write_row operation. */ > @@ -8479,6 +8481,8 @@ ha_innobase::update_row( > > DBUG_ENTER("ha_innobase::update_row"); > > + DBUG_ASSERT(is_root_handler() || table->file->ht != innodb_hton_ptr); > + > if (is_read_only()) { > DBUG_RETURN(HA_ERR_TABLE_READONLY); > } else if (!trx_is_started(trx)) { > @@ -8653,6 +8657,8 @@ ha_innobase::delete_row( > > DBUG_ENTER("ha_innobase::delete_row"); > > + DBUG_ASSERT(is_root_handler() || table->file->ht != innodb_hton_ptr); > + > if (is_read_only()) { > DBUG_RETURN(HA_ERR_TABLE_READONLY); > } else if (!trx_is_started(trx)) { > diff --git a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc > index 4057c3ea859..b7fadd86a23 100644 > --- a/storage/myisam/ha_myisam.cc > +++ b/storage/myisam/ha_myisam.cc > @@ -983,6 +983,7 @@ int ha_myisam::close(void) > > int ha_myisam::write_row(const uchar *buf) > { > + DBUG_ASSERT(is_root_handler() || table->file->ht != ht); > /* > If we have an auto_increment column and we are writing a changed row > or a new row, then update the auto_increment value in the record. > @@ -1957,11 +1958,13 @@ bool ha_myisam::is_crashed() const > > int ha_myisam::update_row(const uchar *old_data, const uchar *new_data) > { > + DBUG_ASSERT(is_root_handler() || table->file->ht != ht); > return mi_update(file,old_data,new_data); > } > > int ha_myisam::delete_row(const uchar *buf) > { > + DBUG_ASSERT(is_root_handler() || table->file->ht != ht); > return mi_delete(file,buf); > } > > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 5

[Maria-developers] MTR fails on undefined {cpus} array in My/SysInfo.pm on armhf and armel
by Otto Kekäläinen 28 May '23

28 May '23

Hi! I noticed that the autopkgtests on Debian on armhf and armel that run the mariadb-test-run fail on error: starting mysql-test-tun.pl... Logging: ./mysql-test-run.pl --force --testcase-timeout=120 --suite-timeout=540 --retry=3 -- ... Collecting tests... Installing system database... Can't use an undefined value as an ARRAY reference at lib/My/SysInfo.pm line 166. Logs for full context: https://ci.debian.net/data/autopkgtest/unstable/armel/m/mariadb/33870767/lo… https://ci.debian.net/data/autopkgtest/unstable/armhf/m/mariadb/33870768/lo… This line 166 in mysql-test/lib/My/SysInfo.pm has: # Return the number of cpus found sub num_cpus { my ($self)= @_; return int(@{$self->{cpus}}) or confess "INTERNAL ERROR: No cpus in list"; } Can somebody with strong Perl skills help me understand 1) Where this cpus array is inherited from? 2) Why might it have now regressed in MariaDB 10.11.3 and why is this erroring only on armel/armhf but not other architectures? 3) What should I do wrap this section in so that if cpus is not set, it would just default to '1'? - Otto

3 6

Re: [Maria-developers] c75f774d278: MDEV-31058 ER_KEY_NOT_FOUND upon concurrent CHANGE column autoinc and DML
by Sergei Golubchik 27 May '23

27 May '23

Hi, Nikita, On May 24, Nikita Malyavin wrote: > revision-id: c75f774d278 (mariadb-11.0.1-118-gc75f774d278) > parent(s): 319207175bd > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-05-15 19:43:49 +0300 > message: > > MDEV-31058 ER_KEY_NOT_FOUND upon concurrent CHANGE column autoinc and DML > > When column is changed to autoinc, ALTER TABLE may update zero/NULL values, > if NO_AUTO_VALUE_ON_ZERO mode is not enabled. > > Forbid this for LOCK=NONE for the unreliable cases. > The cases are described in online_alter_check_autoinc. just disable online for any ALTER that adds an autoinc attribute to an existing column. See MDEV. > diff --git a/mysql-test/main/alter_table_online.test b/mysql-test/main/alter_table_online.test > index b9e147c812d..7ebe553aabb 100644 > --- a/mysql-test/main/alter_table_online.test > +++ b/mysql-test/main/alter_table_online.test > @@ -135,3 +135,71 @@ alter table t1 add d int, lock=none; > set system_versioning_alter_history= default; > drop table t1; > > +--echo # > +--echo # MDEV-31058 ER_KEY_NOT_FOUND upon concurrent CHANGE column autoinc > +--echo # and DML > +--echo # > +create table t (a serial, b int) engine=innodb; > +--error ER_ALTER_OPERATION_NOT_SUPPORTED_REASON > +alter table t drop a, modify b serial, algorithm=copy, lock=none; > + > +set statement sql_mode= NO_AUTO_VALUE_ON_ZERO for > +alter table t drop a, modify b serial, algorithm=copy, lock=none; I don't think it should work, see my comment in the MDEV > +--echo # Finally good. > +alter table t modify d int auto_increment, add key(d), > + algorithm=copy, lock=none; no, it's not good either, as far as I understand > + > +drop table t; lots of unvisible spaces at line ends, please remove them > + > +--echo # MDEV-31172 Server crash or ASAN errors in online_alter_check_autoinc > +create table t (a int, b int, c char(8), key(a,b,c)); > +alter table t modify c int auto_increment key, algorithm=copy; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

[Maria-developers] Regression in crossbuilding MariaDB
by Otto Kekäläinen 26 May '23

26 May '23

Hello! While working on https://salsa.debian.org/mariadb-team/mariadb-server/-/merge_requests/46 (Import new upstream version 10.11.3 to Debian) I noticed that amd64->arm64 crossbuilding no longer works. The CI shows that Salsa-CI job `test-crossbuild-arm64` no longer passes. Based on Salsa-CI runs I can see that it [still worked in upstream commit e55e761e](https://salsa.debian.org/otto/mariadb-server/-/jobs/4217220) but [failed in upstream commit 06b443be](https://salsa.debian.org/otto/mariadb-server/-/jobs/4217664), thus some of these commits in between regressed it: ``` 06b443be34e Nes CC 3.3 4a668c18926 MDEV-29401 InnoDB history list length increased in 10.6 compared to 10.5 f272463b028 Cleanup of MDEV-14974: --port ignored for --host=localhost bb1d1dc8465 Merge 10.5 into 10.6 c5e50c48bba MDEV-30837: Remove usage of AWK in autobake-debs.sh f99a891858a MDEV-30837: Remove usage of AWK from Debian init and postinst scripts 5740638c4c3 MDEV-31132 Deadlock between DDL and purge of InnoDB history d4265fbde58 MDEV-26055: Correct the formula for adaptive flushing 898320b5f8a MDEV-30804 addendum for 10.6+ branches c22ab93f8af MDEV-26827 fixup: Prevent a hang in LRU eviction 818d5e48146 Merge 10.5 into 10.6 0976afec889 MDEV-31114 Assertion !...is_waiting() failed in os_aio_wait_until_no_pending_writes() 2c567b2fa37 MDEV-30996 insert.. select in presence of full text index freezes all other commits at commit time 51e62cb3b3b MDEV-26782 InnoDB temporary tablespace: reclaiming of free space does not work 204e7225dce Cleanup: MONITOR_EXISTING trx_undo_slots_used, trx_undo_slots_cached 86767bcc0f1 MDEV-29593 Purge misses a chance to free not-yet-reused undo pages 40eff3f8681 MDEV-26827 fixup: hangs and !os_aio_pending_writes() assertion failures ``` Example of error in (cross)build log: ``` Building CXX object CMakeFiles/cmTC_9b8d6.dir/src.cxx.o /usr/lib/ccache/c++ -DHAVE_INCONSISTENT_MISSING_DESTRUCTOR_OVERRIDE -g -O2 -ffile-prefix-map=/builds/otto/mariadb-server/debian/output/source_dir=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -Wdate-time -D_FORTIFY_SOURCE=2 -pie -fPIC -fstack-protector --param=ssp-buffer-size=4 -Wall -Wextra -Woverloaded-virtual -Wconversion -g -Wsuggest-override -Winconsistent-missing-destructor-override -o CMakeFiles/cmTC_9b8d6.dir/src.cxx.o -c /builds/otto/mariadb-server/debian/output/source_dir/builddir-native/CMakeFiles/CMakeScratch/TryCompile-Nn98w6/src.cxx c++: error: unrecognized command-line option '-Winconsistent-missing-destructor-override' gmake[3]: *** [CMakeFiles/cmTC_9b8d6.dir/build.make:78: CMakeFiles/cmTC_9b8d6.dir/src.cxx.o] Error 1 gmake[3]: Leaving directory '/builds/otto/mariadb-server/debian/output/source_dir/builddir-native/CMakeFiles/CMakeScratch/TryCompile-Nn98w6' gmake[2]: *** [Makefile:127: cmTC_9b8d6/fast] Error 2 ``` Is this enough information for somebody to guess what regressed? - Otto

4 6

[Maria-developers] Understanding binlog group commit (MDEV-232, MDEV-532, MDEV-25611, MDEV-18959)
by Marko Mäkelä 22 May '23

22 May '23

Hi Kristian, I am happy to see you being active on the maria-developers list. I don’t think we have collaborated in the past. I have been working on the InnoDB storage engine under MySQL 4.0 through 5.7 (and some 8.0) since 2003, and since 2016 under MariaDB Server, mostly 10.1 and later. I understand that you were an early contributor to MariaDB and implemented some durability I/O optimization between the "distributed transaction" of binlog and the InnoDB write-ahead log (redo log, ib_logfile0). I think that some improvement in this area is long overdue. I find it hard to read the high-level descriptions. Nowhere in https://jira.mariadb.org/browse/MDEV-232 or https://jira.mariadb.org/browse/MDEV-532 I see any mention of any key terminology: * InnoDB log sequence number (LSN): a logical point of time for any persistent InnoDB changes. Measured in bytes. Mini-transactions (atomic changes of InnoDB pages) start and end at some LSN. Corresponds to the Oracle system change number (SCN). * Binlog offset: a logical point of time for any changes at the replication level. Measured in bytes. Replication events such as statements or row-level events start and end at some binlog offset. * The LSN or binlog offset of a COMMIT, XA PREPARE, XA ROLLBACK, or XA COMMIT. There is quite a bit of code in InnoDB that I would like to get rid of. Some of it is related to the data members trx_t::flush_log_later and trx_t::must_flush_log_later. There is also the confusingly named handlerton::commit_checkpoint_request that has nothing to do with log checkpoints, but with some kind of a durability request that fails to identify the minimum logical time up to which everything is desired to be durable. (My talk https://fosdem.org/2022/schedule/event/mariadb_innodb/ explains what a log checkpoint is and how recovery works.) Some years ago, in MDEV-21534 Vladislav Vaintroub introduced write_lock and flush_lock in InnoDB. This works pretty well: a Sysbench workload with innodb_flush_log_at_trx_commit=1 will linearly improve throughput as you add more client connection threads. I think that something similar would be beneficial for the binlog, to keep track of the latest written offset and the latest write offset after which fdatasync() has completed. I am not familiar with the binlog code. A colleague recently claimed to me that LOCK_commit_ordered is a huge bottleneck. As you can see, the write_lock and flush_lock are better because the lock requests include a notion of logical time: If everything up to "my" time has been written or durably written, I do not care to wait. In MDEV-27774 (MariaDB Server 10.8) this was improved further: multiple mini-transactions can copy their local log buffer to the shared log_sys.buf concurrently. It seems to me that the missing tracking of logical time in the data structures is causing hangs like MDEV-25611. I feel that the design is based on a wrong assumption that the binlog-covered transactions are the only source of InnoDB log writes. That has never been the case: For example, there have always been background threads or tasks that purge the history of committed transactions. The task MDEV-18959 (ensuring that one fdatasync() per durable transaction suffices) is also related to this. I think that the simplest solution (assuming that moving the binlog to a bunch of transactional tables is out of the question) is to ensure that one of the logs (binlog and other write-ahead logs) is always ahead. If we choose that the binlog must be ahead, then in InnoDB the write_lock acquisition would have to be preceded by a binlog_write_lock acquisition that will ensure that any relevant changes of persistent state (COMMIT, XA PREPARE, XA ROLLBACK, XA COMMIT) that could be part of the InnoDB log write must have been durably written to the binlog. With best regards, Marko -- Marko Mäkelä, Lead Developer InnoDB MariaDB plc

3 7

Re: [Maria-developers] Understanding binlog group commit (MDEV-232, MDEV-532, MDEV-25611, MDEV-18959)
by Marko Mäkelä 16 May '23

16 May '23

On Tue, May 16, 2023 at 1:37 PM Andrei Elkin <andrei.elkin(a)mariadb.com> wrote: > Can you comment on chances for `flush_lsn` never advance to the value of > `write_lsn` when/because Innodb has nothing to commit (so at such a time > RM (or PURGE or FLUSH LOGS that have been mentioned too) might be run)? If the last write to the log was an InnoDB internal one, it could be done by log_write_up_to(lsn, false). The log_sys.flushed_to_disk_lsn would only be advanced (to somewhere closer to log_sys.lsn) if there was a subsequent call to log_write_up_to(lsn, true). Log checkpoint would advance log_sys.flushed_to_disk_lsn, as should a user transaction commit. This could be a possible explanation. -- Marko Mäkelä, Lead Developer InnoDB MariaDB plc

1 0

Re: [Maria-developers] d67c3f88883: MDEV-27744 InnoDB: Failing assertion: !cursor->index->is_committed() in row0ins.cc (from row_ins_sec_index_entry_by_modify) | Assertion `0' failed in row_upd_sec_index_entry (debug) | Corruption
by Sergei Golubchik 11 May '23

11 May '23

Hi, Alexander, A question: if the frm will *always* be created in the canonical, non-ORACLE mode and always parsed in the same mode, will we even need these schema-qualified native functions? okay, the schema-qualified trick will help to avoid creating REGEX_REPLACE_ORACLE and so on. But is it needed to fix MDEV-27744? More comments below On Apr 27, Alexander Barkov wrote: > revision-id: d67c3f88883 (mariadb-10.3.33-61-gd67c3f88883) > parent(s): 7355f7b1f5c > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-04-22 15:35:16 +0400 > message: > > MDEV-27744 InnoDB: Failing assertion: !cursor->index->is_committed() in row0ins.cc (from row_ins_sec_index_entry_by_modify) | Assertion `0' failed in row_upd_sec_index_entry (debug) | Corruption > > The crash happened with an indexed virtual column whose > value is evaluated using a function that has a different meaning > in sql_mode='' vs sql_mode=ORACLE: ... good comment > diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc > index 59d2ea60715..8e83ee02782 100644 > --- a/sql/sql_partition.cc > +++ b/sql/sql_partition.cc > @@ -2591,11 +2591,9 @@ char *generate_partition_syntax_for_frm(THD *thd, partition_info *part_info, > HA_CREATE_INFO *create_info, > Alter_info *alter_info) > { > - sql_mode_t old_mode= thd->variables.sql_mode; > - thd->variables.sql_mode &= ~MODE_ANSI_QUOTES; > + Sql_mode_save_for_frm_handling sql_mode_save(thd); right. thanks > char *res= generate_partition_syntax(thd, part_info, buf_length, > true, create_info, alter_info); > - thd->variables.sql_mode= old_mode; > return res; > } > > diff --git a/sql/unireg.cc b/sql/unireg.cc > index efbb5e773b4..3b37944afa5 100644 > --- a/sql/unireg.cc > +++ b/sql/unireg.cc > @@ -652,6 +649,7 @@ static bool pack_expression(String *buf, Virtual_column_info *vcol, > static bool pack_vcols(String *buf, List<Create_field> &create_fields, > List<Virtual_column_info> *check_constraint_list) > { > + Sql_mode_save_for_frm_handling sql_mode_save(current_thd); well, it was done above pack_vcols(), because the caller had access to the thd. Why not to keep it that way? > List_iterator<Create_field> it(create_fields); > Create_field *field; > > diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc > index f41414f8ae9..0f5c7519d07 100644 > --- a/sql/item_cmpfunc.cc > +++ b/sql/item_cmpfunc.cc > @@ -3292,9 +3292,24 @@ void Item_func_case_simple::print(String *str, enum_query_type query_type) > } > > > +Item_func_decode_oracle * > +Item_func_decode_oracle::create(THD *thd, const LEX_CSTRING &name, > + List<Item> *item_list) > +{ > + if (unlikely(!item_list || item_list->elements < 3)) > + { > + wrong_param_count_error(oracle_schema_ref.name(), name); > + return NULL; > + } > + return new (thd->mem_root) Item_func_decode_oracle(thd, *item_list); > +} why? items are normally created in the Create_xxx::create* methods, defined in item_create.cc. Why did you delegate that to static methods in Item_func_xxx ? > + > + > void Item_func_decode_oracle::print(String *str, enum_query_type query_type) > { > - str->append(func_name()); > + print_sql_mode_dependent_name(str, query_type, > + oracle_schema_ref, > + Item_func_decode_oracle::func_name()); > str->append('('); > args[0]->print(str, query_type); > for (uint i= 1, count= when_count() ; i <= count; i++) > diff --git a/sql/mysqld.h b/sql/mysqld.h > index d40e1d170d0..54069c3cd5a 100644 > --- a/sql/mysqld.h > +++ b/sql/mysqld.h > @@ -741,12 +741,25 @@ enum enum_query_type > QT_ITEM_SUBSELECT_ID_ONLY, > > QT_SHOW_SELECT_NUMBER= (1<<10), > + > + //QT_ITEM_IDENT_DISABLE_DB_TABLE_NAMES= (1 <<11), -- this is taken in 10.5 > + > + /// Print sql_mode-dependent functions with the schema qualifier > + /// even if the currently implied (by sql_mode) schema is equal to > + /// to the function schema, e.g. mariadb_schema.concat('a'). > + QT_ITEM_FUNC_FORCE_SCHEMA_NAME= (1 << 12), > + > + /// Print for FRM file. Focus on parse-back. > + /// e.g. VIEW expressions and virtual column expressions > + QT_FOR_FRM= (1 << 13), > + > /// This is used for EXPLAIN EXTENDED extra warnings / Be more detailed > /// Be more detailed than QT_EXPLAIN. > /// Perhaps we should eventually include QT_ITEM_IDENT_SKIP_CURRENT_DATABASE > /// here, as it would give better readable results > QT_EXPLAIN_EXTENDED= QT_TO_SYSTEM_CHARSET| > - QT_SHOW_SELECT_NUMBER, > + QT_SHOW_SELECT_NUMBER| > + QT_ITEM_FUNC_FORCE_SCHEMA_NAME, don't do this. let's try to keep the behavior *exactly as before* for people who don't switch between sql_modes all the time on the second thought. why do you need QT_ITEM_FUNC_FORCE_SCHEMA_NAME at all? Besides EXPLAIN EXTENDED (where we'd better not use it), it's only used in generating view's sql for I_S. It's not supposed to be used for generating views, so why would it need QT_ITEM_FUNC_FORCE_SCHEMA_NAME? May be it can be deleted altogether? > > // If an expression is constant, print the expression, not the value > // it evaluates to. Should be used for error messages, so that they > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 8268ebe0bcd..6b988938f0b 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -6589,6 +6589,50 @@ class Sql_mode_save > sql_mode_t old_mode; // SQL mode saved at construction time. > }; > > + > +/* > + Save the current sql_mode. Switch off sql_mode flags which can prevent > + normal parsing of VIEWs, expressions in generated columns. > + Restore the old sql_mode on destructor. > +*/ > +class Sql_mode_save_for_frm_handling: public Sql_mode_save > +{ > +public: > + Sql_mode_save_for_frm_handling(THD *thd) > + :Sql_mode_save(thd) > + { > + /* > + - MODE_REAL_AS_FLOAT affect only CREATE TABLE parsing > + + MODE_PIPES_AS_CONCAT affect expression parsing > + + MODE_ANSI_QUOTES affect expression parsing > + + MODE_IGNORE_SPACE affect expression parsing > + - MODE_IGNORE_BAD_TABLE_OPTIONS affect only CREATE/ALTER TABLE parsing > + * MODE_ONLY_FULL_GROUP_BY affect execution > + * MODE_NO_UNSIGNED_SUBTRACTION affect execution > + - MODE_NO_DIR_IN_CREATE affect table creation only > + - MODE_POSTGRESQL compounded from other modes > + - MODE_ORACLE affects Item creation (e.g for CONCAT) should be '+' for MODE_ORACLE, not '-', right? > + - MODE_MSSQL compounded from other modes > + - MODE_DB2 compounded from other modes > + - MODE_MAXDB affect only CREATE TABLE parsing > + - MODE_NO_KEY_OPTIONS affect only SHOW > + - MODE_NO_TABLE_OPTIONS affect only SHOW > + - MODE_NO_FIELD_OPTIONS affect only SHOW > + - MODE_MYSQL323 affect only SHOW > + - MODE_MYSQL40 affect only SHOW > + - MODE_ANSI compounded from other modes > + (+ transaction mode) > + ? MODE_NO_AUTO_VALUE_ON_ZERO affect UPDATEs > + + MODE_NO_BACKSLASH_ESCAPES affect expression parsing > + + MODE_EMPTY_STRING_IS_NULL affect expression parsing > + */ > + thd->variables.sql_mode&= ~(MODE_PIPES_AS_CONCAT | MODE_ANSI_QUOTES | > + MODE_IGNORE_SPACE | MODE_NO_BACKSLASH_ESCAPES | > + MODE_ORACLE | MODE_EMPTY_STRING_IS_NULL); > + }; > +}; > + > + > class Abort_on_warning_instant_set > { > THD *m_thd; > diff --git a/sql/sql_schema.h b/sql/sql_schema.h > index 7c8f284d526..36914e36990 100644 > --- a/sql/sql_schema.h > +++ b/sql/sql_schema.h > @@ -33,6 +36,34 @@ class Schema > { > return src; > } > + > + > + /** > + Find the native function builder associated with a given function name. > + @param thd The current thread > + @param name The native function name > + @return The native function builder associated with the name, or NULL > + */ > + virtual Create_func *find_native_function_builder(THD *thd, > + const LEX_CSTRING &name) > + const; > + /** > + Find a native function builder, return an error if not found, > + build an Item otherwise. > + */ > + Item *make_item_func_call_native(THD *thd, > + const Lex_ident_sys &name, > + List<Item> *args) const; > + > + // Builders for native SQL function with a special syntax in sql_yacc.yy > + virtual Item *make_item_func_substr(THD *thd, > + const Lex_substring_spec_st &spec) const; > + > + virtual Item *make_item_func_trim(THD *thd, const Lex_trim_st &spec) const; > + virtual Item *make_item_func_replace(THD *thd, > + Item *subj, > + Item *find, > + Item *replace) const; Why no Lex_replace_spec_st ? > /* > For now we have *hard-coded* compatibility schemas: > schema_mariadb, schema_oracle, schema_maxdb. > @@ -66,5 +97,6 @@ class Schema > > > extern Schema mariadb_schema; > +extern const Schema &oracle_schema_ref; ??? why did you declare mariadb_schema extern, but oracle_schema_ref only via a const ref? > > #endif // SQL_SCHEMA_H_INCLUDED > diff --git a/sql/item_create.h b/sql/item_create.h > index 894e9777b8d..3fadaecb090 100644 > --- a/sql/item_create.h > +++ b/sql/item_create.h > @@ -215,9 +205,48 @@ struct Native_func_registry > Create_func *builder; > }; > > + > +class Native_functions_hash: public HASH > +{ > +public: > + Native_functions_hash() > + { > + bzero(this, sizeof(*this)); > + } > + ~Native_functions_hash() > + { > + /* > + No automatic free. > + The the upper level code should call cleanup() explicitly. why is that? > + */ > + DBUG_ASSERT(!records); > + } > + bool init(size_t count); > + bool init(const Native_func_registry array[], size_t count) > + { > + return init(count) || append(array); > + } > + bool append(const Native_func_registry array[]); > + bool remove(const Native_func_registry array[]); > + void cleanup(); > + /** > + Find the native function builder associated with a given function name. > + @param thd The current thread > + @param name The native function name > + @return The native function builder associated with the name, or NULL > + */ > + Create_func *find(THD *thd, const LEX_CSTRING &name) const; > +}; > + > +extern Native_functions_hash native_functions_hash; > +extern Native_functions_hash native_functions_hash_oracle_overrides; > + > +extern const Native_func_registry func_array[]; > +extern const size_t func_array_length; > + > int item_create_init(); > -int item_create_append(Native_func_registry array[]); > -int item_create_remove(Native_func_registry array[]); > +int item_create_append(const Native_func_registry array[]); > +int item_create_remove(const Native_func_registry array[]); > void item_create_cleanup(); > > Item *create_func_dyncol_create(THD *thd, List<DYNCALL_CREATE_DEF> &list); > diff --git a/sql/sql_schema.cc b/sql/sql_schema.cc > index 0bf4a63c2f8..e2993f51c6d 100644 > --- a/sql/sql_schema.cc > +++ b/sql/sql_schema.cc > @@ -78,3 +89,86 @@ Schema *Schema::find_implied(THD *thd) > return &maxdb_schema; > return &mariadb_schema; > } > + > + > +Create_func * > +Schema::find_native_function_builder(THD *thd, const LEX_CSTRING &name) const > +{ > + return native_functions_hash.find(thd, name); > +} > + > + > +Create_func * > +Schema_oracle::find_native_function_builder(THD *thd, > + const LEX_CSTRING &name) const > +{ > + Create_func *create= native_functions_hash_oracle_overrides.find(thd, name); > + if (create) > + return create; > + return native_functions_hash.find(thd, name); may be populate native_functions_hash_oracle will all names to do only one lookup here? this will be used often in oracle mode > +} > + > + > +Item *Schema::make_item_func_call_native(THD *thd, > + const Lex_ident_sys &name, > + List<Item> *args) const > +{ > + Create_func *builder= find_native_function_builder(thd, name); > + if (builder) > + return builder->create_func(thd, &name, args); > + my_error(ER_FUNCTION_NOT_DEFINED, MYF(0), name.str); > + return NULL; > +} > + > + > +Item *Schema::make_item_func_trim(THD *thd, const Lex_trim_st &spec) const > +{ > + return spec.make_item_func_trim_std(thd); why does trim is constructed via helper in spec, but substr and replace are not? > +} > + > + > +Item *Schema::make_item_func_substr(THD *thd, > + const Lex_substring_spec_st &spec) const > +{ > + return spec.m_for ? > + new (thd->mem_root) Item_func_substr(thd, spec.m_subject, spec.m_from, > + spec.m_for) : > + new (thd->mem_root) Item_func_substr(thd, spec.m_subject, spec.m_from); > +} > + > + > +Item *Schema_oracle::make_item_func_substr(THD *thd, > + const Lex_substring_spec_st &spec) const > +{ > + return spec.m_for ? > + new (thd->mem_root) Item_func_substr_oracle(thd, spec.m_subject, > + spec.m_from, > + spec.m_for) : > + new (thd->mem_root) Item_func_substr_oracle(thd, spec.m_subject, > + spec.m_from); > +} > + > + > +Item *Schema_oracle::make_item_func_trim(THD *thd, > + const Lex_trim_st &spec) const > +{ > + return spec.make_item_func_trim_oracle(thd); > +} > + > + > +Item *Schema::make_item_func_replace(THD *thd, > + Item *subj, > + Item *find, > + Item *replace) const > +{ > + return new (thd->mem_root) Item_func_replace(thd, subj, find, replace); > +} > + > + > +Item *Schema_oracle::make_item_func_replace(THD *thd, > + Item *subj, > + Item *find, > + Item *replace) const > +{ > + return new (thd->mem_root) Item_func_replace_oracle(thd, subj, find, replace); > +} > diff --git a/sql/item_func.h b/sql/item_func.h > index 754b1cd1eb2..52bf7f661d1 100644 > --- a/sql/item_func.h > +++ b/sql/item_func.h > @@ -56,8 +56,117 @@ class Item_func :public Item_func_or_sum > bool check_argument_types_can_return_text(uint start, uint end) const; > bool check_argument_types_can_return_date(uint start, uint end) const; > bool check_argument_types_can_return_time(uint start, uint end) const; > + > + void print_schema_qualified_name(String *to, > + const LEX_CSTRING &schema_name, > + const char *function_name) const > + { > + // e.g. oracle_schema.func() > + to->append(schema_name); > + to->append('.'); > + to->append(function_name); > + } > + > + void print_name_with_optional_suffix(String *to, > + const char *function_name, > + const LEX_CSTRING &suffix) const > + { > + // e.g. func_oracle() > + to->append(function_name); > + if (suffix.length) > + { > + to->append("_"); > + to->append(suffix); > + } > + } > + > + void print_sql_mode_dependent_name(String *to, enum_query_type query_type, > + const Schema &schema, > + const char *function_name, > + const LEX_CSTRING &suffix) const > + { > + if (query_type & QT_ITEM_FUNC_FORCE_SCHEMA_NAME) > + { > + DBUG_ASSERT((query_type & QT_FOR_FRM) == 0); > + print_schema_qualified_name(to, schema.name(), function_name); > + } > + else if (query_type & QT_FOR_FRM) > + { > + // e.g. substr_oracle() > + DBUG_ASSERT((query_type & QT_ITEM_FUNC_FORCE_SCHEMA_NAME) == 0); > + print_name_with_optional_suffix(to, function_name, suffix); > + } > + else if (&schema != Schema::find_implied(current_thd)) > + print_schema_qualified_name(to, schema.name(), function_name); > + else > + to->append(function_name); > + } > + > + void print_sql_mode_dependent_name(String *to, enum_query_type query_type, > + const Schema &schema, > + const char *function_name) const > + { > + static const LEX_CSTRING oracle= {STRING_WITH_LEN("oracle")}; can it be oracle_schema.name() instead? > + static const LEX_CSTRING empty= {NULL, 0}; > + const LEX_CSTRING suffix= &schema == &oracle_schema_ref ? oracle : empty; > + print_sql_mode_dependent_name(to, query_type, > + schema, function_name, suffix); > + } > + void print_sql_mode_dependent_name(String *to, enum_query_type query_type, > + const char *function_name) const > + { > + print_sql_mode_dependent_name(to, query_type, *schema(), function_name); > + } > + void print_sql_mode_dependent(String *to, enum_query_type query_type) > + { > + print_sql_mode_dependent_name(to, query_type, func_name()); > + print_args_parenthesized(to, query_type); > + } > public: > > + // Print an error message for a builtin-schema qualified function call > + static void wrong_param_count_error(const LEX_CSTRING &schema_name, > + const LEX_CSTRING &func_name); > + > + // Check that the number of arguments is greater or equal to "expected" > + static bool create_check_args_ge(const LEX_CSTRING &name, > + const List<Item> *item_list, > + uint expected) > + { > + DBUG_ASSERT(expected > 0); > + if (!item_list || item_list->elements < expected) > + { > + my_error(ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT, MYF(0), name.str); > + return true; > + } > + return false; > + } > + > + // Check that the number of arguments is less or equal to "expected" > + static bool create_check_args_le(const LEX_CSTRING &name, > + const List<Item> *item_list, > + uint expected) > + { > + DBUG_ASSERT(expected > 0); > + if (!item_list || item_list->elements > expected) > + { > + my_error(ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT, MYF(0), name.str); > + return true; > + } > + return false; > + } > + > + // Check that the number of arguments is in the allowed range > + static bool create_check_args_between(const LEX_CSTRING &name, > + const List<Item> *item_list, > + uint min_arg_count, > + uint max_arg_count) > + { > + DBUG_ASSERT(min_arg_count < max_arg_count); > + return create_check_args_ge(name, item_list, min_arg_count) || > + create_check_args_le(name, item_list, max_arg_count); > + } > + > table_map not_null_tables_cache; > > enum Functype { UNKNOWN_FUNC,EQ_FUNC,EQUAL_FUNC,NE_FUNC,LT_FUNC,LE_FUNC, > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 3770488f3f4..e0bafc75a36 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -11235,6 +11236,34 @@ function_call_generic: > if (unlikely(!($$= Lex->make_item_func_call_generic(thd, &$1, &$3, &$5, $7)))) > MYSQL_YYABORT; > } > + | ident_cli '.' REPLACE '(' expr ',' expr ',' expr ')' > + { > + if (unlikely(!($$= Lex->make_item_func_replace(thd, $1, $3, > + $5, $7, $9)))) > + MYSQL_YYABORT; > + } > + | ident_cli '.' SUBSTRING '(' substring_operands ')' > + { > + if (unlikely(!($$= Lex->make_item_func_substr(thd, $1, $3, $5)))) > + MYSQL_YYABORT; > + } > + | ident_cli '.' TRIM '(' trim_operands ')' > + { > + if (unlikely(!($$= $5.make_item_func_trim(thd, $1, $3)))) > + MYSQL_YYABORT; > + } > + /* > + We don't add a qualified syntax for TRIM_ORACLE here, > + as this syntax is not absolutely required: > + SELECT mariadb_schema.TRIM_ORACLE(..); > + What absolutely required is only: > + SELECT mariadb_schema.TRIM(..); > + Adding a qualified syntax for TRIM_ORACLE would be tricky because > + it is a non-reserved keyword. To avoid new shift/reduce conflicts > + it would require grammar changes, like introducing a new rule > + ident_step2_cli (which would include everything that ident_cli > + includes but TRIM_ORACLE). Did you add a qualified syntax for any other xxx_ORACLE functions? > + */ > ; > > fulltext_options: > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > index 7f1b362d91d..6b0a351f76e 100644 > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -2008,7 +2023,59 @@ bool Lex_input_stream::get_7bit_or_8bit_ident(THD *thd, uchar *last_char) > } > > > +/* > + Resolve special SQL functions that have a qualified syntax in sql_yacc.yy. > + These functions are not listed in the native function registry > + because of a special syntax, or a reserved keyword: > + > + mariadb_schema.SUBSTRING('a' FROM 1 FOR 2) -- Special syntax > + mariadb_schema.TRIM(BOTH ' ' FROM 'a') -- Special syntax > + mariadb_schema.REPLACE('a','b','c') -- Verb keyword > +*/ > + > +int Lex_input_stream::find_keyword_qualified_special_func(Lex_ident_cli_st *str, > + uint length) const > +{ > + /* > + There are many other special functions, see the following grammar rules: > + function_call_keyword > + function_call_nonkeyword > + Here we resolve only those that have a qualified syntax to handle > + different behavior in different @@sql_mode settings. > + > + Other special functions do not work in qualified context: > + SELECT mariadb_schema.year(now()); -- Function year is not defined > + SELECT mariadb_schema.now(); -- Function now is not defined > + > + We don't resolve TRIM_ORACLE here, because it does not have > + a qualified syntax yet. Search for "trim_operands" in sql_yacc.yy > + to find more comments. > + */ > + static LEX_CSTRING funcs[]= > + { > + {STRING_WITH_LEN("SUBSTRING")}, > + {STRING_WITH_LEN("SUBSTR")}, > + {STRING_WITH_LEN("TRIM")}, > + {STRING_WITH_LEN("REPLACE")} > + }; > + > + int tokval= find_keyword(str, length, true); > + if (!tokval) > + return 0; > + for (size_t i= 0; i < array_elements(funcs); i++) > + { > + CHARSET_INFO *cs= system_charset_info; > + if (!cs->coll->strnncollsp(cs, may be strcasecmp? or with cs=latin1? you know the repertoire is ascii, so you can cut corners here. > + (const uchar *) m_tok_start, length, > + (const uchar *) funcs[i].str, funcs[i].length)) > + return tokval; > + } > + return 0; > +} > + > + > -int Lex_input_stream::scan_ident_sysvar(THD *thd, Lex_ident_cli_st *str) > +int Lex_input_stream::scan_ident_common(THD *thd, Lex_ident_cli_st *str, > + Ident_mode mode) > { > uchar last_char; > uint length; > @@ -8035,30 +8133,49 @@ bool LEX::add_grant_command(THD *thd, enum_sql_command sql_command_arg, > } > > > +const Schema * > +LEX::find_func_schema_by_name_or_error(const Lex_ident_sys &schema, > + const Lex_ident_sys &func) > +{ > + Schema *res= Schema::find_by_name(schema); > + if (res) > + return res; > + Database_qualified_name qname(schema, func); > + my_error(ER_FUNCTION_NOT_DEFINED, MYF(0), ErrConvDQName(&qname).ptr()); eh, not quite. I can define a function with the same name as a native function in any db. E.g. create test.replace(a int) returns int and it'll be callable as test.replace(1) So ER_FUNCTION_NOT_DEFINED is wrong here. But note that even though test.substr() is possible, it should not allow substr() special syntax. > + return NULL; > +} > -Item *LEX::make_item_func_substr(THD *thd, Item *a, Item *b, Item *c) > -{ > - return (thd->variables.sql_mode & MODE_ORACLE) ? > - new (thd->mem_root) Item_func_substr_oracle(thd, a, b, c) : > - new (thd->mem_root) Item_func_substr(thd, a, b, c); > -} > > > -Item *LEX::make_item_func_substr(THD *thd, Item *a, Item *b) > -{ > - return (thd->variables.sql_mode & MODE_ORACLE) ? > - new (thd->mem_root) Item_func_substr_oracle(thd, a, b) : > - new (thd->mem_root) Item_func_substr(thd, a, b); > -} > +Item *LEX::make_item_func_substr(THD *thd, > + const Lex_ident_cli_st &schema_name_cli, > + const Lex_ident_cli_st &func_name_cli, > + const Lex_substring_spec_st &spec) > +{ > + Lex_ident_sys schema_name(thd, &schema_name_cli); > + Lex_ident_sys func_name(thd, &func_name_cli); > + if (schema_name.is_null() || func_name.is_null()) > + return NULL; // EOM > + const Schema *schema= find_func_schema_by_name_or_error(schema_name, > + func_name); > + return schema ? schema->make_item_func_substr(thd, spec) : NULL; > +} > > > Item *LEX::make_item_func_replace(THD *thd, > + const Lex_ident_cli_st &schema_name_cli, > + const Lex_ident_cli_st &func_name_cli, > Item *org, > Item *find, > Item *replace) > { > - return (thd->variables.sql_mode & MODE_ORACLE) ? > - new (thd->mem_root) Item_func_replace_oracle(thd, org, find, replace) : > - new (thd->mem_root) Item_func_replace(thd, org, find, replace); > + Lex_ident_sys schema_name(thd, &schema_name_cli); > + Lex_ident_sys func_name(thd, &func_name_cli); > + if (schema_name.is_null() || func_name.is_null()) > + return NULL; // EOM > + const Schema *schema= find_func_schema_by_name_or_error(schema_name, > + func_name); > + return schema ? schema->make_item_func_replace(thd, org, find, replace) : > + NULL; > } > > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

[Maria-developers] START ALTER replication test cases
by Kristian Nielsen 10 May '23

10 May '23

Hi Sachin, Andrei, I'm looking at test cases for START ALTER / Lag-free Alter on Slave. And I happened to notice something in test case rpl.rpl_start_alter_bugs: --connection master set global binlog_alter_two_phase=true; # ... ALTER TABLE t1 DROP PRIMARY KEY; ALTER TABLE t1 ADD UNIQUE KEY ui (i); ALTER TABLE t1 ADD PRIMARY KEY (i); Note the set *global* binlog_alter_two_phase. This will not affect the testcase at all, as the ALTER TABLE queries are using the *session* value (which defaults to false). This can be verified by checking the binlog after running the testcase, mysql-test/var/mysqld.1/data/master-bin.000001 . It has only normal ALTER TABLE query events, no START ALTER / COMMIT ALTER. So it looks strange that a start_alter test doesn't use START ALTER, and I just wanted to point this out to you in case it is a mistake and maybe the test coverage of START ALTER is less than intended? A quick grep shows similar SET GLOBAL in other start_alter tests, but I did not investigate if they have the same issue. Hope this helps, - Kristian.

2 2

Re: [Maria-developers] da5a72f32d4: MDEV-31033 ER_KEY_NOT_FOUND upon online COPY ALTER on a partitioned table
by Sergei Golubchik 10 May '23

10 May '23

Hi, Nikita, On May 05, Nikita Malyavin wrote: > revision-id: da5a72f32d4 (mariadb-11.0.1-114-gda5a72f32d4) > parent(s): 0d6584c019c > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-05-01 01:15:41 +0300 > message: > > MDEV-31033 ER_KEY_NOT_FOUND upon online COPY ALTER on a partitioned table > > The row events were applied "twice": once for the ha_partition, and > one more time for the underlying storage engine. > > There's no such problem in binlog/rpl, because > ha_partiton::row_logging is normally set to false. s/ha_partiton/ha_partition/ > The proposed fix adds table->skip_online_logging field, preventing the > underlying engines from replicating online events. Only upper layer > engine should do this. > > table->skip_online_logging is initialized to 0 together with the table > itself. Normally it is always 0, except for the intermediate > overriding part, once it is checked in handler::binlog_log_row. > > Note on MDEV-31040 (Server crashes in MYSQL_LOG::is_open upon ALTER on > partitioned table): > First the solution was to override table->s->online_alter_binlog, and > set it to NULL during the partitioning operation, which was > ridiculously wrong: no-one is supposed to modify that field outside > ALTER! That field write access is protected by EXCLUSIVE metadata > lock. I don't think you need to list your different (non-working) attemps to fix the bug in the commit comment. Just explain the fix that works, not the fix that doesn't. > In comparison, table->skip_online_logging is a connection-local field. > > The test is a non-deterministic one. It wasn't convenient to construct > a deterministic test here, besides for the case that will never be > repeated. Is it still the case? Your test in alter_table_online_debug.test with debug_sync - is it non-deterministic? > diff --git a/sql/handler.cc b/sql/handler.cc > index 3fcbb5171d6..a7e229e8f8d 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -7280,7 +7280,8 @@ int handler::binlog_log_row(const uchar *before_record, > log_func, row_logging_has_trans); > > #ifdef HAVE_REPLICATION > - if (unlikely(!error && table->s->online_alter_binlog)) > + if (unlikely(!error && table->s->online_alter_binlog && > + !table->skip_online_logging)) this doesn't need any juggling with skip_online_logging. This problem is already solved for, say, long uniques. You can use this == table->file condition to filter out individual partitions and have your binlog_log_row_online_alter called only for the main ha_partition handler. > error= binlog_log_row_online_alter(table, before_record, after_record, > log_func); > #endif // HAVE_REPLICATION Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 5