June 2023 - developers - lists.mariadb.org

Client-server protocol improvement proposal: Connection redirection
by Lenski, Daniel 18 Aug '23

18 Aug '23

Hi! I would like to propose a new feature in the MariaDB client-server protocol<https://mariadb.com/kb/en/clientserver-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<https://mariadb.com/kb/en/thread-pool-in-mariadb/#configuring-the-extra-port>, which is intended for emergency administrative use), and it will instead respond to all clients with an error packet<https://mariadb.com/kb/en/err_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. The feature is also designed to be extensible in a forwards-compatible way. The SERVER_REDIRECT_MODE system variable could be expanded to support additional redirection modes. For example, it could be augmented to redirect new connections only from specific IP subnets, or to direct the client to interpret the SERVER_REDIRECT_TARGET variable in a new form (such as a<https://dev.mysql.com/doc/connector-j/8.0/en/connector-j-reference-jdbc-url…> JDBC-compatible mysql:// URL<https://dev.mysql.com/doc/connector-j/8.0/en/connector-j-reference-jdbc-url…> as proposed in MDEV-15935<https://jira.mariadb.org/browse/MDEV-15935>). While researching and designing this connection redirection mechanism, I discovered or rediscovered a couple of major TLS-related security vulnerabilities in the MariaDB Connector/C client [1, 2]. These vulnerabilities affect all MariaDB users who care about the confidentiality and authenticity of their client/server connections, and they become even more severe if a connection redirection mechanism exists — because this entails the client trusting the server to send it instructions for creating a new network connection. Unless these are resolved, any connection redirection mechanism will be susceptible to hijacking and redirection to an attacker-controlled server even if the client thinks it is using TLS/SSL. What are the use cases for this connection redirection feature? Even in this simple, initial form, the feature is already useful for database administrators who want to gracefully shut down a MariaDB server (for example, to upgrade it), and to redirect new client connections to a replica or alternate server that can accept the traffic. Server-side connection redirection can be combined with existing MariaDB server features, such as SET GLOBAL READ_ONLY=ON and KILLing of idle or long-running connections, in order to build a full switchover solution for shunting traffic from one server to another as gracefully as possible. Currently, MariaDB database administrators tend to use proxies and wrappers (such as HikariCP) to redirect client connections from one MariaDB server to another. Supporting this redirection mechanism directly in the server, and in the client-server protocol, will allow many simple use cases of redirection without extra software, making MariaDB easier and simpler to manage for minimal downtime situations. Please post on this mailing list what you think about this suggestion. I am looking forward to polishing this feature and submitting it in a form that meets the quality requirements for new features from MariaDB core developers. Thank you! Daniel Lenski Amazon RDS MySQL/MariaDB engine team --- Notes on TLS vulnerabilities in MariaDB Connector/C: [1] CONC-648<https://jira.mariadb.org/browse/CONC-648>, reported by me on 5 June: when a Connector/C-based client is using connections secured by TLS/SSL, Connector/C will nevertheless receive plain-text, unencrypted error packets prior to the TLS handshake, and present them to the client in a way that’s indistinguishable from TLS-protected error packets sent by the server after the TLS handshake. This longstanding vulnerability is a consequence of the tangled and unsystematic way that the Connector/C codebase interleaves the MariaDB application-layer authentication protocol with the transport-layer security protocol. My standalone mariadb-connector-c PR#223<https://github.com/mariadb-corporation/mariadb-connector-c/pull/223> fixes the most straightforward manifestation of this vulnerability. Until this issue is resolved in Connector/C, it will be trivial for MITM attackers to inject plaintext connection redirection packets into MariaDB client/server connections, and thereby redirect clients to attacker-controlled servers despite those clients’ intention to use only TLS-secured connections. [2] MDEV-28634<https://jira.mariadb.org/browse/MDEV-28634>, reported almost 3 years ago by Geoff Montee: Connector/C clients which specify --ssl without --ssl-verify-server-cert will allow their connections to be silently downgraded to unencrypted, plaintext connections. This vulnerability can be used by on-path attackers to trivially downgrade most MariaDB client-server connections from TLS-encrypted to plaintext. My standalone mariadb-connector-c PR#224<https://github.com/mariadb-corporation/mariadb-connector-c/pull/224> fixes this. It is a backwards-incompatible change in that clients which specify --ssl (and thus clearly expect TLS-secured connections) will no longer succeed in connecting to servers which don’t support TLS. This should be considered a good thing: MariaDB Connector/C-based clients have been misleading their users about whether their connections are guaranteed to be encrypted, and will no longer do so. Essentially all software that purports to secure network connections with TLS have implemented similar changes, most of it about a decade ago when the pervasiveness of such attacks<https://en.wikipedia.org/wiki/Global_surveillance_disclosures_(2013%E2%80%9…> (by ISPs, censors, intelligenceagencies, etc) became very widely-known.

8 28

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

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