diff options
| -rw-r--r-- | mysql-test/suite/gcol/r/virtual_index_drop.result | 69 | ||||
| -rw-r--r-- | mysql-test/suite/gcol/t/virtual_index_drop.test | 71 | ||||
| -rw-r--r-- | storage/innobase/dict/dict0dict.cc | 2 | ||||
| -rw-r--r-- | storage/innobase/dict/dict0mem.cc | 2 | ||||
| -rw-r--r-- | storage/innobase/handler/handler0alter.cc | 75 | ||||
| -rw-r--r-- | storage/innobase/include/dict0mem.h | 81 | ||||
| -rw-r--r-- | storage/innobase/include/row0merge.h | 20 | ||||
| -rw-r--r-- | storage/innobase/row/row0merge.cc | 29 | ||||
| -rw-r--r-- | storage/innobase/row/row0purge.cc | 2 |
9 files changed, 300 insertions, 51 deletions
diff --git a/mysql-test/suite/gcol/r/virtual_index_drop.result b/mysql-test/suite/gcol/r/virtual_index_drop.result new file mode 100644 index 00000000000..012e61be459 --- /dev/null +++ b/mysql-test/suite/gcol/r/virtual_index_drop.result @@ -0,0 +1,69 @@ +# +# MDEV-24971 InnoDB access freed virtual column +# after rollback of secondary index +# +CREATE TABLE t1(f1 INT, f2 INT AS (f1 + 2) VIRTUAL)ENGINE=InnoDB; +INSERT INTO t1(f1) VALUES(1), (1); +ALTER TABLE t1 ADD UNIQUE INDEX(f2), ALGORITHM=INPLACE, LOCK=EXCLUSIVE; +ERROR 23000: Duplicate entry '3' for key 'f2' +SHOW CREATE TABLE t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `f1` int(11) DEFAULT NULL, + `f2` int(11) GENERATED ALWAYS AS (`f1` + 2) VIRTUAL +) ENGINE=InnoDB DEFAULT CHARSET=latin1 +DROP TABLE t1; +CREATE TABLE t1(f1 INT, f2 INT AS (f1 + 2) VIRTUAL)ENGINE=InnoDB; +INSERT INTO t1(f1) VALUES(1), (1); +ALTER TABLE t1 ADD UNIQUE INDEX(f2), ALGORITHM=INPLACE, LOCK=SHARED; +ERROR 23000: Duplicate entry '3' for key 'f2' +SHOW CREATE TABLE t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `f1` int(11) DEFAULT NULL, + `f2` int(11) GENERATED ALWAYS AS (`f1` + 2) VIRTUAL +) ENGINE=InnoDB DEFAULT CHARSET=latin1 +DROP TABLE t1; +CREATE TABLE t1(f1 INT, f2 INT AS (f1) VIRTUAL)ENGINE=InnoDB; +SET DEBUG_DBUG="+d,create_index_fail"; +SET DEBUG_SYNC="innodb_inplace_alter_table_enter SIGNAL con1_go WAIT_FOR alter_signal"; +ALTER TABLE t1 ADD COLUMN f3 INT AS (f1) VIRTUAL, ADD INDEX(f2, f3); +connect con1,localhost,root,,,; +SET DEBUG_SYNC="now WAIT_FOR con1_go"; +BEGIN; +SELECT * FROM t1; +f1 f2 +SET DEBUG_SYNC="now SIGNAL alter_signal"; +connection default; +ERROR 23000: Duplicate entry '' for key '*UNKNOWN*' +connection con1; +rollback; +connection default; +SHOW CREATE TABLE t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `f1` int(11) DEFAULT NULL, + `f2` int(11) GENERATED ALWAYS AS (`f1`) VIRTUAL +) ENGINE=InnoDB DEFAULT CHARSET=latin1 +DROP TABLE t1; +CREATE TABLE t1(f1 INT, f2 INT AS (f1) VIRTUAL)ENGINE=InnoDB; +SET DEBUG_DBUG="+d,create_index_fail"; +SET DEBUG_SYNC="innodb_inplace_alter_table_enter SIGNAL con1_go WAIT_FOR alter_signal"; +ALTER TABLE t1 ADD INDEX(f2); +connection con1; +SET DEBUG_SYNC="now WAIT_FOR con1_go"; +BEGIN; +INSERT INTO t1(f1) VALUES(1); +SET DEBUG_SYNC="now SIGNAL alter_signal"; +connection default; +ERROR 23000: Duplicate entry '' for key '*UNKNOWN*' +connection con1; +rollback; +connection default; +disconnect con1; +DROP TABLE t1; +CREATE TABLE t1(f1 CHAR(100), f2 CHAR(100) as (f1) VIRTUAL)ENGINE=InnoDB; +ALTER TABLE t1 ADD COLUMN f3 CHAR(100) AS (f2) VIRTUAL, ADD INDEX(f3(10), f1, f3(12)); +ERROR 42S21: Duplicate column name 'f3' +DROP TABLE t1; +SET DEBUG_SYNC=RESET; diff --git a/mysql-test/suite/gcol/t/virtual_index_drop.test b/mysql-test/suite/gcol/t/virtual_index_drop.test new file mode 100644 index 00000000000..016832b9e6d --- /dev/null +++ b/mysql-test/suite/gcol/t/virtual_index_drop.test @@ -0,0 +1,71 @@ +--source include/have_innodb.inc +--source include/have_debug.inc + +--echo # +--echo # MDEV-24971 InnoDB access freed virtual column +--echo # after rollback of secondary index +--echo # + +# Exclusive lock must not defer the index removal + +CREATE TABLE t1(f1 INT, f2 INT AS (f1 + 2) VIRTUAL)ENGINE=InnoDB; +INSERT INTO t1(f1) VALUES(1), (1); +--error ER_DUP_ENTRY +ALTER TABLE t1 ADD UNIQUE INDEX(f2), ALGORITHM=INPLACE, LOCK=EXCLUSIVE; +SHOW CREATE TABLE t1; +DROP TABLE t1; + +# If Shared lock and table doesn't have any other open handle +# then InnoDB must not defer the index removal + +CREATE TABLE t1(f1 INT, f2 INT AS (f1 + 2) VIRTUAL)ENGINE=InnoDB; +INSERT INTO t1(f1) VALUES(1), (1); +--error ER_DUP_ENTRY +ALTER TABLE t1 ADD UNIQUE INDEX(f2), ALGORITHM=INPLACE, LOCK=SHARED; +SHOW CREATE TABLE t1; +DROP TABLE t1; + +# InnoDB should store the newly dropped virtual column into +# new_vcol_info in index when rollback of alter happens + +CREATE TABLE t1(f1 INT, f2 INT AS (f1) VIRTUAL)ENGINE=InnoDB; +SET DEBUG_DBUG="+d,create_index_fail"; +SET DEBUG_SYNC="innodb_inplace_alter_table_enter SIGNAL con1_go WAIT_FOR alter_signal"; +SEND ALTER TABLE t1 ADD COLUMN f3 INT AS (f1) VIRTUAL, ADD INDEX(f2, f3); +connect(con1,localhost,root,,,); +SET DEBUG_SYNC="now WAIT_FOR con1_go"; +BEGIN; +SELECT * FROM t1; +SET DEBUG_SYNC="now SIGNAL alter_signal"; +connection default; +--error ER_DUP_ENTRY +reap; +connection con1; +rollback; +connection default; +SHOW CREATE TABLE t1; +DROP TABLE t1; + +CREATE TABLE t1(f1 INT, f2 INT AS (f1) VIRTUAL)ENGINE=InnoDB; +SET DEBUG_DBUG="+d,create_index_fail"; +SET DEBUG_SYNC="innodb_inplace_alter_table_enter SIGNAL con1_go WAIT_FOR alter_signal"; +send ALTER TABLE t1 ADD INDEX(f2); +connection con1; +SET DEBUG_SYNC="now WAIT_FOR con1_go"; +BEGIN; +INSERT INTO t1(f1) VALUES(1); +SET DEBUG_SYNC="now SIGNAL alter_signal"; +connection default; +--error ER_DUP_ENTRY +reap; +connection con1; +rollback; +connection default; +disconnect con1; +DROP TABLE t1; + +CREATE TABLE t1(f1 CHAR(100), f2 CHAR(100) as (f1) VIRTUAL)ENGINE=InnoDB; +--error ER_DUP_FIELDNAME +ALTER TABLE t1 ADD COLUMN f3 CHAR(100) AS (f2) VIRTUAL, ADD INDEX(f3(10), f1, f3(12)); +DROP TABLE t1; +SET DEBUG_SYNC=RESET; diff --git a/storage/innobase/dict/dict0dict.cc b/storage/innobase/dict/dict0dict.cc index 6f546dfbd94..d6330cb5906 100644 --- a/storage/innobase/dict/dict0dict.cc +++ b/storage/innobase/dict/dict0dict.cc @@ -297,7 +297,7 @@ dict_table_try_drop_aborted( && !UT_LIST_GET_FIRST(table->locks)) { /* Silence a debug assertion in row_merge_drop_indexes(). */ ut_d(table->acquire()); - row_merge_drop_indexes(trx, table, TRUE); + row_merge_drop_indexes(trx, table, true); ut_d(table->release()); ut_ad(table->get_ref_count() == ref_count); trx_commit_for_mysql(trx); diff --git a/storage/innobase/dict/dict0mem.cc b/storage/innobase/dict/dict0mem.cc index efefa40e69f..1667e1a0b20 100644 --- a/storage/innobase/dict/dict0mem.cc +++ b/storage/innobase/dict/dict0mem.cc @@ -911,7 +911,7 @@ dict_mem_fill_vcol_from_v_indexes( Later virtual column set will be refreshed during loading of table. */ if (!dict_index_has_virtual(index) - || index->has_new_v_col) { + || index->has_new_v_col()) { continue; } diff --git a/storage/innobase/handler/handler0alter.cc b/storage/innobase/handler/handler0alter.cc index ccc3fe3b921..1f6dbe1eda9 100644 --- a/storage/innobase/handler/handler0alter.cc +++ b/storage/innobase/handler/handler0alter.cc @@ -248,13 +248,6 @@ struct ha_innobase_inplace_ctx : public inplace_alter_handler_ctx @return whether the table will be rebuilt */ bool need_rebuild () const { return(old_table != new_table); } - /** Clear uncommmitted added indexes after a failed operation. */ - void clear_added_indexes() - { - for (ulint i= 0; i < num_to_add_index; i++) - add_index[i]->detach_columns(true); - } - /** Share context between partitions. @param[in] ctx context from another partition of the table */ void set_shared_data(const inplace_alter_handler_ctx& ctx) @@ -272,6 +265,45 @@ struct ha_innobase_inplace_ctx : public inplace_alter_handler_ctx } } + /** @return whether the given column is being added */ + bool is_new_vcol(const dict_v_col_t &v_col) const + { + for (ulint i= 0; i < num_to_add_vcol; i++) + if (&add_vcol[i] == &v_col) + return true; + return false; + } + + /** During rollback, make newly added indexes point to + newly added virtual columns. */ + void clean_new_vcol_index() + { + ut_ad(old_table == new_table); + const dict_index_t *index= dict_table_get_first_index(old_table); + while ((index= dict_table_get_next_index(index)) != NULL) + { + if (!index->has_virtual() || index->is_committed()) + continue; + ulint n_drop_new_vcol= index->get_new_n_vcol(); + for (ulint i= 0; n_drop_new_vcol && i < index->n_fields; i++) + { + dict_col_t *col= index->fields[i].col; + /* Skip the non-virtual and old virtual columns */ + if (!col->is_virtual()) + continue; + dict_v_col_t *vcol= reinterpret_cast<dict_v_col_t*>(col); + if (!is_new_vcol(*vcol)) + continue; + + dict_v_col_t *drop_vcol= index->new_vcol_info-> + add_drop_v_col(index->heap, vcol, n_drop_new_vcol); + /* Re-assign the index field with newly stored virtual column */ + index->fields[i].col= reinterpret_cast<dict_col_t*>(drop_vcol); + n_drop_new_vcol--; + } + } + } + private: // Disable copying ha_innobase_inplace_ctx(const ha_innobase_inplace_ctx&); @@ -2722,7 +2754,7 @@ online_retry_drop_indexes_low( ut_ad(table->get_ref_count() >= 1); if (table->drop_aborted) { - row_merge_drop_indexes(trx, table, TRUE); + row_merge_drop_indexes(trx, table, true); } } @@ -5146,7 +5178,7 @@ error_handled: online_retry_drop_indexes_with_trx(user_table, ctx->trx); } else { ut_ad(!ctx->need_rebuild()); - row_merge_drop_indexes(ctx->trx, user_table, TRUE); + row_merge_drop_indexes(ctx->trx, user_table, true); trx_commit_for_mysql(ctx->trx); } @@ -6388,7 +6420,6 @@ oom: that we hold at most a shared lock on the table. */ m_prebuilt->trx->error_info = NULL; ctx->trx->error_state = DB_SUCCESS; - ctx->clear_added_indexes(); DBUG_RETURN(true); } @@ -6483,17 +6514,18 @@ temparary index prefix @param table the TABLE @param locked TRUE=table locked, FALSE=may need to do a lazy drop @param trx the transaction -*/ -static MY_ATTRIBUTE((nonnull)) +@param alter_trx transaction which takes S-lock on the table + while creating the index */ +static void innobase_rollback_sec_index( -/*========================*/ - dict_table_t* user_table, - const TABLE* table, - ibool locked, - trx_t* trx) + dict_table_t* user_table, + const TABLE* table, + bool locked, + trx_t* trx, + const trx_t* alter_trx=NULL) { - row_merge_drop_indexes(trx, user_table, locked); + row_merge_drop_indexes(trx, user_table, locked, alter_trx); /* Free the table->fts only if there is no FTS_DOC_ID in the table */ @@ -6587,7 +6619,12 @@ rollback_inplace_alter_table( DBUG_ASSERT(ctx->new_table == prebuilt->table); innobase_rollback_sec_index( - prebuilt->table, table, FALSE, ctx->trx); + prebuilt->table, table, + (ha_alter_info->alter_info->requested_lock + == Alter_info::ALTER_TABLE_LOCK_EXCLUSIVE), + ctx->trx, prebuilt->trx); + + ctx->clean_new_vcol_index(); } trx_commit_for_mysql(ctx->trx); diff --git a/storage/innobase/include/dict0mem.h b/storage/innobase/include/dict0mem.h index a4defe4d92a..7ca1ad9ecd3 100644 --- a/storage/innobase/include/dict0mem.h +++ b/storage/innobase/include/dict0mem.h @@ -671,6 +671,35 @@ struct dict_v_col_t{ }; +/** Data structure for newly added virtual column in a index. +It is used only during rollback_inplace_alter_table() of +addition of index depending on newly added virtual columns +and uses index heap. Should be freed when index is being +removed from cache. */ +struct dict_add_v_col_info +{ + ulint n_v_col; + dict_v_col_t *v_col; + + /** Add the newly added virtual column while rollbacking + the index which contains new virtual columns + @param col virtual column to be duplicated + @param offset offset where to duplicate virtual column */ + dict_v_col_t* add_drop_v_col(mem_heap_t *heap, dict_v_col_t *col, + ulint offset) + { + ut_ad(n_v_col); + ut_ad(offset < n_v_col); + if (!v_col) + v_col= static_cast<dict_v_col_t*> + (mem_heap_alloc(heap, n_v_col * sizeof *v_col)); + new (&v_col[offset]) dict_v_col_t(); + v_col[offset].m_col= col->m_col; + v_col[offset].v_pos= col->v_pos; + return &v_col[offset]; + } +}; + /** Data structure for newly added virtual column in a table */ struct dict_add_v_col_t{ /** number of new virtual column */ @@ -892,9 +921,13 @@ struct dict_index_t{ dict_field_t* fields; /*!< array of field descriptions */ st_mysql_ftparser* parser; /*!< fulltext parser plugin */ - bool has_new_v_col; - /*!< whether it has a newly added virtual - column in ALTER */ + + /** It just indicates whether newly added virtual column + during alter. It stores column in case of alter failure. + It should use heap from dict_index_t. It should be freed + while removing the index from table. */ + dict_add_v_col_info* new_vcol_info; + bool index_fts_syncing;/*!< Whether the fts index is still syncing in the background; FIXME: remove this and use MDL */ @@ -1028,9 +1061,8 @@ struct dict_index_t{ /** @return whether the index is corrupted */ inline bool is_corrupted() const; - /** Detach the virtual columns from the index that is to be removed. - @param whether to reset fields[].col */ - void detach_columns(bool clear= false) + /** Detach the virtual columns from the index that is to be removed. */ + void detach_columns() { if (!has_virtual()) return; @@ -1040,14 +1072,36 @@ struct dict_index_t{ if (!col || !col->is_virtual()) continue; col->detach(*this); - if (clear) - fields[i].col= NULL; } } /** @return whether this is the change buffer */ bool is_ibuf() const { return UNIV_UNLIKELY(type & DICT_IBUF); } + /** Assign the number of new column to be added as a part + of the index + @param n_vcol number of virtual columns to be added */ + void assign_new_v_col(ulint n_vcol) + { + new_vcol_info= static_cast<dict_add_v_col_info*>( + mem_heap_zalloc(heap, sizeof *new_vcol_info)); + new_vcol_info->n_v_col= n_vcol; + } + + /* @return whether index has new virtual column */ + bool has_new_v_col() const + { + return new_vcol_info != NULL; + } + + /* @return number of newly added virtual column */ + ulint get_new_n_vcol() const + { + if (new_vcol_info) + return new_vcol_info->n_v_col; + return 0; + } + #ifdef BTR_CUR_HASH_ADAPT /** @return a clone of this */ dict_index_t* clone() const; @@ -1870,6 +1924,17 @@ public: /** mysql_row_templ_t for base columns used for compute the virtual columns */ dict_vcol_templ_t* vc_templ; + + /* @return whether the table has any other transcation lock + other than the given transaction */ + bool has_lock_other_than(const trx_t *trx) const + { + for (lock_t *lock= UT_LIST_GET_FIRST(locks); lock; + lock= UT_LIST_GET_NEXT(un_member.tab_lock.locks, lock)) + if (lock->trx != trx) + return true; + return false; + } }; inline bool table_name_t::is_temporary() const diff --git a/storage/innobase/include/row0merge.h b/storage/innobase/include/row0merge.h index 0d48fbd2e8a..dfd1d9fb9fd 100644 --- a/storage/innobase/include/row0merge.h +++ b/storage/innobase/include/row0merge.h @@ -167,18 +167,20 @@ row_merge_drop_indexes_dict( table_id_t table_id)/*!< in: table identifier */ MY_ATTRIBUTE((nonnull)); -/*********************************************************************//** -Drop those indexes which were created before an error occurred. +/** Drop indexes that were created before an error occurred. The data dictionary must have been locked exclusively by the caller, -because the transaction will not be committed. */ +because the transaction will not be committed. +@param trx dictionary transaction +@param table table containing the indexes +@param locked True if table is locked, + false - may need to do lazy drop +@param alter_trx Alter table transaction */ void row_merge_drop_indexes( -/*===================*/ - trx_t* trx, /*!< in/out: transaction */ - dict_table_t* table, /*!< in/out: table containing the indexes */ - ibool locked) /*!< in: TRUE=table locked, - FALSE=may need to do a lazy drop */ - MY_ATTRIBUTE((nonnull)); + trx_t* trx, + dict_table_t* table, + bool locked, + const trx_t* alter_trx=NULL); /*********************************************************************//** Drop all partially created indexes during crash recovery. */ diff --git a/storage/innobase/row/row0merge.cc b/storage/innobase/row/row0merge.cc index 835d74043fd..e3b3f2c2762 100644 --- a/storage/innobase/row/row0merge.cc +++ b/storage/innobase/row/row0merge.cc @@ -3696,17 +3696,20 @@ row_merge_drop_indexes_dict( trx->op_info = ""; } -/*********************************************************************//** -Drop indexes that were created before an error occurred. +/** Drop indexes that were created before an error occurred. The data dictionary must have been locked exclusively by the caller, -because the transaction will not be committed. */ +because the transaction will not be committed. +@param trx dictionary transaction +@param table table containing the indexes +@param locked True if table is locked, + false - may need to do lazy drop +@param alter_trx Alter table transaction */ void row_merge_drop_indexes( -/*===================*/ - trx_t* trx, /*!< in/out: dictionary transaction */ - dict_table_t* table, /*!< in/out: table containing the indexes */ - ibool locked) /*!< in: TRUE=table locked, - FALSE=may need to do a lazy drop */ + trx_t* trx, + dict_table_t* table, + bool locked, + const trx_t* alter_trx) { dict_index_t* index; dict_index_t* next_index; @@ -3732,7 +3735,7 @@ row_merge_drop_indexes( A concurrent purge will be prevented by dict_operation_lock. */ if (!locked && (table->get_ref_count() > 1 - || UT_LIST_GET_FIRST(table->locks))) { + || table->has_lock_other_than(alter_trx))) { /* We will have to drop the indexes later, when the table is guaranteed to be no longer in use. Mark the indexes as incomplete and corrupted, so that other @@ -4363,7 +4366,7 @@ row_merge_create_index( dberr_t err; ulint n_fields = index_def->n_fields; ulint i; - bool has_new_v_col = false; + ulint n_add_vcol = 0; DBUG_ENTER("row_merge_create_index"); @@ -4391,7 +4394,7 @@ row_merge_create_index( ut_ad(ifield->col_no >= table->n_v_def); name = add_v->v_col_name[ ifield->col_no - table->n_v_def]; - has_new_v_col = true; + n_add_vcol++; } else { name = dict_table_get_v_col_name( table, ifield->col_no); @@ -4410,7 +4413,9 @@ row_merge_create_index( if (err == DB_SUCCESS) { ut_ad(index != index_template); index->parser = index_def->parser; - index->has_new_v_col = has_new_v_col; + if (n_add_vcol) { + index->assign_new_v_col(n_add_vcol); + } /* Note the id of the transaction that created this index, we use it to restrict readers from accessing this index, to ensure read consistency. */ diff --git a/storage/innobase/row/row0purge.cc b/storage/innobase/row/row0purge.cc index e79784a5a5f..024625e9e0a 100644 --- a/storage/innobase/row/row0purge.cc +++ b/storage/innobase/row/row0purge.cc @@ -729,7 +729,7 @@ row_purge_skip_uncommitted_virtual_index( not support LOCK=NONE when adding an index on newly added virtual column.*/ while (index != NULL && dict_index_has_virtual(index) - && !index->is_committed() && index->has_new_v_col) { + && !index->is_committed() && index->has_new_v_col()) { index = dict_table_get_next_index(index); } } |