- developers - lists.mariadb.org

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

29 Oct '19

Hi, Sachin! Looks good now, thanks. See comments below On Oct 29, Sachin Setiya wrote: > revision-id: 02362a77d0d (mariadb-10.4.7-111-g02362a77d0d) > parent(s): 52f3829b95c > author: Sachin <sachin.setiya(a)mariadb.com> > committer: Sachin <sachin.setiya(a)mariadb.com> > timestamp: 2019-10-14 16:10:54 +0530 > message: > > MDEV-19848 Server crashes in check_vcol_forward_refs upon INSERT DELAYED into table with long blob key > > Problem:- Insert delayed is not working with Long Unique Index. > It is failing with > 1. INSERT DELAYED INTO t1 VALUES(); > 2. INSERT DELAYED INTO t1 VALUES(1); > 3. Potential Race condition When Insert DELAYED gets dup key error(After fix), > And it will change original table key_info by calling > re_setup_keyinfo_hash, And second thread is in check_duplicate_long_entries > 4. Insert delayed into INVISIBLE COLUMN will also not work. > > There are 4 main issue > > 1. while calling make_new_field we forgot to & LONG_UNIQUE_HASH_FIELD > flag into new field flags. > > 2. New field created created into get_local_table by make_new_field does > not respect old field visibility, Assigning old field visibility will > solve Problem 4 and part of problem 2. > > 3. As we know Problem 3 race condition is caused because table and > delayed table share same key_info, So we will make a copy of original table > key_info in get_local_table. > > 4. In parse_vcol_defs we have this code block > keypart->field->vcol_info= > table->field[keypart->field->field_index]->vcol_info; > Which is wrong because we should not change original > table->field->vcol_info with vcol_info which is create on delayed > thread. > > --- > mysql-test/main/long_unique_bugs.result | 29 +++++++++++++++++++++++++++++ > mysql-test/main/long_unique_bugs.test | 32 ++++++++++++++++++++++++++++++++ > sql/field.cc | 7 ++++++- > sql/sql_insert.cc | 31 +++++++++++++++++++++++++++++++ > sql/table.cc | 31 ++++++++++++++++++++++++++++++- > sql/table.h | 2 ++ > 6 files changed, 130 insertions(+), 2 deletions(-) > > diff --git a/mysql-test/main/long_unique_bugs.test b/mysql-test/main/long_unique_bugs.test > index 13a4e1367a0..365393aa2ea 100644 > --- a/mysql-test/main/long_unique_bugs.test > +++ b/mysql-test/main/long_unique_bugs.test > @@ -340,3 +340,35 @@ while ($count) > --eval $insert_stmt > --enable_query_log > drop table t1; > +# > +# MDEV-19848 Server crashes in check_vcol_forward_refs upon INSERT DELAYED into table with long blob key > +# > +CREATE TABLE t1 (a blob, UNIQUE(a)) ENGINE=MyISAM; > +INSERT DELAYED t1 () VALUES (1); > +INSERT t1 () VALUES (2); > +# Cleanup > +DROP TABLE t1; > +CREATE TABLE t1 (a char(50), UNIQUE(a(10)) USING HASH); > +INSERT DELAYED t1 () VALUES (1); > +INSERT t1 () VALUES (2); > +# Cleanup > +DROP TABLE t1; > +CREATE TABLE t1 ( > + a CHAR(128), > + b CHAR(128) AS (a), > + c varchar(5000), > + UNIQUE(c,b(64)) > +) ENGINE=myisam; > +INSERT DELAYED t1 (a,c) VALUES (1,1); > +--sleep 1 > +INSERT t1 (a,c) VALUES (2,2); > +INSERT t1 (a,c) VALUES (3,3); > +drop table t1; > +create table t1(a int , b int invisible); > +insert into t1 values(1); > +insert delayed into t1(a,b) values(2,2); > +--echo #Should not fails "Should not fail" > +insert delayed into t1 values(2); I suspect there's a race condition here. You cannot know that INSERT DELAYED will insert rows before you do your SELECT. Other tests usually do FLUSH TABLE t1 before SELECT. Alternatively you can use include/wait_condition.inc file to wait for, say, SELECT COUNT(*) = 3 > +select a,b from t1 order by a; > +# Cleanup > +DROP TABLE t1; > diff --git a/sql/field.cc b/sql/field.cc > index 0eb53f40a54..d1187237f23 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -2373,8 +2373,13 @@ Field *Field::make_new_field(MEM_ROOT *root, TABLE *new_table, > tmp->flags&= (NOT_NULL_FLAG | BLOB_FLAG | UNSIGNED_FLAG | > ZEROFILL_FLAG | BINARY_FLAG | ENUM_FLAG | SET_FLAG | > VERS_SYS_START_FLAG | VERS_SYS_END_FLAG | > - VERS_UPDATE_UNVERSIONED_FLAG); > + VERS_UPDATE_UNVERSIONED_FLAG | LONG_UNIQUE_HASH_FIELD); > tmp->reset_fields(); > + /* > + Calling make_new_field will return a VISIBLE field, If caller function > + wants original visibility he should change it later. "visibility it should change" > + This is done because view created on invisible fields are visible. "because invisible fields explicitly named in a view become visible" > + */ > tmp->invisible= VISIBLE; > return tmp; > } > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index fe6c5fa8ec4..d80044ec37d 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -2607,6 +2607,11 @@ TABLE *Delayed_insert::get_local_table(THD* client_thd) > { > if (!(*field= (*org_field)->make_new_field(client_thd->mem_root, copy, 1))) > goto error; > + /* > + We want same visibility as of original table because we are just creating > + a clone for delayed insert. > + */ > + (*field)->invisible= (*org_field)->invisible; I'm surprised it matters. I'd thought that all sanity checks, and in particular whether number of columns matches the number of values, are done in the connection user thread before submitting the job to the delayed insert thread. > (*field)->unireg_check= (*org_field)->unireg_check; > (*field)->orig_table= copy; // Remove connection > (*field)->move_field_offset(adjust_ptrs); // Point at copy->record[0] > @@ -2621,7 +2626,33 @@ TABLE *Delayed_insert::get_local_table(THD* client_thd) > if (share->virtual_fields || share->default_expressions || > share->default_fields) > { > + /* > + If we have long unique table then delayed insert can modify key structure > + (re/setup_keyinfo_hash_all) of original table when it gets insert error, > + parse_vcol_defs will also modify key_info structure. So it is better to > + clone the table->key_info for copy table. > + We will not be cloning key_part_info or even changing any field ptr. > + Because re/setup_keyinfo_hash_all only modify key_info array. So it will > + be like having new key_info array for copy table with old key_part_info > + ptr. > + */ > + if (share->long_unique_table) > + { > + KEY *key_info; > + if (!(key_info= (KEY*) client_thd->alloc(share->keys*sizeof(KEY)))) > + goto error; > + copy->key_info= key_info; > + memcpy(key_info, table->key_info, sizeof(KEY)*share->keys); > + } > + /* > + parse_vcol_defs expects key_infos to be in user defined format. > + */ > + copy->setup_keyinfo_hash_all(); > bool error_reported= FALSE; > + /* > + We won't be calling re_setup_keyinfo_hash because parse_vcol_defs changes > + key_infos to storage engine format > + */ > if (unlikely(parse_vcol_defs(client_thd, client_thd->mem_root, copy, > &error_reported, > VCOL_INIT_DEPENDENCY_FAILURE_IS_WARNING))) > diff --git a/sql/table.cc b/sql/table.cc > index 718d0dce072..d0fcd449098 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -1223,7 +1223,16 @@ bool parse_vcol_defs(THD *thd, MEM_ROOT *mem_root, TABLE *table, > new (mem_root) Item_field(thd, keypart->field), > new (mem_root) Item_int(thd, length)); > list_item->fix_fields(thd, NULL); > - keypart->field->vcol_info= > + /* > + Do not change the vcol_info when vcol_info->expr is not NULL > + This will happen in the case of Delayed_insert::get_local_table() > + And if we change the vcol_info in Delayed insert , then original > + table field->vcol_info will be created on delayed insert thread > + mem_root. > + */ > + if (!keypart->field->vcol_info || > + !keypart->field->vcol_info->expr) > + keypart->field->vcol_info= > table->field[keypart->field->field_index]->vcol_info; > } > else > @@ -9084,6 +9093,26 @@ void re_setup_keyinfo_hash(KEY *key_info) > key_info->ext_key_parts= 1; > key_info->flags&= ~HA_NOSAME; > } > + > +/* > + call setup_keyinfo_hash for all keys in table > + */ > +void TABLE::setup_keyinfo_hash_all() > +{ > + for (uint i= 0; i < s->keys; i++) > + if (key_info[i].algorithm == HA_KEY_ALG_LONG_HASH) > + setup_keyinfo_hash(&key_info[i]); also... I remember you wanted to rename these methods to something more descriptive. > +} > + > +/* > + call re_setup_keyinfo_hash for all keys in table > + */ > +void TABLE::re_setup_keyinfo_hash_all() don't add this method, you aren't using it anywhere > +{ > + for (uint i= 0; i < s->keys; i++) > + if (key_info[i].algorithm == HA_KEY_ALG_LONG_HASH) > + re_setup_keyinfo_hash(&key_info[i]); > +} > /** > @brief clone of current handler. > Creates a clone of handler used in update for Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 96569b793a3: MDEV-19761 - Before Trigger not processed for Not Null Column
by Sergei Golubchik 29 Oct '19

29 Oct '19

Hi, Anel! On Oct 29, Anel Husakovic wrote: > revision-id: 96569b793a3 (mariadb-10.1.39-121-g96569b793a3) > parent(s): e32f29b7f31 > author: Anel Husakovic <anel(a)mariadb.org> > committer: Anel Husakovic <anel(a)mariadb.org> > timestamp: 2019-07-24 23:50:29 -0700 > message: > > MDEV-19761 - Before Trigger not processed for Not Null Column here you should have a commit comment explaining the bug and your solution. > --- > mysql-test/r/trigger.result | 6 +++- > mysql-test/r/trigger_no_defaults-11698.result | 29 +++++++++++++++++++ > mysql-test/r/trigger_null-8605.result | 2 +- > mysql-test/suite/funcs_1/r/innodb_trig_09.result | 4 +-- > mysql-test/suite/funcs_1/r/memory_trig_09.result | 4 +-- > mysql-test/suite/funcs_1/r/myisam_trig_09.result | 4 +-- > mysql-test/t/trigger_no_defaults-11698.test | 37 ++++++++++++++++++++---- > sql/sql_base.cc | 23 +++++++++++++++ > sql/sql_trigger.cc | 2 +- > sql/sql_trigger.h | 4 +++ > 10 files changed, 97 insertions(+), 18 deletions(-) > > diff --git a/mysql-test/r/trigger.result b/mysql-test/r/trigger.result > index 8852a622251..04bb9d9be68 100644 > --- a/mysql-test/r/trigger.result > +++ b/mysql-test/r/trigger.result > @@ -366,6 +366,8 @@ create trigger trg1 before update on t1 for each row set @a:= @a + new.j - old.j > create trigger trg2 after update on t1 for each row set @b:= "Fired"; > set @a:= 0, @b:= ""; > update t1, t2 set j = j + 10 where t1.i = t2.i; > +Warnings: > +Warning 1048 Column 'k' cannot be null this looks wrong. Why there's suddenly a warning here? > select @a, @b; > @a @b > 10 Fired > @@ -1235,11 +1237,13 @@ insert into t1 values (1,1), (2,2), (3,3); > create trigger t1_bu before update on t1 for each row > set new.j = new.j + 10; > update t1 set i= i+ 10 where j > 2; > +Warnings: > +Warning 1048 Column 'j' cannot be null and here > select * from t1; > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index e8bdff8b48f..d9fe37c83da 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -8959,6 +8959,29 @@ void switch_to_nullable_trigger_fields(List<Item> &items, TABLE *table) > > while ((item= it++)) > item->walk(&Item::switch_to_nullable_fields_processor, 1, (uchar*)field); > + uint16 cnt=0; > + uchar *nptr; > + nptr= (uchar*)alloc_root(&table->mem_root, (table->s->fields - table->s->null_fields + 7)/8); > + // First find null_ptr for NULL field in case of mixed NULL and NOT NULL fields > + for (Field **f= field; *f; f++) > + { > + if (table->field[cnt]->null_ptr) > + { > + nptr= table->field[cnt]->null_ptr; > + break; > + } > + } > + for (Field **f= field; *f; f++) > + { > + if (!table->field[cnt]->null_ptr) > + { > + (*f)->null_bit= 1<<(cnt+1); > + (*f)->flags&= ~(NOT_NULL_FLAG); > + (*f)->null_ptr= nptr; > + } > + cnt++; > + } > + bzero(nptr, (table->s->fields - table->s->null_fields + 7)/8); what are you doing here? > table->triggers->reset_extra_null_bitmap(); > } > } > diff --git a/sql/sql_trigger.cc b/sql/sql_trigger.cc > index 4ecd8139921..7de2fde0e5c 100644 > --- a/sql/sql_trigger.cc > +++ b/sql/sql_trigger.cc > @@ -1118,7 +1118,7 @@ bool Table_triggers_list::prepare_record_accessors(TABLE *table) > > f->flags= (*fld)->flags; > f->null_ptr= null_ptr; > - f->null_bit= null_bit; > + f->null_bit= (*fld)->null_bit; > if (null_bit == 128) > null_ptr++, null_bit= 1; > else > diff --git a/sql/sql_trigger.h b/sql/sql_trigger.h > index f451dfda1ee..c1174bfd36e 100644 > --- a/sql/sql_trigger.h > +++ b/sql/sql_trigger.h > @@ -226,6 +226,10 @@ class Table_triggers_list: public Sql_alloc > trigger_table->s->null_fields + 7)/8; > bzero(extra_null_bitmap, null_bytes); > } > + uchar *get_extra_null_bitmap() const > + { > + return extra_null_bitmap; > + } This is not used anywhere > private: > bool prepare_record_accessors(TABLE *table); > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

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