From 67e2028161d1f653a852f1a4679ff5e523296218 Mon Sep 17 00:00:00 2001 From: Sergei Golubchik Date: Sat, 14 Jan 2017 14:56:01 +0100 Subject: [PATCH 1/4] MDEV-9690 concurrent queries with virtual columns crash in temporal code Item_func_le included Arg_comparator. Arg_comparator remembered the current_thd during fix_fields and used that value during execution to allocate Item_cache in get_datetime_value(). But for vcols fix_fields and val_int can happen in different threads. Same bug for Item_func_in using in_datetime or cmp_item_datetime, both also remembered current_thd at fix_fields() to use it later for get_datetime_value(). As a fix, these objects no longer remember the current_thd, and get_datetime_value() uses current_thd at run time. This should not increase the number of current_thd calls much, as Item_cache is created only once anyway. --- mysql-test/suite/vcol/r/wrong_arena.result | 39 ++++++++++++++++++++++ mysql-test/suite/vcol/t/wrong_arena.test | 22 ++++++++++++ sql/item_cmpfunc.cc | 22 ++++++------ sql/item_cmpfunc.h | 12 +++---- 4 files changed, 77 insertions(+), 18 deletions(-) create mode 100644 mysql-test/suite/vcol/r/wrong_arena.result create mode 100644 mysql-test/suite/vcol/t/wrong_arena.test diff --git a/mysql-test/suite/vcol/r/wrong_arena.result b/mysql-test/suite/vcol/r/wrong_arena.result new file mode 100644 index 00000000000..d542c82458e --- /dev/null +++ b/mysql-test/suite/vcol/r/wrong_arena.result @@ -0,0 +1,39 @@ +create table t1 (a datetime, +# get_datetime_value +b int as (a > 1), # Arg_comparator +c int as (a in (1,2,3)), # in_datetime +d int as ((a,a) in ((1,1),(2,1),(NULL,1))) # cmp_item_datetime +); +Warnings: +Warning 1292 Incorrect datetime value: '1' +Warning 1292 Incorrect datetime value: '2' +Warning 1292 Incorrect datetime value: '3' +show create table t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `a` datetime DEFAULT NULL, + `b` int(11) AS (a > 1) VIRTUAL, + `c` int(11) AS (a in (1,2,3)) VIRTUAL, + `d` int(11) AS ((a,a) in ((1,1),(2,1),(NULL,1))) VIRTUAL +) ENGINE=MyISAM DEFAULT CHARSET=latin1 +Warnings: +Warning 1292 Incorrect datetime value: '1' +Warning 1292 Incorrect datetime value: '2' +Warning 1292 Incorrect datetime value: '3' +insert t1 (a) values ('2010-10-10 10:10:10'); +select * from t1; +a b c d +2010-10-10 10:10:10 1 0 0 +Warnings: +Warning 1292 Incorrect datetime value: '1' +Warning 1292 Incorrect datetime value: '1' +Warning 1292 Incorrect datetime value: '2' +Warning 1292 Incorrect datetime value: '1' +select * from t1; +a b c d +2010-10-10 10:10:10 1 0 0 +Warnings: +Warning 1292 Incorrect datetime value: '1' +Warning 1292 Incorrect datetime value: '2' +Warning 1292 Incorrect datetime value: '1' +drop table t1; diff --git a/mysql-test/suite/vcol/t/wrong_arena.test b/mysql-test/suite/vcol/t/wrong_arena.test new file mode 100644 index 00000000000..8bf06bdb9bd --- /dev/null +++ b/mysql-test/suite/vcol/t/wrong_arena.test @@ -0,0 +1,22 @@ +# +# This tests various issues when vcol items allocate memory (e.g. more items) +# not in the TABLE::expr_arena. +# + +# +# MDEV-9690 concurrent queries with virtual columns crash in temporal code +# +create table t1 (a datetime, + # get_datetime_value + b int as (a > 1), # Arg_comparator + c int as (a in (1,2,3)), # in_datetime + d int as ((a,a) in ((1,1),(2,1),(NULL,1))) # cmp_item_datetime +); +show create table t1; +connect con1, localhost, root; +insert t1 (a) values ('2010-10-10 10:10:10'); +select * from t1; +disconnect con1; +connection default; +select * from t1; +drop table t1; diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc index 3bd0b5b3fa2..46566206094 100644 --- a/sql/item_cmpfunc.cc +++ b/sql/item_cmpfunc.cc @@ -777,7 +777,7 @@ int Arg_comparator::set_cmp_func(Item_result_field *owner_arg, Item **a1, Item **a2, Item_result type) { - thd= current_thd; + THD *thd= current_thd; owner= owner_arg; set_null= set_null && owner_arg; a= a1; @@ -846,7 +846,6 @@ Item** Arg_comparator::cache_converted_constant(THD *thd_arg, Item **value, void Arg_comparator::set_datetime_cmp_func(Item_result_field *owner_arg, Item **a1, Item **b1) { - thd= current_thd; owner= owner_arg; a= a1; b= b1; @@ -919,6 +918,9 @@ get_datetime_value(THD *thd, Item ***item_arg, Item **cache_arg, if (cache_arg && item->const_item() && !(item->type() == Item::CACHE_ITEM && item->cmp_type() == TIME_RESULT)) { + if (!thd) + thd= current_thd; + Query_arena backup; Query_arena *save_arena= thd->switch_to_arena_for_cached_items(&backup); Item_cache_temporal *cache= new Item_cache_temporal(f_type); @@ -959,12 +961,12 @@ int Arg_comparator::compare_datetime() owner->null_value= 1; /* Get DATE/DATETIME/TIME value of the 'a' item. */ - a_value= get_datetime_value(thd, &a, &a_cache, *b, &a_is_null); + a_value= get_datetime_value(0, &a, &a_cache, *b, &a_is_null); if (a_is_null) return -1; /* Get DATE/DATETIME/TIME value of the 'b' item. */ - b_value= get_datetime_value(thd, &b, &b_cache, *a, &b_is_null); + b_value= get_datetime_value(0, &b, &b_cache, *a, &b_is_null); if (b_is_null) return -1; @@ -982,10 +984,10 @@ int Arg_comparator::compare_e_datetime() longlong a_value, b_value; /* Get DATE/DATETIME/TIME value of the 'a' item. */ - a_value= get_datetime_value(thd, &a, &a_cache, *b, &a_is_null); + a_value= get_datetime_value(0, &a, &a_cache, *b, &a_is_null); /* Get DATE/DATETIME/TIME value of the 'b' item. */ - b_value= get_datetime_value(thd, &b, &b_cache, *a, &b_is_null); + b_value= get_datetime_value(0, &b, &b_cache, *a, &b_is_null); return a_is_null || b_is_null ? a_is_null == b_is_null : a_value == b_value; } @@ -3600,7 +3602,7 @@ void in_datetime::set(uint pos,Item *item) bool is_null; struct packed_longlong *buff= &((packed_longlong*) base)[pos]; - buff->val= get_datetime_value(thd, &tmp_item, 0, warn_item, &is_null); + buff->val= get_datetime_value(0, &tmp_item, 0, warn_item, &is_null); buff->unsigned_flag= 1L; } @@ -3608,7 +3610,7 @@ uchar *in_datetime::get_value(Item *item) { bool is_null; Item **tmp_item= lval_cache ? &lval_cache : &item; - tmp.val= get_datetime_value(thd, &tmp_item, &lval_cache, warn_item, &is_null); + tmp.val= get_datetime_value(0, &tmp_item, &lval_cache, warn_item, &is_null); if (item->null_value) return 0; tmp.unsigned_flag= 1L; @@ -3852,7 +3854,7 @@ void cmp_item_datetime::store_value(Item *item) { bool is_null; Item **tmp_item= lval_cache ? &lval_cache : &item; - value= get_datetime_value(thd, &tmp_item, &lval_cache, warn_item, &is_null); + value= get_datetime_value(0, &tmp_item, &lval_cache, warn_item, &is_null); } @@ -3861,7 +3863,7 @@ int cmp_item_datetime::cmp(Item *arg) bool is_null; Item **tmp_item= &arg; return value != - get_datetime_value(thd, &tmp_item, 0, warn_item, &is_null); + get_datetime_value(0, &tmp_item, 0, warn_item, &is_null); } diff --git a/sql/item_cmpfunc.h b/sql/item_cmpfunc.h index 0194f9cd0e0..a8befa47b95 100644 --- a/sql/item_cmpfunc.h +++ b/sql/item_cmpfunc.h @@ -44,7 +44,6 @@ class Arg_comparator: public Sql_alloc Arg_comparator *comparators; // used only for compare_row() double precision; /* Fields used in DATE/DATETIME comparison. */ - THD *thd; Item *a_cache, *b_cache; // Cached values of a and b items // when one of arguments is NULL. public: @@ -52,10 +51,10 @@ public: /* Allow owner function to use string buffers. */ String value1, value2; - Arg_comparator(): set_null(TRUE), comparators(0), thd(0), + Arg_comparator(): set_null(TRUE), comparators(0), a_cache(0), b_cache(0) {}; Arg_comparator(Item **a1, Item **a2): a(a1), b(a2), set_null(TRUE), - comparators(0), thd(0), a_cache(0), b_cache(0) {}; + comparators(0), a_cache(0), b_cache(0) {}; int set_compare_func(Item_result_field *owner, Item_result type); inline int set_compare_func(Item_result_field *owner_arg) @@ -944,15 +943,13 @@ public: class in_datetime :public in_longlong { public: - THD *thd; /* An item used to issue warnings. */ Item *warn_item; /* Cache for the left item. */ Item *lval_cache; in_datetime(Item *warn_item_arg, uint elements) - :in_longlong(elements), thd(current_thd), warn_item(warn_item_arg), - lval_cache(0) {}; + :in_longlong(elements), warn_item(warn_item_arg), lval_cache(0) {}; void set(uint pos,Item *item); uchar *get_value(Item *item); Item* create_item() @@ -1112,14 +1109,13 @@ class cmp_item_datetime :public cmp_item { longlong value; public: - THD *thd; /* Item used for issuing warnings. */ Item *warn_item; /* Cache for the left item. */ Item *lval_cache; cmp_item_datetime(Item *warn_item_arg) - :thd(current_thd), warn_item(warn_item_arg), lval_cache(0) {} + : warn_item(warn_item_arg), lval_cache(0) {} void store_value(Item *item); int cmp(Item *arg); int compare(cmp_item *ci); From 798fcb541698cbf51f1ee33f44b023c11dc2b784 Mon Sep 17 00:00:00 2001 From: Sergei Golubchik Date: Sat, 14 Jan 2017 20:55:33 +0100 Subject: [PATCH 2/4] bugfix: cmp_item_row::alloc_comparators() allocated on the wrong arena it used current_thd->alloc() and allocated on the thd's execution arena, not on table->expr_arena. Remove THD::arena_for_cached_items that is temporarily set in update_virtual_fields(), and replaces THD arena in get_datetime_value(). Instead set THD arena to table->expr_arena for the whole duration of update_virtual_fields() --- mysql-test/suite/vcol/r/wrong_arena.result | 19 +++++++++++++------ mysql-test/suite/vcol/t/wrong_arena.test | 4 +++- sql/item_cmpfunc.cc | 5 ----- sql/sql_class.cc | 1 - sql/sql_class.h | 19 ------------------- sql/table.cc | 6 ++++-- 6 files changed, 20 insertions(+), 34 deletions(-) diff --git a/mysql-test/suite/vcol/r/wrong_arena.result b/mysql-test/suite/vcol/r/wrong_arena.result index d542c82458e..af41ea89ab5 100644 --- a/mysql-test/suite/vcol/r/wrong_arena.result +++ b/mysql-test/suite/vcol/r/wrong_arena.result @@ -2,7 +2,9 @@ create table t1 (a datetime, # get_datetime_value b int as (a > 1), # Arg_comparator c int as (a in (1,2,3)), # in_datetime -d int as ((a,a) in ((1,1),(2,1),(NULL,1))) # cmp_item_datetime +d int as ((a,a) in ((1,1),(2,1),(NULL,1))), # cmp_item_datetime +# other issues +e int as ((a,1) in ((1,1),(2,1),(NULL,1))) # cmp_item_row::alloc_comparators() ); Warnings: Warning 1292 Incorrect datetime value: '1' @@ -14,7 +16,8 @@ t1 CREATE TABLE `t1` ( `a` datetime DEFAULT NULL, `b` int(11) AS (a > 1) VIRTUAL, `c` int(11) AS (a in (1,2,3)) VIRTUAL, - `d` int(11) AS ((a,a) in ((1,1),(2,1),(NULL,1))) VIRTUAL + `d` int(11) AS ((a,a) in ((1,1),(2,1),(NULL,1))) VIRTUAL, + `e` int(11) AS ((a,1) in ((1,1),(2,1),(NULL,1))) VIRTUAL ) ENGINE=MyISAM DEFAULT CHARSET=latin1 Warnings: Warning 1292 Incorrect datetime value: '1' @@ -22,18 +25,22 @@ Warning 1292 Incorrect datetime value: '2' Warning 1292 Incorrect datetime value: '3' insert t1 (a) values ('2010-10-10 10:10:10'); select * from t1; -a b c d -2010-10-10 10:10:10 1 0 0 +a b c d e +2010-10-10 10:10:10 1 0 0 NULL Warnings: Warning 1292 Incorrect datetime value: '1' Warning 1292 Incorrect datetime value: '1' Warning 1292 Incorrect datetime value: '2' Warning 1292 Incorrect datetime value: '1' +Warning 1292 Incorrect datetime value: '1' +Warning 1292 Incorrect datetime value: '2' select * from t1; -a b c d -2010-10-10 10:10:10 1 0 0 +a b c d e +2010-10-10 10:10:10 1 0 0 NULL Warnings: Warning 1292 Incorrect datetime value: '1' Warning 1292 Incorrect datetime value: '2' Warning 1292 Incorrect datetime value: '1' +Warning 1292 Incorrect datetime value: '1' +Warning 1292 Incorrect datetime value: '2' drop table t1; diff --git a/mysql-test/suite/vcol/t/wrong_arena.test b/mysql-test/suite/vcol/t/wrong_arena.test index 8bf06bdb9bd..4276437f446 100644 --- a/mysql-test/suite/vcol/t/wrong_arena.test +++ b/mysql-test/suite/vcol/t/wrong_arena.test @@ -10,7 +10,9 @@ create table t1 (a datetime, # get_datetime_value b int as (a > 1), # Arg_comparator c int as (a in (1,2,3)), # in_datetime - d int as ((a,a) in ((1,1),(2,1),(NULL,1))) # cmp_item_datetime + d int as ((a,a) in ((1,1),(2,1),(NULL,1))), # cmp_item_datetime + # other issues + e int as ((a,1) in ((1,1),(2,1),(NULL,1))) # cmp_item_row::alloc_comparators() ); show create table t1; connect con1, localhost, root; diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc index 46566206094..ebe088e5092 100644 --- a/sql/item_cmpfunc.cc +++ b/sql/item_cmpfunc.cc @@ -921,12 +921,7 @@ get_datetime_value(THD *thd, Item ***item_arg, Item **cache_arg, if (!thd) thd= current_thd; - Query_arena backup; - Query_arena *save_arena= thd->switch_to_arena_for_cached_items(&backup); Item_cache_temporal *cache= new Item_cache_temporal(f_type); - if (save_arena) - thd->set_query_arena(save_arena); - cache->store_packed(value, item); *cache_arg= cache; *item_arg= cache_arg; diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 62339b2690a..93af4d3bb4d 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -957,7 +957,6 @@ THD::THD() m_internal_handler= NULL; m_binlog_invoker= FALSE; - arena_for_cached_items= 0; memset(&invoker_user, 0, sizeof(invoker_user)); memset(&invoker_host, 0, sizeof(invoker_host)); prepare_derived_at_open= FALSE; diff --git a/sql/sql_class.h b/sql/sql_class.h index 27bc40e3761..5dd7cd18a5d 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -3102,26 +3102,7 @@ public: } } -private: - /* - This reference points to the table arena when the expression - for a virtual column is being evaluated - */ - Query_arena *arena_for_cached_items; - public: - void reset_arena_for_cached_items(Query_arena *new_arena) - { - arena_for_cached_items= new_arena; - } - Query_arena *switch_to_arena_for_cached_items(Query_arena *backup) - { - if (!arena_for_cached_items) - return 0; - set_n_backup_active_arena(arena_for_cached_items, backup); - return backup; - } - void clear_wakeup_ready() { wakeup_ready= false; } /* Sleep waiting for others to wake us up with signal_wakeup_ready(). diff --git a/sql/table.cc b/sql/table.cc index 1330560b6b6..9d52d5f87a2 100644 --- a/sql/table.cc +++ b/sql/table.cc @@ -6575,7 +6575,9 @@ int update_virtual_fields(THD *thd, TABLE *table, int error __attribute__ ((unused))= 0; DBUG_ASSERT(table && table->vfield); - thd->reset_arena_for_cached_items(table->expr_arena); + Query_arena backup_arena; + thd->set_n_backup_active_arena(table->expr_arena, &backup_arena); + /* Iterate over virtual fields in the table */ for (vfield_ptr= table->vfield; *vfield_ptr; vfield_ptr++) { @@ -6593,7 +6595,7 @@ int update_virtual_fields(THD *thd, TABLE *table, DBUG_PRINT("info", ("field '%s' - skipped", vfield->field_name)); } } - thd->reset_arena_for_cached_items(0); + thd->restore_active_arena(table->expr_arena, &backup_arena); DBUG_RETURN(0); } From b948b5f7c64c6430d98dc31b7e9f71d990b40ec1 Mon Sep 17 00:00:00 2001 From: Sergei Golubchik Date: Sat, 14 Jan 2017 21:23:00 +0100 Subject: [PATCH 3/4] bugfix: Item_func_min_max stored thd internally It was used for get_datetime_value() and for thd->is_error(). But in fact, get_datetime_value() never used thd argument, because the cache ptr argument was NULL. And thd->is_error() check was not needed at that place at all. --- mysql-test/suite/vcol/r/wrong_arena.result | 15 +++++++++++++++ mysql-test/suite/vcol/t/wrong_arena.test | 11 +++++++++++ sql/item_func.cc | 7 ++----- sql/item_func.h | 1 - 4 files changed, 28 insertions(+), 6 deletions(-) diff --git a/mysql-test/suite/vcol/r/wrong_arena.result b/mysql-test/suite/vcol/r/wrong_arena.result index af41ea89ab5..172b59d6c4c 100644 --- a/mysql-test/suite/vcol/r/wrong_arena.result +++ b/mysql-test/suite/vcol/r/wrong_arena.result @@ -44,3 +44,18 @@ Warning 1292 Incorrect datetime value: '1' Warning 1292 Incorrect datetime value: '1' Warning 1292 Incorrect datetime value: '2' drop table t1; +create table t1 (a datetime, +b datetime as (least(a,1)) # Item_func_min_max::get_date +); +insert t1 (a) values ('2010-10-10 10:10:10'); +select * from t1; +a b +2010-10-10 10:10:10 0000-00-00 00:00:00 +Warnings: +Warning 1292 Incorrect datetime value: '1' +select * from t1; +a b +2010-10-10 10:10:10 0000-00-00 00:00:00 +Warnings: +Warning 1292 Incorrect datetime value: '1' +drop table t1; diff --git a/mysql-test/suite/vcol/t/wrong_arena.test b/mysql-test/suite/vcol/t/wrong_arena.test index 4276437f446..484f1fe685d 100644 --- a/mysql-test/suite/vcol/t/wrong_arena.test +++ b/mysql-test/suite/vcol/t/wrong_arena.test @@ -22,3 +22,14 @@ disconnect con1; connection default; select * from t1; drop table t1; + +connect con1, localhost, root; +create table t1 (a datetime, + b datetime as (least(a,1)) # Item_func_min_max::get_date +); +insert t1 (a) values ('2010-10-10 10:10:10'); +select * from t1; +disconnect con1; +connection default; +select * from t1; +drop table t1; diff --git a/sql/item_func.cc b/sql/item_func.cc index 89d3cd9e32a..cfccd66ea8a 100644 --- a/sql/item_func.cc +++ b/sql/item_func.cc @@ -2856,7 +2856,6 @@ void Item_func_min_max::fix_length_and_dec() decimals=0; max_length=0; maybe_null=0; - thd= current_thd; cmp_type=args[0]->result_type(); for (uint i=0 ; i < arg_count ; i++) @@ -2929,13 +2928,11 @@ bool Item_func_min_max::get_date(MYSQL_TIME *ltime, ulonglong fuzzy_date) { Item **arg= args + i; bool is_null; - longlong res= get_datetime_value(thd, &arg, 0, compare_as_dates, &is_null); + longlong res= get_datetime_value(0, &arg, 0, compare_as_dates, &is_null); /* Check if we need to stop (because of error or KILL) and stop the loop */ - if (thd->is_error() || args[i]->null_value) - { + if (args[i]->null_value) return (null_value= 1); - } if (i == 0 || (res < min_max ? cmp_sign : -cmp_sign) > 0) min_max= res; diff --git a/sql/item_func.h b/sql/item_func.h index 0da38e22c7f..d60801745fe 100644 --- a/sql/item_func.h +++ b/sql/item_func.h @@ -1068,7 +1068,6 @@ class Item_func_min_max :public Item_func int cmp_sign; /* An item used for issuing warnings while string to DATETIME conversion. */ Item *compare_as_dates; - THD *thd; protected: enum_field_types cached_field_type; public: From e4e801d4789d3992c1f04b260de3af4c9e5e6b0c Mon Sep 17 00:00:00 2001 From: Sergei Golubchik Date: Tue, 17 Jan 2017 11:15:21 +0100 Subject: [PATCH 4/4] connect: compilation errors and few obvious bugs --- storage/connect/filamzip.cpp | 8 ++++---- storage/connect/ioapi.h | 5 +++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/storage/connect/filamzip.cpp b/storage/connect/filamzip.cpp index 65013e170e4..22fe95aebad 100644 --- a/storage/connect/filamzip.cpp +++ b/storage/connect/filamzip.cpp @@ -97,7 +97,7 @@ loopStart: if (!*++pat) return TRUE; goto loopStart; default: - if (mapCaseTable[*s] != mapCaseTable[*p]) + if (mapCaseTable[(unsigned)*s] != mapCaseTable[(unsigned)*p]) goto starCheck; break; } /* endswitch */ @@ -151,7 +151,7 @@ int ZIPUTIL::findEntry(PGLOBAL g, bool next) if (rc == UNZ_END_OF_LIST_OF_FILE) return RC_EF; else if (rc != UNZ_OK) { - sprintf(g->Message, "unzGoToNextFile rc = ", rc); + sprintf(g->Message, "unzGoToNextFile rc = %d", rc); return RC_FX; } // endif rc @@ -261,7 +261,7 @@ bool ZIPUTIL::OpenTable(PGLOBAL g, MODE mode, char *fn) fp->Memory = memory; fp->Mode = mode; fp->File = this; - fp->Handle = NULL; + fp->Handle = 0; } // endif fp } else @@ -297,7 +297,7 @@ bool ZIPUTIL::openEntry(PGLOBAL g) memory = new char[size + 1]; if ((rc = unzReadCurrentFile(zipfile, memory, size)) < 0) { - sprintf(g->Message, "unzReadCurrentFile rc = ", rc); + sprintf(g->Message, "unzReadCurrentFile rc = %d", rc); unzCloseCurrentFile(zipfile); free(memory); memory = NULL; diff --git a/storage/connect/ioapi.h b/storage/connect/ioapi.h index 8dcbdb06e35..4fa73002053 100644 --- a/storage/connect/ioapi.h +++ b/storage/connect/ioapi.h @@ -129,8 +129,9 @@ extern "C" { #endif #endif - - +#ifndef OF +#define OF(args) args +#endif typedef voidpf (ZCALLBACK *open_file_func) OF((voidpf opaque, const char* filename, int mode)); typedef uLong (ZCALLBACK *read_file_func) OF((voidpf opaque, voidpf stream, void* buf, uLong size));