- developers - lists.mariadb.org

Re: [Maria-developers] 49ecf935415: MDEV-27009 Add UCA-14.0.0 collations
by Sergei Golubchik 16 Jun '22

16 Jun '22

Hi, Alexander, On Mar 14, Alexander Barkov wrote: > revision-id: 49ecf935415 (mariadb-10.6.1-335-g49ecf935415) > parent(s): c67789f63c8 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-02-28 14:04:58 +0400 > message: > > MDEV-27009 Add UCA-14.0.0 collations please, list all user visible changes there. Mainly that collations are now decoupled from charsets. New syntax in CREATE TABLE, changes in I_S tables, etc. > diff --git a/mysql-test/include/ctype_utf_uca1400_ids.inc b/mysql-test/include/ctype_utf_uca1400_ids.inc > new file mode 100644 > index 00000000000..09cf49fc0e7 > --- /dev/null > +++ b/mysql-test/include/ctype_utf_uca1400_ids.inc > @@ -0,0 +1,17 @@ file names are confusing. better rename ctype_ucs_uca1400_ids.inc to something like ctype_convert_uca1400_ids and ctype_utf_uca1400_ids to ctype_set_names_uca1400_ids or something like that, to show what they do. > + > +--disable_ps_protocol > +--enable_metadata > +DELIMITER $$; > +FOR rec IN (SELECT COLLATION_NAME > + FROM INFORMATION_SCHEMA.COLLATION_CHARACTER_SET_APPLICABILITY > + WHERE CHARACTER_SET_NAME=@charset > + AND COLLATION_NAME RLIKE 'uca1400' > + ORDER BY ID) > +DO > + EXECUTE IMMEDIATE CONCAT('SET NAMES ',@charset,' COLLATE ', rec.COLLATION_NAME); > + SELECT rec.COLLATION_NAME; > +END FOR; > +$$ > +DELIMITER ;$$ > +--disable_metadata > +--enable_ps_protocol > diff --git a/include/m_ctype.h b/include/m_ctype.h > index 4c6628b72b3..706764ead2a 100644 > --- a/include/m_ctype.h > +++ b/include/m_ctype.h > @@ -34,7 +34,9 @@ enum loglevel { > extern "C" { > #endif > > -#define MY_CS_NAME_SIZE 32 > +#define MY_CS_CHARACTER_SET_NAME_SIZE 32 > +#define MY_CS_COLLATION_NAME_SIZE 64 That's FULL_COLLATION_NAME_SIZE, right? > + > #define MY_CS_CTYPE_TABLE_SIZE 257 > #define MY_CS_TO_LOWER_TABLE_SIZE 256 > #define MY_CS_TO_UPPER_TABLE_SIZE 256 > @@ -240,6 +242,46 @@ typedef enum enum_repertoire_t > } my_repertoire_t; > > > +/* ID compatibility */ > +typedef enum enum_collation_id_type > +{ > + MY_COLLATION_ID_TYPE_PRECISE= 0, > + MY_COLLATION_ID_TYPE_COMPAT_100800= 1 > +} my_collation_id_type_t; > + > + > +/* Collation name display modes */ > +typedef enum enum_collation_name_mode > +{ > + MY_COLLATION_NAME_MODE_FULL= 0, > + MY_COLLATION_NAME_MODE_CONTEXT= 1 > +} my_collation_name_mode_t; > + > + > +/* Level flags */ > +#define MY_CS_LEVEL_BIT_PRIMARY 0x00 > +#define MY_CS_LEVEL_BIT_SECONDARY 0x01 > +#define MY_CS_LEVEL_BIT_TERTIARY 0x02 > +#define MY_CS_LEVEL_BIT_QUATERNARY 0x03 > + > +#define MY_CS_COLL_LEVELS_S1 (1<<MY_CS_LEVEL_BIT_PRIMARY) > + > +#define MY_CS_COLL_LEVELS_AI_CS (1<<MY_CS_LEVEL_BIT_PRIMARY)| \ > + (1<<MY_CS_LEVEL_BIT_TERTIARY) > + > +#define MY_CS_COLL_LEVELS_S2 (1<<MY_CS_LEVEL_BIT_PRIMARY)| \ > + (1<<MY_CS_LEVEL_BIT_SECONDARY) > + > +#define MY_CS_COLL_LEVELS_S3 (1<<MY_CS_LEVEL_BIT_PRIMARY)| \ > + (1<<MY_CS_LEVEL_BIT_SECONDARY) | \ > + (1<<MY_CS_LEVEL_BIT_TERTIARY) AI_CS and S3 don't seem to be used yet > + > +#define MY_CS_COLL_LEVELS_S4 (1<<MY_CS_LEVEL_BIT_PRIMARY)| \ > + (1<<MY_CS_LEVEL_BIT_SECONDARY) | \ > + (1<<MY_CS_LEVEL_BIT_TERTIARY) | \ > + (1<<MY_CS_LEVEL_BIT_QUATERNARY) > + > + > /* Flags for strxfrm */ > #define MY_STRXFRM_LEVEL1 0x00000001 /* for primary weights */ > #define MY_STRXFRM_LEVEL2 0x00000002 /* for secondary weights */ > diff --git a/sql/sql_alter.cc b/sql/sql_alter.cc > index 86c6e9a27f8..9ddd482ad57 100644 > --- a/sql/sql_alter.cc > +++ b/sql/sql_alter.cc > @@ -546,6 +546,7 @@ bool Sql_cmd_alter_table::execute(THD *thd) > > result= mysql_alter_table(thd, &select_lex->db, &lex->name, > &create_info, > + lex->create_info.default_charset_collation, I don't see why you need a new argument here. It's create_info.default_charset_collation, so, mysql_alter_table already gets it in create_info. All other mysql_alter_table invocations also take create_info argument and can get default_charset_collation from there > first_table, > &alter_info, > select_lex->order_list.elements, > diff --git a/sql/sql_partition_admin.cc b/sql/sql_partition_admin.cc > index fb1ae0d5fc7..4188dde252b 100644 > --- a/sql/sql_partition_admin.cc > +++ b/sql/sql_partition_admin.cc > @@ -211,6 +211,7 @@ bool compare_table_with_partition(THD *thd, TABLE *table, TABLE *part_table, > part_table->use_all_columns(); > table->use_all_columns(); > if (unlikely(mysql_prepare_alter_table(thd, part_table, &part_create_info, > + Lex_maybe_default_charset_collation(), Same. Can be in part_create_info > &part_alter_info, &part_alter_ctx))) > { > my_error(ER_TABLES_DIFFERENT_METADATA, MYF(0)); > diff --git a/sql/sql_i_s.h b/sql/sql_i_s.h > index bed2e886718..5ff06d32231 100644 > --- a/sql/sql_i_s.h > +++ b/sql/sql_i_s.h > @@ -162,6 +162,11 @@ class Yesno: public Varchar > { > public: > Yesno(): Varchar(3) { } > + static LEX_CSTRING value(bool val) > + { > + return val ? Lex_cstring(STRING_WITH_LEN("Yes")) : > + Lex_cstring(); > + } eh... please, rename the class from Yesno to something like Yesempty or Yes_or_empty, something that says that the second should not be Lex_cstring(STRING_WITH_LEN("No")) > }; > > > diff --git a/sql/table.cc b/sql/table.cc > index a683a78ff49..c28cb2bd928 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -3491,6 +3493,16 @@ int TABLE_SHARE::init_from_sql_statement_string(THD *thd, bool write, > else > thd->set_n_backup_active_arena(arena, &backup); > > + /* > + THD::reset_db() does not set THD::db_charset, > + so it keeps pointing to the character set and collation > + of the current database, rather than the database of the > + new initialized table. Hmm, is that correct? Could you check other invocation of thd->reset_db()? Perhaps they all need to switch charset? In that case it should be done inside THD::reset_db(). Or may be they have to use mysql_change_db_impl() instead? > + Let's call get_default_db_collation() before reset_db(). > + This forces the db.opt file to be loaded. > + */ > + db_cs= get_default_db_collation(thd, db.str); > + > thd->reset_db(&db); > lex_start(thd); > > @@ -3498,6 +3510,11 @@ int TABLE_SHARE::init_from_sql_statement_string(THD *thd, bool write, > sql_unusable_for_discovery(thd, hton, sql_copy)))) > goto ret; > > + if (!(thd->lex->create_info.default_table_charset= > + thd->lex->create_info.default_charset_collation. > + resolved_to_character_set(db_cs, db_cs))) > + DBUG_RETURN(true); How could this (and similar if()'s in other files) fail? > + > thd->lex->create_info.db_type= hton; > #ifdef WITH_PARTITION_STORAGE_ENGINE > thd->work_part_info= 0; // For partitioning > diff --git a/sql/mysys_charset.h b/sql/mysys_charset.h > new file mode 100644 > index 00000000000..86eaeedd432 > --- /dev/null > +++ b/sql/mysys_charset.h > @@ -0,0 +1,44 @@ > +#ifndef MYSYS_CHARSET > +#define MYSYS_CHARSET > + > +/* Copyright (c) 2021, 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 */ > + > + > +#include "my_sys.h" > + > + > +class Charset_loader_mysys: public MY_CHARSET_LOADER > +{ > +public: > + Charset_loader_mysys() > + { > + my_charset_loader_init_mysys(this); > + } > + void raise_unknown_collation_error(const char *name, > + CHARSET_INFO *name_cs) const; > + CHARSET_INFO *get_charset(const char *cs_name, uint cs_flags, myf my_flags); > + CHARSET_INFO *get_exact_collation(const char *name, myf utf8_flag); > + CHARSET_INFO *get_contextually_typed_collation(CHARSET_INFO *cs, > + const char *name); > + CHARSET_INFO *get_contextually_typed_collation(const char *name); > + CHARSET_INFO *get_contextually_typed_collation_or_error(CHARSET_INFO *cs, > + const char *name); > + CHARSET_INFO *find_default_collation(CHARSET_INFO *cs); > + CHARSET_INFO *find_bin_collation_or_error(CHARSET_INFO *cs); > +}; you can have C++ code in mysys too, you know, no need to put it in sql/mysys* > + > +#endif // MYSYS_CHARSET > + > diff --git a/strings/ctype-simple.c b/strings/ctype-simple.c > index b579f0af203..d09dfba86ed 100644 > --- a/strings/ctype-simple.c > +++ b/strings/ctype-simple.c > @@ -1940,13 +1941,26 @@ my_bool my_propagate_complex(CHARSET_INFO *cs __attribute__((unused)), > } > > > +void my_ci_set_strength(struct charset_info_st *cs, uint strength) > +{ > + DBUG_ASSERT(strength > 0 && strength <= MY_STRXFRM_NLEVELS); don't use && in asserts, please create two separate asserts instead: DBUG_ASSERT(strength > 0); DBUG_ASSERT(strength <= MY_STRXFRM_NLEVELS); > + cs->levels_for_order= ((1 << strength) - 1); why do you still use the old concept of "strength"? Why not to use bitmap consistently everywhere? > +} > + > + > +void my_ci_set_level_flags(struct charset_info_st *cs, uint flags) > +{ > + DBUG_ASSERT(flags < (1<<MY_STRXFRM_NLEVELS)); > + cs->levels_for_order= flags; > +} > + > /* > Normalize strxfrm flags > > SYNOPSIS: > my_strxfrm_flag_normalize() > + cs - the CHARSET_INFO pointer > flags - non-normalized flags > - nlevels - number of levels > > NOTES: > If levels are omitted, then 1-maximum is assumed. > diff --git a/sql/handler.h b/sql/handler.h > index 8ad521e189a..1e82f37b1e7 100644 > --- a/sql/handler.h > +++ b/sql/handler.h > @@ -2409,7 +2386,32 @@ struct Table_specification_st: public HA_CREATE_INFO, > { > HA_CREATE_INFO::options= 0; > DDL_options_st::init(); > + default_charset_collation.init(); > + } > + > + bool > + add_alter_list_item_convert_to_charset(const Lex_charset_collation_st &cl) > + { > + /* > + cs cannot be NULL, as sql_yacc.yy translates > + CONVERT TO CHARACTER SET DEFAULT > + to > + CONVERT TO CHARACTER SET <character-set-of-the-current-database> > + TODO: Shouldn't we postpone resolution of DEFAULT until the > + character set of the table owner database is loaded from its db.opt? > + */ > + DBUG_ASSERT(cl.charset_collation()); > + DBUG_ASSERT(!cl.is_contextually_typed_collation()); > + alter_table_convert_to_charset= cl.charset_collation(); > + default_charset_collation.Lex_charset_collation_st::operator=(cl); looks quite ugly. can you do, like, default_charset_collation.set(cl) ? > + used_fields|= (HA_CREATE_USED_CHARSET | HA_CREATE_USED_DEFAULT_CHARSET); > + return false; > } > + bool add_table_option_default_charset(CHARSET_INFO *cs); > + bool add_table_option_default_collation(const Lex_charset_collation_st &cl); > + bool resolve_db_charset_and_collation(THD *thd, > + const LEX_CSTRING &db, > + bool is_alter); > }; > > > diff --git a/strings/ctype-uca1400data.h b/strings/ctype-uca1400data.h > new file mode 100644 > index 00000000000..da95dcfde54 > --- /dev/null > +++ b/strings/ctype-uca1400data.h > @@ -0,0 +1,44151 @@ > +/* > + Generated from allkeys.txt version '14.0.0' > +*/ if it's generated, do you need to check it in? perhaps it should be generated during the build? you've checked in allkeys1400.txt anyway. > +static const uint16 uca1400_p000[]= { /* 0000 (4 weights per char) */ > +0x0000,0x0000,0x0000,0x0000, 0x0000,0x0000,0x0000,0x0000, /* 0000 */ > +0x0000,0x0000,0x0000,0x0000, 0x0000,0x0000,0x0000,0x0000, /* 0002 */ > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > index 6ca10267187..d115401a855 100644 > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -542,6 +542,30 @@ bool LEX::add_alter_list(LEX_CSTRING name, LEX_CSTRING new_name, bool exists) > } > > > +bool LEX::add_alter_list_item_convert_to_charset( > + THD *thd, > + CHARSET_INFO *cs, > + const Lex_charset_collation_st &cl) > +{ > + if (!cs) > + { > + Lex_charset_collation_st tmp; > + tmp.set_charset_collate_default(thd->variables.collation_database); Hmm, what if one is doing ALTER TABLE db.test CHARSET DEFAULT and current db is not `db` but `test` ? > + if (!(cs= tmp.charset_collation())) > + return true; // Should not actually happen assert? > + } > + > + Lex_explicit_charset_opt_collate tmp(cs, false); > + if (tmp.merge_opt_collate_or_error(cl) || > + create_info.add_alter_list_item_convert_to_charset( > + Lex_charset_collation(tmp))) > + return true; > + > + alter_info.flags|= ALTER_CONVERT_TO; > + return false; > +} > + > + > void LEX::init_last_field(Column_definition *field, > const LEX_CSTRING *field_name) > { > @@ -11871,29 +11869,41 @@ CHARSET_INFO *Lex_collation_st::find_default_collation(CHARSET_INFO *cs) > "def" is the upper level CHARACTER SET clause (e.g. of a table) > */ > CHARSET_INFO * > -Lex_collation_st::resolved_to_character_set(CHARSET_INFO *def) const > +Lex_charset_collation_st::resolved_to_character_set(CHARSET_INFO *def) const > { > DBUG_ASSERT(def); > - if (m_type != TYPE_CONTEXTUALLY_TYPED) > - { > - if (!m_collation) > - return def; // Empty - not typed at all > - return m_collation; // Explicitly typed > + > + switch (m_type) { > + case TYPE_EMPTY: > + return def; > + case TYPE_CHARACTER_SET: > + DBUG_ASSERT(m_ci); > + return m_ci; > + case TYPE_COLLATE_EXACT: > + DBUG_ASSERT(m_ci); > + return m_ci; > + case TYPE_COLLATE_CONTEXTUALLY_TYPED: > + break; > } > > // Contextually typed > - DBUG_ASSERT(m_collation); > + DBUG_ASSERT(m_ci); > > - if (m_collation == &my_charset_bin) // CHAR(10) BINARY > - return find_bin_collation(def); > + Charset_loader_mysys loader; > + if (is_contextually_typed_binary_style()) // CHAR(10) BINARY > + return loader.find_bin_collation_or_error(def); > > - if (m_collation == &my_charset_latin1) // CHAR(10) COLLATE DEFAULT > - return find_default_collation(def); > + if (is_contextually_typed_collate_default()) // CHAR(10) COLLATE DEFAULT > + return loader.find_default_collation(def); > + > + const LEX_CSTRING context_name= collation_name_context_suffix(); I'd rather put this in assert, not in if(). Like - if (!strncasecmp(context_name.str, STRING_WITH_LEN("uca1400_"))) + DBUG_ASSERT(!strncasecmp(context_cl_name.str, STRING_WITH_LEN("uca1400_"))); > + if (!strncasecmp(context_name.str, STRING_WITH_LEN("uca1400_"))) > + return loader.get_contextually_typed_collation_or_error(def, > + context_name.str); > > /* > - Non-binary and non-default contextually typed collation. > + Non-binary, non-default, non-uca1400 contextually typed collation. > We don't have such yet - the parser cannot produce this. > - But will have soon, e.g. "uca1400_as_ci". > */ > DBUG_ASSERT(0); > return NULL; > @@ -11944,58 +11972,106 @@ bool Lex_collation_st:: > CHAR(10) BINARY .. COLLATE latin1_bin > CHAR(10) COLLATE uca1400_as_ci .. COLLATE latin1_bin > */ > - if (collation() == &my_charset_latin1 && > - !(cl.collation()->state & MY_CS_PRIMARY)) > + if (is_contextually_typed_collate_default() && > + !cl.charset_collation()->default_flag()) > { > - my_error(ER_CONFLICTING_DECLARATIONS, MYF(0), > - "COLLATE ", "DEFAULT", "COLLATE ", > - cl.collation()->coll_name.str); > + error_conflicting_collations_or_styles(*this, cl); > return true; > } > - if (collation() == &my_charset_bin && > - !(cl.collation()->state & MY_CS_BINSORT)) > + > + if (is_contextually_typed_binary_style() && > + !cl.charset_collation()->binsort_flag()) > { > - my_error(ER_CONFLICTING_DECLARATIONS, MYF(0), > - "", "BINARY", "COLLATE ", cl.collation()->coll_name.str); > + error_conflicting_collations_or_styles(*this, cl); > return true; > } > *this= cl; > return false; > } > > - if (cl.is_contextually_typed_collation()) > - { > + DBUG_ASSERT(0); > + return false; > +} > + > + > +bool > +Lex_explicit_charset_opt_collate:: > + merge_collate_or_error(const Lex_charset_collation_st &cl) > +{ > + DBUG_ASSERT(cl.type() != Lex_charset_collation_st::TYPE_CHARACTER_SET); > + > + switch (cl.type()) { > + case Lex_charset_collation_st::TYPE_EMPTY: > + return false; > + case Lex_charset_collation_st::TYPE_CHARACTER_SET: > + DBUG_ASSERT(0); > + return false; > + case Lex_charset_collation_st::TYPE_COLLATE_EXACT: > /* > - EXPLICIT + CONTEXT > - CHAR(10) COLLATE latin1_bin .. COLLATE DEFAULT - not supported > - CHAR(10) COLLATE latin1_bin .. COLLATE uca1400_as_ci - not yet > + EXPLICIT + EXPLICIT > + CHAR(10) CHARACTER SET latin1 .. COLLATE latin1_bin > + CHAR(10) CHARACTER SET latin1 COLLATE latin1_bin .. COLLATE latin1_bin > + CHAR(10) COLLATE latin1_bin .. COLLATE latin1_bin > + CHAR(10) COLLATE latin1_bin .. COLLATE latin1_bin > + CHAR(10) CHARACTER SET latin1 BINARY .. COLLATE latin1_bin > */ > - DBUG_ASSERT(0); // Not possible yet > + if (m_with_collate && m_ci != cl.charset_collation()) > + { > + my_error(ER_CONFLICTING_DECLARATIONS, MYF(0), > + "COLLATE ", m_ci->coll_name.str, > + "COLLATE ", cl.charset_collation()->coll_name.str); > + return true; > + } > + if (!my_charset_same(m_ci, cl.charset_collation())) > + { > + my_error(ER_COLLATION_CHARSET_MISMATCH, MYF(0), > + cl.charset_collation()->coll_name.str, m_ci->cs_name.str); > + return true; > + } > + m_ci= cl.charset_collation(); > + m_with_collate= true; > return false; > - } > > - /* > - EXPLICIT + EXPLICIT > - CHAR(10) CHARACTER SET latin1 .. COLLATE latin1_bin > - CHAR(10) CHARACTER SET latin1 COLLATE latin1_bin .. COLLATE latin1_bin > - CHAR(10) COLLATE latin1_bin .. COLLATE latin1_bin > - CHAR(10) COLLATE latin1_bin .. COLLATE latin1_bin > - CHAR(10) CHARACTER SET latin1 BINARY .. COLLATE latin1_bin > - */ > - if (type() == TYPE_EXPLICIT && collation() != cl.collation()) > - { > - my_error(ER_CONFLICTING_DECLARATIONS, MYF(0), > - "COLLATE ", collation()->coll_name.str, > - "COLLATE ", cl.collation()->coll_name.str); > - return true; > - } > - if (!my_charset_same(collation(), cl.collation())) > - { > - my_error(ER_COLLATION_CHARSET_MISMATCH, MYF(0), > - cl.collation()->coll_name.str, collation()->cs_name.str); > - return true; > + case Lex_charset_collation_st::TYPE_COLLATE_CONTEXTUALLY_TYPED: > + if (cl.is_contextually_typed_collate_default()) > + { > + /* > + SET NAMES latin1 COLLATE DEFAULT; > + ALTER TABLE t1 CONVERT TO CHARACTER SET latin1 COLLATE DEFAULT; > + */ > + CHARSET_INFO *tmp= Charset_loader_mysys().find_default_collation(m_ci); > + if (!tmp) > + return true; > + m_ci= tmp; > + m_with_collate= true; > + return false; > + } > + else > + { > + /* > + EXPLICIT + CONTEXT > + CHAR(10) COLLATE latin1_bin .. COLLATE DEFAULT not possible yet > + CHAR(10) COLLATE latin1_bin .. COLLATE uca1400_as_ci > + */ > + > + const LEX_CSTRING context_cl_name= cl.collation_name_context_suffix(); > + if (!strncasecmp(context_cl_name.str, STRING_WITH_LEN("uca1400_"))) Like above, better DBUG_ASSERT(!strncasecmp(context_cl_name.str, STRING_WITH_LEN("uca1400_"))) > + { > + CHARSET_INFO *tmp; > + Charset_loader_mysys loader; > + if (!(tmp= loader.get_contextually_typed_collation_or_error(m_ci, > + context_cl_name.str))) > + return true; > + m_with_collate= true; > + m_ci= tmp; > + return false; > + } > + > + DBUG_ASSERT(0); // Not possible yet > + return false; > + } > } > - *this= cl; > + DBUG_ASSERT(0); > return false; > } > > diff --git a/strings/ctype-uca.c b/strings/ctype-uca.c > index b89916f3b20..3e6b4e4ce43 100644 > --- a/strings/ctype-uca.c > +++ b/strings/ctype-uca.c > @@ -30542,7 +30613,7 @@ static const char vietnamese[]= > Myanmar, according to CLDR Revision 8900. > http://unicode.org/cldr/trac/browser/trunk/common/collation/my.xml > */ > -static const char myanmar[]= "[shift-after-method expand][version 5.2.0]" > +static const char myanmar[]= "[shift-after-method expand]" What's going on with myanmar? You removed a version here and added &my_uca_v520 below in its charset_info_st. What does this change mean? > /* Tones */ > "&\\u108C" > "<\\u1037" > @@ -37627,7 +37825,7 @@ struct charset_info_st my_charset_utf32_myanmar_uca_ci= > NULL, /* to_lower */ > NULL, /* to_upper */ > NULL, /* sort_order */ > - NULL, /* uca */ > + &my_uca_v520, /* uca */ What does this change? > NULL, /* tab_to_uni */ > NULL, /* tab_from_uni */ > &my_unicase_unicode520,/* caseinfo */ Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

[Maria-developers] Review #3 for: MDEV-26278 Elimination: consider GROUP BY as unique key
by Sergey Petrunia 13 Jun '22

13 Jun '22

Hi Oleg, Please fix compile errors on this platform: https://buildbot.mariadb.org/#/builders/146/builds/13126/steps/5/logs/stdio also please apply the attached diff that fixes the coding style and wording of comments (input and/or adjustments are welcome). After this, the patch is ok to push. The next steps are to create a branch with name like preview-10.10-mdev26278-table-elimination and to officially submit it for preview tree build and testing. > commit 7275bf4939248959b5017314d6b0393296831c0c (HEAD, origin/bb-10.9-MDEV-26278-look) > Author: Oleg Smirnov <olernov(a)gmail.com> > Date: Thu Feb 17 22:53:37 2022 +0700 > > MDEV-26278 Add functionality to eliminate derived tables from the query > > Elimination of unnecessary tables from SQL queries is already present > in MariaDB. But it only works for regular tables and not for derived ones. > > Imagine we have a view: > CREATE VIEW v1 AS SELECT a, b, max(c) AS maxc FROM t1 GROUP BY a, b > > Due to "GROUP BY a, b" the values of combinations {a, b} are unique, > and this fact can be treated as like derived table "v1" has a unique key > on fields {a, b}. > > Suppose we have a SQL query: > SELECT t2.* FROM t2 LEFT JOIN v1 ON t2.a=v1.a and t2.b=v1.b > > 1. Since {v1.a, v1.b} is unique and both these fields are bound to t2, > "v1" is functionally dependent on t2. > This means every record of "t2" will be either joined with > a single record of "v1" or NULL-complemented. > 2. No fields of "v1" are present on the SELECT list > > These two facts allow the server to completely exclude (eliminate) > the derived table "v1" from the query. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

2 1

Re: [Maria-developers] 8c2c0e6079a: MDEV-27265 Improve contraction performance in UCA collations
by Sergei Golubchik 11 Jun '22

11 Jun '22

Hi, Alexander, Why haven't you used HASH? On Jun 11, Alexander Barkov wrote: > revision-id: 8c2c0e6079a (mariadb-10.9.1-13-g8c2c0e6079a) > parent(s): 27fa5c10eff > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-05-26 13:17:34 +0400 > message: > > MDEV-27265 Improve contraction performance in UCA collations > > Adding a hash table for contractions. > > The old code iterated through all items in MY_CONTRACTIONS, > and was much slower, especially for those contractions > in the end of the list. > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] MDEV-26278 Elimination: consider GROUP BY as unique key
by Sergey Petrunia 09 Jun '22

09 Jun '22

Hi Oleg, Please find the review below. > commit 20609d374b3a82c46291ee5e87cfadd26de47817 > Author: Oleg Smirnov <olernov(a)gmail.com> > Date: Thu Feb 17 22:53:37 2022 +0700 > > MDEV-26278 Elimination: consider GROUP BY as unique key Please provide a more verbose description. We should aim for it to provide sufficient info for one to understand what the optimization does and how that is achieved. Some general comments: The patch uses this formatting for multi-line comments: > + /* GROUP BY expression is considered as a synthetic "unique key" > + for the derived table. Add this key as a dependency */ While the coding style and actual MariaDB code would use this: /* GROUP BY expression is considered as a synthetic "unique key" for the derived table. Add this key as a dependency */ Please use this style for all comments. Also, the patch uses the term "synthetic unique key". I think the term is not very good: It can be confused with "synthetic primary key", also I read "synthetic" as something that actually exists, while in our case the key doesn't exist. Would using "Pseudo-key" be better? > diff --git a/sql/table.h b/sql/table.h > index 497502b2d06..fd598556cc3 100644 > --- a/sql/table.h > +++ b/sql/table.h > @@ -2872,6 +2872,9 @@ struct TABLE_LIST > } > } > > + void mark_derived_as_eliminated() { m_is_derived_eliminated= true; } > + bool is_derived_eliminated() const { return m_is_derived_eliminated; } > + So, TABLE_LIST objects are created non-eliminated and then they are eliminated, and that is forever. TABLE_LIST object has the same lifetime as the statement, that is, it will survive multiple executions of a prepared statement. On the other hand, Table Elimination optimization is done in eliminate_tables() call which is invoked for every statement execution. This looks like a dangerous mismatch. I'll continue below > private: > bool prep_check_option(THD *thd, uint8 check_opt_type); > bool prep_where(THD *thd, Item **conds, bool no_where_clause); > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > index 3bf6d9abf57..02ef5cd7ad7 100644 > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -11786,7 +11786,7 @@ bool SELECT_LEX_UNIT::explainable() const > EXPLAIN/ANALYZE unit, when: > (1) if it's a subquery - it's not part of eliminated WHERE/ON clause. > (2) if it's a CTE - it's not hanging (needed for execution) > - (3) if it's a derived - it's not merged > + (3) if it's a derived - it's not merged or eliminated > if it's not 1/2/3 - it's some weird internal thing, ignore it > */ > return item ? > @@ -11795,6 +11795,7 @@ bool SELECT_LEX_UNIT::explainable() const > derived && derived->derived_result && > !with_element->is_hanging_recursive(): // (2) > derived ? > - derived->is_materialized_derived() : // (3) > + derived->is_materialized_derived() && // (3) > + !derived->is_derived_eliminated() : > false; > } I think, there's no need for m_is_derived_eliminated. You can have this check in is_derived_eliminated(): this->derived->table->map & this->outer_select()->join->eliminated_tables Please try doing this. > diff --git a/mysql-test/main/table_elim.result b/mysql-test/main/table_elim.result > index deff0623370..842330f3713 100644 > --- a/mysql-test/main/table_elim.result > +++ b/mysql-test/main/table_elim.result > @@ -704,3 +704,252 @@ LIMIT 1; > PostID Voted > 1 NULL > DROP TABLE t1,t2; > +# > +# MDEV-26278: Table elimination does not work across derived tables > +# > +create table t1 (a int, b int); > +insert into t1 select seq, seq+10 from seq_1_to_10; > +create table t11 ( > +a int not null, > +b int, > +key(a) > +); > +insert into t11 select A.seq, A.seq+B.seq > +from > +seq_1_to_100 A, > +seq_1_to_1000 B; Do you really need 100K rows table? I think 10K rows will be just as good. > diff --git a/sql/opt_table_elimination.cc b/sql/opt_table_elimination.cc > index 8c4720bdec4..fe2155191ca 100644 > --- a/sql/opt_table_elimination.cc > +++ b/sql/opt_table_elimination.cc ... > @@ -278,6 +286,8 @@ class Dep_value_field : public Dep_value > Dep_module_key *key_dep; > /* Otherwise, this and advance */ > uint equality_no; > + > + Dep_module_synthetic_key *synth_key_dep; Please add a comment for the above that we should try that one, too. @@ -447,6 +463,55 @@ const size_t Dep_module::iterator_size= > MY_MAX(Dep_module_expr::iterator_size, Dep_module_key::iterator_size); > > > +/* A synthetic unique key module for a derived table. > + For example, a derived table > + SELECT t11.a, count(*) from t1 LEFT JOIN t2 ON t1.id = t2.fk GROUP BY t11.a > + has unique values in its first field (t11.a) due to GROUP BY expression > + so this can be considered as a unique key for this derived table > +*/ > + > +class Dep_module_synthetic_key : public Dep_module > +{ > +public: > + Dep_module_synthetic_key(Dep_value_table *table_arg, > + std::vector<field_index_t>&& field_indexes) > + : table(table_arg), derived_table_field_indexes(field_indexes) > + { > + unbound_args= static_cast<uint>(field_indexes.size()); > + } > + > + Dep_value_table * table; Coding style: "*table". ... > @@ -1603,7 +1674,73 @@ Dep_value_table *Dep_analysis_context::create_table_value(TABLE *table) > key_list= &(key_dep->next_table_key); > } > } > - return table_deps[table->tablenr]= tbl_dep; > + > + auto select_unit= table_list->get_unit(); > + SELECT_LEX *first_select= nullptr; > + if (select_unit) Please move the code that creates the pseudo-key into a separate function. ... > + > +int Dep_analysis_context::find_field_in_list(List<Item> &fields_list, > + Item *field) Please add a one-line comment describing what function does. > @@ -1786,6 +1941,18 @@ Dep_module* Dep_value_field::get_next_unbound_module(Dep_analysis_context *dac, > } > else > di->key_dep= NULL; > + > + Dep_module_synthetic_key *synth_key_dep= di->synth_key_dep; > + if (synth_key_dep && !synth_key_dep->is_applicable() && > + std::find(synth_key_dep->derived_table_field_indexes.begin(), > + synth_key_dep->derived_table_field_indexes.end(), > + field->field_index) != std::end(synth_key_dep->derived_table_field_indexes)) Please introduce a method like bool Dep_module_synthetic_key::covers_field(int field_index); Will this also allow to make Dep_module_synthetic_key::derived_table_field_indexes a private member? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 1

[Maria-developers] Introduction of Weijun
by Weijun Huang 07 Jun '22

07 Jun '22

Hi everyone! My name is Weijun Huang and I was selected for GSoC to work with MariaDB. I'm from China and I am studying at Technical University of Denmark, majoring in Human-centered Artificial Intelligence. My nickname on Zulip is Weijun Huang, and on Github it's Weijun-H. My project for GSoC in 2022 is called "Add Regression Functions for MariaDB." I've already started to work with MariaDB Server. Now I'm doing the implementation of REGR_SXX, REGR_SYY, and REGR_SXY I'm looking forward to working on this project. It would be an excellent experience for me! Best regard Weijun Huang

1 0

Re: [Maria-developers] 2fdcfaaba09: MDEV-27832 disable binary logging for SQL SERVICE.
by Sergei Golubchik 06 May '22

06 May '22

Hi, Alexey, Looks good. Minor comments below. Ok to push after addressing them. On May 06, Alexey Botchkov wrote: > revision-id: 2fdcfaaba09 (mariadb-10.7.2-8-g2fdcfaaba09) > parent(s): 33fd136c61b > author: Alexey Botchkov > committer: Alexey Botchkov > timestamp: 2022-05-05 15:03:38 +0400 > message: > > MDEV-27832 disable binary logging for SQL SERVICE. > > Binary logging is now disabled for the queries run by SQL SERVICE. > The binlogging can be turned on with the 'SET SQL_LOG_BIN=On' query. > > diff --git a/mysql-test/suite/plugins/t/test_sql_service.test b/mysql-test/suite/plugins/t/test_sql_service.test > index b80d78fe6e5..0fd86fccd1a 100644 > --- a/mysql-test/suite/plugins/t/test_sql_service.test > +++ b/mysql-test/suite/plugins/t/test_sql_service.test > @@ -58,3 +73,7 @@ drop table t1; > > uninstall plugin test_sql_service; > > +--replace_column 2 # 5 # > +--replace_regex /xid=[0-9]+/xid=XX/ /GTID [0-9]+-[0-9]+-[0-9]+/GTID #-#-#/ > +SHOW BINLOG EVENTS LIMIT 3,100; use --source include/show_binlog_events.inc It does all the replaces, provides a unified output everywhere, it'll be updated if show binlog event output changes. No need to duplicate that. > + > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc > index e025147c71e..d3b77fbece3 100644 > --- a/sql/sql_prepare.cc > +++ b/sql/sql_prepare.cc > @@ -5617,12 +5617,26 @@ class Protocol_local : public Protocol_text > THD *new_thd; > Security_context empty_ctx; > > + my_bool do_log_bin; > + > Protocol_local(THD *thd_arg, THD *new_thd_arg, ulong prealloc) : > Protocol_text(thd_arg, prealloc), > cur_data(0), first_data(0), data_tail(&first_data), alloc(0), > - new_thd(new_thd_arg) > + new_thd(new_thd_arg), do_log_bin(FALSE) > {} > > + void set_binlog_vars(my_bool *sav_log_bin) > + { > + *sav_log_bin= thd->variables.sql_log_bin; > + thd->variables.sql_log_bin= do_log_bin; > + thd->set_binlog_bit(); > + } > + void restore_binlog_vars(my_bool &sav_log_bin) this doesn't need to be a reference, just a normal pass-by-value will do. > + { > + do_log_bin= thd->variables.sql_log_bin; > + thd->variables.sql_log_bin= sav_log_bin; > + thd->set_binlog_bit(); > + } > protected: > bool net_store_data(const uchar *from, size_t length); > bool net_store_data_cs(const uchar *from, size_t length, Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 9d6be81c0da: MDEV-27832 disable binary logging for SQL SERVICE.
by Sergei Golubchik 05 May '22

05 May '22

Hi, Alexey, On Mar 02, Alexey Botchkov wrote: > revision-id: 9d6be81c0da (mariadb-10.7.2-8-g9d6be81c0da) > parent(s): 33fd136c61b > author: Alexey Botchkov > committer: Alexey Botchkov > timestamp: 2022-02-15 13:37:59 +0400 > message: > > MDEV-27832 disable binary logging for SQL SERVICE. > > Binary logging is now disabled for the queries run by SQL SERVICE. > The binlogging can be turned on with the 'SET SQL_LOG_BIN=On' query. > > diff --git a/mysql-test/suite/plugins/r/test_sql_service.result b/mysql-test/suite/plugins/r/test_sql_service.result > index 00f0411b665..943bb239e5f 100644 > --- a/mysql-test/suite/plugins/r/test_sql_service.result > +++ b/mysql-test/suite/plugins/r/test_sql_service.result > @@ -2,6 +2,19 @@ install plugin test_sql_service soname 'test_sql_service'; > show status like 'test_sql_service_passed'; > Variable_name Value > Test_sql_service_passed 1 > +set global test_sql_service_execute_sql_global= 'create table test.t1 select 1 as a, @@SQL_LOG_BIN'; > +set global test_sql_service_execute_sql_local= 'insert into test.t1 select 2 as a, @@SQL_LOG_BIN'; > +set global test_sql_service_execute_sql_global= 'SET SQL_LOG_BIN=1'; > +set global test_sql_service_execute_sql_global= 'insert into test.t1 select 3 as a, @@SQL_LOG_BIN'; > +set global test_sql_service_execute_sql_global= 'SET SQL_LOG_BIN=0'; > +set global test_sql_service_execute_sql_global= 'insert into test.t1 select 4 as a, @@SQL_LOG_BIN'; > +select * from t1 order by a; this is not a test. Please, enable binlog and see whether queries are actually logged. Also, add a test for, say, sql_auto_is_null: set global test_sql_service_execute_sql_global='set sql_auto_is_null=1'; set global test_sql_service_execute_sql_global='insert test.t1 select 5, @@sql_auto_is_null'; select * from t1 where a=5; > +a @@SQL_LOG_BIN > +1 0 > +2 0 > +3 1 > +4 0 > +drop table t1; > set global test_sql_service_run_test= 1; > show status like 'test_sql_service_passed'; > Variable_name Value > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc > index e025147c71e..48681ac0b6f 100644 > --- a/sql/sql_prepare.cc > +++ b/sql/sql_prepare.cc > @@ -5617,12 +5617,30 @@ class Protocol_local : public Protocol_text > THD *new_thd; > Security_context empty_ctx; > > + my_bool do_log_bin; > + > Protocol_local(THD *thd_arg, THD *new_thd_arg, ulong prealloc) : > Protocol_text(thd_arg, prealloc), > cur_data(0), first_data(0), data_tail(&first_data), alloc(0), > - new_thd(new_thd_arg) > + new_thd(new_thd_arg), do_log_bin(FALSE) > {} > > + void set_binlog_vars(my_bool *sav_log_bin, ulonglong *sav_bits) > + { > + *sav_log_bin= thd->variables.sql_log_bin; > + *sav_bits= thd->variables.option_bits; > + > + if ((thd->variables.sql_log_bin= do_log_bin)) > + thd->variables.option_bits|= OPTION_BIN_LOG; > + else > + thd->variables.option_bits&= ~OPTION_BIN_LOG; > + } > + void restore_binlog_vars(my_bool &sav_log_bin, ulonglong &sav_bits) > + { > + do_log_bin= thd->variables.sql_log_bin; > + thd->variables.sql_log_bin= sav_log_bin; > + thd->variables.option_bits= sav_bits; that's what sql_auto_is_null test is for.You destroy all other option_bits here. Better don't save/restore option_bits, but use the same if() as in set_binlog_vars(). Or, even better, reuse fix_sql_log_bin_after_update(). I'd suggest to make it a method in THD > + } > protected: > bool net_store_data(const uchar *from, size_t length); > bool net_store_data_cs(const uchar *from, size_t length, > @@ -6230,12 +6248,17 @@ loc_advanced_command(MYSQL *mysql, enum enum_server_command command, > Ed_connection con(p->thd); > Security_context *ctx_orig= p->thd->security_ctx; > MYSQL_LEX_STRING sql_text; > + my_bool log_bin_orig; > + ulonglong option_bits_orig; > + p->set_binlog_vars(&log_bin_orig, &option_bits_orig); > + > DBUG_ASSERT(current_thd == p->thd); > sql_text.str= (char *) arg; > sql_text.length= arg_length; > p->thd->security_ctx= &p->empty_ctx; > result= con.execute_direct(p, sql_text); > p->thd->security_ctx= ctx_orig; > + p->restore_binlog_vars(log_bin_orig, option_bits_orig); > } > if (skip_check) > result= 0; > @@ -6391,6 +6414,9 @@ extern "C" MYSQL *mysql_real_connect_local(MYSQL *mysql) > new_thd->security_ctx->skip_grants(); > new_thd->query_cache_is_applicable= 0; > new_thd->variables.wsrep_on= 0; > + new_thd->variables.sql_log_bin= 0; > + new_thd->variables.option_bits&= ~OPTION_BIN_LOG; > + > /* > TOSO: decide if we should turn the auditing off > for such threads. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 4ba5b444b4b: MDEV-28437: Assertion `!eliminated' failed in Item_subselect::exec
by Sergei Golubchik 04 May '22

04 May '22

Hi, Sergei, On May 04, Sergei Petrunia wrote: > revision-id: 4ba5b444b4b (mariadb-10.2.43-97-g4ba5b444b4b) > parent(s): 70555454b4c > author: Sergei Petrunia > committer: Sergei Petrunia > timestamp: 2022-05-04 12:24:48 +0300 > message: > > MDEV-28437: Assertion `!eliminated' failed in Item_subselect::exec > > (This is the assert that was added in fix for MDEV-26047) > > Table elimination may remove an ON expression from an outer join. > However SELECT_LEX::update_used_tables() will still call > > item->walk(&Item::eval_not_null_tables) > > for eliminated expressions. If the subquery is constant and cheap > Item_cond_and will attempt to evaluate it, which will trigger an > assert. > The fix is not to call update_used_tables() or eval_not_null_tables() > for ON expressions that were eliminated. > > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > index 22ee8801e3a..8e718f2a942 100644 > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -4229,7 +4229,7 @@ void SELECT_LEX::update_used_tables() > } > } > while ((embedding= embedding->embedding)); > - if (tl->on_expr) > + if (tl->on_expr && !is_eliminated_table(join->eliminated_tables, tl)) > { > tl->on_expr->update_used_tables(); > tl->on_expr->walk(&Item::eval_not_null_tables, 0, NULL); > @@ -4253,8 +4253,11 @@ void SELECT_LEX::update_used_tables() > if (embedding->on_expr && > embedding->nested_join->join_list.head() == tl) > { can embedding be eliminated? if yes - does it make sense to do anything in tl's below that eliminated embedding? Perhaps it should be (in the do..while above), like do { bool maybe_null; if ((maybe_null= MY_TEST(embedding->outer_join))) { tl->table->maybe_null= maybe_null; break; } + if (is_eliminated_table(join->eliminated_tables, embedding)) + tl= embedding->embedding; } while ((embedding= embedding->embedding)); > + if (!is_eliminated_table(join->eliminated_tables, embedding)) > + { > embedding->on_expr->update_used_tables(); > embedding->on_expr->walk(&Item::eval_not_null_tables, 0, NULL); > + } > } > tl= embedding; > embedding= tl->embedding; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 85cc56875e9: MDEV-19398: Assertion `item1->type() == Item::FIELD_ITEM ...
by Sergei Golubchik 03 May '22

03 May '22

Hi, Sergei, The solutions looks fine Two comments below, about the implementation. ok to push after addressing them On May 03, Sergei Petrunia wrote: > revision-id: 85cc56875e9 (mariadb-10.2.43-75-g85cc56875e9) > parent(s): 3b6c04f44c4 > author: Sergei Petrunia > committer: Sergei Petrunia > timestamp: 2022-05-03 14:06:27 +0300 > message: > > MDEV-19398: Assertion `item1->type() == Item::FIELD_ITEM ... > > Window Functions code tries to minimize the number of times it > needs to sort the select's resultset by finding "compatible" > OVER (PARTITION BY ... ORDER BY ...) clauses. > > This employs compare_order_elements(). That function assumed that > the order expressions are Item_field-derived objects (that refer > to a temp.table). But this is not always the case: one can > construct queries order expressions are arbitrary item expressions. > > Add handling for such expressions: sort them according to the window > specification they appeared in. > This means we cannot detect that two compatible PARTITION BY clauses > that use expressions can share the sorting step. > But at least we won't crash. > can share the same sort > > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 989ca0c8803..457849a7569 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -8624,6 +8624,7 @@ bool st_select_lex::add_window_def(THD *thd, > fields_in_window_functions+= win_part_list_ptr->elements + > win_order_list_ptr->elements; > } > + win_def->win_spec_number= window_specs.elements; > return (win_def == NULL || window_specs.push_back(win_def)); > } > > @@ -8651,6 +8652,7 @@ bool st_select_lex::add_window_spec(THD *thd, > win_order_list_ptr->elements; > } > thd->lex->win_spec= win_spec; > + win_spec->win_spec_number= window_specs.elements; > return (win_spec == NULL || window_specs.push_back(win_spec)); > } > > diff --git a/sql/sql_window.cc b/sql/sql_window.cc > index 3ef751bc5b9..fbf7aca0197 100644 > --- a/sql/sql_window.cc > +++ b/sql/sql_window.cc > @@ -312,15 +312,44 @@ setup_windows(THD *thd, Ref_ptr_array ref_pointer_array, TABLE_LIST *tables, > #define CMP_GT 2 // Greater then > > static > -int compare_order_elements(ORDER *ord1, ORDER *ord2) > +int compare_order_elements(ORDER *ord1, int weight1, > + ORDER *ord2, int weight2) > { > if (*ord1->item == *ord2->item && ord1->direction == ord2->direction) > return CMP_EQ; > Item *item1= (*ord1->item)->real_item(); > Item *item2= (*ord2->item)->real_item(); > - DBUG_ASSERT(item1->type() == Item::FIELD_ITEM && > - item2->type() == Item::FIELD_ITEM); > - ptrdiff_t cmp= ((Item_field *) item1)->field - ((Item_field *) item2)->field; > + > + bool item1_field= (item1->type() == Item::FIELD_ITEM); > + bool item2_field= (item2->type() == Item::FIELD_ITEM); > + > + ptrdiff_t cmp; > + if (item1_field && item2_field) > + cmp= ((Item_field *) item1)->field - ((Item_field *) item2)->field; 1. why is this order stable? I'd rather sort by item->field->field_index 2. please, add an assert, that item1->field->table == item2->field->table > + else if (item1_field && !item2_field) > + return CMP_LT; > + else if (!item1_field && item2_field) > + return CMP_LT; > + else > + { > + /* > + Ok, item1_field==NULL and item2_field==NULL. > + We're not able to compare Item expressions. Order them according to > + their passed "weight" (which comes from Window_spec::win_spec_number): > + */ > + if (weight1 != weight2) > + cmp= weight1 - weight2; > + else > + { > + /* > + The weight is the same. That is, the elements come from the same > + window specification... This shouldn't happen. > + */ > + DBUG_ASSERT(0); > + cmp= item1 - item2; > + } > + } > + > if (cmp == 0) > { Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 1c0731e0544: MDEV-26695: Number of an invalid row is not calculated for table value constructor
by Sergei Golubchik 26 Apr '22

26 Apr '22

Hi, Rucha, Looks good. please also check that row_number values match what's in a warning, and then ok to push! On Apr 26, Rucha Deodhar wrote: > On Tue, Apr 26, 2022 at 12:41 AM Sergei Golubchik <serg(a)mariadb.org> wrote: > > what if you combine it with union? > > create table t (a char(1)) values ('a'),('b') union values ('foo'); > > > > please, add a test for that. > > > > > It gives 3 as row the number (in warning). Added a test for it. > Please find it here: 1c0731e0544db4d2f5c107473776bbd6d4d3e2f0 > > > On Apr 25, Rucha Deodhar wrote: > > > revision-id: 1c0731e0544 (mariadb-10.7.1-33-g1c0731e0544) > > > parent(s): 5e6fd4e8043 > > > author: Rucha Deodhar > > > committer: Rucha Deodhar > > > timestamp: 2022-01-23 13:48:18 +0530 > > > message: > > > > > > MDEV-26695: Number of an invalid row is not calculated for table value > > > constructor > > > > > > Analysis: counter does not increment while sending rows for table value > > > constructor and so row_number assumes the default value (0 in this case). > > > Fix: Increment the counter to avoid counter using default value. > > > > > > diff --git a/sql/sql_tvc.cc b/sql/sql_tvc.cc > > > index bdaf6829fbd..7db0ab217ca 100644 > > > --- a/sql/sql_tvc.cc > > > +++ b/sql/sql_tvc.cc > > > @@ -422,7 +422,9 @@ bool table_value_constr::exec(SELECT_LEX *sl) > > > DBUG_ENTER("table_value_constr::exec"); > > > List_iterator_fast<List_item> li(lists_of_values); > > > List_item *elem; > > > + THD *cur_thd= sl->parent_lex->thd; > > > ha_rows send_records= 0; > > > + int rc=0; > > > > > > if (select_options & SELECT_DESCRIBE) > > > DBUG_RETURN(false); > > > @@ -438,10 +440,10 @@ bool table_value_constr::exec(SELECT_LEX *sl) > > > > > > while ((elem= li++)) > > > { > > > + cur_thd->get_stmt_da()->inc_current_row_for_warning(); > > > if (send_records >= sl->master_unit()->lim.get_select_limit()) > > > break; > > > - int rc= > > > - result->send_data_with_check(*elem, sl->master_unit(), > > send_records); > > > + rc= result->send_data_with_check(*elem, sl->master_unit(), > > send_records); > > > > you don't need to do that anymore (it was needed in your first patch, > > but not here) > > > > Removed. > > > > if (!rc) > > > send_records++; > > > else if (rc > 0) > > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 1c0731e0544: MDEV-26695: Number of an invalid row is not calculated for table value constructor
by Sergei Golubchik 25 Apr '22

25 Apr '22

Hi, Rucha, what if you combine it with union? create table t (a char(1)) values ('a'),('b') union values ('foo'); please, add a test for that. On Apr 25, Rucha Deodhar wrote: > revision-id: 1c0731e0544 (mariadb-10.7.1-33-g1c0731e0544) > parent(s): 5e6fd4e8043 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2022-01-23 13:48:18 +0530 > message: > > MDEV-26695: Number of an invalid row is not calculated for table value > constructor > > Analysis: counter does not increment while sending rows for table value > constructor and so row_number assumes the default value (0 in this case). > Fix: Increment the counter to avoid counter using default value. > > diff --git a/sql/sql_tvc.cc b/sql/sql_tvc.cc > index bdaf6829fbd..7db0ab217ca 100644 > --- a/sql/sql_tvc.cc > +++ b/sql/sql_tvc.cc > @@ -422,7 +422,9 @@ bool table_value_constr::exec(SELECT_LEX *sl) > DBUG_ENTER("table_value_constr::exec"); > List_iterator_fast<List_item> li(lists_of_values); > List_item *elem; > + THD *cur_thd= sl->parent_lex->thd; > ha_rows send_records= 0; > + int rc=0; > > if (select_options & SELECT_DESCRIBE) > DBUG_RETURN(false); > @@ -438,10 +440,10 @@ bool table_value_constr::exec(SELECT_LEX *sl) > > while ((elem= li++)) > { > + cur_thd->get_stmt_da()->inc_current_row_for_warning(); > if (send_records >= sl->master_unit()->lim.get_select_limit()) > break; > - int rc= > - result->send_data_with_check(*elem, sl->master_unit(), send_records); > + rc= result->send_data_with_check(*elem, sl->master_unit(), send_records); you don't need to do that anymore (it was needed in your first patch, but not here) > if (!rc) > send_records++; > else if (rc > 0) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 585cd1f52e7: MDEV-26875: Wrong user in SET DEFAULT ROLE error
by Sergei Golubchik 25 Apr '22

25 Apr '22

Hi, Anel, On Apr 25, Anel Husakovic wrote: > revision-id: 585cd1f52e7 (mariadb-10.5.14-10-g585cd1f52e7) > parent(s): 52b32c60c26 > author: Anel Husakovic > committer: Anel Husakovic > timestamp: 2022-02-14 13:59:24 +0100 > message: > > MDEV-26875: Wrong user in SET DEFAULT ROLE error > > - Caused by 7c02e8717de5, where 957cb7b7ba35 introduced the bug. > > Reviewed by: > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index c4e66cf5d73..e83bc5635dc 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -3277,10 +3277,14 @@ static int check_user_can_set_role(THD *thd, const char *user, > check_role_is_granted_callback, > NULL) == -1)) > { > - /* Role is not granted but current user can see the role */ > - my_printf_error(ER_INVALID_ROLE, "User %`s@%`s has not been granted role %`s", > - MYF(0), thd->security_ctx->priv_user, > - thd->security_ctx->priv_host, rolename); > + /* If the SET ROLE is applied on the anonymous user, host is null */ > + if (!host) > + my_printf_error(ER_INVALID_ROLE, "User %`s@%`s has not been granted role %`s", > + MYF(0), thd->security_ctx->priv_user, thd->security_ctx->priv_host, rolename); I don't understand it. 1) what does it mean? why for anonymous user you print priv_user@priv_host ? 2) do you have any tests for that? There are none in the commit > + else > + /* Role is not granted but current user can see the role */ > + my_printf_error(ER_INVALID_ROLE, "User %`s@%`s has not been granted role %`s", > + MYF(0), user, host, rolename); > } > else > { Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 5f8ffc7271a: MDEV-20207: Assertion `! is_set()' failed in Diagnostics_area::set_eof_status upon HANDLER READ
by Sergei Golubchik 25 Apr '22

25 Apr '22

Hi, Rucha, The fix is ok to push. But as a general comment, better try to define variables in the smallest scope where they're used. In particular here it could've been { MY_BITMAP *old_map= dbug_tmp_use_all_columns(table, &table->write_set); - (void) item->save_in_field(key_part->field, 1); + int res= item->save_in_field(key_part->field, 1); because you don't need `res` outside of that scope. Makes the code easier to read. Of course, there can be exceptions. For example, if the variable is an object and its initialization is very expensive. But whether you move `int res` declaration inside or not - it's ok to push, no need to ask for another review. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org On Apr 25, Rucha Deodhar wrote: > revision-id: 5f8ffc7271a (mariadb-10.2.40-251-g5f8ffc7271a) > parent(s): 2cbf92522b5 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2022-01-27 01:18:19 +0530 > message: > > MDEV-20207: Assertion `! is_set()' failed in Diagnostics_area::set_eof_status > upon HANDLER READ > > Analysis: The error state is not stored while checking condition and key name. > Fix: Return true while checking condition and key name if error is reported > because geometry object can't be created from the data in the index value for > HANDLER READ. > > diff --git a/sql/sql_handler.cc b/sql/sql_handler.cc > index ec3756eceba..7c002a68d9e 100644 > --- a/sql/sql_handler.cc > +++ b/sql/sql_handler.cc > @@ -554,6 +554,7 @@ mysql_ha_fix_cond_and_key(SQL_HANDLER *handler, > { > THD *thd= handler->thd; > TABLE *table= handler->table; > + int res= 0; > if (cond) > { > /* This can only be true for temp tables */ > @@ -613,8 +614,10 @@ mysql_ha_fix_cond_and_key(SQL_HANDLER *handler, > if (!in_prepare) > { > MY_BITMAP *old_map= dbug_tmp_use_all_columns(table, &table->write_set); > - (void) item->save_in_field(key_part->field, 1); > + res= item->save_in_field(key_part->field, 1); > dbug_tmp_restore_column_map(&table->write_set, old_map); > + if (res) > + return 1; > } > key_len+= key_part->store_length; > keypart_map= (keypart_map << 1) | 1;

1 0

Re: [Maria-developers] 14cc679c95e: MDEV-27831 Let the SQL SERVICE user set the current user name.
by Sergei Golubchik 22 Apr '22

22 Apr '22

Hi, Alexey, On Mar 04, Alexey Botchkov wrote: > revision-id: 14cc679c95e (mariadb-10.7.2-8-g14cc679c95e) > parent(s): 33fd136c61b > author: Alexey Botchkov > committer: Alexey Botchkov > timestamp: 2022-02-15 11:35:18 +0400 > message: > > MDEV-27831 Let the SQL SERVICE user set the current user name. > > The 'user' argument added to the mysql_real_connect_local. I think this is wrong on many levels. A plugin name is already known inside the plugin, you shouldn't force the plugin to pass its own name as an argument in multiple places, the server should determine it automatically. You should not set current_user to an arbitrary string for audit plugin to see it. current_user is the name of the user account and it's used in many places as such. Try, for example, to create a view or a stored procedure. Who will be a definer? Setting only user() might be ok. Setting @@proxy_user or @@external_user is even better, if your audit plugin can show them. @@external_user would be the best, I think it's purely informational. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] 96b40a5e2f5: 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 22 Apr '22

22 Apr '22

Hi, Alexander, Some comments/questions here. I'll need to look at the patch again after I understand why you did it this way better. On Apr 18, Alexander Barkov wrote: > revision-id: 96b40a5e2f5 (mariadb-10.3.33-61-g96b40a5e2f5) > parent(s): 7355f7b1f5c > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-04-07 16:22:17 +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 A detailed comment here is a must. What was the problem, how it was fixed. > diff --git a/sql/item_func.h b/sql/item_func.h > index 754b1cd1eb2..d844e45280a 100644 > --- a/sql/item_func.h > +++ b/sql/item_func.h > @@ -35,6 +35,36 @@ extern "C" /* Bug in BSDI include file */ > #include <cmath> > > > +template<class FUNC> > +FUNC* item_func_create_2_3_args(THD *thd, const LEX_CSTRING &name, > + List<Item> *item_list) > +{ > + int arg_count= item_list ? item_list->elements : 0; > + > + switch (arg_count) { > + case 2: > + { > + Item *param_1= item_list->pop(); > + Item *param_2= item_list->pop(); > + return new (get_thd_memroot(thd)) FUNC(thd, param_1, param_2); > + } > + case 3: > + { > + Item *param_1= item_list->pop(); > + Item *param_2= item_list->pop(); > + Item *param_3= item_list->pop(); > + return new (get_thd_memroot(thd)) FUNC(thd, param_1, param_2, param_3); A constructor that takes a List<Item> wouldn't need a switch. And wouldn't need a template either, your create_check_args_ge() isn't a template. > + break; > + } > + default: > + my_error(ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT, MYF(0), name.str); > + break; > + } > + > + return NULL; > +} > + > + > class Item_func :public Item_func_or_sum > { > void sync_with_sum_func_and_with_field(List<Item> &list); > @@ -56,8 +86,41 @@ 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_name_if_needed(String *to) const > + { > + const LEX_CSTRING schema_name= schema_name_cstring(); > + if (schema_name.length) > + { > + to->append(schema_name); > + to->append('.'); > + } this should check that schema_name isn't the same as the schema in the path (implicit path, assumed by the current sql_mode). > + } > + void print_maybe_qualified_name(String *to) const > + { > + print_schema_name_if_needed(to); > + to->append(func_name()); > + } > + > 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) > + { > + if (!item_list || item_list->elements < expected) > + { > + my_error(ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT, MYF(0), name.str); what's the point of your wrong_param_count_error() if you aren't using it? > + return true; > + } > + return false; > + } > + > table_map not_null_tables_cache; > > enum Functype { UNKNOWN_FUNC,EQ_FUNC,EQUAL_FUNC,NE_FUNC,LT_FUNC,LE_FUNC, > diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc > index 75abd9906cd..e45fe6ef6d7 100644 > --- a/sql/item_strfunc.cc > +++ b/sql/item_strfunc.cc > @@ -3204,6 +3239,39 @@ static String *default_pad_str(String *pad_str, CHARSET_INFO *collation) > return pad_str; > } > > + > +Item_func_lpad* > +Item_func_lpad::create(THD *thd, const LEX_CSTRING &name, > + List<Item> *item_list) > +{ > + return item_func_create_2_3_args<Item_func_lpad>(thd, name, item_list); may be item_func_create_2_3_args should be in Item_func? Like create_check_args_ge() is? and may be create_check_args_ge() should be a template too? > +} > + > + > +Item_func_lpad_oracle* > +Item_func_lpad_oracle::create(THD *thd, const LEX_CSTRING &name, > + List<Item> *item_list) > +{ > + return item_func_create_2_3_args<Item_func_lpad_oracle>(thd, name, item_list); > +} > + > + > +Item_func_rpad* > +Item_func_rpad::create(THD *thd, const LEX_CSTRING &name, > + List<Item> *item_list) > +{ > + return item_func_create_2_3_args<Item_func_rpad>(thd, name, item_list); > +} > + > + > +Item_func_rpad_oracle* > +Item_func_rpad_oracle::create(THD *thd, const LEX_CSTRING &name, > + List<Item> *item_list) > +{ > + return item_func_create_2_3_args<Item_func_rpad_oracle>(thd, name, item_list); > +} > + > + > bool Item_func_pad::fix_length_and_dec() > { > if (arg_count == 3) > 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 > @@ -1542,7 +1541,18 @@ int Lex_input_stream::lex_one_token(YYSTYPE *yylval, THD *thd) > if (lex->parsing_options.lookup_keywords_after_qualifier) > next_state= MY_LEX_IDENT_OR_KEYWORD; > else > - next_state= MY_LEX_IDENT_START; // Next is ident (not keyword) > + { > + /* > + Next is: > + - A qualified func with a special syntax: > + mariadb_schema.REPLACE('a','b','c') > + mariadb_schema.SUSTRING('a',1,2) > + mariadb_schema.TRIM('a') > + - Or an identifier otherwise. No keyword lookup is done, > + all keywords are treated as identifiers. > + */ > + next_state= MY_LEX_IDENT_OR_QUALIFIED_SPECIAL_FUNC; I don't understand why you did it in the lexer and not in the parser, like - | REPLACE '(' expr ',' expr ',' expr ')' + | opt_schema REPLACE '(' expr ',' expr ',' expr ')' or (if the above won't work in LARL(1) parser) - | ident_cli '.' ident_cli '(' opt_expr_list ')' + | ident_cli '.' func_2nd_part with + func_2nd_part: ident_cli '(' opt_expr_list ')' + | REPLACE '(' expr ',' expr ',' expr ')' etc (not reviewing lexer changes anymore, until I understand the above) > + } > if (!ident_map[(uchar) yyPeek()]) // Probably ` or " > next_state= MY_LEX_START; > return((int) c); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 4

Re: [Maria-developers] 17548c8a8b6: MDEV-25477 Auto-create breaks replication when triggering event was not replicated
by Sergei Golubchik 20 Apr '22

20 Apr '22

Hi, Aleksey, Basically, it's fine, but I still didn't like that there's a new flag in THD to mark a statement that has to be logged. So I tried to change to log_current_statement. I know you tried that and you told me what tests fails, rpl.create_or_replace_*. I debugged that a bit, and I think the problem is that select_create inherits from select_insert, so CREATE ... SELECT runs your code that is only supposed to be run for INSERT ODKU or REPLACE. This change makes all tests pass (after removing vers_created_partitions): @@ -4225,7 +4225,8 @@ bool select_insert::prepare_eof() thd->clear_error(); else errcode= query_error_code(thd, killed_status == NOT_KILLED); - StatementBinlog stmt_binlog(thd, thd->binlog_need_stmt_format(trans_table)); + StatementBinlog stmt_binlog(thd, !can_rollback_data() && + thd->binlog_need_stmt_format(trans_table)); res= thd->binlog_query(THD::ROW_QUERY_TYPE, thd->query(), thd->query_length(), trans_table, FALSE, FALSE, errcode); On Apr 10, Aleksey Midenkov wrote: > revision-id: 17548c8a8b6 (mariadb-10.6.1-250-g17548c8a8b6) > parent(s): 6282e025a0e > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2022-03-07 23:49:35 +0300 > message: > > MDEV-25477 Auto-create breaks replication when triggering event was not replicated > > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 686e6e70766..4fed1bf7355 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -2914,6 +2914,13 @@ class THD: public THD_count, /* this must be first */ > int binlog_flush_pending_rows_event(bool stmt_end, bool is_transactional); > int binlog_remove_pending_rows_event(bool clear_maps, bool is_transactional); > > + bool binlog_need_stmt_format(bool is_transactional) const > + { > + return vers_created_partitions && !binlog_get_pending_rows_event(is_transactional); > + } > + > + bool binlog_for_noop_dml(bool transactional_table); > + > /** > Determine the binlog format of the current statement. > > @@ -3557,6 +3564,7 @@ class THD: public THD_count, /* this must be first */ > bool tablespace_op; /* This is TRUE in DISCARD/IMPORT TABLESPACE */ > /* True if we have to log the current statement */ > bool log_current_statement; > + uint vers_created_partitions; > /** > True if a slave error. Causes the slave to stop. Not the same > as the statement execution error (is_error()), since > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index 707b8a0d3bf..25551dfa197 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -4223,6 +4225,7 @@ bool select_insert::prepare_eof() > thd->clear_error(); > else > errcode= query_error_code(thd, killed_status == NOT_KILLED); > + StatementBinlog stmt_binlog(thd, thd->binlog_need_stmt_format(trans_table)); > res= thd->binlog_query(THD::ROW_QUERY_TYPE, > thd->query(), thd->query_length(), > trans_table, FALSE, FALSE, errcode); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 8

Re: [Maria-developers] 93493a0e9b5: MDEV-24176 Server crashes after insert in the table with virtual column generated using date_format() and if()
by Sergei Golubchik 16 Apr '22

16 Apr '22

Hi, Aleksey! Could you please explain in the commit comment what the bug was and what you are fixing? I wasn't able to understad that, so I had to debug those crashes myself. So, the reason is, objects allocated in the wrong arena, right? And it was done inconsistently, on different code paths the "re-fix" context was different. I agree that encapsulating all the context in one Vcol_expr_context is a good idea, it'll allow to have the same, correct, context everywhere. But always using expr_arena doesn't necessarily seem to be a correct choice. E.g. Arg_comparator creates Item_cache_temporal on every fix_field. If vcol expr is re-fixed all the time, expr_arena will grow continuously. Same for charset conversion, CASE, IN, etc. I suspect that if the vcol expr is always re-fixed, it has to use stmt arena. Why do you re-fix fields? I can hardly imagine that an Item_field can find itself in a different table. And if it's always in the same table, why to force it to find this table over and over? We've already discussed VCOL_TABLE_ALIAS, no need to go there again. On Jan 06, Aleksey Midenkov wrote: > revision-id: 93493a0e9b5 (mariadb-10.2.40-194-g93493a0e9b5) > parent(s): a565e63c54c > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2021-12-29 00:46:31 +0300 > message: > > MDEV-24176 Server crashes after insert in the table with virtual > column generated using date_format() and if() > > When table is reopened from cache vcol_info contains stale > expression. We refresh expression via TABLE::vcol_fix_exprs() but > first we must prepare a proper context (Vcol_expr_context) and do some > preparations which meet some requirements: > > 1. fields must not be fixed, we unfix them with cleanup() via > cleanup_processor. > > 2. Item_ident must have cached_table cleared. Again, this is stale > data which was already freed. cached_table interferes with > find_field_in_tables(). > > We cannot clear cached_table directly in Item_ident::cleanup() > method. Lots of spaghetti logic depends on cached_table existence even > after cleanup() done. > > 3. If Item_ident has table_name non-NULL we trigger expr update. That > is done via Item_ident::check_vcol_func_processor() and > VCOL_TABLE_ALIAS flag. > > (4-7) The below conditions are prepared by Vcol_expr_context and used in > both fix_session_expr_for_read() and TABLE::vcol_fix_exprs(): > > 4. Any fix_expr() must be done on expr_arena as there may be new items > created. It was a bug in fix_session_expr_for_read(). It was just not > reproduced because there were no second refix. Now refix is done for > more cases so it does reproduce. Tests affected: vcol.binlog > > 5. Also name resolution context must be narrowed to the single table > with all crutches in place. Tests affected: vcol.update > > 6. sql_mode must be clean and not fail expr update. > > sql_mode such as MODE_NO_BACKSLASH_ESCAPES, MODE_NO_ZERO_IN_DATE, etc > must not affect vcol expression update. If the table was created > successfully any further evaluation must not fail. Tests affected: > main.func_like > > 7. Warnings are disabled during expr update. It is assumed that these > warnings were printed before during initialization phase or later > during execution phase. Tests affected: vcol.wrong_arena > > Dictionary: refresh, update and refix are the synonyms for > vcol_fix_exprs() or fix_session_expr_for_read() calls. > > 8. cleanup_excluding_fields_processor() removed. It was just a quick > hack to exclude wrongly working Item_field from processing. Now it > works due to correct execution environment (Vcol_expr_context). > Related to MDEV-10355. > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 12

Re: [Maria-developers] 62d3496969d: MDEV-24176 Server crashes after insert in the table with virtual column generated using date_format() and if()
by Sergei Golubchik 12 Apr '22

12 Apr '22

Hi, Aleksey, This is a review of `git diff 5fbbdbc85ba3 62d3496969db` Basically, no new comments, see below: On Apr 07, Aleksey Midenkov wrote: > diff --git a/libmariadb b/libmariadb > index f6c3d9fd2af..735a7299dba 160000 > --- a/libmariadb > +++ b/libmariadb > @@ -1 +1 @@ > -Subproject commit f6c3d9fd2af5d17db64cc996574aa312efd70fcf > +Subproject commit 735a7299dbae19cc2b82b9697becaf90e9b43047 you've mistakenly checked in a submodule > diff --git a/sql/field.cc b/sql/field.cc > index f18fb25ebe3..04c492e6c69 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -2416,6 +2416,13 @@ int Field::set_default() > if (default_value) > { > Query_arena backup_arena; > + /* > + TODO: this imposes memory leak until table flush when save_in_field() > + does expr_arena allocation. F.ex. case from main.default: > + > + CREATE TABLE t1 (a INT DEFAULT CONCAT('1 ')); > + INSERT INTO t1 VALUES (DEFAULT); I wasn't able to repeat that. In your branch (didn't try in vanilla 10.3) and for the above test case I see no additional allocations in table->mem_root after the table is opened. > + */ > table->in_use->set_n_backup_active_arena(table->expr_arena, &backup_arena); > int rc= default_value->expr->save_in_field(this, 0); > table->in_use->restore_active_arena(table->expr_arena, &backup_arena); > diff --git a/sql/table.cc b/sql/table.cc > index 08d91678b25..d22da17c31a 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -2970,36 +2988,133 @@ static bool fix_vcol_expr(THD *thd, Virtual_column_info *vcol) ... > +bool Vcol_expr_context::init() > +{ > + /* > + As this is vcol expression we must narrow down name resolution to > + single table. > + */ > + if (init_lex_with_single_table(thd, table, &lex)) I've already asked about it in a separate email, so here I'm just marking a line that we haven't fully agreed on yet > + { > + my_error(ER_OUT_OF_RESOURCES, MYF(0)); > + table->map= old_map; > + return true; > + } > + > + lex.sql_command= old_lex->sql_command; > + thd->variables.sql_mode= 0; > + > + TABLE_LIST const *tl= table->pos_in_table_list; > + DBUG_ASSERT(table->pos_in_table_list); > + > + if (table->pos_in_table_list->security_ctx) > thd->security_ctx= tl->security_ctx; > - bool res= fix_session_vcol_expr(thd, vcol); > + > + inited= true; > + return false; > +} > + Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] e3e59c9e38d: MDEV-28266 Crash in Field_string::type_handler when calling procedures
by Sergei Golubchik 10 Apr '22

10 Apr '22

Hi, Oleksandr, On Apr 10, Oleksandr Byelkin wrote: > revision-id: e3e59c9e38d (mariadb-10.6.6-83-ge3e59c9e38d) > parent(s): 4e1ca388381 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2022-04-08 10:37:17 +0200 > message: > > MDEV-28266 Crash in Field_string::type_handler when calling procedures > > In case of only metadata request the cursor lack the call to redirect > Field::orig_table to the temporary table. > > diff --git a/sql/sql_cursor.cc b/sql/sql_cursor.cc > index 0add5845558..155346341f1 100644 > --- a/sql/sql_cursor.cc > +++ b/sql/sql_cursor.cc > @@ -183,6 +183,16 @@ int mysql_open_cursor(THD *thd, select_result *result, > { > Materialized_cursor *materialized_cursor= > result_materialize->materialized_cursor; > + if (result_materialize->view_structure_only()) > + { > + /* > + If it was view_structure_only then only send metadata call was made > + and there was no other call. So we have to "close" the cursor > + explicitly (which redirect orig_table in the fields, and untie > + it with closed tables) > + */ > + materialized_cursor->on_table_fill_finished(); > + } > > if ((rc= materialized_cursor->open(0))) > { > delete materialized_cursor; > goto end; > } normally on_table_fill_finished() is called from Select_materialize::send_eof() and Select_materialize::abort_result_set(). Note that directly after your added code there's materialized_cursor->open(), and there we have int Materialized_cursor::open(JOIN *join __attribute__((unused))) { ... if (!rc) { thd->server_status|= SERVER_STATUS_CURSOR_EXISTS; result->send_eof(); } else { result->abort_result_set(); } That is, one could expect that even without your added code, on_table_fill_finished() would be called. The problem is that result here is sp_cursor::Select_fetch_into_spvars, and sp_cursor::Select_fetch_into_spvars::send_eof() is simply { return FALSE; } So, as a fix I'd suggest to remove on_table_fill_finished() from send_eof/abort_result_set, and put it unconditionally after the if(), to make it work no matter what select_result was chosen. Like this: ====================================== --- a/sql/sql_cursor.cc +++ b/sql/sql_cursor.cc @@ -80,19 +80,7 @@ class Select_materialize: public select_unit Select_materialize(THD *thd_arg, select_result *result_arg): select_unit(thd_arg), result(result_arg), materialized_cursor(0) {} virtual bool send_result_set_metadata(List<Item> &list, uint flags); - bool send_eof() - { - if (materialized_cursor) - materialized_cursor->on_table_fill_finished(); - return false; - } - - void abort_result_set() - { - if (materialized_cursor) - materialized_cursor->on_table_fill_finished(); - } - + bool send_eof() { return false; } bool view_structure_only() const { return result->view_structure_only(); @@ -333,6 +321,8 @@ int Materialized_cursor::open(JOIN *join result->abort_result_set(); } + on_table_fill_finished(); + return rc; } ====================================== with the commit comment saying that "on_table_fill_finished() should always be done at the end of open(), even if result is not Select_materialize, but, for example, Select_fetch_into_spvars" Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] a10facb656b: MDEV-27065 Partitioning tables with custom data directories moves data back to default directory
by Sergei Golubchik 29 Mar '22

29 Mar '22

Hi, Nayuta, On Mar 29, Nayuta Yanagisawa wrote: > revision-id: a10facb656b (mariadb-10.2.43-48-ga10facb656b) > parent(s): 33ff18627ea > author: Nayuta Yanagisawa > committer: Nayuta Yanagisawa > timestamp: 2022-03-29 23:43:16 +0900 > message: > > MDEV-27065 Partitioning tables with custom data directories moves data back to default directory > > --- > .../suite/parts/r/partition_exch_qa_14.result | 22 ++++++++++++++++++++++ > sql/ha_partition.cc | 16 ++++++++++++++++ > 2 files changed, 38 insertions(+) > > diff --git a/mysql-test/suite/parts/r/partition_exch_qa_14.result b/mysql-test/suite/parts/r/partition_exch_qa_14.result > index 1420982436a..52839644f55 100644 > --- a/mysql-test/suite/parts/r/partition_exch_qa_14.result > +++ b/mysql-test/suite/parts/r/partition_exch_qa_14.result > @@ -9,6 +9,9 @@ PARTITION BY RANGE (a) > (PARTITION p0 VALUES LESS THAN (10) DATA DIRECTORY = 'MYSQLTEST_VARDIR/mysql-test-data-dir' INDEX DIRECTORY = 'MYSQLTEST_VARDIR/mysql-test-idx-dir', > PARTITION p1 VALUES LESS THAN (100) DATA DIRECTORY = 'MYSQLTEST_VARDIR/mysql-test-data-dir' INDEX DIRECTORY = 'MYSQLTEST_VARDIR/mysql-test-idx-dir', > PARTITION p2 VALUES LESS THAN (1000) DATA DIRECTORY = 'MYSQLTEST_VARDIR/mysql-test-data-dir' INDEX DIRECTORY = 'MYSQLTEST_VARDIR/mysql-test-idx-dir'); > +Warnings: > +Warning 1618 <DATA DIRECTORY> option ignored > +Warning 1618 <INDEX DIRECTORY> option ignored This is very confusing. There are four DATA DIRECTORY and four INDEX DIRECTORY in the statement. Which one is ignored? (yes, *I* know, the table level one) > @@ -148,6 +154,9 @@ PARTITION BY RANGE (a) > (PARTITION p0 VALUES LESS THAN (10) , > PARTITION p1 VALUES LESS THAN (100) , > PARTITION p2 VALUES LESS THAN (1000) ); > +Warnings: > +Warning 1618 <DATA DIRECTORY> option ignored > +Warning 1618 <INDEX DIRECTORY> option ignored and here DATA/INDEX DIRECTORY behaves not like other options. For a normal option table-level value would apply to all partitions. That is CREATE TABLE t1 (...) OPT="VAL (PARTITION p0 VALUES LESS THAN (10) , PARTITION p1 VALUES LESS THAN (100) , PARTITION p2 VALUES LESS THAN (1000) ); would be the same as CREATE TABLE t1 (...) (PARTITION p0 VALUES LESS THAN (10) OPT="VAL, PARTITION p1 VALUES LESS THAN (100) OPT="VAL, PARTITION p2 VALUES LESS THAN (1000) OPT="VAL); Is that correct? > CREATE TABLE tsp (a INT, b VARCHAR(55), PRIMARY KEY (a)) DATA DIRECTORY = 'MYSQLTEST_VARDIR/mysql-test-data-dir' INDEX DIRECTORY = 'MYSQLTEST_VARDIR/mysql-test-idx-dir' ENGINE = MYISAM > PARTITION BY RANGE (a) > SUBPARTITION BY HASH(a) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 88f8aa20bba: MDEV-23486 RBR can bypass secure_timestamp=YES
by Sergei Golubchik 26 Mar '22

26 Mar '22

Hi, Aleksey! On Jan 09, Aleksey Midenkov wrote: > revision-id: 88f8aa20bba (mariadb-10.3.31-79-g88f8aa20bba) > parent(s): d5451867af1 > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2021-12-23 04:15:39 +0300 > message: > > MDEV-23486 RBR can bypass secure_timestamp=YES > > Pass to a slave whether master table is system-versioned via > TM_SYSTEM_VERSIONED flag. That deprecates m_vers_from_plain > detection. If secure_timestamp == YES or master did not supply row_end > data treat the row as current data and generate timestamps for it. I don't understand why does it matter whether the master is system versioned or not. secure_timestamp=YES means the table has to store actual timestamps when rows were changed. It doesn't trust the master to set timestamps. So it should not matter whether master table is versioned or not, if you don't trust the master, you cannot trust its TM_SYSTEM_VERSIONED flag either. Even if the table was system versioned on the master, the master could've had secure_timestamp=NO and a completely fake history. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] MDEV-24560 SIGSEGV in st_join_table::cleanup
by Sergey Petrunia 25 Mar '22

25 Mar '22

Hi Oleg, > commit 4f41909c3ee1d7e42c56947c9a9156fb78c22d8a > Author: Oleg Smirnov <olernov(a)gmail.com> > Date: Fri Mar 11 21:18:34 2022 +0700 > > MDEV-24560 SIGSEGV in st_join_table::cleanup > > If JOIN::create_postjoin_aggr_table encounters errors during execution > then free_tmp_table() is then called twice for JOIN_TAB::aggr. > The solution is to initialize JOIN_TAB::aggr and JOIN_TAB::table only > on successful completion of JOIN::create_postjoin_aggr_table > > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index c044656e916..8c07014d981 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -4037,10 +4037,6 @@ JOIN::create_postjoin_aggr_table(JOIN_TAB *tab, List<Item> *table_fields, > !tables_list); > if (tab > join_tab) > (tab - 1)->next_select= sub_select_postjoin_aggr; > - if (!(tab->aggr= new (thd->mem_root) AGGR_OP(tab))) > - goto err; > - tab->table= table; > - table->reginfo.join_tab= tab; > > /* if group or order on first table, sort first */ > if ((group_list && simple_group) || > @@ -4089,7 +4085,10 @@ JOIN::create_postjoin_aggr_table(JOIN_TAB *tab, List<Item> *table_fields, > order= NULL; > } > } > - > + if (!(tab->aggr= new (thd->mem_root) AGGR_OP(tab))) > + goto err; > + tab->table= table; > + table->reginfo.join_tab= tab; > DBUG_RETURN(false); I see a potential issue in the patch: The moved code does this assignment: > tab->table= table; Let's look inside this if-branch: >> if ((group_list && simple_group) || >> (implicit_grouping && select_lex->have_window_funcs())) The code inside if() { ... } part seems to be fine: it does not touch the contents of 'tab', in particular tab->table. Let's look at the coed inside the else {... } part: It has this condition: >> if (!group_list && !table->distinct && order && simple_order && >> tab == join_tab + const_tables) >> { Note that tab==join_tab+const_tables. Then, a few lines below: >> add_sorting_to_table(join_tab + const_tables, order)) This is essentially add_sorting_to_table(tab, order). Inside JOIN::add_sorting_to_table one can find this: TABLE *table= tab->table; if ((tab == join_tab + const_tables) && table->pos_in_table_list && table->pos_in_table_list->is_sjm_scan_table()) that is, we access tab->table and tab->table->pos_in_table_list. Which are not assigned yet (because your patch moves the assignment to happen later). I've tried to construct an example to demonstrate this issue, but was not successful. I've made this change: diff --git a/sql/sql_select.cc b/sql/sql_select.cc index c044656e916..5a9a7d7e73f 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -4080,6 +4080,8 @@ JOIN::create_postjoin_aggr_table(JOIN_TAB *tab, List<Item> *table_fields, tab == join_tab + const_tables) { DBUG_PRINT("info",("Sorting for order")); + fprintf(stderr,"AAABBBCCC:%s\n", thd->query()); + DBUG_ASSERT(0); THD_STAGE_INFO(thd, stage_sorting_for_order); if (ordered_index_usage != ordered_index_order_by && and ran the entire testsuite with it. There were no failures. I'm not sure if this is deadcode or we just don't have enough test coverage. I suggest to assume the latter and change the patch to avoid this issue. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 0

Re: [Maria-developers] 3be1e491fb5: MDEV-27743 Remove Lex::charset
by Sergei Golubchik 22 Mar '22

22 Mar '22

Hi, Alexander, All comments from the previous review were addressed and there were no other changes. So, ok to push. On Mar 22, Alexander Barkov wrote: > revision-id: 3be1e491fb5 (mariadb-10.6.1-383-g3be1e491fb5) > parent(s): c69defd3b61 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-03-21 21:33:30 +0400 > message: > > MDEV-27743 Remove Lex::charset > > This patch also fixes: > > MDEV-27690 Crash on `CHARACTER SET csname COLLATE DEFAULT` in column definition > MDEV-27853 Wrong data type on column `COLLATE DEFAULT` and table `COLLATE some_non_default_collation` > MDEV-28067 Multiple conflicting column COLLATE clauses are not rejected > MDEV-28118 Wrong collation of `CAST(.. AS CHAR COLLATE DEFAULT)` > MDEV-28119 Wrong column collation on MODIFY + CONVERT > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 026e52bad84: MDEV-27743 Remove Lex::charset
by Sergei Golubchik 21 Mar '22

21 Mar '22

Hi, Alexander, Please, see below On Mar 18, Alexander Barkov wrote: > revision-id: 026e52bad84 (mariadb-10.6.1-369-g026e52bad84) > parent(s): 369498d6b8c > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-03-18 12:26:03 +0400 > message: > > MDEV-27743 Remove Lex::charset > > This patch also fixes: > > MDEV-27690 Crash on `CHARACTER SET csname COLLATE DEFAULT` in column definition > MDEV-27853 Wrong data type on column `COLLATE DEFAULT` and table `COLLATE some_non_default_collation` > MDEV-28067 Multiple conflicting column COLLATE clauses are not rejected > MDEV-28118 Wrong collation of `CAST(.. AS CHAR COLLATE DEFAULT)` > MDEV-28119 Wrong column collation on MODIFY + CONVERT > > diff --git a/mysql-test/main/ctype_collate_column.test b/mysql-test/main/ctype_collate_column.test > new file mode 100644 > index 00000000000..6a7cf2cc84c > --- /dev/null > +++ b/mysql-test/main/ctype_collate_column.test > @@ -0,0 +1,637 @@ ... > + > +DELIMITER $$; > +CREATE PROCEDURE p1(dt TEXT, cs TEXT, cl0 TEXT, cl1 TEXT, cl2 TEXT, tcs TEXT) > +BEGIN > + DECLARE errstate TEXT DEFAULT NULL; > + DECLARE errno INT DEFAULT NULL; > + DECLARE errmsg TEXT DEFAULT NULL; > + DECLARE query TEXT; > + DECLARE result TEXT; > + DECLARE CONTINUE HANDLER FOR SQLEXCEPTION > + BEGIN > + GET DIAGNOSTICS CONDITION 1 errstate = RETURNED_SQLSTATE, > + errno = MYSQL_ERRNO, errmsg = MESSAGE_TEXT; > + END; > + SET query= CONCAT('CREATE TABLE t1 (a ', dt, ' ', cs, ' ', cl0, > + ' NOT NULL ',cl1, > + ' DEFAULT '''' ', cl2, > + ') ', tcs, ' ENGINE=Memory'); > + EXECUTE IMMEDIATE query; > + IF errmsg IS NOT NULL > + THEN > + SET result=CONCAT('ERROR: ', > + COALESCE(errstate,'<NULL>'), ' ', > + COALESCE(errno,'<NULL>'), ' ', > + COALESCE(errmsg,'<NULL>')); > + INSERT INTO results (dt,cs,cl0,cl1,cl2,tcs,query,result) > + VALUES (dt,cs,cl0,cl1,cl2,tcs,query,result); > + ELSE > + FOR row IN (SELECT CONCAT(COLUMN_TYPE, > + ' CHARACTER SET ', CHARACTER_SET_NAME, > + ' COLLATE ', COLLATION_NAME) AS result > + FROM INFORMATION_SCHEMA.COLUMNS > + WHERE TABLE_SCHEMA='test' AND TABLE_NAME='t1') > + DO > + INSERT INTO results (dt,cs,cl0,cl1,cl2,tcs,query,result) > + VALUES (dt,cs,cl0,cl1,cl2,tcs,query,row.result); > + END FOR; > + DROP TABLE t1; > + END IF; > +END; > +$$ > +DELIMITER ;$$ > + > + > +DELIMITER $$; > +CREATE PROCEDURE p3(dt TEXT) > +BEGIN > + FOR row IN ( > + SELECT cs, cl0, cl1.cl1 AS cl1, cl2.cl1 AS cl2, tcs > + FROM cs, cl0, cl1, cl1 AS cl2, tcs > + ORDER BY cs, cl0, cl1, cl2, tcs > + ) > + DO > + CALL p1('char(10)', row.cs, row.cl0, row.cl1, row.cl2, row.tcs); minor detail: here ^^^ you want to use dt, not 'char(10)' > + END FOR; > +END; > +$$ > +DELIMITER ;$$ > + > + > +--disable_column_names > +CALL p3('char(10)'); > +--enable_column_names > + > +SELECT * FROM results_statistics; isn't it redundant? you select all rows from result anyway. If something will change, the result of that select will already cause the the to fail. Why do you need second mismatch in the result/reject. > + > +--vertical_results > +SELECT query, result, '' AS `` FROM results > +ORDER BY dt, cs, cl0, cl1, cl2, tcs; > +--horizontal_results > + > +DROP PROCEDURE p1; > +DROP PROCEDURE p3; > + > +DROP TABLE cs, cl0, cl1, tcs; ... > +# CREATE TABLE t1 > +# ( > +# a CHAR(10) BINARY NOT NULL DEFAULT '' > +# ) CHARACTER SET cs COLLATE cs_bin; > + > +DELETE FROM results > +WHERE cs='' > + AND cl0='BINARY' > + AND cl1='' > + AND cl2='' > + AND tcs LIKE 'CHARACTER SET%' > + AND result NOT LIKE 'ERROR%' > + AND result RLIKE CONCAT('COLLATE ', tcs_character_set_name, '_bin'); > +SELECT ROW_COUNT(); ... > +DROP FUNCTION is_collate_clause_with_explicit_default_collation; > +DROP FUNCTION is_collate_clause_with_explicit_collation; > +DROP FUNCTION is_conflicting_charset_explicit_collate_explicit; > +DROP FUNCTION is_conflicting_collate_explicit2; > +DROP FUNCTION is_conflicting_collate_default_collate_explicit; > +DROP FUNCTION collate_cs_default_collation; I think the main result is `select * from results` (with order by, etc). It's great, that you added that. Everything else is redundant, I suggest to remove other selects and deletes. > + > + > +--echo # > +--echo # End of 10.9 tests > +--echo # > diff --git a/sql/lex_charset.h b/sql/lex_charset.h > new file mode 100644 > index 00000000000..d95f0bc1920 > --- /dev/null > +++ b/sql/lex_charset.h > @@ -0,0 +1,202 @@ ... > +#define LEX_CHARSET_COLLATION_TYPE_BITS 2 > + static_assert(((1<<LEX_CHARSET_COLLATION_TYPE_BITS)-1) >= > + TYPE_COLLATE_CONTEXTUALLY_TYPED, > + "Lex_charset_collation_st::Type bits check"); > + > +protected: > + CHARSET_INFO *m_ci; > + Type m_type; > +public: > + static CHARSET_INFO *find_bin_collation(CHARSET_INFO *cs); > + static CHARSET_INFO *find_default_collation(CHARSET_INFO *cs); > +public: ... > + void set_contextually_typed_binary_style() > + { > + m_ci= &my_collation_contextually_typed_binary; > + m_type= TYPE_COLLATE_CONTEXTUALLY_TYPED; > + } > + bool is_contextually_typed_collate_default() const > + { > + return m_ci == &my_collation_contextually_typed_default && > + m_type == TYPE_COLLATE_CONTEXTUALLY_TYPED; what does it mean when only one of these two expressions is true? Say, the first true, the second false? Or the first false, the second true? > + } > + bool is_contextually_typed_binary_style() const > + { > + return m_ci == &my_collation_contextually_typed_binary && > + m_type == TYPE_COLLATE_CONTEXTUALLY_TYPED; > + } ... > diff --git a/strings/ctype-bin.c b/strings/ctype-bin.c > index bb746ad90b0..a5c9129ebea 100644 > --- a/strings/ctype-bin.c > +++ b/strings/ctype-bin.c > @@ -630,3 +630,69 @@ struct charset_info_st my_charset_bin = > &my_charset_handler, > &my_collation_binary_handler > }; > + > + > +struct charset_info_st my_collation_contextually_typed_binary= > +{ > + 0,0,0, /* number */ I thought you will never need to resove the &my_collation_contextually_typed_binary pointer, that is, I thought the values in the my_collation_contextually_typed_binary struct should not matter as they'll never be used. Was that wrong? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

Re: [Maria-developers] d8e915a88fc: MDEV-26009 Server crash when calling twice procedure using FOR-loop
by Sergei Golubchik 20 Mar '22

20 Mar '22

Hi, Oleksandr, ok to push also, see below On Mar 20, Oleksandr Byelkin wrote: > revision-id: d8e915a88fc (mariadb-10.3.33-32-gd8e915a88fc) > parent(s): 6a2d88c1322 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2022-03-18 11:13:09 +0100 > message: > > MDEV-26009 Server crash when calling twice procedure using FOR-loop > > The problem was that instructions sp_instr_cursor_copy_struct and > sp_instr_copen uses the same lex, adding and removing "tail" of > prelocked tables and forgetting that tail of all tables is kept in > LEX::query_tables_last. If the LEX used only by one instruction > or the query do not have prelocked tables it is not important. > But to work correctly in all cases LEX::query_tables_last should > be reset to make new tables added in the correct list (after last > table in the LEX instead after last table of the prelocking "tail" > which was cut). > > diff --git a/sql/sp_head.cc b/sql/sp_head.cc > index 57ab31d9edf..96dc78dcf49 100644 > --- a/sql/sp_head.cc > +++ b/sql/sp_head.cc > @@ -3486,6 +3486,11 @@ sp_lex_keeper::reset_lex_and_exec_core(THD *thd, uint *nextp, > lex_query_tables_own_last= m_lex->query_tables_own_last; > prelocking_tables= *lex_query_tables_own_last; > *lex_query_tables_own_last= NULL; > + /* > + Here we cut "tail" of prelocked tables, so have to return last table > + to the original position (wuthout the tail > + */ > + m_lex->query_tables_last= m_lex->query_tables_own_last; 1. the comment looks truncated 2. I don't even think this line needs a comment, this code block removes the list of prelocked tables from the query_tables list. Looking at how query_tables_last and query_tables_own_last are documented: /* Pointer to next_global member of last element in the previous list. */ TABLE_LIST **query_tables_last; /* If non-0 then indicates that query requires prelocking and points to next_global member of last own element in query table list (i.e. last table which was not added to it as part of preparation to prelocking). 0 - indicates that this query does not need prelocking. */ TABLE_LIST **query_tables_own_last; it is pretty clear that after the "tail" is removed, one has to set query_tables_last to point to the end of the list. The fact that it wasn't done was clearly an omission. That is, existing code and existing comments document it pretty well, your comment looks redundant and even confusing to me. So, please, remove the comment and ok to push > m_lex->mark_as_requiring_prelocking(NULL); > } > thd->rollback_item_tree_changes(); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0