- developers - lists.mariadb.org

Re: [Maria-developers] e752eed5354: MDEV-19751 Wrong partitioning by KEY() after key dropped
by Sergei Golubchik 29 Oct '19

29 Oct '19

Hi, Aleksey! On Oct 29, Aleksey Midenkov wrote: > revision-id: e752eed5354 (mariadb-10.4.7-47-ge752eed5354) > parent(s): 8403148452a > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2019-08-19 12:00:13 +0300 > message: > > MDEV-19751 Wrong partitioning by KEY() after key dropped > > Empty partition by key() clause implies that key might be changed by > ALTER, but inplace algorithm must not be used since repartitioning is > required for a different key. good and very clear comment. > diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc > index 10b9c74d868..d76ff5e2e76 100644 > --- a/sql/sql_partition.cc > +++ b/sql/sql_partition.cc > @@ -5903,6 +5903,45 @@ the generated partition syntax in a correct manner. > *partition_changed= TRUE; > } > } > + /* > + Prohibit inplace when key takes part in partitioning expression > + and is altered (dropped). > + */ > + if (!*partition_changed && tab_part_info->part_field_array) > + { > + KEY *key_info= table->key_info; > + List_iterator_fast<Alter_drop> drop_it(alter_info->drop_list); > + for (uint key= 0; key < table->s->keys; key++, key_info++) > + { > + if (key_info->flags & HA_INVISIBLE_KEY) > + continue; > + const char *key_name= key_info->name.str; > + const Alter_drop *drop; > + drop_it.rewind(); > + while ((drop= drop_it++)) > + { > + if (drop->type == Alter_drop::KEY && > + 0 == my_strcasecmp(system_charset_info, key_name, drop->name)) > + break; > + } > + if (!drop) > + continue; > + for (uint kp= 0; kp < key_info->user_defined_key_parts; ++kp) > + { > + const KEY_PART_INFO &key_part= key_info->key_part[kp]; > + for (Field **part_field= tab_part_info->part_field_array; > + *part_field; ++part_field) > + { > + if (*part_field == key_part.field) > + { > + *partition_changed= TRUE; > + goto search_finished; > + } > + } // for (part_field) > + } // for (key_part) > + } // for (key_info) Hmm, if I read it correctly, you iterate over all key parts of every dropped key and see if this field is also part of the partitioning expression. This sounds kind of strange. What if there are two indexes that use the column `a`, say CREATE TABLE t1 (a int, b int, index(a), index(b,a)) and you partition by key(a). Dropping the second index does not change the partitioning, does it? On the other hand, if it's ALTER TABLE ... MODIFY `a` then it can change partitioning, even when the key hasn't changed, right? So, I suspect your check is too strong in some cases and too loose is others. > + search_finished:; > + } > } > if (thd->work_part_info) > { > diff --git a/storage/connect/mysql-test/connect/r/part_file.result b/storage/connect/mysql-test/connect/r/part_file.result > index 3dabd946b50..480de51cce6 100644 > --- a/storage/connect/mysql-test/connect/r/part_file.result > +++ b/storage/connect/mysql-test/connect/r/part_file.result > @@ -308,12 +308,19 @@ EXPLAIN PARTITIONS SELECT * FROM t1 WHERE id = 10; > id select_type table partitions type possible_keys key key_len ref rows Extra > 1 SIMPLE t1 1 ref XID XID 4 const 1 > DROP INDEX XID ON t1; > +Warnings: > +Warning 1105 Data repartition in 1 is unchecked > +Warning 1105 Data repartition in 2 is unchecked > +Warning 1105 Data repartition in 3 is unchecked why did the result file change? could you add a short explanation of it to the commit comment, please? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] Please review MDEV-18319 BIGINT UNSIGNED Performance issue
by Alexander Barkov 29 Oct '19

29 Oct '19

Hi Sergei, Please review a patch for MDEV-18319 https://github.com/MariaDB/server/commit/8a990ad17746927c6d395ec755a262eda5… Thanks

2 1

Re: [Maria-developers] 7a331ec820b: MDEV-18244 Server crashes in ha_innobase::update_thd / ... / ha_partition::update_next_auto_inc_val.
by Sergei Golubchik 25 Oct '19

25 Oct '19

Hi, Alexey! On Oct 25, Alexey Botchkov wrote: > revision-id: 7a331ec820b (mariadb-10.3.18-65-g7a331ec820b) > parent(s): 716d396bb3b > author: Alexey Botchkov <holyfoot(a)mariadb.com> > committer: Alexey Botchkov <holyfoot(a)mariadb.com> > timestamp: 2019-10-22 01:40:48 +0400 > message: > > MDEV-18244 Server crashes in ha_innobase::update_thd / ... / ha_partition::update_next_auto_inc_val. > > Autoincrement calculation should check if tables were opened. > > --- > mysql-test/main/partition_innodb.result | 13 +++++++++++++ > mysql-test/main/partition_innodb.test | 15 +++++++++++++++ > sql/ha_partition.cc | 2 ++ > 3 files changed, 30 insertions(+) > > diff --git a/mysql-test/main/partition_innodb.result b/mysql-test/main/partition_innodb.result > index f3d24347ff9..93a73a785fe 100644 > --- a/mysql-test/main/partition_innodb.result > +++ b/mysql-test/main/partition_innodb.result > @@ -1028,5 +1028,18 @@ COUNT(*) > 2 > DROP TABLE t1; > # > +# MDEV-18244 Server crashes in ha_innobase::update_thd / ... / ha_partition::update_next_auto_inc_val > +# > +CREATE TABLE t1 (a INT) > +ENGINE=InnoDB > +PARTITION BY RANGE (a) ( > +PARTITION p0 VALUES LESS THAN (6), > +PARTITION pn VALUES LESS THAN MAXVALUE > +); > +INSERT INTO t1 VALUES (4),(5),(6); > +ALTER TABLE t1 MODIFY a INT AUTO_INCREMENT PRIMARY KEY; > +UPDATE t1 PARTITION (p0) SET a = 3 WHERE a = 5; > +DROP TABLE t1; > +# > # End of 10.3 tests > # > diff --git a/mysql-test/main/partition_innodb.test b/mysql-test/main/partition_innodb.test > index 629bc29e758..1e0af06b4da 100644 > --- a/mysql-test/main/partition_innodb.test > +++ b/mysql-test/main/partition_innodb.test > @@ -1105,6 +1105,21 @@ INSERT INTO t1 VALUES (1, 7, 8, 9), (2, NULL, NULL, NULL), (3, NULL, NULL, NULL) > SELECT COUNT(*) FROM t1 WHERE x IS NULL AND y IS NULL AND z IS NULL; > DROP TABLE t1; > > +--echo # > +--echo # MDEV-18244 Server crashes in ha_innobase::update_thd / ... / ha_partition::update_next_auto_inc_val > +--echo # > + > +CREATE TABLE t1 (a INT) > + ENGINE=InnoDB > + PARTITION BY RANGE (a) ( > + PARTITION p0 VALUES LESS THAN (6), > + PARTITION pn VALUES LESS THAN MAXVALUE > + ); > +INSERT INTO t1 VALUES (4),(5),(6); > +ALTER TABLE t1 MODIFY a INT AUTO_INCREMENT PRIMARY KEY; > +UPDATE t1 PARTITION (p0) SET a = 3 WHERE a = 5; > +DROP TABLE t1; > + > --echo # > --echo # End of 10.3 tests > --echo # > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc > index 5a78249644d..e461532db1a 100644 > --- a/sql/ha_partition.cc > +++ b/sql/ha_partition.cc > @@ -8174,6 +8174,8 @@ int ha_partition::info(uint flag) > ("checking all partitions for auto_increment_value")); > do > { > + if (!bitmap_is_set(&m_opened_partitions, (uint)(file_array - m_file))) > + continue; > file= *file_array; > file->info(HA_STATUS_AUTO | no_lock_flag); > set_if_bigger(auto_increment_value, It seems that the intention was to find the max auto_increment_value over all partitions. If you skip some, it won't be max anymore. On the other hand, I don't see why UPDATE should get the max auto-inc of all partitions. May be the correct fix would be not to do it in UPDATE? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 52f3829b95c: MDEV-20001 Potential dangerous regression: INSERT INTO >=100 rows fail for myisam table with HASH indexes
by Sergei Golubchik 25 Oct '19

25 Oct '19

Hi, Sachin! This is ok to push. But, still, about Aria. When you push the fix for MDEV-18791, it will be very easy to forget this one. May be you add the same test case for Aria now? It'll fail with something like "not supported" error, which is fine, just use --error. But when you fix MDEV-18791 this test will be enabled and it'll ensure you won't forget to fix Aria for bulk inserts. On Oct 25, Sachin Setiya wrote: > revision-id: 52f3829b95c (mariadb-10.4.7-110-g52f3829b95c) > parent(s): 93d2211e02f > author: Sachin <sachin.setiya(a)mariadb.com> > committer: Sachin <sachin.setiya(a)mariadb.com> > timestamp: 2019-10-09 21:17:32 +0530 > message: > > MDEV-20001 Potential dangerous regression: INSERT INTO >=100 rows fail for myisam table with HASH indexes > > Problem:- > > So the issue is when we do bulk insert with rows > > MI_MIN_ROWS_TO_DISABLE_INDEXES(100) , We try to disable the indexes to > speedup insert. But current logic also disables the long unique indexes. > > Solution:- In ha_myisam::start_bulk_insert if we find long hash index > (HA_KEY_ALG_LONG_HASH) we will not disable the index. > > This commit also refactors the mi_disable_indexes_for_rebuild function, > Since this is function is called at only one place, it is inlined into > start_bulk_insert > > mi_clear_key_active is added into myisamdef.h because now it is also used > in ha_myisam.cc file. > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] a0e3bd09251: Part1: MDEV-20837 Add MariaDB_FUNCTION_PLUGIN
by Sergei Golubchik 16 Oct '19

16 Oct '19

Hi, Alexander! Just a couple of comments, see below: On Oct 16, Alexander Barkov wrote: > revision-id: a0e3bd09251 (mariadb-10.4.4-419-ga0e3bd09251) > parent(s): 22b645ef529 > author: Alexander Barkov <bar(a)mariadb.com> > committer: Alexander Barkov <bar(a)mariadb.com> > timestamp: 2019-10-16 16:26:29 +0400 > message: > > Part1: MDEV-20837 Add MariaDB_FUNCTION_PLUGIN > > - Defining MariaDB_FUNCTION_PLUGIN > - Changing the code in /plugins/type_inet/ and /plugins/type_test/ > to use MariaDB_FUNCTION_PLUGIN instead of MariaDB_FUNCTION_COLLECTION_PLUGIN. > diff --git a/include/mysql/plugin_function.h b/include/mysql/plugin_function.h > new file mode 100644 > index 00000000000..4ce416612e9 > --- /dev/null > +++ b/include/mysql/plugin_function.h > @@ -0,0 +1,58 @@ > +#ifndef MARIADB_PLUGIN_FUNCTION_INCLUDED > +#define MARIADB_PLUGIN_FUNCTION_INCLUDED > +/* Copyright (C) 2019, Alexander Barkov and MariaDB > + > + 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 */ > + > +/** > + @file > + > + Function Plugin API. > + > + This file defines the API for server plugins that manage functions. > +*/ > + > +#ifdef __cplusplus > + > +#include <mysql/plugin.h> > + > +/* > + API for function plugins. (MariaDB_FUNCTION_PLUGIN) > +*/ > +#define MariaDB_FUNCTION_INTERFACE_VERSION (MYSQL_VERSION_ID << 8) > + > + > +class Plugin_function > +{ > + int m_interface_version; > + Create_func *m_builder; > +public: > + Plugin_function(int interface_version, Create_func *builder) > + :m_interface_version(interface_version), > + m_builder(builder) > + { } Why do you need this ^^^ constructor? > + Plugin_function(Create_func *builder) > + :m_interface_version(MariaDB_FUNCTION_INTERFACE_VERSION), > + m_builder(builder) > + { } > + Create_func *create_func() > + { > + return m_builder; > + } > +}; > + > + > +#endif /* __cplusplus */ > + > +#endif /* MARIADB_PLUGIN_FUNCTION_INCLUDED */ > diff --git a/plugin/type_inet/mysql-test/type_inet/func_inet_plugin.result b/plugin/type_inet/mysql-test/type_inet/func_inet_plugin.result > index 4663ae485e2..a9422f2e4fd 100644 > --- a/plugin/type_inet/mysql-test/type_inet/func_inet_plugin.result > +++ b/plugin/type_inet/mysql-test/type_inet/func_inet_plugin.result > @@ -15,14 +16,94 @@ PLUGIN_LICENSE, > PLUGIN_MATURITY, > PLUGIN_AUTH_VERSION > FROM INFORMATION_SCHEMA.PLUGINS > -WHERE PLUGIN_TYPE='FUNCTION COLLECTION' > - AND PLUGIN_NAME='func_inet'; > -PLUGIN_NAME func_inet > +WHERE PLUGIN_TYPE='FUNCTION' > + AND PLUGIN_NAME IN > +('inet_aton', > +'inet_ntoa', > +'inet6_aton', > +'inet6_ntoa', > +'is_ipv4', > +'is_ipv6', > +'is_ipv4_compat', > +'is_ipv4_mapped') > +ORDER BY PLUGIN_NAME; > +---- ---- > +PLUGIN_NAME inet6_aton > PLUGIN_VERSION 1.0 > PLUGIN_STATUS ACTIVE > -PLUGIN_TYPE FUNCTION COLLECTION > +PLUGIN_TYPE FUNCTION > PLUGIN_AUTHOR MariaDB Corporation > -PLUGIN_DESCRIPTION Function collection test > +PLUGIN_DESCRIPTION Function INET6_ATON() > +PLUGIN_LICENSE GPL > +PLUGIN_MATURITY Experimental INET* functions should probably be Alpha, not Experimental. You expect them to be GA one day, don't you? > +PLUGIN_AUTH_VERSION 1.0 > +---- ---- > +PLUGIN_NAME inet6_ntoa > +PLUGIN_VERSION 1.0 > +PLUGIN_STATUS ACTIVE > +PLUGIN_TYPE FUNCTION > +PLUGIN_AUTHOR MariaDB Corporation > +PLUGIN_DESCRIPTION Function INET6_NTOA() > +PLUGIN_LICENSE GPL > +PLUGIN_MATURITY Experimental > +PLUGIN_AUTH_VERSION 1.0 > +---- ---- > +PLUGIN_NAME inet_aton > +PLUGIN_VERSION 1.0 > +PLUGIN_STATUS ACTIVE > +PLUGIN_TYPE FUNCTION > +PLUGIN_AUTHOR MariaDB Corporation > +PLUGIN_DESCRIPTION Function INET_ATON() > +PLUGIN_LICENSE GPL > +PLUGIN_MATURITY Experimental > +PLUGIN_AUTH_VERSION 1.0 > +---- ---- > +PLUGIN_NAME inet_ntoa > +PLUGIN_VERSION 1.0 > +PLUGIN_STATUS ACTIVE > +PLUGIN_TYPE FUNCTION > +PLUGIN_AUTHOR MariaDB Corporation > +PLUGIN_DESCRIPTION Function INET_NTOA() > +PLUGIN_LICENSE GPL > +PLUGIN_MATURITY Experimental > +PLUGIN_AUTH_VERSION 1.0 > +---- ---- > +PLUGIN_NAME is_ipv4 > +PLUGIN_VERSION 1.0 > +PLUGIN_STATUS ACTIVE > +PLUGIN_TYPE FUNCTION > +PLUGIN_AUTHOR MariaDB Corporation > +PLUGIN_DESCRIPTION Function IS_IPV4() > +PLUGIN_LICENSE GPL > +PLUGIN_MATURITY Experimental > +PLUGIN_AUTH_VERSION 1.0 > +---- ---- > +PLUGIN_NAME is_ipv4_compat > +PLUGIN_VERSION 1.0 > +PLUGIN_STATUS ACTIVE > +PLUGIN_TYPE FUNCTION > +PLUGIN_AUTHOR MariaDB Corporation > +PLUGIN_DESCRIPTION Function IS_IPV4_COMPAT() > +PLUGIN_LICENSE GPL > +PLUGIN_MATURITY Experimental > +PLUGIN_AUTH_VERSION 1.0 > +---- ---- > +PLUGIN_NAME is_ipv4_mapped > +PLUGIN_VERSION 1.0 > +PLUGIN_STATUS ACTIVE > +PLUGIN_TYPE FUNCTION > +PLUGIN_AUTHOR MariaDB Corporation > +PLUGIN_DESCRIPTION Function IS_IPV4_MAPPED() > +PLUGIN_LICENSE GPL > +PLUGIN_MATURITY Experimental > +PLUGIN_AUTH_VERSION 1.0 > +---- ---- > +PLUGIN_NAME is_ipv6 > +PLUGIN_VERSION 1.0 > +PLUGIN_STATUS ACTIVE > +PLUGIN_TYPE FUNCTION > +PLUGIN_AUTHOR MariaDB Corporation > +PLUGIN_DESCRIPTION Function IS_IPV6() > PLUGIN_LICENSE GPL > PLUGIN_MATURITY Experimental > PLUGIN_AUTH_VERSION 1.0 > diff --git a/sql/item_create.cc b/sql/item_create.cc > index e8eb76dfc12..e316723cedf 100644 > --- a/sql/item_create.cc > +++ b/sql/item_create.cc > @@ -5704,12 +5657,26 @@ void item_create_cleanup() > { > DBUG_ENTER("item_create_cleanup"); > my_hash_free(& native_functions_hash); > -#ifdef HAVE_SPATIAL > - plugin_function_collection_geometry.deinit(); > -#endif > DBUG_VOID_RETURN; > } > > + > +static Create_func * > +function_plugin_find_native_function_builder(THD *thd, const LEX_CSTRING &name) > +{ > + plugin_ref plugin; > + if ((plugin= my_plugin_lock_by_name(thd, &name, MariaDB_FUNCTION_PLUGIN))) > + { > + Create_func *builder= > + reinterpret_cast<Plugin_function*>(plugin_decl(plugin)->info)-> > + create_func(); > + plugin_unlock(thd, plugin); > + return builder; > + } > + return NULL; So, function plugins cannot be unlocked either? > +} > + > + > Create_func * > find_native_function_builder(THD *thd, const LEX_CSTRING *name) > { > @@ -5724,16 +5691,10 @@ find_native_function_builder(THD *thd, const LEX_CSTRING *name) > if (func && (builder= func->builder)) > return builder; > > - if ((builder= Plugin_find_native_func_builder_param(*name).find(thd))) > + if ((builder= function_plugin_find_native_function_builder(thd, *name))) this is what I mean by "special code path for plugins" and want to get rid of. But not in this commit, I agree. > return builder; > > -#ifdef HAVE_SPATIAL > - if (!builder) > - builder= plugin_function_collection_geometry. > - find_native_function_builder(thd, *name); > -#endif > - > - return builder; > + return NULL; > } > > Create_qfunc * Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] c776ed4d6ec: MDEV-19848 Server crashes in check_vcol_forward_refs upon INSERT DELAYED into table with long blob key
by Sergei Golubchik 14 Oct '19

14 Oct '19

Hi, Sachin! On Aug 20, Sachin Setiya wrote: > revision-id: c776ed4d6ec (mariadb-10.4.5-153-gc776ed4d6ec) > parent(s): 4ca016237f1 > author: Sachin <sachin.setiya(a)mariadb.com> > committer: Sachin <sachin.setiya(a)mariadb.com> > timestamp: 2019-07-30 03:42:21 +0530 > message: > > MDEV-19848 Server crashes in check_vcol_forward_refs upon INSERT DELAYED into table with long blob key > > There are 2 issues > > 1st:- in make_new_field when we & into new field flag we forget > LONG_UNIQUE_HASH_FIELD Flag. > > 2nd:- We are calling parse_vcol_defs on keyinfo , but they are not in right > form. We should call setup_keyinfo_hash_all before calling parse_vcol_defs This looks quite suspicious. Ideally, Delayed_insert::get_local_table() should just create a copy of the table, but not modify the original table. In your case it constantly modifies KEY's of the original table. It's kind of dirty. As for the real bug, see what parse_vcol_defs() is doing for long unique fields. In particular, it sets keypart->field->vcol_info= table->field[keypart->field->field_index]->vcol_info; but the keypart here is in the original Delayed_insert::table, while field and vcol_info is in a copy (created in Delayed_insert::get_local_table). This seems to be just wrong. I'd really prefer Delayed_insert::get_local_table to not change anything in the Delayed_insert::table. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

[Maria-developers] Performance of index intersection
by David Sickmiller 13 Oct '19

13 Oct '19

Hi! I've been using MySQL/MariaDB for two decades but have more recently been working with Elasticsearch. I knew to expect an inverted index to speed up querying full text fields, but I've been surprised (and a bit annoyed) at how fast ES can query structured data. (In my case, I'm largely looking for intersections of a number of varchar fields with lowish cardinality, e.g. WHERE country = 'US' AND client = 'Microsoft' AND status = 'Completed'.) Elasticsearch seems to have several things going on, but I think a core aspect, to use RDMS terminology, is that each column is indexed, and index unions/intersections are used if the WHERE clause references multiple columns. I've heard that MySQL/MariaDB has the ability to merge indexes, but I've rarely observed it in person. Googling for it yields a bunch of StackOverflow posts complaining how slow it is, with responses agreeing and explaining how to disable it. If I'm reading the MySQL/MariaDB code correctly, it looks like MariaDB will intersect indexes by looping through each index, reading the rowids of all matching keys and then, at the end (or once the buffer is full), checking whether each rowid is present in each index. I wonder if there's an opportunity to speed this up. If we first read in the rowids from one index (ideally the one with the fewest matches), we could tell the storage engine that, when reading the next index, it should skip over rowids lower than the next candidate intersection. In the best case scenario, I think this could enable InnoDB to use its page directory to skip past some of the keys, improving the performance from O(n) to O(log n). That said, this is all new to me. Maybe there's an obvious reason this wouldn't make much of an improvement, or maybe I've overlooked that it's already been done. However, if it looks promising to you folk, and it's something you'd consider merging, I'd be willing to attempt writing a PR for it. Thank you, David Sickmiller

3 6

Re: [Maria-developers] Missing memory barrier in parallel replication error handler in wait_for_prior_commit()?
by Kristian Nielsen 11 Oct '19

11 Oct '19

sujatha <sujatha.sivakumar(a)mariadb.com> writes: > I have a doubt. A simple fix as per the earlier mail discussion would > be to swap the > > order of assignments as shown below. > > wakeup_error= true > waitee= NULL > Why cannot we use the simpler approach. Please provide your inputs. This is because of the need for memory barriers, to prevent compiler and/or modern high-performance CPUs from re-ordering the memory accesses. There is nothing in C that requires the compiler to make the two assignments in the same order as in the source code. And even if it does, the CPU is free to perform the writes and/or the corresponding loads in the opposite order (the x86 architecture doesn't do that, but other architectures do). There are many resources on the net on the need for memory barriers, for example this from one of your collegues: https://mariadb.org/wp-content/uploads/2017/11/2017-11-Memory-barriers.pdf But it will be difficult to make a test case, I think. Because after correcting the order of the stores there is nowhere to put a debug-sleep, and there also is no easy way to force the CPU to take the incorrect memory order. - Kristian.

1 0

Re: [Maria-developers] 4ca016237f1: MDEV-20001 Potential dangerous regression: INSERT INTO >=100 rows fail for myisam table with HASH indexes
by Sergei Golubchik 09 Oct '19

09 Oct '19

Hi, Sachin! On Aug 20, Sachin Setiya wrote: > revision-id: 4ca016237f1 (mariadb-10.4.5-152-g4ca016237f1) > parent(s): 4a5cd407289 > author: Sachin <sachin.setiya(a)mariadb.com> > committer: Sachin <sachin.setiya(a)mariadb.com> > timestamp: 2019-07-29 19:33:05 +0530 > message: > > MDEV-20001 Potential dangerous regression: INSERT INTO >=100 rows fail for myisam table with HASH indexes > > Dont deactivate the long unique keys on bulk insert. > > diff --git a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc > index f478e01e441..c1169737911 100644 > --- a/storage/myisam/ha_myisam.cc > +++ b/storage/myisam/ha_myisam.cc > @@ -1749,7 +1749,16 @@ void ha_myisam::start_bulk_insert(ha_rows rows, uint flags) > else > { > my_bool all_keys= MY_TEST(flags & HA_CREATE_UNIQUE_INDEX_BY_SORT); > - mi_disable_indexes_for_rebuild(file, rows, all_keys); > + if (table->s->long_unique_table) > + { > + ulonglong hash_key_map= 0ULL; > + for(uint i= 0; i < table->s->keys; i++) > + if (table->key_info[i].algorithm == HA_KEY_ALG_LONG_HASH) > + mi_set_key_active(hash_key_map, i); > + mi_disable_indexes_for_rebuild(file, rows, all_keys, hash_key_map); > + } > + else > + mi_disable_indexes_for_rebuild(file, rows, all_keys, 0ULL); I agree with the fix, but here's a comment about the implementation. mi_disable_indexes_for_rebuild() is _only_ used here, nowhere else. So, I think I'd just removed mi_disable_indexes_for_rebuild(), inlined it here and do everything in one loop. Note, that you also need to fix Aria engine, it has almost the same code. And add a test for it, please. > } > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] [Commits] d48294d15c4: MDEV-13694: Wrong result upon GROUP BY with orderby_uses_equalities=on
by Sergey Petrunia 09 Oct '19

09 Oct '19

Hi Varun, This patch is a step in the right direction, but I think we're not quite there yet. The patch adds logic about semi-join materialization into the filesort code This makes things really messy. I think, some isolation would be beneficial. The first suggestion: In Sort_param (as this structure is visible in find_all_keys), add members that control the behavior inside find_all_keys: - unpack_function (default is NULL) - bool set_all_read_bits (default is FALSE) Now, Sort_param is allocated in filesort(), so we will have to add the same members in "class Filesort" and have filesort() copy them to Sort_param (necessary because find_all_keys() doesn't see the "Filesort" object). Then, these members can be set in the SQL layer e.g. in JOIN::add_sorting_to_table which creates the Filesort structure. (It would be ideal if semi-join code set them, but this is probably not possible to achieve as Filesort is set up after the semi-join code is set up). If after all these changes "JOIN_TAB::unpacking_to_base_table_fields()" is still there, please rename it to "unpack_..." so that it follows the established convention. That's all the input. On Sat, Sep 28, 2019 at 02:12:01PM +0530, Varun wrote: > revision-id: d48294d15c4895215d5facef97fc80c03cd6b4b0 (mariadb-10.4.4-341-gd48294d15c4) > parent(s): a340af922361e3958e5d6653c8b840771db282f2 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2019-09-28 13:06:44 +0530 > message: > > MDEV-13694: Wrong result upon GROUP BY with orderby_uses_equalities=on > > For the case when the SJM scan table is the first table in the join order, > then if we want to do the sorting on the SJM scan table, then we need to > make sure that we unpack the values to base table fields in two cases: > 1) Reading the SJM table and writing the sort-keys inside the sort-buffer > 2) Reading the sorted data from the sort file > > --- > mysql-test/main/order_by.result | 138 +++++++++++++++++++++++++++++++++++++++- > mysql-test/main/order_by.test | 34 ++++++++++ > sql/filesort.cc | 17 +++++ > sql/opt_subselect.cc | 10 ++- > sql/records.cc | 13 ++++ > sql/records.h | 1 + > sql/sql_select.cc | 89 ++++++++++---------------- > sql/sql_select.h | 4 +- > sql/table.h | 1 + > 9 files changed, 246 insertions(+), 61 deletions(-) > > diff --git a/mysql-test/main/order_by.result b/mysql-test/main/order_by.result > index b059cc686cd..e74583670fc 100644 > --- a/mysql-test/main/order_by.result > +++ b/mysql-test/main/order_by.result > @@ -3322,7 +3322,7 @@ WHERE books.library_id = 8663 AND > books.scheduled_for_removal=0 ) > ORDER BY wings.id; > id select_type table type possible_keys key key_len ref rows filtered Extra > -1 PRIMARY <subquery2> ALL distinct_key NULL NULL NULL 2 100.00 Using temporary; Using filesort > +1 PRIMARY <subquery2> ALL distinct_key NULL NULL NULL 2 100.00 Using filesort > 1 PRIMARY wings eq_ref PRIMARY PRIMARY 4 test.books.wings_id 1 100.00 > 2 MATERIALIZED books ref library_idx library_idx 4 const 2 100.00 Using where > Warnings: > @@ -3436,3 +3436,139 @@ Note 1003 select `test`.`t4`.`a` AS `a`,`test`.`t4`.`b` AS `b`,`test`.`t4`.`c` A > set histogram_size=@tmp_h, histogram_type=@tmp_ht, use_stat_tables=@tmp_u, > optimizer_use_condition_selectivity=@tmp_o; > drop table t1,t2,t3,t4; > +# > +# MDEV-13694: Wrong result upon GROUP BY with orderby_uses_equalities=on > +# > +CREATE TABLE t1 (a INT, b int, primary key(a)); > +CREATE TABLE t2 (a INT, b INT); > +INSERT INTO t1 (a,b) VALUES (58,1),(96,2),(273,3),(23,4),(231,5),(525,6), > +(2354,7),(321421,3),(535,2),(4535,3); > +INSERT INTO t2 (a,b) VALUES (58,3),(96,3),(273,3); > +# Join order should have the SJM scan table as the first table for both > +# the queries with GROUP BY and ORDER BY clause. > +EXPLAIN SELECT t1.a > +FROM t1 > +WHERE t1.a IN (SELECT a FROM t2 WHERE b=3) > +ORDER BY t1.a DESC; > +id select_type table type possible_keys key key_len ref rows Extra > +1 PRIMARY <subquery2> ALL distinct_key NULL NULL NULL 3 Using filesort > +1 PRIMARY t1 eq_ref PRIMARY PRIMARY 4 test.t2.a 1 Using index > +2 MATERIALIZED t2 ALL NULL NULL NULL NULL 3 Using where > +EXPLAIN FORMAT=JSON SELECT t1.a > +FROM t1 > +WHERE t1.a IN (SELECT a FROM t2 WHERE b=3) > +ORDER BY t1.a DESC; > +EXPLAIN > +{ > + "query_block": { > + "select_id": 1, > + "read_sorted_file": { > + "filesort": { > + "sort_key": "t1.a desc", > + "table": { > + "table_name": "<subquery2>", > + "access_type": "ALL", > + "possible_keys": ["distinct_key"], > + "rows": 3, > + "filtered": 100, > + "materialized": { > + "unique": 1, > + "query_block": { > + "select_id": 2, > + "table": { > + "table_name": "t2", > + "access_type": "ALL", > + "rows": 3, > + "filtered": 100, > + "attached_condition": "t2.b = 3 and t2.a is not null" > + } > + } > + } > + } > + } > + }, > + "table": { > + "table_name": "t1", > + "access_type": "eq_ref", > + "possible_keys": ["PRIMARY"], > + "key": "PRIMARY", > + "key_length": "4", > + "used_key_parts": ["a"], > + "ref": ["test.t2.a"], > + "rows": 1, > + "filtered": 100, > + "using_index": true > + } > + } > +} > +SELECT t1.a > +FROM t1 > +WHERE t1.a IN (SELECT a FROM t2 WHERE b=3) > +ORDER BY t1.a DESC; > +a > +273 > +96 > +58 > +EXPLAIN SELECT t1.a, group_concat(t1.b) > +FROM t1 > +WHERE t1.a IN (SELECT a FROM t2 WHERE b=3) > +GROUP BY t1.a DESC; > +id select_type table type possible_keys key key_len ref rows Extra > +1 PRIMARY <subquery2> ALL distinct_key NULL NULL NULL 3 Using filesort > +1 PRIMARY t1 eq_ref PRIMARY PRIMARY 4 test.t2.a 1 > +2 MATERIALIZED t2 ALL NULL NULL NULL NULL 3 Using where > +EXPLAIN FORMAT=JSON SELECT t1.a, group_concat(t1.b) > +FROM t1 > +WHERE t1.a IN (SELECT a FROM t2 WHERE b=3) > +GROUP BY t1.a DESC; > +EXPLAIN > +{ > + "query_block": { > + "select_id": 1, > + "read_sorted_file": { > + "filesort": { > + "sort_key": "t1.a desc", > + "table": { > + "table_name": "<subquery2>", > + "access_type": "ALL", > + "possible_keys": ["distinct_key"], > + "rows": 3, > + "filtered": 100, > + "materialized": { > + "unique": 1, > + "query_block": { > + "select_id": 2, > + "table": { > + "table_name": "t2", > + "access_type": "ALL", > + "rows": 3, > + "filtered": 100, > + "attached_condition": "t2.b = 3 and t2.a is not null" > + } > + } > + } > + } > + } > + }, > + "table": { > + "table_name": "t1", > + "access_type": "eq_ref", > + "possible_keys": ["PRIMARY"], > + "key": "PRIMARY", > + "key_length": "4", > + "used_key_parts": ["a"], > + "ref": ["test.t2.a"], > + "rows": 1, > + "filtered": 100 > + } > + } > +} > +SELECT t1.a, group_concat(t1.b) > +FROM t1 > +WHERE t1.a IN (SELECT a FROM t2 WHERE b=3) > +GROUP BY t1.a DESC; > +a group_concat(t1.b) > +273 3 > +96 2 > +58 1 > +DROP TABLE t1, t2; > diff --git a/mysql-test/main/order_by.test b/mysql-test/main/order_by.test > index 934c503302f..b3e43d27e2f 100644 > --- a/mysql-test/main/order_by.test > +++ b/mysql-test/main/order_by.test > @@ -2276,3 +2276,37 @@ set histogram_size=@tmp_h, histogram_type=@tmp_ht, use_stat_tables=@tmp_u, > optimizer_use_condition_selectivity=@tmp_o; > > drop table t1,t2,t3,t4; > + > + > +--echo # > +--echo # MDEV-13694: Wrong result upon GROUP BY with orderby_uses_equalities=on > +--echo # > + > +CREATE TABLE t1 (a INT, b int, primary key(a)); > +CREATE TABLE t2 (a INT, b INT); > + > +INSERT INTO t1 (a,b) VALUES (58,1),(96,2),(273,3),(23,4),(231,5),(525,6), > + (2354,7),(321421,3),(535,2),(4535,3); > +INSERT INTO t2 (a,b) VALUES (58,3),(96,3),(273,3); > + > +--echo # Join order should have the SJM scan table as the first table for both > +--echo # the queries with GROUP BY and ORDER BY clause. > + > +let $query= SELECT t1.a > + FROM t1 > + WHERE t1.a IN (SELECT a FROM t2 WHERE b=3) > + ORDER BY t1.a DESC; > + > +eval EXPLAIN $query; > +eval EXPLAIN FORMAT=JSON $query; > +eval $query; > + > +let $query= SELECT t1.a, group_concat(t1.b) > + FROM t1 > + WHERE t1.a IN (SELECT a FROM t2 WHERE b=3) > + GROUP BY t1.a DESC; > + > +eval EXPLAIN $query; > +eval EXPLAIN FORMAT=JSON $query; > +eval $query; > +DROP TABLE t1, t2; > diff --git a/sql/filesort.cc b/sql/filesort.cc > index 3f4291cfb1f..e5c83293e9f 100644 > --- a/sql/filesort.cc > +++ b/sql/filesort.cc > @@ -716,11 +716,21 @@ static ha_rows find_all_keys(THD *thd, Sort_param *param, SQL_SELECT *select, > *found_rows= 0; > ref_pos= &file->ref[0]; > next_pos=ref_pos; > + JOIN_TAB *tab= sort_form->reginfo.join_tab; > + JOIN *join= tab ? tab->join : NULL; > + bool first_is_in_sjm_nest= FALSE; > > DBUG_EXECUTE_IF("show_explain_in_find_all_keys", > dbug_serve_apcs(thd, 1); > ); > > + if (join && join->table_count != join->const_tables && > + (join->join_tab + join->const_tables == tab)) > + { > + TABLE_LIST *tbl_for_first= sort_form->pos_in_table_list; > + first_is_in_sjm_nest= tbl_for_first && tbl_for_first->is_sjm_scan_table(); > + } > + > if (!quick_select) > { > next_pos=(uchar*) 0; /* Find records in sequence */ > @@ -756,13 +766,20 @@ static ha_rows find_all_keys(THD *thd, Sort_param *param, SQL_SELECT *select, > goto err; > } > > + if (first_is_in_sjm_nest) > + sort_form->column_bitmaps_set(save_read_set, save_write_set); > + > DEBUG_SYNC(thd, "after_index_merge_phase1"); > for (;;) > { > if (quick_select) > error= select->quick->get_next(); > else /* Not quick-select */ > + { > error= file->ha_rnd_next(sort_form->record[0]); > + if (first_is_in_sjm_nest) > + tab->unpacking_to_base_table_fields(); > + } > if (unlikely(error)) > break; > file->position(sort_form->record[0]); > diff --git a/sql/opt_subselect.cc b/sql/opt_subselect.cc > index 87458357865..f837a6394af 100644 > --- a/sql/opt_subselect.cc > +++ b/sql/opt_subselect.cc > @@ -4252,11 +4252,11 @@ bool setup_sj_materialization_part2(JOIN_TAB *sjm_tab) > sjm_tab->type= JT_ALL; > > /* Initialize full scan */ > - sjm_tab->read_first_record= join_read_record_no_init; > + sjm_tab->read_first_record= join_init_read_record; > sjm_tab->read_record.copy_field= sjm->copy_field; > sjm_tab->read_record.copy_field_end= sjm->copy_field + > sjm->sjm_table_cols.elements; > - sjm_tab->read_record.read_record_func= rr_sequential_and_unpack; > + sjm_tab->read_record.read_record_func= read_record_func_for_rr_and_unpack; > } > > sjm_tab->bush_children->end[-1].next_select= end_sj_materialize; > @@ -7105,3 +7105,9 @@ bool Item_in_subselect::pushdown_cond_for_in_subquery(THD *thd, Item *cond) > thd->lex->current_select= save_curr_select; > DBUG_RETURN(FALSE); > } > + > + > +bool TABLE_LIST::is_sjm_scan_table() > +{ > + return is_active_sjm() && sj_mat_info->is_sj_scan; > +} > diff --git a/sql/records.cc b/sql/records.cc > index 3d709182a4e..f6885f773d5 100644 > --- a/sql/records.cc > +++ b/sql/records.cc > @@ -709,3 +709,16 @@ static int rr_cmp(uchar *a,uchar *b) > return (int) a[7] - (int) b[7]; > #endif > } > + > + > +int read_record_func_for_rr_and_unpack(READ_RECORD *info) > +{ > + int error; > + if ((error= info->read_record_func_and_unpack_calls(info))) > + return error; > + > + for (Copy_field *cp= info->copy_field; cp != info->copy_field_end; cp++) > + (*cp->do_copy)(cp); > + > + return error; > +} > diff --git a/sql/records.h b/sql/records.h > index faf0d13c9a9..037a06b9d34 100644 > --- a/sql/records.h > +++ b/sql/records.h > @@ -55,6 +55,7 @@ struct READ_RECORD > TABLE *table; /* Head-form */ > Unlock_row_func unlock_row; > Read_func read_record_func; > + Read_func read_record_func_and_unpack_calls; > THD *thd; > SQL_SELECT *select; > uint ref_length, reclength, rec_cache_size, error_offset; > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 36d9eda3383..28bc57c692f 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -14015,37 +14015,8 @@ remove_const(JOIN *join,ORDER *first_order, COND *cond, > can be used without tmp. table. > */ > bool can_subst_to_first_table= false; > - bool first_is_in_sjm_nest= false; > - if (first_is_base_table) > - { > - TABLE_LIST *tbl_for_first= > - join->join_tab[join->const_tables].table->pos_in_table_list; > - first_is_in_sjm_nest= tbl_for_first->sj_mat_info && > - tbl_for_first->sj_mat_info->is_used; > - } > - /* > - Currently we do not employ the optimization that uses multiple > - equalities for ORDER BY to remove tmp table in the case when > - the first table happens to be the result of materialization of > - a semi-join nest ( <=> first_is_in_sjm_nest == true). > - > - When a semi-join nest is materialized and scanned to look for > - possible matches in the remaining tables for every its row > - the fields from the result of materialization are copied > - into the record buffers of tables from the semi-join nest. > - So these copies are used to access the remaining tables rather > - than the fields from the result of materialization. > - > - Unfortunately now this so-called 'copy back' technique is > - supported only if the rows are scanned with the rr_sequential > - function, but not with other rr_* functions that are employed > - when the result of materialization is required to be sorted. > - > - TODO: either to support 'copy back' technique for the above case, > - or to get rid of this technique altogether. > - */ > if (optimizer_flag(join->thd, OPTIMIZER_SWITCH_ORDERBY_EQ_PROP) && > - first_is_base_table && !first_is_in_sjm_nest && > + first_is_base_table && > order->item[0]->real_item()->type() == Item::FIELD_ITEM && > join->cond_equal) > { > @@ -19922,19 +19893,6 @@ do_select(JOIN *join, Procedure *procedure) > } > > > -int rr_sequential_and_unpack(READ_RECORD *info) > -{ > - int error; > - if (unlikely((error= rr_sequential(info)))) > - return error; > - > - for (Copy_field *cp= info->copy_field; cp != info->copy_field_end; cp++) > - (*cp->do_copy)(cp); > - > - return error; > -} > - > - > /** > @brief > Instantiates temporary table > @@ -21223,6 +21181,8 @@ bool test_if_use_dynamic_range_scan(JOIN_TAB *join_tab) > > int join_init_read_record(JOIN_TAB *tab) > { > + bool need_unpacking= FALSE; > + JOIN *join= tab->join; > /* > Note: the query plan tree for the below operations is constructed in > save_agg_explain_data. > @@ -21232,6 +21192,12 @@ int join_init_read_record(JOIN_TAB *tab) > if (tab->filesort && tab->sort_table()) // Sort table. > return 1; > > + if (join->top_join_tab_count != join->const_tables) > + { > + TABLE_LIST *tbl= tab->table->pos_in_table_list; > + need_unpacking= tbl ? tbl->is_sjm_scan_table() : FALSE; > + } > + > tab->build_range_rowid_filter_if_needed(); > > DBUG_EXECUTE_IF("kill_join_init_read_record", > @@ -21249,16 +21215,6 @@ int join_init_read_record(JOIN_TAB *tab) > if (!tab->preread_init_done && tab->preread_init()) > return 1; > > - > - if (init_read_record(&tab->read_record, tab->join->thd, tab->table, > - tab->select, tab->filesort_result, 1,1, FALSE)) > - return 1; > - return tab->read_record.read_record(); > -} > - > -int > -join_read_record_no_init(JOIN_TAB *tab) > -{ > Copy_field *save_copy, *save_copy_end; > > /* > @@ -21268,12 +21224,20 @@ join_read_record_no_init(JOIN_TAB *tab) > save_copy= tab->read_record.copy_field; > save_copy_end= tab->read_record.copy_field_end; > > - init_read_record(&tab->read_record, tab->join->thd, tab->table, > - tab->select, tab->filesort_result, 1, 1, FALSE); > + > + if (init_read_record(&tab->read_record, tab->join->thd, tab->table, > + tab->select, tab->filesort_result, 1, 1, FALSE)) > + return 1; > > tab->read_record.copy_field= save_copy; > tab->read_record.copy_field_end= save_copy_end; > - tab->read_record.read_record_func= rr_sequential_and_unpack; > + > + if (need_unpacking) > + { > + tab->read_record.read_record_func_and_unpack_calls= > + tab->read_record.read_record_func; > + tab->read_record.read_record_func = read_record_func_for_rr_and_unpack; > + } > > return tab->read_record.read_record(); > } > @@ -28981,6 +28945,19 @@ void build_notnull_conds_for_inner_nest_of_outer_join(JOIN *join, > } > > > +/* > + @brief > + Unpacking temp table fields to base table fields. > +*/ > + > +void JOIN_TAB::unpacking_to_base_table_fields() > +{ > + for (Copy_field *cp= read_record.copy_field; > + cp != read_record.copy_field_end; cp++) > + (*cp->do_copy)(cp); > +} > + > + > /** > @} (end of group Query_Optimizer) > */ > diff --git a/sql/sql_select.h b/sql/sql_select.h > index 4f7bf49f635..545d4a788cc 100644 > --- a/sql/sql_select.h > +++ b/sql/sql_select.h > @@ -223,7 +223,7 @@ typedef enum_nested_loop_state > (*Next_select_func)(JOIN *, struct st_join_table *, bool); > Next_select_func setup_end_select_func(JOIN *join, JOIN_TAB *tab); > int rr_sequential(READ_RECORD *info); > -int rr_sequential_and_unpack(READ_RECORD *info); > +int read_record_func_for_rr_and_unpack(READ_RECORD *info); > Item *remove_pushed_top_conjuncts(THD *thd, Item *cond); > Item *and_new_conditions_to_optimized_cond(THD *thd, Item *cond, > COND_EQUAL **cond_eq, > @@ -676,6 +676,7 @@ typedef struct st_join_table { > table_map remaining_tables); > bool fix_splitting(SplM_plan_info *spl_plan, table_map remaining_tables, > bool is_const_table); > + void unpacking_to_base_table_fields(); > } JOIN_TAB; > > > @@ -2352,7 +2353,6 @@ create_virtual_tmp_table(THD *thd, Field *field) > > int test_if_item_cache_changed(List<Cached_item> &list); > int join_init_read_record(JOIN_TAB *tab); > -int join_read_record_no_init(JOIN_TAB *tab); > void set_position(JOIN *join,uint idx,JOIN_TAB *table,KEYUSE *key); > inline Item * and_items(THD *thd, Item* cond, Item *item) > { > diff --git a/sql/table.h b/sql/table.h > index 1a7e5fbd4dc..35ba9bbb95d 100644 > --- a/sql/table.h > +++ b/sql/table.h > @@ -2622,6 +2622,7 @@ struct TABLE_LIST > */ > const char *get_table_name() const { return view != NULL ? view_name.str : table_name.str; } > bool is_active_sjm(); > + bool is_sjm_scan_table(); > bool is_jtbm() { return MY_TEST(jtbm_subselect != NULL); } > st_select_lex_unit *get_unit(); > st_select_lex *get_single_select(); > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0