From 9827c5e1031ca5ebe1c6c7d4f5eac3cce5749d8e Mon Sep 17 00:00:00 2001 From: Sachin Date: Tue, 26 Jun 2018 11:33:58 +0530 Subject: [PATCH 1/8] MDEV-16192 Table 't' is specified twice, both as a target for 'CREATE' and... as a separate source for data Actually MDEV-15867 and MDEV-16192 are same, Slave adds "or replace" to create table stmt. So create table t1 is create or replace on slave. So this bug is not because of replication, We can get this bug on general server if we manually add or replace to create query. Problem:- So if we try to create table t1 (same name as of temp table t1 ) via CREATE or replace TABLE t AS SELECT * FROM t; Since in this query we are creating table from select * from t1 , we call unique_table function to see whether if source and destination table are same. But there is one issue unique_table does not account if source table is tmp table in this case source and destination table can be same. Solution:- We will change find_dup_table to not to look for temp table if CHECK_DUP_SKIP_TEMP_TABLE flag is on. --- mysql-test/r/create_replace_tmp.result | 4 ++++ mysql-test/suite/rpl/r/rpl_15867.result | 9 +++++++++ mysql-test/suite/rpl/t/rpl_15867.test | 11 +++++++++++ mysql-test/t/create_replace_tmp.test | 4 ++++ sql/sql_base.cc | 6 ++++++ sql/sql_base.h | 1 + sql/sql_parse.cc | 4 ++-- 7 files changed, 37 insertions(+), 2 deletions(-) create mode 100644 mysql-test/r/create_replace_tmp.result create mode 100644 mysql-test/suite/rpl/r/rpl_15867.result create mode 100644 mysql-test/suite/rpl/t/rpl_15867.test create mode 100644 mysql-test/t/create_replace_tmp.test diff --git a/mysql-test/r/create_replace_tmp.result b/mysql-test/r/create_replace_tmp.result new file mode 100644 index 00000000000..0239f4d4817 --- /dev/null +++ b/mysql-test/r/create_replace_tmp.result @@ -0,0 +1,4 @@ +CREATE TEMPORARY TABLE t (i INT); +CREATE or replace TABLE t AS SELECT * FROM t; +DROP TEMPORARY TABLE t; +DROP TABLE t; diff --git a/mysql-test/suite/rpl/r/rpl_15867.result b/mysql-test/suite/rpl/r/rpl_15867.result new file mode 100644 index 00000000000..9cb63266a29 --- /dev/null +++ b/mysql-test/suite/rpl/r/rpl_15867.result @@ -0,0 +1,9 @@ +include/master-slave.inc +[connection master] +CREATE TEMPORARY TABLE t (i INT); +CREATE TABLE t AS SELECT * FROM t; +connection slave; +connection master; +DROP TEMPORARY TABLE t; +DROP TABLE t; +include/rpl_end.inc diff --git a/mysql-test/suite/rpl/t/rpl_15867.test b/mysql-test/suite/rpl/t/rpl_15867.test new file mode 100644 index 00000000000..6de39041bb1 --- /dev/null +++ b/mysql-test/suite/rpl/t/rpl_15867.test @@ -0,0 +1,11 @@ +--source include/master-slave.inc +CREATE TEMPORARY TABLE t (i INT); +CREATE TABLE t AS SELECT * FROM t; + +--sync_slave_with_master + +# Cleanup +--connection master +DROP TEMPORARY TABLE t; +DROP TABLE t; +--source include/rpl_end.inc diff --git a/mysql-test/t/create_replace_tmp.test b/mysql-test/t/create_replace_tmp.test new file mode 100644 index 00000000000..0239f4d4817 --- /dev/null +++ b/mysql-test/t/create_replace_tmp.test @@ -0,0 +1,4 @@ +CREATE TEMPORARY TABLE t (i INT); +CREATE or replace TABLE t AS SELECT * FROM t; +DROP TEMPORARY TABLE t; +DROP TABLE t; diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 32e42daf7c4..0ac7a112161 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -1031,6 +1031,12 @@ retry: if (res->table && (res->table == table->table)) continue; + /* Skip if table is tmp table */ + if (check_flag & CHECK_DUP_SKIP_TEMP_TABLE && + res->table && res->table->s->tmp_table != NO_TMP_TABLE) + { + continue; + } if (check_flag & CHECK_DUP_FOR_CREATE) DBUG_RETURN(res); diff --git a/sql/sql_base.h b/sql/sql_base.h index 914cdcd4512..4f99111cbd9 100644 --- a/sql/sql_base.h +++ b/sql/sql_base.h @@ -64,6 +64,7 @@ enum find_item_error_report_type {REPORT_ALL_ERRORS, REPORT_EXCEPT_NOT_FOUND, /* Flag bits for unique_table() */ #define CHECK_DUP_ALLOW_DIFFERENT_ALIAS 1 #define CHECK_DUP_FOR_CREATE 2 +#define CHECK_DUP_SKIP_TEMP_TABLE 4 uint get_table_def_key(const TABLE_LIST *table_list, const char **key); TABLE *open_ltable(THD *thd, TABLE_LIST *table_list, thr_lock_type update, diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index df0ee2bd680..3ec7b54e0a9 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -3910,8 +3910,8 @@ mysql_execute_command(THD *thd) { TABLE_LIST *duplicate; if ((duplicate= unique_table(thd, lex->query_tables, - lex->query_tables->next_global, - CHECK_DUP_FOR_CREATE))) + lex->query_tables->next_global, + CHECK_DUP_FOR_CREATE | CHECK_DUP_SKIP_TEMP_TABLE))) { update_non_unique_table_error(lex->query_tables, "CREATE", duplicate); From d0d073b1aaa30997307bc7aa686d3715f8c22da0 Mon Sep 17 00:00:00 2001 From: Igor Babaev Date: Fri, 20 Jul 2018 19:32:28 -0700 Subject: [PATCH 2/8] Corrected and added back the test case for MDEV-15151. --- .../r/cte_recursive_not_embedded.result | 23 ++++++++++ mysql-test/t/cte_recursive_not_embedded.test | 42 +++++++++++++++++++ 2 files changed, 65 insertions(+) create mode 100644 mysql-test/r/cte_recursive_not_embedded.result create mode 100644 mysql-test/t/cte_recursive_not_embedded.test diff --git a/mysql-test/r/cte_recursive_not_embedded.result b/mysql-test/r/cte_recursive_not_embedded.result new file mode 100644 index 00000000000..202864be159 --- /dev/null +++ b/mysql-test/r/cte_recursive_not_embedded.result @@ -0,0 +1,23 @@ +# +# MDEV-15151: function with recursive CTE using no base tables +# (duplicate of MDEV-16661) +# +connection default; +CREATE TABLE t1 (id int KEY); +INSERT INTO t1 VALUES (0), (1),(2); +CREATE OR REPLACE FUNCTION func() RETURNS int +RETURN +( +WITH recursive cte AS +(SELECT 1 a UNION SELECT cte.* FROM cte natural join t1) +SELECT * FROM cte limit 1 +); +connect con1,localhost,root,,test; +SELECT func(); +connect con2,localhost,root,,test; +disconnect con2; +connection con1; +disconnect con1; +connection default; +DROP FUNCTION func; +DROP TABLE t1; diff --git a/mysql-test/t/cte_recursive_not_embedded.test b/mysql-test/t/cte_recursive_not_embedded.test new file mode 100644 index 00000000000..4dadf400681 --- /dev/null +++ b/mysql-test/t/cte_recursive_not_embedded.test @@ -0,0 +1,42 @@ +--source include/not_embedded.inc + +--echo # +--echo # MDEV-15151: function with recursive CTE using no base tables +--echo # (duplicate of MDEV-16661) +--echo # + +--connection default + +CREATE TABLE t1 (id int KEY); +INSERT INTO t1 VALUES (0), (1),(2); + +CREATE OR REPLACE FUNCTION func() RETURNS int +RETURN +( + WITH recursive cte AS + (SELECT 1 a UNION SELECT cte.* FROM cte natural join t1) + SELECT * FROM cte limit 1 +); + +--connect (con1,localhost,root,,test) + +--let $conid= `SELECT CONNECTION_ID()` +--send SELECT func() + +--connect (con2,localhost,root,,test) +--disable_query_log +--eval KILL QUERY $conid +--enable_query_log +--disconnect con2 + +--disable_result_log +--connection con1 +--error 0,ER_QUERY_INTERRUPTED +--reap +--disconnect con1 +--enable_result_log + +--connection default + +DROP FUNCTION func; +DROP TABLE t1; From 73af07536612e4f76ca24974cfa17ddd282d50a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Mon, 23 Jul 2018 13:31:10 +0300 Subject: [PATCH 3/8] Reduce the number of rw_lock_own() calls Replace pairs of rw_lock_own() calls with calls to rw_lock_own_flagged(). These calls are only present in debug builds. --- storage/innobase/btr/btr0sea.cc | 24 ++++++++++++------------ storage/innobase/buf/buf0buf.cc | 17 +++++++++-------- storage/innobase/buf/buf0lru.cc | 9 ++++----- storage/innobase/gis/gis0sea.cc | 5 ++--- storage/innobase/include/ha0ha.ic | 8 +++----- storage/innobase/include/sync0rw.ic | 3 +-- storage/innobase/row/row0log.cc | 4 ++-- storage/innobase/row/row0sel.cc | 4 ++-- storage/innobase/row/row0umod.cc | 4 ++-- storage/innobase/sync/sync0rw.cc | 8 ++++---- storage/innobase/trx/trx0i_s.cc | 22 ++++++++-------------- 11 files changed, 49 insertions(+), 59 deletions(-) diff --git a/storage/innobase/btr/btr0sea.cc b/storage/innobase/btr/btr0sea.cc index 7419ed2bfd5..829e0c47e39 100644 --- a/storage/innobase/btr/btr0sea.cc +++ b/storage/innobase/btr/btr0sea.cc @@ -529,10 +529,10 @@ btr_search_update_block_hash_info( buf_block_t* block, const btr_cur_t* cursor) { - ut_ad(!rw_lock_own(btr_get_search_latch(cursor->index), RW_LOCK_S)); - ut_ad(!rw_lock_own(btr_get_search_latch(cursor->index), RW_LOCK_X)); - ut_ad(rw_lock_own(&block->lock, RW_LOCK_S) - || rw_lock_own(&block->lock, RW_LOCK_X)); + ut_ad(!rw_lock_own_flagged(btr_get_search_latch(cursor->index), + RW_LOCK_FLAG_X | RW_LOCK_FLAG_S)); + ut_ad(rw_lock_own_flagged(&block->lock, + RW_LOCK_FLAG_X | RW_LOCK_FLAG_S)); info->last_hash_succ = FALSE; @@ -607,8 +607,8 @@ btr_search_update_hash_ref( ut_ad(cursor->flag == BTR_CUR_HASH_FAIL); ut_ad(rw_lock_own(btr_get_search_latch(cursor->index), RW_LOCK_X)); - ut_ad(rw_lock_own(&(block->lock), RW_LOCK_S) - || rw_lock_own(&(block->lock), RW_LOCK_X)); + ut_ad(rw_lock_own_flagged(&block->lock, + RW_LOCK_FLAG_X | RW_LOCK_FLAG_S)); ut_ad(page_align(btr_cur_get_rec(cursor)) == block->frame); ut_ad(page_is_leaf(block->frame)); assert_block_ahi_valid(block); @@ -667,8 +667,8 @@ btr_search_info_update_slow( buf_block_t* block; ibool build_index; - ut_ad(!rw_lock_own(btr_get_search_latch(cursor->index), RW_LOCK_S)); - ut_ad(!rw_lock_own(btr_get_search_latch(cursor->index), RW_LOCK_X)); + ut_ad(!rw_lock_own_flagged(btr_get_search_latch(cursor->index), + RW_LOCK_FLAG_X | RW_LOCK_FLAG_S)); block = btr_cur_get_block(cursor); @@ -1138,8 +1138,8 @@ retry: ut_ad(block->page.buf_fix_count == 0 || buf_block_get_state(block) == BUF_BLOCK_REMOVE_HASH - || rw_lock_own(&block->lock, RW_LOCK_S) - || rw_lock_own(&block->lock, RW_LOCK_X)); + || rw_lock_own_flagged(&block->lock, + RW_LOCK_FLAG_X | RW_LOCK_FLAG_S)); ut_ad(page_is_leaf(block->frame)); /* We must not dereference index here, because it could be freed @@ -1393,8 +1393,8 @@ btr_search_build_page_hash_index( ut_ad(page_is_leaf(block->frame)); ut_ad(!rw_lock_own(btr_get_search_latch(index), RW_LOCK_X)); - ut_ad(rw_lock_own(&(block->lock), RW_LOCK_S) - || rw_lock_own(&(block->lock), RW_LOCK_X)); + ut_ad(rw_lock_own_flagged(&block->lock, + RW_LOCK_FLAG_X | RW_LOCK_FLAG_S)); btr_search_s_lock(index); diff --git a/storage/innobase/buf/buf0buf.cc b/storage/innobase/buf/buf0buf.cc index 30fe1f8f616..0a5a59c9829 100644 --- a/storage/innobase/buf/buf0buf.cc +++ b/storage/innobase/buf/buf0buf.cc @@ -4448,8 +4448,9 @@ loop: case BUF_GET_IF_IN_POOL_OR_WATCH: case BUF_PEEK_IF_IN_POOL: case BUF_EVICT_IF_IN_POOL: - ut_ad(!rw_lock_own(hash_lock, RW_LOCK_X)); - ut_ad(!rw_lock_own(hash_lock, RW_LOCK_S)); + ut_ad(!rw_lock_own_flagged( + hash_lock, + RW_LOCK_FLAG_X | RW_LOCK_FLAG_S)); return(NULL); } @@ -4785,8 +4786,8 @@ evict_from_pool: ut_ad(block == fix_block); ut_ad(fix_block->page.buf_fix_count > 0); - ut_ad(!rw_lock_own(hash_lock, RW_LOCK_X)); - ut_ad(!rw_lock_own(hash_lock, RW_LOCK_S)); + ut_ad(!rw_lock_own_flagged(hash_lock, + RW_LOCK_FLAG_X | RW_LOCK_FLAG_S)); ut_ad(buf_block_get_state(fix_block) == BUF_BLOCK_FILE_PAGE); @@ -4962,8 +4963,8 @@ evict_from_pool: ut_a(ibuf_count_get(fix_block->page.id) == 0); #endif - ut_ad(!rw_lock_own(hash_lock, RW_LOCK_X)); - ut_ad(!rw_lock_own(hash_lock, RW_LOCK_S)); + ut_ad(!rw_lock_own_flagged(hash_lock, + RW_LOCK_FLAG_X | RW_LOCK_FLAG_S)); return(fix_block); } @@ -5639,8 +5640,8 @@ func_exit: ibuf_mtr_commit(&mtr); } - ut_ad(!rw_lock_own(hash_lock, RW_LOCK_X)); - ut_ad(!rw_lock_own(hash_lock, RW_LOCK_S)); + ut_ad(!rw_lock_own_flagged(hash_lock, + RW_LOCK_FLAG_X | RW_LOCK_FLAG_S)); ut_ad(!bpage || buf_page_in_file(bpage)); return(bpage); diff --git a/storage/innobase/buf/buf0lru.cc b/storage/innobase/buf/buf0lru.cc index 71144ee0f6e..d71d9d08511 100644 --- a/storage/innobase/buf/buf0lru.cc +++ b/storage/innobase/buf/buf0lru.cc @@ -1630,8 +1630,8 @@ func_exit: } /* buf_LRU_block_remove_hashed() releases the hash_lock */ - ut_ad(!rw_lock_own(hash_lock, RW_LOCK_X) - && !rw_lock_own(hash_lock, RW_LOCK_S)); + ut_ad(!rw_lock_own_flagged(hash_lock, + RW_LOCK_FLAG_X | RW_LOCK_FLAG_S)); /* We have just freed a BUF_BLOCK_FILE_PAGE. If b != NULL then it was a compressed page with an uncompressed frame and @@ -2184,9 +2184,8 @@ buf_LRU_free_one_page( } /* buf_LRU_block_remove_hashed() releases hash_lock and block_mutex */ - ut_ad(!rw_lock_own(hash_lock, RW_LOCK_X) - && !rw_lock_own(hash_lock, RW_LOCK_S)); - + ut_ad(!rw_lock_own_flagged(hash_lock, + RW_LOCK_FLAG_X | RW_LOCK_FLAG_S)); ut_ad(!mutex_own(block_mutex)); } diff --git a/storage/innobase/gis/gis0sea.cc b/storage/innobase/gis/gis0sea.cc index 173fc76ddfc..c45f2ecec59 100644 --- a/storage/innobase/gis/gis0sea.cc +++ b/storage/innobase/gis/gis0sea.cc @@ -258,9 +258,8 @@ rtr_pcur_getnext_from_path( rtr_info->tree_savepoints[tree_idx] = mtr_set_savepoint(mtr); #ifdef UNIV_RTR_DEBUG - ut_ad(!(rw_lock_own(&btr_cur->page_cur.block->lock, RW_LOCK_X) - || - rw_lock_own(&btr_cur->page_cur.block->lock, RW_LOCK_S)) + ut_ad(!(rw_lock_own_flagged(&btr_cur->page_cur.block->lock, + RW_LOCK_FLAG_X | RW_LOCK_FLAG_S)) || my_latch_mode == BTR_MODIFY_TREE || my_latch_mode == BTR_CONT_MODIFY_TREE || !page_is_leaf(buf_block_get_frame( diff --git a/storage/innobase/include/ha0ha.ic b/storage/innobase/include/ha0ha.ic index 1513df209ad..3c699ab9b0c 100644 --- a/storage/innobase/include/ha0ha.ic +++ b/storage/innobase/include/ha0ha.ic @@ -1,6 +1,7 @@ /***************************************************************************** Copyright (c) 1994, 2015, Oracle and/or its affiliates. All Rights Reserved. +Copyright (c) 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 @@ -137,11 +138,8 @@ hash_assert_can_search( if (table->type == HASH_TABLE_SYNC_MUTEX) { ut_ad(mutex_own(hash_get_mutex(table, fold))); } else if (table->type == HASH_TABLE_SYNC_RW_LOCK) { -# ifdef UNIV_DEBUG - rw_lock_t* lock = hash_get_lock(table, fold); - ut_ad(rw_lock_own(lock, RW_LOCK_X) - || rw_lock_own(lock, RW_LOCK_S)); -# endif + ut_ad(rw_lock_own_flagged(hash_get_lock(table, fold), + RW_LOCK_FLAG_X | RW_LOCK_FLAG_S)); } else { ut_ad(table->type == HASH_TABLE_SYNC_NONE); } diff --git a/storage/innobase/include/sync0rw.ic b/storage/innobase/include/sync0rw.ic index 21872cc8bee..404c7cb9b86 100644 --- a/storage/innobase/include/sync0rw.ic +++ b/storage/innobase/include/sync0rw.ic @@ -281,8 +281,7 @@ rw_lock_s_lock_func( the threads which have s-locked a latch. This would use some CPU time. */ - ut_ad(!rw_lock_own(lock, RW_LOCK_S)); /* see NOTE above */ - ut_ad(!rw_lock_own(lock, RW_LOCK_X)); + ut_ad(!rw_lock_own_flagged(lock, RW_LOCK_FLAG_X | RW_LOCK_FLAG_S)); if (!rw_lock_s_lock_low(lock, pass, file_name, line)) { diff --git a/storage/innobase/row/row0log.cc b/storage/innobase/row/row0log.cc index 55500b9533d..7e6760da372 100644 --- a/storage/innobase/row/row0log.cc +++ b/storage/innobase/row/row0log.cc @@ -290,8 +290,8 @@ row_log_online_op( ut_ad(dtuple_validate(tuple)); ut_ad(dtuple_get_n_fields(tuple) == dict_index_get_n_fields(index)); - ut_ad(rw_lock_own(dict_index_get_lock(index), RW_LOCK_S) - || rw_lock_own(dict_index_get_lock(index), RW_LOCK_X)); + ut_ad(rw_lock_own_flagged(&index->lock, + RW_LOCK_FLAG_X | RW_LOCK_FLAG_S)); if (index->is_corrupted()) { return; diff --git a/storage/innobase/row/row0sel.cc b/storage/innobase/row/row0sel.cc index 68a81615e88..d21fd4a10ed 100644 --- a/storage/innobase/row/row0sel.cc +++ b/storage/innobase/row/row0sel.cc @@ -1102,8 +1102,8 @@ sel_set_rtr_rec_lock( rw_lock_x_lock(&(match->block.lock)); retry: cur_block = btr_pcur_get_block(pcur); - ut_ad(rw_lock_own(&(match->block.lock), RW_LOCK_X) - || rw_lock_own(&(match->block.lock), RW_LOCK_S)); + ut_ad(rw_lock_own_flagged(&match->block.lock, + RW_LOCK_FLAG_X | RW_LOCK_FLAG_S)); ut_ad(page_is_leaf(buf_block_get_frame(cur_block))); err = lock_sec_rec_read_check_and_lock( diff --git a/storage/innobase/row/row0umod.cc b/storage/innobase/row/row0umod.cc index 5f28f096286..b5c8d7c4ce2 100644 --- a/storage/innobase/row/row0umod.cc +++ b/storage/innobase/row/row0umod.cc @@ -265,8 +265,8 @@ row_undo_mod_clust( ut_ad(thr_get_trx(thr) == node->trx); ut_ad(node->trx->dict_operation_lock_mode); ut_ad(node->trx->in_rollback); - ut_ad(rw_lock_own(dict_operation_lock, RW_LOCK_S) - || rw_lock_own(dict_operation_lock, RW_LOCK_X)); + ut_ad(rw_lock_own_flagged(dict_operation_lock, + RW_LOCK_FLAG_X | RW_LOCK_FLAG_S)); log_free_check(); pcur = &node->pcur; diff --git a/storage/innobase/sync/sync0rw.cc b/storage/innobase/sync/sync0rw.cc index 37b64910713..c985cf9ee82 100644 --- a/storage/innobase/sync/sync0rw.cc +++ b/storage/innobase/sync/sync0rw.cc @@ -2,7 +2,7 @@ Copyright (c) 1995, 2016, Oracle and/or its affiliates. All Rights Reserved. Copyright (c) 2008, Google Inc. -Copyright (c) 2017, MariaDB Corporation. +Copyright (c) 2017, 2018, MariaDB Corporation. Portions of this file contain modifications contributed and copyrighted by Google, Inc. Those modifications are gratefully acknowledged and are described @@ -1097,12 +1097,12 @@ rw_lock_own_flagged( const rw_lock_debug_t* info = *it; - ut_ad(os_thread_eq(info->thread_id, os_thread_get_curr_id())); - - if (info->pass != 0) { + if (info->pass) { continue; } + ut_ad(os_thread_eq(info->thread_id, os_thread_get_curr_id())); + switch (info->lock_type) { case RW_LOCK_S: diff --git a/storage/innobase/trx/trx0i_s.cc b/storage/innobase/trx/trx0i_s.cc index f0dc2fb78bd..363b61b0cfe 100644 --- a/storage/innobase/trx/trx0i_s.cc +++ b/storage/innobase/trx/trx0i_s.cc @@ -1,7 +1,7 @@ /***************************************************************************** Copyright (c) 2007, 2015, 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 @@ -1510,26 +1510,20 @@ cache_select_table( trx_i_s_cache_t* cache, /*!< in: whole cache */ enum i_s_table table) /*!< in: which table */ { - i_s_table_cache_t* table_cache; - - ut_ad(rw_lock_own(cache->rw_lock, RW_LOCK_S) - || rw_lock_own(cache->rw_lock, RW_LOCK_X)); + ut_ad(rw_lock_own_flagged(cache->rw_lock, + RW_LOCK_FLAG_X | RW_LOCK_FLAG_S)); switch (table) { case I_S_INNODB_TRX: - table_cache = &cache->innodb_trx; - break; + return &cache->innodb_trx; case I_S_INNODB_LOCKS: - table_cache = &cache->innodb_locks; - break; + return &cache->innodb_locks; case I_S_INNODB_LOCK_WAITS: - table_cache = &cache->innodb_lock_waits; - break; - default: - ut_error; + return &cache->innodb_lock_waits; } - return(table_cache); + ut_error; + return NULL; } /*******************************************************************//** From b660261be1975535624eeccbe52a8b93e0aaa3bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Mon, 23 Jul 2018 15:58:31 +0300 Subject: [PATCH 4/8] ut_print_buf_hex(): Correctly dump the hex This should affect at least rec_printer() output. --- storage/innobase/ut/ut0ut.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/storage/innobase/ut/ut0ut.cc b/storage/innobase/ut/ut0ut.cc index f655c800901..baae817f217 100644 --- a/storage/innobase/ut/ut0ut.cc +++ b/storage/innobase/ut/ut0ut.cc @@ -357,7 +357,7 @@ ut_print_buf_hex( for (data = static_cast(buf), i = 0; i < len; i++) { byte b = *data++; - o << hexdigit[(int) b >> 16] << hexdigit[b & 15]; + o << hexdigit[int(b) >> 4] << hexdigit[b & 15]; } o << ")"; From 730f6c912c5746e405c62f6c28b571771de0f5bf Mon Sep 17 00:00:00 2001 From: Thirunarayanan Balathandayuthapani Date: Mon, 23 Jul 2018 13:42:19 +0300 Subject: [PATCH 5/8] MDEV-16779 Assertion !rw_lock_own failed upon purge This is a regression caused by the fix of MDEV-15855. purge_vcol_info_t::set_used(): Add a missing condition. row_purge_poss_sec(): Invoke set_used() in order to have !is_first_fetch() when retrying. --- storage/innobase/include/row0types.h | 4 +++- storage/innobase/row/row0purge.cc | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/storage/innobase/include/row0types.h b/storage/innobase/include/row0types.h index 1bff3767253..005dc2ad95f 100644 --- a/storage/innobase/include/row0types.h +++ b/storage/innobase/include/row0types.h @@ -92,7 +92,9 @@ struct purge_vcol_info_t return; } - first_use = used = true; + if (!used) { + first_use = used = true; + } } /** Check whether it fetches mariadb table for the first time. diff --git a/storage/innobase/row/row0purge.cc b/storage/innobase/row/row0purge.cc index 8fe0a59eb3c..59387f926ff 100644 --- a/storage/innobase/row/row0purge.cc +++ b/storage/innobase/row/row0purge.cc @@ -345,6 +345,7 @@ retry_purge_sec: if (node->vcol_info.is_first_fetch()) { if (node->vcol_info.mariadb_table) { + node->vcol_info.set_used(); goto retry_purge_sec; } From a7a0c533c20531aa658941a885a61b96f22110b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Mon, 23 Jul 2018 17:50:56 +0300 Subject: [PATCH 6/8] Follow-up to MDEV-15855: Remove bogus debug assertions During a table-rebuilding operation, the function table_name_parse() can encounter a table name that starts with #sql. Here is an example of a failure: CURRENT_TEST: gcol.innodb_virtual_basic mysqltest: At line 1204: query 'alter table t drop column d ' failed: 2013: Lost connection to MySQL server during query Let us just remove these bogus debug assertions. If the final renaming phase during ALTER TABLE never fails, it should not do any harm to skip the purge. If it does fail, then we might end up 'leaking' some delete-marked records in the indexes on virtual columns of the original table, and these garbage records would keep consuming space until the indexes are dropped or the table is successfully rebuilt. --- storage/innobase/handler/ha_innodb.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index deb6f05e853..7000865a7cd 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -21661,7 +21661,6 @@ static TABLE* innodb_acquire_mdl(THD* thd, dict_table_t* table) if (!table_name_parse(table->name, db_buf, tbl_buf, db_buf_len, tbl_buf_len)) { - ut_ad(!"invalid table name"); return NULL; } @@ -21697,7 +21696,6 @@ fail: if (!table_name_parse(table->name, db_buf1, tbl_buf1, db_buf1_len, tbl_buf1_len)) { - ut_ad(!"invalid table name"); goto release_fail; } @@ -21745,7 +21743,6 @@ static TABLE* innodb_find_table_for_vc(THD* thd, dict_table_t* table) if (!table_name_parse(table->name, db_buf, tbl_buf, db_buf_len, tbl_buf_len)) { - ut_ad(!"invalid table name"); return NULL; } From c5ba13dda0464b1316bfb45efbcdcc924817f0de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Mon, 23 Jul 2018 18:23:54 +0300 Subject: [PATCH 7/8] MDEV-15855 cleanup: Privatize purge_vcol_info_t Declare all fields of purge_vcol_info_t private, and add accessor functions. --- storage/innobase/include/row0types.h | 36 ++++++++++++++++++++++++++++ storage/innobase/row/row0purge.cc | 16 +++++++------ storage/innobase/row/row0vers.cc | 11 ++++----- 3 files changed, 50 insertions(+), 13 deletions(-) diff --git a/storage/innobase/include/row0types.h b/storage/innobase/include/row0types.h index 005dc2ad95f..d2aef89f695 100644 --- a/storage/innobase/include/row0types.h +++ b/storage/innobase/include/row0types.h @@ -56,6 +56,7 @@ struct TABLE; /** Purge virtual column node information. */ struct purge_vcol_info_t { +private: /** Is there a possible need to evaluate virtual columns? */ bool requested; /** Do we have to evaluate virtual columns (using mariadb_table)? */ @@ -67,6 +68,7 @@ struct purge_vcol_info_t /** MariaDB table opened for virtual column computation. */ TABLE* mariadb_table; +public: /** Reset the state. */ void reset() { @@ -81,6 +83,29 @@ struct purge_vcol_info_t or doesn't try to calculate virtual column. */ bool validate() const { return !used || mariadb_table; } + /** @return the table handle for evaluating virtual columns */ + TABLE* table() const { return mariadb_table; } + + /** Set the table handle for evaluating virtual columns. + @param[in] table table handle */ + void set_table(TABLE* table) + { + ut_ad(!table || is_first_fetch()); + mariadb_table = table; + } + + /** Note that virtual column information may be needed. */ + void set_requested() + { + ut_ad(!used); + ut_ad(!first_use); + ut_ad(!mariadb_table); + requested = true; + } + + /** @return whether the virtual column information may be needed */ + bool is_requested() const { return requested; } + /** Note that the virtual column information is needed. */ void set_used() { @@ -97,11 +122,22 @@ struct purge_vcol_info_t } } + /** @return whether the virtual column information is needed */ + bool is_used() const + { + ut_ad(!first_use || used); + ut_ad(!used || requested); + ut_ad(used || !mariadb_table); + return used; + } + /** Check whether it fetches mariadb table for the first time. @return true if first time tries to open mariadb table. */ bool is_first_fetch() const { ut_ad(!first_use || used); + ut_ad(!used || requested); + ut_ad(used || !mariadb_table); return first_use; } }; diff --git a/storage/innobase/row/row0purge.cc b/storage/innobase/row/row0purge.cc index 59387f926ff..cc2dcc9b798 100644 --- a/storage/innobase/row/row0purge.cc +++ b/storage/innobase/row/row0purge.cc @@ -137,7 +137,7 @@ row_purge_remove_clust_if_poss_low( rec_offs_init(offsets_); ut_ad(rw_lock_own(dict_operation_lock, RW_LOCK_S) - || node->vcol_info.used); + || node->vcol_info.is_used()); index = dict_table_get_first_index(node->table); @@ -250,7 +250,7 @@ static void row_purge_store_vsec_cur( return; } - node->vcol_info.requested = true; + node->vcol_info.set_requested(); btr_pcur_store_position(sec_pcur, sec_mtr); @@ -318,7 +318,7 @@ row_purge_poss_sec( ut_ad(!dict_index_is_clust(index)); - const bool store_cur = sec_mtr && !node->vcol_info.used + const bool store_cur = sec_mtr && !node->vcol_info.is_used() && dict_index_has_virtual(index); if (store_cur) { @@ -327,7 +327,7 @@ row_purge_poss_sec( /* The PRIMARY KEY value was not found in the clustered index. The secondary index record found. We can purge the secondary index record. */ - if (!node->vcol_info.requested) { + if (!node->vcol_info.is_requested()) { ut_ad(!node->found_clust); return true; } @@ -344,7 +344,9 @@ retry_purge_sec: &node->vcol_info); if (node->vcol_info.is_first_fetch()) { - if (node->vcol_info.mariadb_table) { + ut_ad(store_cur); + + if (node->vcol_info.table()) { node->vcol_info.set_used(); goto retry_purge_sec; } @@ -801,7 +803,7 @@ row_purge_upd_exist_or_extern_func( mem_heap_t* heap; ut_ad(rw_lock_own(dict_operation_lock, RW_LOCK_S) - || node->vcol_info.used); + || node->vcol_info.is_used()); ut_ad(!node->table->skip_alter_undo); if (node->rec_type == TRX_UNDO_UPD_DEL_REC @@ -1139,7 +1141,7 @@ row_purge( bool purged = row_purge_record( node, undo_rec, thr, updated_extern); - if (!node->vcol_info.used) { + if (!node->vcol_info.is_used()) { rw_lock_s_unlock(dict_operation_lock); } diff --git a/storage/innobase/row/row0vers.cc b/storage/innobase/row/row0vers.cc index 9ced22fd54b..f58f0a47bd5 100644 --- a/storage/innobase/row/row0vers.cc +++ b/storage/innobase/row/row0vers.cc @@ -457,7 +457,7 @@ row_vers_build_clust_v_col( if (vcol_info != NULL) { vcol_info->set_used(); - maria_table = vcol_info->mariadb_table; + maria_table = vcol_info->table(); } innobase_allocate_row_for_vcol(thd, index, @@ -466,9 +466,8 @@ row_vers_build_clust_v_col( &record, &vcol_storage); - if (vcol_info && !vcol_info->mariadb_table) { - vcol_info->mariadb_table = maria_table; - ut_ad(!maria_table || vcol_info->is_first_fetch()); + if (vcol_info && !vcol_info->table()) { + vcol_info->set_table(maria_table); goto func_exit; } @@ -834,7 +833,7 @@ row_vers_build_cur_vrow( rec, *clust_offsets, NULL, NULL, NULL, NULL, heap); - if (vcol_info && !vcol_info->used) { + if (vcol_info && !vcol_info->is_used()) { mtr->commit(); } @@ -955,7 +954,7 @@ row_vers_old_has_index_entry( if (trx_undo_roll_ptr_is_insert(t_roll_ptr) || dbug_v_purge) { - if (vcol_info && !vcol_info->used) { + if (vcol_info && !vcol_info->is_used()) { mtr->commit(); } From 9d1f3bf2e9425db8e352ee80e7b456dd6ef73fcb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Mon, 23 Jul 2018 18:23:54 +0300 Subject: [PATCH 8/8] row_purge_poss_sec(): Add debug instrumentation This helped fix MDEV-16779. --- storage/innobase/row/row0purge.cc | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/storage/innobase/row/row0purge.cc b/storage/innobase/row/row0purge.cc index cc2dcc9b798..7e7dd794f1b 100644 --- a/storage/innobase/row/row0purge.cc +++ b/storage/innobase/row/row0purge.cc @@ -323,6 +323,8 @@ row_purge_poss_sec( if (store_cur) { row_purge_store_vsec_cur(node, index, sec_pcur, sec_mtr); + ut_ad(sec_mtr->has_committed() + == node->vcol_info.is_requested()); /* The PRIMARY KEY value was not found in the clustered index. The secondary index record found. We can purge @@ -346,7 +348,15 @@ retry_purge_sec: if (node->vcol_info.is_first_fetch()) { ut_ad(store_cur); - if (node->vcol_info.table()) { + const TABLE* t= node->vcol_info.table(); + DBUG_LOG("purge", "retry " << t + << (is_tree ? " tree" : " leaf") + << index->name << "," << index->table->name + << ": " << rec_printer(entry).str()); + + ut_ad(mtr.has_committed()); + + if (t) { node->vcol_info.set_used(); goto retry_purge_sec; } @@ -360,9 +370,11 @@ retry_purge_sec: if (node->found_clust) { btr_pcur_commit_specify_mtr(&node->pcur, &mtr); } else { - mtr_commit(&mtr); + mtr.commit(); } + ut_ad(mtr.has_committed()); + if (store_cur && !row_purge_restore_vsec_cur( node, index, sec_pcur, sec_mtr, is_tree)) { return false;