commit 02362a77d0d458f8bbdca50f660e7bfca1470a35 Author: Sachin <sachin.setiya@mariadb.com> Date: Mon Oct 14 15:29:14 2019 +0530 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. diff --git a/mysql-test/main/long_unique_bugs.result b/mysql-test/main/long_unique_bugs.result index c0ba4d0b87d..16e825f0905 100644 --- a/mysql-test/main/long_unique_bugs.result +++ b/mysql-test/main/long_unique_bugs.result @@ -270,3 +270,32 @@ ERROR 42000: Specified key was too long; max key length is 1000 bytes create table t1(a int, unique(a) using hash); #BULK insert > 100 rows (MI_MIN_ROWS_TO_DISABLE_INDEXES) drop table t1; +CREATE TABLE t1 (a blob, UNIQUE(a)) ENGINE=MyISAM; +INSERT DELAYED t1 () VALUES (1); +INSERT t1 () VALUES (2); +DROP TABLE t1; +CREATE TABLE t1 (a char(50), UNIQUE(a(10)) USING HASH); +INSERT DELAYED t1 () VALUES (1); +INSERT t1 () VALUES (2); +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); +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); +#Should not fails +insert delayed into t1 values(2); +select a,b from t1 order by a; +a b +1 NULL +2 2 +2 NULL +DROP TABLE t1; 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 +insert delayed into t1 values(2); +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. + This is done because view created on invisible fields are 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; (*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]); +} + +/* + call re_setup_keyinfo_hash for all keys in table + */ +void TABLE::re_setup_keyinfo_hash_all() +{ + 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 diff --git a/sql/table.h b/sql/table.h index 1c721624d5d..76d11bbd604 100644 --- a/sql/table.h +++ b/sql/table.h @@ -1612,6 +1612,8 @@ struct TABLE void vers_update_fields(); void vers_update_end(); void find_constraint_correlated_indexes(); + void setup_keyinfo_hash_all(); + void re_setup_keyinfo_hash_all(); void clone_handler_for_update(); void delete_update_handler(); -- Regards Sachin Setiya Software Engineer at MariaDB