From 519060da452925a2f8274b1d30c7518830299948 Mon Sep 17 00:00:00 2001 From: Jacob Mathew Date: Mon, 21 May 2018 19:17:03 -0700 Subject: [PATCH 1/8] MDEV-12900: spider tests failed in buildbot with valgrind The failures with valgrind occur as a result of Spider sometimes using the wrong transaction for operations in background threads that send requests to the data nodes. The use of the wrong transaction caused the networking to the data nodes to use the wrong thread in some cases. Valgrind eventually detects this when such a thread is destroyed before it is used to disconnect from the data node by that wrong transaction when it is freed. I have fixed the problem by correcting the transaction used in each of these cases. Author: Jacob Mathew. Reviewer: Kentoku Shiba. Cherry-Picked: Commit afe5a51 on branch 10.2 --- storage/spider/spd_conn.cc | 29 ++++++------------------ storage/spider/spd_ping_table.cc | 22 ------------------ storage/spider/spd_table.cc | 39 -------------------------------- 3 files changed, 7 insertions(+), 83 deletions(-) diff --git a/storage/spider/spd_conn.cc b/storage/spider/spd_conn.cc index fc3d9ecc25f..5de41ac0f98 100644 --- a/storage/spider/spd_conn.cc +++ b/storage/spider/spd_conn.cc @@ -69,9 +69,6 @@ extern PSI_thread_key spd_key_thd_bg_crd; extern PSI_thread_key spd_key_thd_bg_mon; #endif #endif - -extern pthread_mutex_t spider_global_trx_mutex; -extern SPIDER_TRX *spider_global_trx; #endif HASH spider_open_connections; @@ -2807,9 +2804,6 @@ void *spider_bg_sts_action( DBUG_RETURN(NULL); } share->bg_sts_thd = thd; -/* - spider.trx = spider_global_trx; -*/ spider.trx = trx; spider.share = share; spider.conns = conns; @@ -2922,13 +2916,11 @@ void *spider_bg_sts_action( { if (!conns[spider.search_link_idx]) { - pthread_mutex_lock(&spider_global_trx_mutex); spider_get_conn(share, spider.search_link_idx, share->conn_keys[spider.search_link_idx], - spider_global_trx, &spider, FALSE, FALSE, SPIDER_CONN_KIND_MYSQL, + trx, &spider, FALSE, FALSE, SPIDER_CONN_KIND_MYSQL, &error_num); conns[spider.search_link_idx]->error_mode = 0; - pthread_mutex_unlock(&spider_global_trx_mutex); /* if ( error_num && @@ -2937,7 +2929,7 @@ void *spider_bg_sts_action( ) { lex_start(thd); error_num = spider_ping_table_mon_from_table( - spider_global_trx, + trx, thd, share, (uint32) share->monitoring_sid[spider.search_link_idx], @@ -2958,7 +2950,6 @@ void *spider_bg_sts_action( } if (spider.search_link_idx != -1 && conns[spider.search_link_idx]) { - DBUG_ASSERT(!conns[spider.search_link_idx]->thd); #ifdef WITH_PARTITION_STORAGE_ENGINE if (spider_get_sts(share, spider.search_link_idx, share->bg_sts_try_time, &spider, @@ -2979,7 +2970,7 @@ void *spider_bg_sts_action( ) { lex_start(thd); error_num = spider_ping_table_mon_from_table( - spider_global_trx, + trx, thd, share, (uint32) share->monitoring_sid[spider.search_link_idx], @@ -3192,9 +3183,6 @@ void *spider_bg_crd_action( table.s = share->table_share; table.field = share->table_share->field; table.key_info = share->table_share->key_info; -/* - spider.trx = spider_global_trx; -*/ spider.trx = trx; spider.change_table_ptr(&table, share->table_share); spider.share = share; @@ -3308,13 +3296,11 @@ void *spider_bg_crd_action( { if (!conns[spider.search_link_idx]) { - pthread_mutex_lock(&spider_global_trx_mutex); spider_get_conn(share, spider.search_link_idx, share->conn_keys[spider.search_link_idx], - spider_global_trx, &spider, FALSE, FALSE, SPIDER_CONN_KIND_MYSQL, + trx, &spider, FALSE, FALSE, SPIDER_CONN_KIND_MYSQL, &error_num); conns[spider.search_link_idx]->error_mode = 0; - pthread_mutex_unlock(&spider_global_trx_mutex); /* if ( error_num && @@ -3323,7 +3309,7 @@ void *spider_bg_crd_action( ) { lex_start(thd); error_num = spider_ping_table_mon_from_table( - spider_global_trx, + trx, thd, share, (uint32) share->monitoring_sid[spider.search_link_idx], @@ -3344,7 +3330,6 @@ void *spider_bg_crd_action( } if (spider.search_link_idx != -1 && conns[spider.search_link_idx]) { - DBUG_ASSERT(!conns[spider.search_link_idx]->thd); #ifdef WITH_PARTITION_STORAGE_ENGINE if (spider_get_crd(share, spider.search_link_idx, share->bg_crd_try_time, &spider, &table, @@ -3365,7 +3350,7 @@ void *spider_bg_crd_action( ) { lex_start(thd); error_num = spider_ping_table_mon_from_table( - spider_global_trx, + trx, thd, share, (uint32) share->monitoring_sid[spider.search_link_idx], @@ -3717,7 +3702,7 @@ void *spider_bg_mon_action( { lex_start(thd); error_num = spider_ping_table_mon_from_table( - spider_global_trx, + trx, thd, share, (uint32) share->monitoring_sid[link_idx], diff --git a/storage/spider/spd_ping_table.cc b/storage/spider/spd_ping_table.cc index 77a2969d061..6f04e08dc93 100644 --- a/storage/spider/spd_ping_table.cc +++ b/storage/spider/spd_ping_table.cc @@ -52,11 +52,6 @@ extern PSI_mutex_key spd_key_mutex_mon_list_update_status; extern PSI_mutex_key spd_key_mutex_mon_table_cache; #endif -#ifndef WITHOUT_SPIDER_BG_SEARCH -extern pthread_mutex_t spider_global_trx_mutex; -extern SPIDER_TRX *spider_global_trx; -#endif - HASH *spider_udf_table_mon_list_hash; uint spider_udf_table_mon_list_hash_id; const char *spider_udf_table_mon_list_hash_func_name; @@ -130,7 +125,6 @@ SPIDER_TABLE_MON_LIST *spider_get_ping_table_mon_list( ) #endif { - DBUG_ASSERT(trx != spider_global_trx); if ( table_mon_list && table_mon_list->mon_table_cache_version != mon_table_cache_version @@ -608,29 +602,17 @@ SPIDER_CONN *spider_get_ping_table_tgt_conn( ) { SPIDER_CONN *conn; DBUG_ENTER("spider_get_ping_table_tgt_conn"); -#ifndef WITHOUT_SPIDER_BG_SEARCH - if (trx == spider_global_trx) - pthread_mutex_lock(&spider_global_trx_mutex); -#endif if ( !(conn = spider_get_conn( share, 0, share->conn_keys[0], trx, NULL, FALSE, FALSE, SPIDER_CONN_KIND_MYSQL, error_num)) ) { -#ifndef WITHOUT_SPIDER_BG_SEARCH - if (trx == spider_global_trx) - pthread_mutex_unlock(&spider_global_trx_mutex); -#endif my_error(ER_CONNECT_TO_FOREIGN_DATA_SOURCE, MYF(0), share->server_names[0]); *error_num = ER_CONNECT_TO_FOREIGN_DATA_SOURCE; goto error; } #ifndef DBUG_OFF - if (trx == spider_global_trx) - { - DBUG_ASSERT(!conn->thd); - } DBUG_PRINT("info",("spider conn->thd=%p", conn->thd)); if (conn->thd) { @@ -638,10 +620,6 @@ SPIDER_CONN *spider_get_ping_table_tgt_conn( } #endif conn->error_mode = 0; -#ifndef WITHOUT_SPIDER_BG_SEARCH - if (trx == spider_global_trx) - pthread_mutex_unlock(&spider_global_trx_mutex); -#endif DBUG_RETURN(conn); error: diff --git a/storage/spider/spd_table.cc b/storage/spider/spd_table.cc index 2f666fef2f4..130edc5ee8a 100644 --- a/storage/spider/spd_table.cc +++ b/storage/spider/spd_table.cc @@ -98,9 +98,6 @@ PSI_mutex_key spd_key_mutex_conn; PSI_mutex_key spd_key_mutex_hs_r_conn; PSI_mutex_key spd_key_mutex_hs_w_conn; #endif -#ifndef WITHOUT_SPIDER_BG_SEARCH -PSI_mutex_key spd_key_mutex_global_trx; -#endif PSI_mutex_key spd_key_mutex_open_conn; PSI_mutex_key spd_key_mutex_allocated_thds; PSI_mutex_key spd_key_mutex_mon_table_cache; @@ -144,9 +141,6 @@ static PSI_mutex_info all_spider_mutexes[]= #if defined(HS_HAS_SQLCOM) && defined(HAVE_HANDLERSOCKET) { &spd_key_mutex_hs_r_conn, "hs_r_conn", PSI_FLAG_GLOBAL}, { &spd_key_mutex_hs_w_conn, "hs_w_conn", PSI_FLAG_GLOBAL}, -#endif -#ifndef WITHOUT_SPIDER_BG_SEARCH - { &spd_key_mutex_global_trx, "global_trx", PSI_FLAG_GLOBAL}, #endif { &spd_key_mutex_open_conn, "open_conn", PSI_FLAG_GLOBAL}, { &spd_key_mutex_allocated_thds, "allocated_thds", PSI_FLAG_GLOBAL}, @@ -301,9 +295,6 @@ pthread_mutex_t spider_allocated_thds_mutex; #ifndef WITHOUT_SPIDER_BG_SEARCH pthread_attr_t spider_pt_attr; - -pthread_mutex_t spider_global_trx_mutex; -SPIDER_TRX *spider_global_trx; #endif extern pthread_mutex_t spider_mem_calc_mutex; @@ -6090,10 +6081,6 @@ int spider_db_done( do_delete_thd = TRUE; } -#ifndef WITHOUT_SPIDER_BG_SEARCH - spider_free_trx(spider_global_trx, TRUE); -#endif - for (roop_count = SPIDER_DBTON_SIZE - 1; roop_count >= 0; roop_count--) { if (spider_dbton[roop_count].deinit) @@ -6261,9 +6248,6 @@ int spider_db_done( pthread_mutex_destroy(&spider_mon_table_cache_mutex); pthread_mutex_destroy(&spider_allocated_thds_mutex); pthread_mutex_destroy(&spider_open_conn_mutex); -#ifndef WITHOUT_SPIDER_BG_SEARCH - pthread_mutex_destroy(&spider_global_trx_mutex); -#endif #if defined(HS_HAS_SQLCOM) && defined(HAVE_HANDLERSOCKET) pthread_mutex_destroy(&spider_hs_w_conn_mutex); pthread_mutex_destroy(&spider_hs_r_conn_mutex); @@ -6510,18 +6494,6 @@ int spider_db_init( error_num = HA_ERR_OUT_OF_MEM; goto error_conn_mutex_init; } -#ifndef WITHOUT_SPIDER_BG_SEARCH -#if MYSQL_VERSION_ID < 50500 - if (pthread_mutex_init(&spider_global_trx_mutex, MY_MUTEX_INIT_FAST)) -#else - if (mysql_mutex_init(spd_key_mutex_global_trx, - &spider_global_trx_mutex, MY_MUTEX_INIT_FAST)) -#endif - { - error_num = HA_ERR_OUT_OF_MEM; - goto error_global_trx_mutex_init; - } -#endif #if MYSQL_VERSION_ID < 50500 if (pthread_mutex_init(&spider_open_conn_mutex, MY_MUTEX_INIT_FAST)) #else @@ -6786,16 +6758,9 @@ int spider_db_init( } } -#ifndef WITHOUT_SPIDER_BG_SEARCH - if (!(spider_global_trx = spider_get_trx(NULL, FALSE, &error_num))) - goto error; -#endif - DBUG_RETURN(0); #ifndef WITHOUT_SPIDER_BG_SEARCH -error: - roop_count = SPIDER_DBTON_SIZE; error_init_dbton: for (roop_count--; roop_count >= 0; roop_count--) { @@ -6897,10 +6862,6 @@ error_hs_r_conn_mutex_init: #endif pthread_mutex_destroy(&spider_open_conn_mutex); error_open_conn_mutex_init: -#ifndef WITHOUT_SPIDER_BG_SEARCH - pthread_mutex_destroy(&spider_global_trx_mutex); -error_global_trx_mutex_init: -#endif pthread_mutex_destroy(&spider_conn_mutex); error_conn_mutex_init: pthread_mutex_destroy(&spider_lgtm_tblhnd_share_mutex); From 5797cbaf4ab93cdae191486c6c82be79b83c56d2 Mon Sep 17 00:00:00 2001 From: sachin Date: Fri, 18 May 2018 17:35:33 +0530 Subject: [PATCH 2/8] MDEV-10259 mysqld crash with certain statement length and... order with Galera and encrypt-tmp-files=1 Problem:- If trans_cache (IO_CACHE) uses encrypted tmp file then on next DML server will crash. Case:- Lets take a case , we have a table t1 , We try to do 2 inserts in t1 1. A really long insert so that trans_cache has to use temp_file 2. Just a small insert Analysis:- Actually server crashes from inside of galera library. /lib64/libc.so.6(abort+0x175)[0x7fb5ba779dc5] /usr/lib64/galera/libgalera_smm.so(_ZN6galera3FSMINS_9TrxHandle5State... mysys/stacktrace.c:247(my_print_stacktrace)[0x7fb5a714940e] sql/signal_handler.cc:160(handle_fatal_signal)[0x7fb5a715c1bd] sql/wsrep_hton.cc:257(wsrep_rollback)[0x7fb5bcce923a] sql/wsrep_hton.cc:268(wsrep_rollback)[0x7fb5bcce9368] sql/handler.cc:1658(ha_rollback_trans(THD*, bool))[0x7fb5bcd4f41a] sql/handler.cc:1483(ha_commit_trans(THD*, bool))[0x7fb5bcd4f804] but actual issue is not in galera but in mariadb, because for 2nd insert we should never call rollback. We are calling rollback because log_and_order fails it fails because write_cache fails , It fails because after reinit_io_cache(trans_cache) , my_b_bytes_in_cache says 0 so we look into tmp_file for data , which is obviously wrong since temp was used for previous insert and it no longer exist. wsrep_write_cache_inc() reads the IO_CACHE in a loop, filling it with my_b_fill() until it returns "0 bytes read". Later MYSQL_BIN_LOG::write_cache() does the same. wsrep_write_cache_inc() assumes that reading a zero bytes past EOF leaves the old data in the cache Solution:- There is two issue in my_b_encr_read 1st we should never equal read_end to info->buffer. I mean this does not make sense read_end should always point to end of buffer. 2nd For most of the case(apart from async IO_CACHE) info->pos_in_file should be equal to info->buffer position wrt to temp file , since in this case we are not changing info->buffer it should remain unchanged. --- .../galera/r/galera_encrypt_tmp_files.result | 35 +++++++++ .../galera/t/galera_encrypt_tmp_files.cnf | 8 +++ .../galera/t/galera_encrypt_tmp_files.test | 57 +++++++++++++++ sql/mf_iocache_encr.cc | 4 +- unittest/sql/mf_iocache-t.cc | 72 ++++++++++++++++++- 5 files changed, 173 insertions(+), 3 deletions(-) create mode 100644 mysql-test/suite/galera/r/galera_encrypt_tmp_files.result create mode 100644 mysql-test/suite/galera/t/galera_encrypt_tmp_files.cnf create mode 100644 mysql-test/suite/galera/t/galera_encrypt_tmp_files.test diff --git a/mysql-test/suite/galera/r/galera_encrypt_tmp_files.result b/mysql-test/suite/galera/r/galera_encrypt_tmp_files.result new file mode 100644 index 00000000000..63f6c281af1 --- /dev/null +++ b/mysql-test/suite/galera/r/galera_encrypt_tmp_files.result @@ -0,0 +1,35 @@ +SELECT VARIABLE_VALUE = 'Synced' FROM INFORMATION_SCHEMA.GLOBAL_STATUS WHERE VARIABLE_NAME = 'wsrep_local_state_comment'; +VARIABLE_VALUE = 'Synced' +1 +SELECT VARIABLE_VALUE = 2 FROM INFORMATION_SCHEMA.GLOBAL_STATUS WHERE VARIABLE_NAME = 'wsrep_cluster_size'; +VARIABLE_VALUE = 2 +1 +CREATE TABLE t1 (f1 INTEGER PRIMARY KEY) Engine=InnoDB; +INSERT INTO t1 VALUES (1); +SELECT VARIABLE_VALUE = 'Synced' FROM INFORMATION_SCHEMA.GLOBAL_STATUS WHERE VARIABLE_NAME = 'wsrep_local_state_comment'; +VARIABLE_VALUE = 'Synced' +1 +SELECT VARIABLE_VALUE = 2 FROM INFORMATION_SCHEMA.GLOBAL_STATUS WHERE VARIABLE_NAME = 'wsrep_cluster_size'; +VARIABLE_VALUE = 2 +1 +SELECT COUNT(*) = 1 FROM t1; +COUNT(*) = 1 +1 +DROP TABLE t1; +CREATE TABLE `t1` ( +`col1` int(11) NOT NULL, +`col2` varchar(64) NOT NULL DEFAULT '', +`col3` varchar(32) NOT NULL DEFAULT '0', +`col4` varchar(64) NOT NULL DEFAULT '', +`col5` tinyint(4) NOT NULL DEFAULT '0', +`col6` int(11) NOT NULL DEFAULT '0', +`col7` varchar(64) NOT NULL DEFAULT '', +`col8` tinyint(4) NOT NULL DEFAULT '0', +`col9` tinyint(4) NOT NULL DEFAULT '0', +`col10` text NOT NULL, +`col11` varchar(255) NOT NULL DEFAULT '', +`col12` tinyint(4) NOT NULL DEFAULT '1' +) ; +create table t2 (test int); +insert into t2 values (1); +drop table t1,t2; diff --git a/mysql-test/suite/galera/t/galera_encrypt_tmp_files.cnf b/mysql-test/suite/galera/t/galera_encrypt_tmp_files.cnf new file mode 100644 index 00000000000..0f7f80b7d0b --- /dev/null +++ b/mysql-test/suite/galera/t/galera_encrypt_tmp_files.cnf @@ -0,0 +1,8 @@ +!include ../galera_2nodes.cnf +[mysqld] + +encrypt-tmp-files = 1 +plugin-load-add= @ENV.FILE_KEY_MANAGEMENT_SO +file-key-management +loose-file-key-management-filename= @ENV.MYSQL_TEST_DIR/std_data/keys.txt +log-bin diff --git a/mysql-test/suite/galera/t/galera_encrypt_tmp_files.test b/mysql-test/suite/galera/t/galera_encrypt_tmp_files.test new file mode 100644 index 00000000000..c42c3dbd98a --- /dev/null +++ b/mysql-test/suite/galera/t/galera_encrypt_tmp_files.test @@ -0,0 +1,57 @@ +# This file tests that mariadb cluster should not crash when encrypt_tmp_file +# is enabled + +--source include/galera_cluster.inc +--source include/have_innodb.inc + +SELECT VARIABLE_VALUE = 'Synced' FROM INFORMATION_SCHEMA.GLOBAL_STATUS WHERE VARIABLE_NAME = 'wsrep_local_state_comment'; +SELECT VARIABLE_VALUE = 2 FROM INFORMATION_SCHEMA.GLOBAL_STATUS WHERE VARIABLE_NAME = 'wsrep_cluster_size'; + +CREATE TABLE t1 (f1 INTEGER PRIMARY KEY) Engine=InnoDB; +INSERT INTO t1 VALUES (1); + +--connection node_2 +SELECT VARIABLE_VALUE = 'Synced' FROM INFORMATION_SCHEMA.GLOBAL_STATUS WHERE VARIABLE_NAME = 'wsrep_local_state_comment'; +SELECT VARIABLE_VALUE = 2 FROM INFORMATION_SCHEMA.GLOBAL_STATUS WHERE VARIABLE_NAME = 'wsrep_cluster_size'; + +SELECT COUNT(*) = 1 FROM t1; + +DROP TABLE t1; + +--connection node_1 + +CREATE TABLE `t1` ( + `col1` int(11) NOT NULL, + `col2` varchar(64) NOT NULL DEFAULT '', + `col3` varchar(32) NOT NULL DEFAULT '0', + `col4` varchar(64) NOT NULL DEFAULT '', + `col5` tinyint(4) NOT NULL DEFAULT '0', + `col6` int(11) NOT NULL DEFAULT '0', + `col7` varchar(64) NOT NULL DEFAULT '', + `col8` tinyint(4) NOT NULL DEFAULT '0', + `col9` tinyint(4) NOT NULL DEFAULT '0', + `col10` text NOT NULL, + `col11` varchar(255) NOT NULL DEFAULT '', + `col12` tinyint(4) NOT NULL DEFAULT '1' +) ; + +#Although we just need $counter >= 907 for IO_CACHE to use +#encrypted temp file. Just on safe side I am using $counter +# = 1100 +--disable_query_log +--let $counter=1100 +--let $query= (1,'test','test','test',0,0,'-1',0,0,'','',-1) +while($counter) +{ + --let $query= $query ,(1,'test','test','test',0,0,'-1',0,0,'','',-1) + --dec $counter +} +--let $query= INSERT INTO t1 values $query ; +--eval $query +--enable_query_log +#INSERT INTO `t1` VALUE + +create table t2 (test int); +insert into t2 values (1); + +drop table t1,t2; diff --git a/sql/mf_iocache_encr.cc b/sql/mf_iocache_encr.cc index 149e6feb605..f26a437a25a 100644 --- a/sql/mf_iocache_encr.cc +++ b/sql/mf_iocache_encr.cc @@ -49,8 +49,8 @@ static int my_b_encr_read(IO_CACHE *info, uchar *Buffer, size_t Count) if (pos_in_file == info->end_of_file) { - info->read_pos= info->read_end= info->buffer; - info->pos_in_file= pos_in_file; + /* reading past EOF should not empty the cache */ + info->read_pos= info->read_end; info->error= 0; DBUG_RETURN(MY_TEST(Count)); } diff --git a/unittest/sql/mf_iocache-t.cc b/unittest/sql/mf_iocache-t.cc index 31f98562521..1b04f8eb0d3 100644 --- a/unittest/sql/mf_iocache-t.cc +++ b/unittest/sql/mf_iocache-t.cc @@ -187,10 +187,76 @@ void mdev9044() close_cached_file(&info); } +/* 2 Reads (with my_b_fill) in cache makes second read to fail */ +void mdev10259() +{ + int res; + uchar buf[200]; + memset(buf, FILL, sizeof(buf)); + + diag("MDEV-10259- mysqld crash with certain statement length and order with" + " Galera and encrypt-tmp-files=1"); + + init_io_cache_encryption(); + + res= open_cached_file(&info, 0, 0, CACHE_SIZE, 0); + ok(res == 0, "open_cached_file" INFO_TAIL); + + res= my_b_write(&info, buf, sizeof(buf)); + ok(res == 0 && info.pos_in_file == 0, "200 write" INFO_TAIL); + + res= my_b_flush_io_cache(&info, 1); + ok(res == 0, "flush" INFO_TAIL); + + ulong saved_pos= my_b_tell(&info); + res= reinit_io_cache(&info, READ_CACHE, 0, 0, 0); + ok(res == 0, "reinit READ_CACHE" INFO_TAIL); + + res= my_b_fill(&info); + ok(res == 200, "fill" INFO_TAIL); + + res= my_b_fill(&info); + ok(res == 0, "fill" INFO_TAIL); + + res= my_b_fill(&info); + ok(res == 0, "fill" INFO_TAIL); + + res= reinit_io_cache(&info, WRITE_CACHE, saved_pos, 0, 0); + ok(res == 0, "reinit WRITE_CACHE" INFO_TAIL); + + res= reinit_io_cache(&info, READ_CACHE, 0, 0, 0); + ok(res == 0, "reinit READ_CACHE" INFO_TAIL); + + ok(200 == my_b_bytes_in_cache(&info),"my_b_bytes_in_cache == 200"); + + res= my_b_fill(&info); + ok(res == 0, "fill" INFO_TAIL); + + res= my_b_fill(&info); + ok(res == 0, "fill" INFO_TAIL); + + res= my_b_fill(&info); + ok(res == 0, "fill" INFO_TAIL); + + res= reinit_io_cache(&info, WRITE_CACHE, saved_pos, 0, 0); + ok(res == 0, "reinit WRITE_CACHE" INFO_TAIL); + + res= reinit_io_cache(&info, READ_CACHE, 0, 0, 0); + ok(res == 0, "reinit READ_CACHE" INFO_TAIL); + + ok(200 == my_b_bytes_in_cache(&info),"my_b_bytes_in_cache == 200"); + + res= my_b_read(&info, buf, sizeof(buf)) || data_bad(buf, sizeof(buf)); + ok(res == 0 && info.pos_in_file == 0, "large read" INFO_TAIL); + + close_cached_file(&info); + +} + int main(int argc __attribute__((unused)),char *argv[]) { MY_INIT(argv[0]); - plan(29); + plan(46); /* temp files with and without encryption */ encrypt_tmp_files= 1; @@ -202,6 +268,10 @@ int main(int argc __attribute__((unused)),char *argv[]) /* regression tests */ mdev9044(); + encrypt_tmp_files= 1; + mdev10259(); + encrypt_tmp_files= 0; + my_end(0); return exit_status(); } From 2f3779d31cbbabcb171c22237253c82146118446 Mon Sep 17 00:00:00 2001 From: Monty Date: Sun, 20 May 2018 14:19:14 +0300 Subject: [PATCH 3/8] Fixes for Aria transaction handling with lock tables MDEV-10130 Assertion `share->in_trans == 0' failed in storage/maria/ma_close.c MDEV-10378 Assertion `trn' failed in virtual int ha_maria::start_stmt The problem was that maria_handler->trn was not properly reset at commit/rollback and ha_maria::exernal_lock() could get confused because. There was some old code in ha_maria::implicit_commit() that tried to take care of this, but it was not bullet proof. Fixed by adding list of all tables that is part of the maria transaction to TRN. A nice side effect was of the fix is that loops in ha_maria::implict_commit() got to be much simpler. Other things: - Fixed a bug in mysql_admin_table() where argument open_for_modify was wrongly reset for the next table in the chain - rollback admin command also in case of fatal error. - Split _ma_set_trn_for_table() to three version to simplify code and debugging. - Several new asserts to detect the original problem (that file was not properly removed from trn before calling ma_close()) --- mysql-test/suite/maria/lock.result | 10 +++ mysql-test/suite/maria/lock.test | 12 +++ sql/sql_admin.cc | 12 +-- sql/sql_handler.cc | 4 +- storage/maria/ha_maria.cc | 137 ++++++++++++----------------- storage/maria/ha_maria.h | 1 + storage/maria/ma_blockrec.c | 3 +- storage/maria/ma_close.c | 2 + storage/maria/ma_commit.c | 17 ++-- storage/maria/ma_extra.c | 4 +- storage/maria/ma_open.c | 4 +- storage/maria/ma_state.c | 48 ++++++++-- storage/maria/ma_state.h | 2 +- storage/maria/ma_trnman.h | 65 ++++++++++++++ storage/maria/maria_def.h | 14 +-- storage/maria/trnman.c | 1 + storage/maria/trnman.h | 3 +- 17 files changed, 219 insertions(+), 120 deletions(-) create mode 100644 storage/maria/ma_trnman.h diff --git a/mysql-test/suite/maria/lock.result b/mysql-test/suite/maria/lock.result index 90250568ef5..1c9d6b18100 100644 --- a/mysql-test/suite/maria/lock.result +++ b/mysql-test/suite/maria/lock.result @@ -99,3 +99,13 @@ f2 3 unlock tables; DROP TABLE t1,t2,tmp; +# +# MDEV-10378 Assertion `trn' failed in virtual int ha_maria::start_stmt +# +CREATE TABLE t1 (f1 VARCHAR(3), f2 INT, pk INT, PRIMARY KEY (pk)) ENGINE=Aria; +INSERT INTO t1 VALUES ('foo',10,1), ('foo',1,2); +LOCK TABLE t1 WRITE; +ALTER TABLE t1 ADD UNIQUE KEY (f1); +ERROR 23000: Duplicate entry 'foo' for key 'f1' +ALTER TABLE t1 ADD KEY (f2); +DROP TABLE t1; diff --git a/mysql-test/suite/maria/lock.test b/mysql-test/suite/maria/lock.test index 57447a18c55..4f3d4e8065e 100644 --- a/mysql-test/suite/maria/lock.test +++ b/mysql-test/suite/maria/lock.test @@ -105,3 +105,15 @@ INSERT INTO t2 (f2) SELECT f3 FROM tmp AS tmp_alias; select * from t2; unlock tables; DROP TABLE t1,t2,tmp; + +--echo # +--echo # MDEV-10378 Assertion `trn' failed in virtual int ha_maria::start_stmt +--echo # + +CREATE TABLE t1 (f1 VARCHAR(3), f2 INT, pk INT, PRIMARY KEY (pk)) ENGINE=Aria; +INSERT INTO t1 VALUES ('foo',10,1), ('foo',1,2); +LOCK TABLE t1 WRITE; +--error ER_DUP_ENTRY +ALTER TABLE t1 ADD UNIQUE KEY (f1); +ALTER TABLE t1 ADD KEY (f2); +DROP TABLE t1; diff --git a/sql/sql_admin.cc b/sql/sql_admin.cc index 4fe51a2189e..10fcbd56e18 100644 --- a/sql/sql_admin.cc +++ b/sql/sql_admin.cc @@ -302,7 +302,7 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, HA_CHECK_OPT* check_opt, const char *operator_name, thr_lock_type lock_type, - bool open_for_modify, + bool org_open_for_modify, bool repair_table_use_frm, uint extra_open_options, int (*prepare_func)(THD *, TABLE_LIST *, @@ -359,10 +359,10 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, for (table= tables; table; table= table->next_local) { char table_name[SAFE_NAME_LEN*2+2]; - char* db = table->db; + char *db= table->db; bool fatal_error=0; bool open_error; - + bool open_for_modify= org_open_for_modify; DBUG_PRINT("admin", ("table: '%s'.'%s'", table->db, table->table_name)); strxmov(table_name, db, ".", table->table_name, NullS); thd->open_options|= extra_open_options; @@ -395,8 +395,8 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, /* CHECK TABLE command is allowed for views as well. Check on alter flags - to differentiate from ALTER TABLE...CHECK PARTITION on which view is not - allowed. + to differentiate from ALTER TABLE...CHECK PARTITION on which view is + not allowed. */ if (lex->alter_info.flags & Alter_info::ALTER_ADMIN_PARTITION || view_operator_func == NULL) @@ -1053,7 +1053,7 @@ send_result_message: } } /* Error path, a admin command failed. */ - if (thd->transaction_rollback_request) + if (thd->transaction_rollback_request || fatal_error) { /* Unlikely, but transaction rollback was requested by one of storage diff --git a/sql/sql_handler.cc b/sql/sql_handler.cc index 5fc7c20d409..52ce42e2e6a 100644 --- a/sql/sql_handler.cc +++ b/sql/sql_handler.cc @@ -155,10 +155,11 @@ static void mysql_ha_close_table(SQL_HANDLER *handler) { THD *thd= handler->thd; TABLE *table= handler->table; + DBUG_ENTER("mysql_ha_close_table"); /* check if table was already closed */ if (!table) - return; + DBUG_VOID_RETURN; if (!table->s->tmp_table) { @@ -184,6 +185,7 @@ static void mysql_ha_close_table(SQL_HANDLER *handler) } my_free(handler->lock); handler->init(); + DBUG_VOID_RETURN; } /* diff --git a/storage/maria/ha_maria.cc b/storage/maria/ha_maria.cc index dbd4a3331df..e56cc943390 100644 --- a/storage/maria/ha_maria.cc +++ b/storage/maria/ha_maria.cc @@ -37,6 +37,7 @@ C_MODE_START #include "ma_checkpoint.h" #include "ma_recovery.h" C_MODE_END +#include "ma_trnman.h" //#include "sql_priv.h" #include "protocol.h" @@ -1384,7 +1385,8 @@ int ha_maria::check(THD * thd, HA_CHECK_OPT * check_opt) } /* Reset trn, that may have been set by repair */ - _ma_set_trn_for_table(file, old_trn); + if (old_trn && old_trn != file->trn) + _ma_set_trn_for_table(file, old_trn); thd_proc_info(thd, old_proc_info); thd_progress_end(thd); return error ? HA_ADMIN_CORRUPT : HA_ADMIN_OK; @@ -1518,7 +1520,8 @@ int ha_maria::zerofill(THD * thd, HA_CHECK_OPT *check_opt) error=maria_zerofill(param, file, share->open_file_name.str); /* Reset trn, that may have been set by repair */ - _ma_set_trn_for_table(file, old_trn); + if (old_trn && old_trn != file->trn) + _ma_set_trn_for_table(file, old_trn); if (!error) { @@ -1758,7 +1761,8 @@ int ha_maria::repair(THD *thd, HA_CHECK *param, bool do_optimize) maria_lock_database(file, F_UNLCK); /* Reset trn, that may have been set by repair */ - _ma_set_trn_for_table(file, old_trn); + if (old_trn && old_trn != file->trn) + _ma_set_trn_for_table(file, old_trn); error= error ? HA_ADMIN_FAILED : (optimize_done ? (write_log_record_for_repair(param, file) ? HA_ADMIN_FAILED : @@ -2579,9 +2583,12 @@ int ha_maria::extra(enum ha_extra_function operation) without calling commit/rollback in between. If file->trn is not set we can't remove file->share from the transaction list in the extra() call. - We also ensure that we set file->trn to 0 if THD_TRN is 0 as in - this case we have already freed the trn. This can happen when one - implicit_commit() is called as part of alter table. + In current code we don't have to do this for HA_EXTRA_PREPARE_FOR_RENAME + as this is only used the intermediate table used by ALTER TABLE which + is not part of the transaction (it's not in the TRN list). Better to + keep this for now, to not break anything in a stable release. + When HA_EXTRA_PREPARE_FOR_RENAME is not handled below, we can change + the warnings in _ma_remove_table_from_trnman() to asserts. table->in_use is not set in the case this is a done as part of closefrm() as part of drop table. @@ -2594,7 +2601,7 @@ int ha_maria::extra(enum ha_extra_function operation) { THD *thd= table->in_use; TRN *trn= THD_TRN; - _ma_set_trn_for_table(file, trn); + _ma_set_tmp_trn_for_table(file, trn); } DBUG_ASSERT(file->s->base.born_transactional || file->trn == 0 || file->trn == &dummy_transaction_object); @@ -2710,6 +2717,7 @@ int ha_maria::external_lock(THD *thd, int lock_type) if (file->trn) { /* This can only happen with tables created with clone() */ + DBUG_PRINT("info",("file->trn: %p", file->trn)); trnman_increment_locked_tables(file->trn); } @@ -2730,7 +2738,7 @@ int ha_maria::external_lock(THD *thd, int lock_type) } else { - TRN *trn= THD_TRN; + TRN *trn= (file->trn != &dummy_transaction_object ? file->trn : 0); /* End of transaction */ /* @@ -2745,8 +2753,7 @@ int ha_maria::external_lock(THD *thd, int lock_type) */ if (_ma_reenable_logging_for_table(file, TRUE)) DBUG_RETURN(1); - /** @todo zero file->trn also in commit and rollback */ - _ma_set_trn_for_table(file, NULL); // Safety + _ma_reset_trn_for_table(file); /* Ensure that file->state points to the current number of rows. This is needed if someone calls maria_info() without first doing an @@ -2802,13 +2809,6 @@ int ha_maria::start_stmt(THD *thd, thr_lock_type lock_type) DBUG_ASSERT(lock_type != TL_UNLOCK); DBUG_ASSERT(file->trn == trn); - /* - If there was an implicit commit under this LOCK TABLES by a previous - statement (like a DDL), at least if that previous statement was about a - different ha_maria than 'this' then this->file->trn is a stale - pointer. We fix it: - */ - _ma_set_trn_for_table(file, trn); /* As external_lock() was already called, don't increment locked_tables. Note that we call the function below possibly several times when @@ -2833,6 +2833,23 @@ int ha_maria::start_stmt(THD *thd, thr_lock_type lock_type) } +/* + Reset THD_TRN and all file->trn related to the transaction + This is needed as some calls, like extra() or external_lock() may access + it before next transaction is started +*/ + +static void reset_thd_trn(THD *thd, MARIA_HA *first_table) +{ + DBUG_ENTER("reset_thd_trn"); + THD_TRN= NULL; + for (MARIA_HA *table= first_table; table ; + table= table->trn_next) + _ma_reset_trn_for_table(table); + DBUG_VOID_RETURN; +} + + /** Performs an implicit commit of the Maria transaction and creates a new one. @@ -2856,9 +2873,9 @@ int ha_maria::implicit_commit(THD *thd, bool new_trn) TRN *trn; int error; uint locked_tables; - DYNAMIC_ARRAY used_tables; - + MARIA_HA *used_tables, *trn_next; DBUG_ENTER("ha_maria::implicit_commit"); + if (!maria_hton || !(trn= THD_TRN)) DBUG_RETURN(0); if (!new_trn && (thd->locked_tables_mode == LTM_LOCK_TABLES || @@ -2876,48 +2893,16 @@ int ha_maria::implicit_commit(THD *thd, bool new_trn) locked_tables= trnman_has_locked_tables(trn); - if (new_trn && trn && trn->used_tables) - { - MARIA_USED_TABLES *tables; - /* - Save locked tables so that we can move them to another transaction - We are using a dynamic array as locked_tables in some cases can be - smaller than the used_tables list (for example when the server does - early unlock of tables. - */ - - my_init_dynamic_array2(&used_tables, sizeof(MARIA_SHARE*), (void*) 0, - locked_tables, 8, MYF(MY_THREAD_SPECIFIC)); - for (tables= (MARIA_USED_TABLES*) trn->used_tables; - tables; - tables= tables->next) - { - if (tables->share->base.born_transactional) - { - if (insert_dynamic(&used_tables, (uchar*) &tables->share)) - { - error= HA_ERR_OUT_OF_MEM; - goto end_and_free; - } - } - } - } - else - bzero(&used_tables, sizeof(used_tables)); - + used_tables= (MARIA_HA*) trn->used_instances; error= 0; if (unlikely(ma_commit(trn))) error= 1; if (!new_trn) { - /* - To be extra safe, we should also reset file->trn for all open - tables as some calls, like extra() may access it. We take care - of this in extra() by resetting file->trn if THD_TRN is 0. - */ - THD_TRN= NULL; + reset_thd_trn(thd, used_tables); goto end; } + /* We need to create a new transaction and put it in THD_TRN. Indeed, tables may be under LOCK TABLES, and so they will start the next @@ -2927,8 +2912,9 @@ int ha_maria::implicit_commit(THD *thd, bool new_trn) THD_TRN= trn; if (unlikely(trn == NULL)) { + reset_thd_trn(thd, used_tables); error= HA_ERR_OUT_OF_MEM; - goto end_and_free; + goto end; } /* Move all locked tables to the new transaction @@ -2938,35 +2924,25 @@ int ha_maria::implicit_commit(THD *thd, bool new_trn) in check table, we use the table without calling start_stmt(). */ - uint i; - for (i= 0 ; i < used_tables.elements ; i++) + for (MARIA_HA *handler= used_tables; handler ; + handler= trn_next) { - MARIA_SHARE *share; - LIST *handlers; + trn_next= handler->trn_next; + DBUG_ASSERT(handler->s->base.born_transactional); - share= *(dynamic_element(&used_tables, i, MARIA_SHARE**)); - /* Find table instances that was used in this transaction */ - for (handlers= share->open_list; handlers; handlers= handlers->next) + /* If handler uses versioning */ + if (handler->s->lock_key_trees) { - MARIA_HA *handler= (MARIA_HA*) handlers->data; - if (handler->external_ref && - ((TABLE*) handler->external_ref)->in_use == thd) - { - _ma_set_trn_for_table(handler, trn); - /* If handler uses versioning */ - if (handler->s->lock_key_trees) - { - if (_ma_setup_live_state(handler)) - error= HA_ERR_OUT_OF_MEM; - } - } + /* _ma_set_trn_for_table() will be called indirectly */ + if (_ma_setup_live_state(handler)) + error= HA_ERR_OUT_OF_MEM; } + else + _ma_set_trn_for_table(handler, trn); } /* This is just a commit, tables stay locked if they were: */ trnman_reset_locked_tables(trn, locked_tables); -end_and_free: - delete_dynamic(&used_tables); end: DBUG_RETURN(error); } @@ -3340,10 +3316,10 @@ static int maria_commit(handlerton *hton __attribute__ ((unused)), trnman_set_flags(trn, trnman_get_flags(trn) & ~TRN_STATE_INFO_LOGGED); /* statement or transaction ? */ - if ((thd->variables.option_bits & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)) && !all) + if ((thd->variables.option_bits & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)) && + !all) DBUG_RETURN(0); // end of statement - DBUG_PRINT("info", ("THD_TRN set to 0x0")); - THD_TRN= 0; + reset_thd_trn(thd, (MARIA_HA*) trn->used_instances); DBUG_RETURN(ma_commit(trn)); // end of transaction } @@ -3360,8 +3336,7 @@ static int maria_rollback(handlerton *hton __attribute__ ((unused)), trnman_rollback_statement(trn); DBUG_RETURN(0); // end of statement } - DBUG_PRINT("info", ("THD_TRN set to 0x0")); - THD_TRN= 0; + reset_thd_trn(thd, (MARIA_HA*) trn->used_instances); DBUG_RETURN(trnman_rollback_trn(trn) ? HA_ERR_OUT_OF_MEM : 0); // end of transaction } diff --git a/storage/maria/ha_maria.h b/storage/maria/ha_maria.h index 2b99c31ec5d..16adc0c19a2 100644 --- a/storage/maria/ha_maria.h +++ b/storage/maria/ha_maria.h @@ -193,6 +193,7 @@ public: private: DsMrr_impl ds_mrr; friend ICP_RESULT index_cond_func_maria(void *arg); + friend void reset_thd_trn(THD *thd); }; #endif /* HA_MARIA_INCLUDED */ diff --git a/storage/maria/ma_blockrec.c b/storage/maria/ma_blockrec.c index 9c92f8b63e5..ac46ac95101 100644 --- a/storage/maria/ma_blockrec.c +++ b/storage/maria/ma_blockrec.c @@ -271,6 +271,7 @@ #include "maria_def.h" #include "ma_blockrec.h" #include "trnman.h" +#include "ma_trnman.h" #include "ma_key_recover.h" #include "ma_recovery_util.h" #include @@ -7488,7 +7489,7 @@ void maria_ignore_trids(MARIA_HA *info) if (info->s->base.born_transactional) { if (!info->trn) - _ma_set_trn_for_table(info, &dummy_transaction_object); + _ma_set_tmp_trn_for_table(info, &dummy_transaction_object); /* Ignore transaction id when row is read */ info->trn->min_read_from= ~(TrID) 0; } diff --git a/storage/maria/ma_close.c b/storage/maria/ma_close.c index 4532b029126..75a996313e3 100644 --- a/storage/maria/ma_close.c +++ b/storage/maria/ma_close.c @@ -36,6 +36,8 @@ int maria_close(register MARIA_HA *info) /* Check that we have unlocked key delete-links properly */ DBUG_ASSERT(info->key_del_used == 0); + /* Check that file is not part of any uncommited transactions */ + DBUG_ASSERT(info->trn == 0 || info->trn == &dummy_transaction_object); if (share->reopen == 1) { diff --git a/storage/maria/ma_commit.c b/storage/maria/ma_commit.c index 46db3ca4ae5..7ad5e23e5f7 100644 --- a/storage/maria/ma_commit.c +++ b/storage/maria/ma_commit.c @@ -15,6 +15,7 @@ #include "maria_def.h" #include "trnman.h" +#include "ma_trnman.h" /** writes a COMMIT record to log and commits transaction in memory @@ -43,9 +44,9 @@ int ma_commit(TRN *trn) COMMIT record) and this is not an issue as * transaction's updates were not made visible to other transactions * "commit ok" was not sent to client - Alternatively, Recovery might commit trn (if MY_MIN(rec_lsn) is before COMMIT - record), which is ok too. All in all it means that "trn committed" is not - 100% equal to "COMMIT record written". + Alternatively, Recovery might commit trn (if MY_MIN(rec_lsn) is before + COMMIT record), which is ok too. All in all it means that "trn committed" + is not 100% equal to "COMMIT record written". - if COMMIT record is written after trnman_commit_trn(): if crash happens between the two, trn will be rolled back which is an issue (transaction's updates were made visible to other transactions). @@ -93,7 +94,12 @@ int ma_commit(TRN *trn) int maria_commit(MARIA_HA *info) { - return info->s->now_transactional ? ma_commit(info->trn) : 0; + TRN *trn; + if (!info->s->now_transactional) + return 0; + trn= info->trn; + info->trn= 0; /* checked in maria_close() */ + return ma_commit(trn); } @@ -120,10 +126,7 @@ int maria_begin(MARIA_HA *info) TRN *trn= trnman_new_trn(0); if (unlikely(!trn)) DBUG_RETURN(HA_ERR_OUT_OF_MEM); - - DBUG_PRINT("info", ("TRN set to 0x%lx", (ulong) trn)); _ma_set_trn_for_table(info, trn); } DBUG_RETURN(0); } - diff --git a/storage/maria/ma_extra.c b/storage/maria/ma_extra.c index d2e81184630..dc04e8aacde 100644 --- a/storage/maria/ma_extra.c +++ b/storage/maria/ma_extra.c @@ -345,7 +345,7 @@ int maria_extra(MARIA_HA *info, enum ha_extra_function function, _ma_decrement_open_count(info, 0); if (info->trn) { - _ma_remove_table_from_trnman(share, info->trn); + _ma_remove_table_from_trnman(info); /* Ensure we don't point to the deleted data in trn */ info->state= info->state_start= &share->state.state; } @@ -408,7 +408,7 @@ int maria_extra(MARIA_HA *info, enum ha_extra_function function, if (info->trn) { mysql_mutex_lock(&share->intern_lock); - _ma_remove_table_from_trnman(share, info->trn); + _ma_remove_table_from_trnman(info); /* Ensure we don't point to the deleted data in trn */ info->state= info->state_start= &share->state.state; mysql_mutex_unlock(&share->intern_lock); diff --git a/storage/maria/ma_open.c b/storage/maria/ma_open.c index 1db85180fcf..8fda846172b 100644 --- a/storage/maria/ma_open.c +++ b/storage/maria/ma_open.c @@ -19,6 +19,8 @@ #include "ma_sp_defs.h" #include "ma_rt_index.h" #include "ma_blockrec.h" +#include "trnman.h" +#include "ma_trnman.h" #include #if defined(MSDOS) || defined(__WIN__) @@ -183,7 +185,7 @@ static MARIA_HA *maria_clone_internal(MARIA_SHARE *share, if (!share->base.born_transactional) /* For transactional ones ... */ { /* ... force crash if no trn given */ - _ma_set_trn_for_table(&info, &dummy_transaction_object); + _ma_set_tmp_trn_for_table(&info, &dummy_transaction_object); info.state= &share->state.state; /* Change global values by default */ } else diff --git a/storage/maria/ma_state.c b/storage/maria/ma_state.c index 1b5a55cc91d..d3fa5acf198 100644 --- a/storage/maria/ma_state.c +++ b/storage/maria/ma_state.c @@ -66,7 +66,7 @@ my_bool _ma_setup_live_state(MARIA_HA *info) DBUG_RETURN(1); trn= info->trn; - for (tables= (MARIA_USED_TABLES*) info->trn->used_tables; + for (tables= (MARIA_USED_TABLES*) trn->used_tables; tables; tables= tables->next) { @@ -551,6 +551,7 @@ my_bool _ma_trnman_end_trans_hook(TRN *trn, my_bool commit, my_free(tables); } trn->used_tables= 0; + trn->used_instances= 0; DBUG_RETURN(error); } @@ -565,18 +566,25 @@ my_bool _ma_trnman_end_trans_hook(TRN *trn, my_bool commit, share->internal_lock must be locked when function is called */ -void _ma_remove_table_from_trnman(MARIA_SHARE *share, TRN *trn) +void _ma_remove_table_from_trnman(MARIA_HA *info) { + MARIA_SHARE *share= info->s; + TRN *trn= info->trn; MARIA_USED_TABLES *tables, **prev; + MARIA_HA *handler, **prev_file; DBUG_ENTER("_ma_remove_table_from_trnman"); DBUG_PRINT("enter", ("trn: %p used_tables: %p share: %p in_trans: %d", trn, trn->used_tables, share, share->in_trans)); mysql_mutex_assert_owner(&share->intern_lock); + + if (trn == &dummy_transaction_object) + DBUG_VOID_RETURN; - for (prev= (MARIA_USED_TABLES**) (char*) &trn->used_tables, tables= *prev; - tables; - tables= *prev) + /* First remove share from used_tables */ + for (prev= (MARIA_USED_TABLES**) (char*) &trn->used_tables; + (tables= *prev); + prev= &tables->next) { if (tables->share == share) { @@ -585,8 +593,36 @@ void _ma_remove_table_from_trnman(MARIA_SHARE *share, TRN *trn) my_free(tables); break; } - prev= &tables->next; } + if (tables != 0) + { + /* + This can only happens in case of rename of intermediate table as + part of alter table + */ + DBUG_PRINT("warning", ("share: %p where not in used_tables_list", share)); + } + + /* unlink table from used_instances */ + for (prev_file= (MARIA_HA**) &trn->used_instances; + (handler= *prev_file); + prev_file= &handler->trn_next) + { + if (handler == info) + { + *prev_file= info->trn_next; + break; + } + } + if (handler != 0) + { + /* + This can only happens in case of rename of intermediate table as + part of alter table + */ + DBUG_PRINT("warning", ("table: %p where not in used_instances", info)); + } + info->trn= 0; /* Not part of trans anymore */ DBUG_VOID_RETURN; } diff --git a/storage/maria/ma_state.h b/storage/maria/ma_state.h index d662b9899f8..d47595ee08d 100644 --- a/storage/maria/ma_state.h +++ b/storage/maria/ma_state.h @@ -84,5 +84,5 @@ my_bool _ma_row_visible_non_transactional_table(MARIA_HA *info); my_bool _ma_row_visible_transactional_table(MARIA_HA *info); void _ma_remove_not_visible_states_with_lock(struct st_maria_share *share, my_bool all); -void _ma_remove_table_from_trnman(struct st_maria_share *share, TRN *trn); +void _ma_remove_table_from_trnman(MARIA_HA *info); void _ma_reset_history(struct st_maria_share *share); diff --git a/storage/maria/ma_trnman.h b/storage/maria/ma_trnman.h new file mode 100644 index 00000000000..9bfd1f0d047 --- /dev/null +++ b/storage/maria/ma_trnman.h @@ -0,0 +1,65 @@ +/* Copyright (C) 2006-2008 MySQL AB, 2008-2009 Sun Microsystems, Inc. + + 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., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ + +#ifndef _ma_trnman_h +#define _ma_trnman_h + +/** + Sets table's trn and prints debug information + Links table into used_instances if new_trn is not 0 + + @param tbl MARIA_HA of table + @param newtrn what to put into tbl->trn +*/ + +static inline void _ma_set_trn_for_table(MARIA_HA *tbl, TRN *newtrn) +{ + DBUG_PRINT("info",("table: %p trn: %p -> %p", + tbl, tbl->trn, newtrn)); + + /* check that we are not calling this twice in a row */ + DBUG_ASSERT(newtrn->used_instances != (void*) tbl); + + tbl->trn= newtrn; + /* Link into used list */ + tbl->trn_next= (MARIA_HA*) newtrn->used_instances; + newtrn->used_instances= tbl; +} + + +/* + Same as _ma_set_trn_for_table(), but don't link table into used_instance list + Used when we want to temporary set trn for a table in extra() +*/ + +static inline void _ma_set_tmp_trn_for_table(MARIA_HA *tbl, TRN *newtrn) +{ + DBUG_PRINT("info",("table: %p trn: %p -> %p", + tbl, tbl->trn, newtrn)); + tbl->trn= newtrn; +} + + +/* + Reset TRN in table +*/ + +static inline void _ma_reset_trn_for_table(MARIA_HA *tbl) +{ + DBUG_PRINT("info",("table: %p trn: %p -> NULL", tbl, tbl->trn)); + tbl->trn= 0; +} + +#endif /* _ma_trnman_h */ diff --git a/storage/maria/maria_def.h b/storage/maria/maria_def.h index e4fd80d1151..bbf29d8b21a 100644 --- a/storage/maria/maria_def.h +++ b/storage/maria/maria_def.h @@ -576,6 +576,7 @@ struct st_maria_handler { MARIA_SHARE *s; /* Shared between open:s */ struct st_ma_transaction *trn; /* Pointer to active transaction */ + struct st_maria_handler *trn_next; MARIA_STATUS_INFO *state, state_save; MARIA_STATUS_INFO *state_start; /* State at start of transaction */ MARIA_USED_TABLES *used_tables; @@ -824,19 +825,6 @@ struct st_maria_handler #define get_pack_length(length) ((length) >= 255 ? 3 : 1) #define _ma_have_versioning(info) ((info)->row_flag & ROW_FLAG_TRANSID) -/** - Sets table's trn and prints debug information - @param tbl MARIA_HA of table - @param newtrn what to put into tbl->trn - @note cast of newtrn is because %p of NULL gives warning (NULL is int) -*/ -#define _ma_set_trn_for_table(tbl, newtrn) do { \ - DBUG_PRINT("info",("table: %p trn: %p -> %p", \ - (tbl), (tbl)->trn, (void *)(newtrn))); \ - (tbl)->trn= (newtrn); \ - } while (0) - - #define MARIA_MIN_BLOCK_LENGTH 20 /* Because of delete-link */ /* Don't use to small record-blocks */ #define MARIA_EXTEND_BLOCK_LENGTH 20 diff --git a/storage/maria/trnman.c b/storage/maria/trnman.c index daccf3550c2..1ad655b8a22 100644 --- a/storage/maria/trnman.c +++ b/storage/maria/trnman.c @@ -366,6 +366,7 @@ TRN *trnman_new_trn(WT_THD *wt) trn->commit_trid= MAX_TRID; trn->rec_lsn= trn->undo_lsn= trn->first_undo_lsn= 0; trn->used_tables= 0; + trn->used_instances= 0; trn->locked_tables= 0; trn->flags= 0; diff --git a/storage/maria/trnman.h b/storage/maria/trnman.h index 77e2916390a..057d9396473 100644 --- a/storage/maria/trnman.h +++ b/storage/maria/trnman.h @@ -46,7 +46,8 @@ struct st_ma_transaction LF_PINS *pins; WT_THD *wt; mysql_mutex_t state_lock; - void *used_tables; /**< Tables used by transaction */ + void *used_tables; /**< Table shares used by transaction */ + void *used_instances; /* table files used by transaction */ TRN *next, *prev; TrID trid, min_read_from, commit_trid; LSN rec_lsn, undo_lsn; From da71c1bad79ee11009b3b28a2a745709e04020cf Mon Sep 17 00:00:00 2001 From: Monty Date: Tue, 22 May 2018 14:36:06 +0300 Subject: [PATCH 4/8] MDEV-16229 Replication aborts with ER_VIEW_SELECT_TMPTABLE after half-failed RENAME Problem was that detection of temporary tables was all wrong for RENAME TABLE. (Temporary tables where opened by top level call to open_temporary_tables(), which can't detect if a temporary table was renamed to something and then reused). Fixed by adding proper parsing of rename list to check against the current name of a table at each rename stage. Also change do_rename_temporary() to check against the current state of temporary tables, not according to the state of start of RENAME TABLE. --- mysql-test/r/grant2.result | 3 +- mysql-test/r/rename.result | 66 ++++++++++++++++++++++++++++ mysql-test/suite/rpl/r/rename.result | 34 ++++++++++++++ mysql-test/suite/rpl/t/rename.test | 33 ++++++++++++++ mysql-test/t/grant2.test | 3 +- mysql-test/t/rename.test | 46 +++++++++++++++++++ sql/sql_parse.cc | 65 ++++++++++++++++++++++++++- sql/sql_rename.cc | 2 +- 8 files changed, 245 insertions(+), 7 deletions(-) create mode 100644 mysql-test/suite/rpl/r/rename.result create mode 100644 mysql-test/suite/rpl/t/rename.test diff --git a/mysql-test/r/grant2.result b/mysql-test/r/grant2.result index 9e9b3ffc4e5..70292240d84 100644 --- a/mysql-test/r/grant2.result +++ b/mysql-test/r/grant2.result @@ -705,10 +705,9 @@ LOAD INDEX INTO CACHE t3; Table Op Msg_type Msg_text mysqltest_db1.t3 preload_keys status OK # -# RENAME (doesn't work for temporary tables, thus should fail). +# RENAME should work for temporary tables # RENAME TABLE t3 TO t3_1; -ERROR 42000: INSERT, CREATE command denied to user 'mysqltest_u1'@'localhost' for table 't3_1' # # HANDLER OPEN/READ/CLOSE. # diff --git a/mysql-test/r/rename.result b/mysql-test/r/rename.result index 74370ba74dd..c90aaf24e9b 100644 --- a/mysql-test/r/rename.result +++ b/mysql-test/r/rename.result @@ -67,3 +67,69 @@ ERROR HY000: 'test.v1' is not BASE TABLE drop view v1; drop table t1; End of 5.0 tests +CREATE OR REPLACE TABLE t1 (a INT); +CREATE OR REPLACE TABLE t2 (a INT); +CREATE OR REPLACE TEMPORARY TABLE t1_tmp (b INT); +CREATE OR REPLACE TEMPORARY TABLE t2_tmp (b INT); +rename table t1 to t2; +ERROR 42S01: Table 't2' already exists +rename table t1 to tmp, tmp to t2; +ERROR 42S01: Table 't2' already exists +rename table t1_tmp to t2_tmp; +ERROR 42S01: Table 't2_tmp' already exists +rename table t1_tmp to tmp, tmp to t2_tmp; +ERROR 42S01: Table 't2_tmp' already exists +show create table t1_tmp; +Table Create Table +t1_tmp CREATE TEMPORARY TABLE `t1_tmp` ( + `b` int(11) DEFAULT NULL +) ENGINE=MyISAM DEFAULT CHARSET=latin1 +show create table t2_tmp; +Table Create Table +t2_tmp CREATE TEMPORARY TABLE `t2_tmp` ( + `b` int(11) DEFAULT NULL +) ENGINE=MyISAM DEFAULT CHARSET=latin1 +rename table t1 to t1_tmp; +rename table t2_tmp to t2; +rename table t2 to tmp, tmp to t2; +rename table t1_tmp to tmp, tmp to t1_tmp; +show tables; +Tables_in_test +t1_tmp +t2 +SHOW CREATE TABLE t1_tmp; +Table Create Table +t1_tmp CREATE TEMPORARY TABLE `t1_tmp` ( + `b` int(11) DEFAULT NULL +) ENGINE=MyISAM DEFAULT CHARSET=latin1 +drop table t1_tmp; +SHOW CREATE TABLE t1_tmp; +Table Create Table +t1_tmp CREATE TABLE `t1_tmp` ( + `a` int(11) DEFAULT NULL +) ENGINE=MyISAM DEFAULT CHARSET=latin1 +drop table t1_tmp; +SHOW CREATE TABLE t2; +Table Create Table +t2 CREATE TEMPORARY TABLE `t2` ( + `b` int(11) DEFAULT NULL +) ENGINE=MyISAM DEFAULT CHARSET=latin1 +drop table t2; +SHOW CREATE TABLE t2; +Table Create Table +t2 CREATE TABLE `t2` ( + `a` int(11) DEFAULT NULL +) ENGINE=MyISAM DEFAULT CHARSET=latin1 +drop table t2; +CREATE TABLE t1 (a INT); +insert into t1 values (1); +CREATE TEMPORARY TABLE t1 (b INT); +insert into t1 values (2); +RENAME TABLE t1 TO tmp, t1 TO t2; +select * from tmp; +b +2 +select * from t2; +a +1 +drop table tmp,t2; diff --git a/mysql-test/suite/rpl/r/rename.result b/mysql-test/suite/rpl/r/rename.result new file mode 100644 index 00000000000..5cedea51c86 --- /dev/null +++ b/mysql-test/suite/rpl/r/rename.result @@ -0,0 +1,34 @@ +include/master-slave.inc +[connection master] +# +# MDEV-16229 Replication aborts with ER_VIEW_SELECT_TMPTABLE after +# half-failed RENAME +# +CREATE TABLE t1 (a INT); +CREATE TEMPORARY TABLE t1 (b INT); +RENAME TABLE t1 TO tmp, tmp TO t1; +SHOW CREATE TABLE t1; +Table Create Table +t1 CREATE TEMPORARY TABLE `t1` ( + `b` int(11) DEFAULT NULL +) ENGINE=MyISAM DEFAULT CHARSET=latin1 +CREATE VIEW v AS SELECT * FROM t1; +ERROR HY000: View's SELECT refers to a temporary table 't1' +RENAME TABLE t1 TO tmp, t1 TO t2; +SHOW CREATE TABLE tmp; +Table Create Table +tmp CREATE TEMPORARY TABLE `tmp` ( + `b` int(11) DEFAULT NULL +) ENGINE=MyISAM DEFAULT CHARSET=latin1 +SHOW CREATE TABLE t2; +Table Create Table +t2 CREATE TABLE `t2` ( + `a` int(11) DEFAULT NULL +) ENGINE=MyISAM DEFAULT CHARSET=latin1 +CREATE VIEW v AS SELECT * FROM tmp; +ERROR HY000: View's SELECT refers to a temporary table 'tmp' +CREATE VIEW v AS SELECT * FROM t2; +DROP VIEW v; +DROP TABLE tmp; +DROP TABLE t2; +include/rpl_end.inc diff --git a/mysql-test/suite/rpl/t/rename.test b/mysql-test/suite/rpl/t/rename.test new file mode 100644 index 00000000000..ac499157918 --- /dev/null +++ b/mysql-test/suite/rpl/t/rename.test @@ -0,0 +1,33 @@ +--source include/have_binlog_format_mixed.inc +--source include/master-slave.inc + +--echo # +--echo # MDEV-16229 Replication aborts with ER_VIEW_SELECT_TMPTABLE after +--echo # half-failed RENAME +--echo # + +CREATE TABLE t1 (a INT); +CREATE TEMPORARY TABLE t1 (b INT); +RENAME TABLE t1 TO tmp, tmp TO t1; +SHOW CREATE TABLE t1; +--error ER_VIEW_SELECT_TMPTABLE +CREATE VIEW v AS SELECT * FROM t1; + +RENAME TABLE t1 TO tmp, t1 TO t2; +SHOW CREATE TABLE tmp; +SHOW CREATE TABLE t2; +--error ER_VIEW_SELECT_TMPTABLE +CREATE VIEW v AS SELECT * FROM tmp; +CREATE VIEW v AS SELECT * FROM t2; + +--sync_slave_with_master + +# Cleanup + +--connection master + +DROP VIEW v; +DROP TABLE tmp; +DROP TABLE t2; + +--source include/rpl_end.inc diff --git a/mysql-test/t/grant2.test b/mysql-test/t/grant2.test index 8590dccd1d1..55eb21ed6f7 100644 --- a/mysql-test/t/grant2.test +++ b/mysql-test/t/grant2.test @@ -873,9 +873,8 @@ CACHE INDEX t3 IN keycache1; LOAD INDEX INTO CACHE t3; --echo # ---echo # RENAME (doesn't work for temporary tables, thus should fail). +--echo # RENAME should work for temporary tables --echo # ---error ER_TABLEACCESS_DENIED_ERROR RENAME TABLE t3 TO t3_1; --echo # diff --git a/mysql-test/t/rename.test b/mysql-test/t/rename.test index a55bc845acc..67732d5b5b9 100644 --- a/mysql-test/t/rename.test +++ b/mysql-test/t/rename.test @@ -95,3 +95,49 @@ drop table t1; --source include/wait_until_count_sessions.inc +# +# Test of rename with temporary tables +# + +CREATE OR REPLACE TABLE t1 (a INT); +CREATE OR REPLACE TABLE t2 (a INT); +CREATE OR REPLACE TEMPORARY TABLE t1_tmp (b INT); +CREATE OR REPLACE TEMPORARY TABLE t2_tmp (b INT); + +# Can't rename table over another one +--error ER_TABLE_EXISTS_ERROR +rename table t1 to t2; +--error ER_TABLE_EXISTS_ERROR +rename table t1 to tmp, tmp to t2; +--error ER_TABLE_EXISTS_ERROR +rename table t1_tmp to t2_tmp; +--error ER_TABLE_EXISTS_ERROR +rename table t1_tmp to tmp, tmp to t2_tmp; + +show create table t1_tmp; +show create table t2_tmp; + +# The following should work +rename table t1 to t1_tmp; +rename table t2_tmp to t2; +rename table t2 to tmp, tmp to t2; +rename table t1_tmp to tmp, tmp to t1_tmp; +show tables; +SHOW CREATE TABLE t1_tmp; +drop table t1_tmp; +SHOW CREATE TABLE t1_tmp; +drop table t1_tmp; +SHOW CREATE TABLE t2; +drop table t2; +SHOW CREATE TABLE t2; +drop table t2; + +CREATE TABLE t1 (a INT); +insert into t1 values (1); +CREATE TEMPORARY TABLE t1 (b INT); +insert into t1 values (2); +RENAME TABLE t1 TO tmp, t1 TO t2; +select * from tmp; +select * from t2; +drop table tmp,t2; + diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 1162aa901d1..28d11ef2e5c 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -495,6 +495,8 @@ void init_update_queries(void) There are other statements that deal with temporary tables and open them, but which are not listed here. The thing is that the order of pre-opening temporary tables for those statements is somewhat custom. + + Note that SQLCOM_RENAME_TABLE should not be in this list! */ sql_command_flags[SQLCOM_CREATE_TABLE]|= CF_PREOPEN_TMP_TABLES; sql_command_flags[SQLCOM_DROP_TABLE]|= CF_PREOPEN_TMP_TABLES; @@ -508,7 +510,6 @@ void init_update_queries(void) sql_command_flags[SQLCOM_INSERT_SELECT]|= CF_PREOPEN_TMP_TABLES; sql_command_flags[SQLCOM_DELETE]|= CF_PREOPEN_TMP_TABLES; sql_command_flags[SQLCOM_DELETE_MULTI]|= CF_PREOPEN_TMP_TABLES; - sql_command_flags[SQLCOM_RENAME_TABLE]|= CF_PREOPEN_TMP_TABLES; sql_command_flags[SQLCOM_REPLACE_SELECT]|= CF_PREOPEN_TMP_TABLES; sql_command_flags[SQLCOM_SELECT]|= CF_PREOPEN_TMP_TABLES; sql_command_flags[SQLCOM_SET_OPTION]|= CF_PREOPEN_TMP_TABLES; @@ -5333,6 +5334,60 @@ static bool execute_show_status(THD *thd, TABLE_LIST *all_tables) } +/* + Find out if a table is a temporary table + + A table is a temporary table if it's a temporary table or + there has been before a temporary table that has been renamed + to the current name. + + Some examples: + A->B B is a temporary table if and only if A is a temp. + A->B, B->C Second B is temp if A is temp + A->B, A->C Second A can't be temp as if A was temp then B is temp + and Second A can only be a normal table. C is also not temp +*/ + +static TABLE *find_temporary_table_for_rename(THD *thd, + TABLE_LIST *first_table, + TABLE_LIST *cur_table) +{ + TABLE_LIST *table; + TABLE *res= 0; + bool found= 0; + DBUG_ENTER("find_temporary_table_for_rename"); + + /* Find last instance when cur_table is in TO part */ + for (table= first_table; + table != cur_table; + table= table->next_local->next_local) + { + TABLE_LIST *next= table->next_local; + + if (!strcmp(table->get_db_name(), cur_table->get_db_name()) && + !strcmp(table->get_table_name(), cur_table->get_table_name())) + { + /* Table was moved away, can't be same as 'table' */ + found= 1; + res= 0; // Table can't be a temporary table + } + if (!strcmp(next->get_db_name(), cur_table->get_db_name()) && + !strcmp(next->get_table_name(), cur_table->get_table_name())) + { + /* + Table has matching name with new name of this table. cur_table should + have same temporary type as this table. + */ + found= 1; + res= table->table; + } + } + if (!found) + res= find_temporary_table(thd, table); + DBUG_RETURN(res); +} + + static bool execute_rename_table(THD *thd, TABLE_LIST *first_table, TABLE_LIST *all_tables) { @@ -5349,13 +5404,19 @@ static bool execute_rename_table(THD *thd, TABLE_LIST *first_table, &table->next_local->grant.m_internal, 0, 0)) return 1; + + /* check if these are refering to temporary tables */ + table->table= find_temporary_table_for_rename(thd, first_table, table); + table->next_local->table= table->table; + TABLE_LIST old_list, new_list; /* we do not need initialize old_list and new_list because we will - come table[0] and table->next[0] there + copy table[0] and table->next[0] there */ old_list= table[0]; new_list= table->next_local[0]; + if (check_grant(thd, ALTER_ACL | DROP_ACL, &old_list, FALSE, 1, FALSE) || (!test_all_bits(table->next_local->grant.privilege, INSERT_ACL | CREATE_ACL) && diff --git a/sql/sql_rename.cc b/sql/sql_rename.cc index 1a9cb842e6a..119f9063724 100644 --- a/sql/sql_rename.cc +++ b/sql/sql_rename.cc @@ -222,7 +222,7 @@ do_rename_temporary(THD *thd, TABLE_LIST *ren_table, TABLE_LIST *new_table, new_alias= (lower_case_table_names == 2) ? new_table->alias : new_table->table_name; - if (is_temporary_table(new_table)) + if (find_temporary_table(thd, new_table)) { my_error(ER_TABLE_EXISTS_ERROR, MYF(0), new_alias); DBUG_RETURN(1); // This can't be skipped From 908676dfd9d981fd0f37a7cf9332abac522f1936 Mon Sep 17 00:00:00 2001 From: Monty Date: Tue, 22 May 2018 23:05:01 +0300 Subject: [PATCH 5/8] MDEV-15308 Assertion `ha_alter_info->alter_info->drop_list.elements Problem was that handle_if_exists_options() didn't correct alter_info->flags when things was removed from the list. --- mysql-test/r/alter_table.result | 61 +++++++++++++++++++++++++++++++++ mysql-test/t/alter_table.test | 32 +++++++++++++++++ sql/sql_table.cc | 29 +++++++++++++--- 3 files changed, 118 insertions(+), 4 deletions(-) diff --git a/mysql-test/r/alter_table.result b/mysql-test/r/alter_table.result index e32037c4be9..bb78df907ad 100644 --- a/mysql-test/r/alter_table.result +++ b/mysql-test/r/alter_table.result @@ -2164,3 +2164,64 @@ t1 CREATE TABLE `t1` ( `b` int(11) DEFAULT NULL ) ENGINE=InnoDB DEFAULT CHARSET=utf8 DROP TABLE t1; +# +# +# MDEV-15308 +# Assertion `ha_alter_info->alter_info->drop_list.elements > 0' failed +# in ha_innodb::prepare_inplace_alter_table +# +CREATE TABLE t1 (a INT, b INT) ENGINE=InnoDB; +ALTER TABLE t1 DROP FOREIGN KEY IF EXISTS fk, DROP COLUMN b; +Warnings: +Note 1091 Can't DROP 'fk'; check that column/key exists +SHOW CREATE TABLE t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `a` int(11) DEFAULT NULL +) ENGINE=InnoDB DEFAULT CHARSET=latin1 +DROP TABLE t1; +CREATE TABLE t1 (a INT, b INT) ENGINE=InnoDB; +ALTER TABLE t1 DROP INDEX IF EXISTS fk, DROP COLUMN b; +Warnings: +Note 1091 Can't DROP 'fk'; check that column/key exists +SHOW CREATE TABLE t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `a` int(11) DEFAULT NULL +) ENGINE=InnoDB DEFAULT CHARSET=latin1 +DROP TABLE t1; +CREATE TABLE t1 (a INT, b INT, c INT, KEY(c)) ENGINE=InnoDB; +ALTER TABLE t1 DROP FOREIGN KEY IF EXISTS fk, DROP COLUMN c; +Warnings: +Note 1091 Can't DROP 'fk'; check that column/key exists +SHOW CREATE TABLE t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `a` int(11) DEFAULT NULL, + `b` int(11) DEFAULT NULL +) ENGINE=InnoDB DEFAULT CHARSET=latin1 +DROP TABLE t1; +CREATE TABLE t1 (a INT, b INT, c INT, KEY c1(c)) ENGINE=InnoDB; +ALTER TABLE t1 DROP FOREIGN KEY IF EXISTS fk, DROP INDEX c1; +Warnings: +Note 1091 Can't DROP 'fk'; check that column/key exists +SHOW CREATE TABLE t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `a` int(11) DEFAULT NULL, + `b` int(11) DEFAULT NULL, + `c` int(11) DEFAULT NULL +) ENGINE=InnoDB DEFAULT CHARSET=latin1 +DROP TABLE t1; +CREATE TABLE t1 (a INT, b INT) ENGINE=InnoDB; +ALTER TABLE t1 DROP INDEX IF EXISTS fk, DROP COLUMN IF EXISTS c; +Warnings: +Note 1091 Can't DROP 'fk'; check that column/key exists +Note 1091 Can't DROP 'c'; check that column/key exists +SHOW CREATE TABLE t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `a` int(11) DEFAULT NULL, + `b` int(11) DEFAULT NULL +) ENGINE=InnoDB DEFAULT CHARSET=latin1 +DROP TABLE t1; diff --git a/mysql-test/t/alter_table.test b/mysql-test/t/alter_table.test index 8fcc7d044e5..585a272de9b 100644 --- a/mysql-test/t/alter_table.test +++ b/mysql-test/t/alter_table.test @@ -1809,3 +1809,35 @@ SHOW CREATE TABLE t1; ALTER TABLE t1 CONVERT TO CHARACTER SET utf8; SHOW CREATE TABLE t1; DROP TABLE t1; + +--echo # +--echo # +--echo # MDEV-15308 +--echo # Assertion `ha_alter_info->alter_info->drop_list.elements > 0' failed +--echo # in ha_innodb::prepare_inplace_alter_table +--echo # + +CREATE TABLE t1 (a INT, b INT) ENGINE=InnoDB; +ALTER TABLE t1 DROP FOREIGN KEY IF EXISTS fk, DROP COLUMN b; +SHOW CREATE TABLE t1; +DROP TABLE t1; + +CREATE TABLE t1 (a INT, b INT) ENGINE=InnoDB; +ALTER TABLE t1 DROP INDEX IF EXISTS fk, DROP COLUMN b; +SHOW CREATE TABLE t1; +DROP TABLE t1; + +CREATE TABLE t1 (a INT, b INT, c INT, KEY(c)) ENGINE=InnoDB; +ALTER TABLE t1 DROP FOREIGN KEY IF EXISTS fk, DROP COLUMN c; +SHOW CREATE TABLE t1; +DROP TABLE t1; + +CREATE TABLE t1 (a INT, b INT, c INT, KEY c1(c)) ENGINE=InnoDB; +ALTER TABLE t1 DROP FOREIGN KEY IF EXISTS fk, DROP INDEX c1; +SHOW CREATE TABLE t1; +DROP TABLE t1; + +CREATE TABLE t1 (a INT, b INT) ENGINE=InnoDB; +ALTER TABLE t1 DROP INDEX IF EXISTS fk, DROP COLUMN IF EXISTS c; +SHOW CREATE TABLE t1; +DROP TABLE t1; diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 27d579a6b19..376c1362cc7 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -5808,10 +5808,28 @@ drop_create_field: List_iterator drop_it(alter_info->drop_list); Alter_drop *drop; bool remove_drop; + ulonglong left_flags= 0; while ((drop= drop_it++)) { + ulonglong cur_flag= 0; + switch (drop->type) { + case Alter_drop::COLUMN: + cur_flag= Alter_info::ALTER_DROP_COLUMN; + break; + case Alter_drop::FOREIGN_KEY: + cur_flag= Alter_info::DROP_FOREIGN_KEY; + break; + case Alter_drop::KEY: + cur_flag= Alter_info::ALTER_DROP_INDEX; + break; + default: + break; + } if (!drop->drop_if_exists) + { + left_flags|= cur_flag; continue; + } remove_drop= TRUE; if (drop->type == Alter_drop::COLUMN) { @@ -5887,12 +5905,15 @@ drop_create_field: ER_CANT_DROP_FIELD_OR_KEY, ER(ER_CANT_DROP_FIELD_OR_KEY), drop->name); drop_it.remove(); - if (alter_info->drop_list.is_empty()) - alter_info->flags&= ~(Alter_info::ALTER_DROP_COLUMN | - Alter_info::ALTER_DROP_INDEX | - Alter_info::DROP_FOREIGN_KEY); } + else + left_flags|= cur_flag; } + /* Reset state to what's left in drop list */ + alter_info->flags&= ~(Alter_info::ALTER_DROP_COLUMN | + Alter_info::ALTER_DROP_INDEX | + Alter_info::DROP_FOREIGN_KEY); + alter_info->flags|= left_flags; } /* ALTER TABLE ADD KEY IF NOT EXISTS */ From a816aa066e5c879a92819d694a93d245e6ec6e47 Mon Sep 17 00:00:00 2001 From: Monty Date: Wed, 23 May 2018 11:26:49 +0300 Subject: [PATCH 6/8] Fixed ASAN heap-use-after-free handler::ha_index_or_rnd_end MDEV-16123 ASAN heap-use-after-free handler::ha_index_or_rnd_end MDEV-13828 Segmentation fault on RENAME TABLE Problem was that destructor called methods for closed table. Fixed by removing code in destructor. --- mysql-test/r/statistics_close.result | 6 ++++++ mysql-test/t/statistics_close.test | 18 ++++++++++++++++++ sql/sql_statistics.cc | 3 ++- 3 files changed, 26 insertions(+), 1 deletion(-) create mode 100644 mysql-test/r/statistics_close.result create mode 100644 mysql-test/t/statistics_close.test diff --git a/mysql-test/r/statistics_close.result b/mysql-test/r/statistics_close.result new file mode 100644 index 00000000000..348681c3311 --- /dev/null +++ b/mysql-test/r/statistics_close.result @@ -0,0 +1,6 @@ +CREATE TABLE t1 (i int); +RENAME TABLE t1 TO t2; +FLUSH TABLES; +DROP TABLE IF EXISTS t1, t2; +Warnings: +Note 1051 Unknown table 'test.t1' diff --git a/mysql-test/t/statistics_close.test b/mysql-test/t/statistics_close.test new file mode 100644 index 00000000000..de22a5a44fe --- /dev/null +++ b/mysql-test/t/statistics_close.test @@ -0,0 +1,18 @@ +# +# MDEV-16123 ASAN heap-use-after-free handler::ha_index_or_rnd_end +# MDEV-13828 Segmentation fault on RENAME TABLE +# + +CREATE TABLE t1 (i int); +--connect (con1,localhost,root,,test) +--send +RENAME TABLE t1 TO t2; +--connection default +FLUSH TABLES; +--connection con1 +--reap + +# Cleanup +--disconnect con1 +--connection default +DROP TABLE IF EXISTS t1, t2; diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc index ce320e87a4f..471749ad346 100644 --- a/sql/sql_statistics.cc +++ b/sql/sql_statistics.cc @@ -1376,7 +1376,8 @@ public: ~Stat_table_write_iter() { - cleanup(); + /* Ensure that cleanup has been run */ + DBUG_ASSERT(rowid_buf == 0); } }; From a61724a3ca187f6eb44fc67948acb76a94efa783 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Thu, 24 May 2018 11:38:34 +0300 Subject: [PATCH 7/8] MDEV-16267 Wrong INFORMATION_SCHEMA.INNODB_BUFFER_PAGE.TABLE_NAME i_s_innodb_buffer_page_fill(), i_s_innodb_buf_page_lru_fill(): Only invoke Field::set_notnull() if the index was found. --- storage/innobase/handler/i_s.cc | 28 +++++++++++++++++++--------- storage/xtradb/handler/i_s.cc | 28 +++++++++++++++++++--------- 2 files changed, 38 insertions(+), 18 deletions(-) diff --git a/storage/innobase/handler/i_s.cc b/storage/innobase/handler/i_s.cc index 41f4114494d..f8aa3bd4637 100644 --- a/storage/innobase/handler/i_s.cc +++ b/storage/innobase/handler/i_s.cc @@ -1,7 +1,7 @@ /***************************************************************************** Copyright (c) 2007, 2016, Oracle and/or its affiliates. All Rights Reserved. -Copyright (c) 2017, MariaDB Corporation. +Copyright (c) 2017, 2018, MariaDB Corporation. This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software @@ -4955,9 +4955,11 @@ i_s_innodb_buffer_page_fill( mutex_enter(&dict_sys->mutex); - if (const dict_index_t* index = - dict_index_get_if_in_cache_low( - page_info->index_id)) { + const dict_index_t* index = + dict_index_get_if_in_cache_low( + page_info->index_id); + + if (index) { table_name_end = innobase_convert_name( table_name, sizeof(table_name), index->table_name, @@ -4980,7 +4982,10 @@ i_s_innodb_buffer_page_fill( OK(ret); - fields[IDX_BUFFER_PAGE_TABLE_NAME]->set_notnull(); + if (index) { + fields[IDX_BUFFER_PAGE_TABLE_NAME] + ->set_notnull(); + } } OK(fields[IDX_BUFFER_PAGE_NUM_RECS]->store( @@ -5657,9 +5662,11 @@ i_s_innodb_buf_page_lru_fill( mutex_enter(&dict_sys->mutex); - if (const dict_index_t* index = - dict_index_get_if_in_cache_low( - page_info->index_id)) { + const dict_index_t* index = + dict_index_get_if_in_cache_low( + page_info->index_id); + + if (index) { table_name_end = innobase_convert_name( table_name, sizeof(table_name), index->table_name, @@ -5682,7 +5689,10 @@ i_s_innodb_buf_page_lru_fill( OK(ret); - fields[IDX_BUF_LRU_PAGE_TABLE_NAME]->set_notnull(); + if (index) { + fields[IDX_BUF_LRU_PAGE_TABLE_NAME] + ->set_notnull(); + } } OK(fields[IDX_BUF_LRU_PAGE_NUM_RECS]->store( diff --git a/storage/xtradb/handler/i_s.cc b/storage/xtradb/handler/i_s.cc index 011bc77dc3c..834bae399fb 100644 --- a/storage/xtradb/handler/i_s.cc +++ b/storage/xtradb/handler/i_s.cc @@ -1,7 +1,7 @@ /***************************************************************************** Copyright (c) 2007, 2016, Oracle and/or its affiliates. All Rights Reserved. -Copyright (c) 2017, MariaDB Corporation. +Copyright (c) 2017, 2018, MariaDB Corporation. This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software @@ -4943,9 +4943,11 @@ i_s_innodb_buffer_page_fill( mutex_enter(&dict_sys->mutex); - if (const dict_index_t* index = - dict_index_get_if_in_cache_low( - page_info->index_id)) { + const dict_index_t* index = + dict_index_get_if_in_cache_low( + page_info->index_id); + + if (index) { table_name_end = innobase_convert_name( table_name, sizeof(table_name), index->table_name, @@ -4968,7 +4970,10 @@ i_s_innodb_buffer_page_fill( OK(ret); - fields[IDX_BUFFER_PAGE_TABLE_NAME]->set_notnull(); + if (index) { + fields[IDX_BUFFER_PAGE_TABLE_NAME] + ->set_notnull(); + } } OK(fields[IDX_BUFFER_PAGE_NUM_RECS]->store( @@ -5642,9 +5647,11 @@ i_s_innodb_buf_page_lru_fill( mutex_enter(&dict_sys->mutex); - if (const dict_index_t* index = - dict_index_get_if_in_cache_low( - page_info->index_id)) { + const dict_index_t* index = + dict_index_get_if_in_cache_low( + page_info->index_id); + + if (index) { table_name_end = innobase_convert_name( table_name, sizeof(table_name), index->table_name, @@ -5667,7 +5674,10 @@ i_s_innodb_buf_page_lru_fill( OK(ret); - fields[IDX_BUF_LRU_PAGE_TABLE_NAME]->set_notnull(); + if (index) { + fields[IDX_BUF_LRU_PAGE_TABLE_NAME] + ->set_notnull(); + } } OK(fields[IDX_BUF_LRU_PAGE_NUM_RECS]->store( From bfed1bfe28980d3b1404f99a44712242a5108ef5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Thu, 24 May 2018 11:55:27 +0300 Subject: [PATCH 8/8] Add a missing dependency to a test --- mysql-test/suite/galera/t/galera_var_dirty_reads.test | 1 + 1 file changed, 1 insertion(+) diff --git a/mysql-test/suite/galera/t/galera_var_dirty_reads.test b/mysql-test/suite/galera/t/galera_var_dirty_reads.test index 138b7c1c703..3e2108868af 100644 --- a/mysql-test/suite/galera/t/galera_var_dirty_reads.test +++ b/mysql-test/suite/galera/t/galera_var_dirty_reads.test @@ -4,6 +4,7 @@ --source include/galera_cluster.inc --source include/have_innodb.inc +--source include/have_perfschema.inc # Save original auto_increment_offset values. --let $node_1=node_1