commit 6d6f012535534c5436ebe76ef0b305f97cf16ca5 Author: Aleksey Midenkov Date: Tue Dec 4 11:20:12 2018 +0300 MDEV-17520 Secondary indexes rebuild when affected by nullable column * STL iterators: dict_index_iterator, altered_nullable_index_iterator, array_iterator. * instant_alter_indexes_possible(): check if all nullable-affected table indexes are already in DROP INDEX clause. * If some indexes are affected, prohibit ALGORITHM=INSTANT. * Create_field::altered indicated whether field was modified by ALTER. * Alter_inplace_info::index_rebuild_count displays count of affected indexes. * FK checking is excluded for rebuilt indexes. * dict_index_t::to_be_rebuilt indicates affected index. diff --git a/mysql-test/suite/innodb/r/instant_alter_null.result b/mysql-test/suite/innodb/r/instant_alter_null.result index f49d60fc301..70f807d44d9 100644 --- a/mysql-test/suite/innodb/r/instant_alter_null.result +++ b/mysql-test/suite/innodb/r/instant_alter_null.result @@ -2,14 +2,6 @@ create table t (a int NOT NULL) engine=innodb row_format= compressed; alter table t modify a int NULL, algorithm=instant; ERROR 0A000: ALGORITHM=INSTANT is not supported for this operation. Try ALGORITHM=INPLACE drop table t; -create table t (a int NOT NULL) engine=innodb row_format= dynamic; -alter table t modify a int NULL, algorithm=instant; -ERROR 0A000: ALGORITHM=INSTANT is not supported for this operation. Try ALGORITHM=INPLACE -drop table t; -create table t (a int NOT NULL) engine=innodb row_format= compact; -alter table t modify a int NULL, algorithm=instant; -ERROR 0A000: ALGORITHM=INSTANT is not supported for this operation. Try ALGORITHM=INPLACE -drop table t; create table t ( id int primary key, a int NOT NULL default 0, @@ -54,3 +46,33 @@ check table t; Table Op Msg_type Msg_text test.t check status OK drop table t; +create table t ( +a int, +b int not null, +c int, +index (a), +index (b), +index (c), +index abc (a, b, c) +) engine=innodb; +alter table t drop index a, drop index c, drop index abc, drop index b, modify b int, algorithm=instant; +check table t; +Table Op Msg_type Msg_text +test.t check status OK +drop table t; +create table t ( +a int, +b int not null, +c int, +index (a), +index (b), +index (c), +index abc (a, b, c) +) engine=innodb; +insert into t values (1, 1, 1); +alter table t modify b int, algorithm=inplace; +update t set b= null; +check table t; +Table Op Msg_type Msg_text +test.t check status OK +drop table t; diff --git a/mysql-test/suite/innodb/t/instant_alter_null.test b/mysql-test/suite/innodb/t/instant_alter_null.test index 69fb1ae4495..133382710ef 100644 --- a/mysql-test/suite/innodb/t/instant_alter_null.test +++ b/mysql-test/suite/innodb/t/instant_alter_null.test @@ -5,16 +5,6 @@ create table t (a int NOT NULL) engine=innodb row_format= compressed; alter table t modify a int NULL, algorithm=instant; drop table t; -create table t (a int NOT NULL) engine=innodb row_format= dynamic; ---error ER_ALTER_OPERATION_NOT_SUPPORTED -alter table t modify a int NULL, algorithm=instant; -drop table t; - -create table t (a int NOT NULL) engine=innodb row_format= compact; ---error ER_ALTER_OPERATION_NOT_SUPPORTED -alter table t modify a int NULL, algorithm=instant; -drop table t; - create table t ( id int primary key, a int NOT NULL default 0, @@ -55,3 +45,47 @@ select * from t; check table t; drop table t; + +create table t ( + a int, + b int not null, + c int, + index (a), + index (b), + index (c), + index abc (a, b, c) +) engine=innodb; + +if (!$MTR_COMBINATION_REDUNDANT) +{ +--disable_query_log +--disable_result_log +--error ER_ALTER_OPERATION_NOT_SUPPORTED +alter table t drop index a, drop index c, modify b int, algorithm=instant; +--error ER_ALTER_OPERATION_NOT_SUPPORTED +alter table t drop index b, modify b int, algorithm=instant; +--enable_query_log +--enable_result_log +} + +alter table t drop index a, drop index c, drop index abc, drop index b, modify b int, algorithm=instant; + +check table t; +drop table t; + +create table t ( + a int, + b int not null, + c int, + index (a), + index (b), + index (c), + index abc (a, b, c) +) engine=innodb; + +insert into t values (1, 1, 1); +alter table t modify b int, algorithm=inplace; +update t set b= null; + +check table t; +drop table t; diff --git a/sql/field.h b/sql/field.h index 97926a356d1..a6752d17e29 100644 --- a/sql/field.h +++ b/sql/field.h @@ -4924,11 +4924,12 @@ class Create_field :public Column_definition uint offset; uint8 interval_id; // For rea_create_table bool create_if_not_exists; // Used in ALTER TABLE IF NOT EXISTS + bool altered; // listed in an ALTER TABLE statement Create_field(): Column_definition(), field(0), option_struct(NULL), - create_if_not_exists(false) + create_if_not_exists(false), altered(false) { change= after= null_clex_str; } @@ -4936,7 +4937,7 @@ class Create_field :public Column_definition Column_definition(thd, old_field, orig_field), change(old_field->field_name), field(old_field), option_struct(old_field->option_struct), - create_if_not_exists(false) + create_if_not_exists(false), altered(false) { after= null_clex_str; } diff --git a/sql/handler.h b/sql/handler.h index e80b963cdc1..065218953ff 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -2379,6 +2379,11 @@ class Alter_inplace_info */ inplace_alter_handler_ctx *handler_ctx; + /** + Pre-context atributes used while handler_ctx is not yet available + */ + uint index_rebuild_count; + /** If the table uses several handlers, like ha_partition uses one handler per partition, this contains a Null terminated array of ctx pointers @@ -2442,6 +2447,7 @@ class Alter_inplace_info index_add_count(0), index_add_buffer(NULL), handler_ctx(NULL), + index_rebuild_count(0), group_commit_ctx(NULL), handler_flags(0), modified_part_info(modified_part_info_arg), diff --git a/sql/sql_table.cc b/sql/sql_table.cc index edd0b95fca0..6e027f0fe79 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -8066,6 +8066,7 @@ mysql_prepare_alter_table(THD *thd, TABLE *table, if (def && field->invisible < INVISIBLE_SYSTEM) { // Field is changed def->field=field; + def->altered= true; /* Add column being updated to the list of new columns. Note that columns with AFTER clauses are added to the end diff --git a/storage/innobase/dict/dict0stats.cc b/storage/innobase/dict/dict0stats.cc index f4b05766e79..7dee6f4ea16 100644 --- a/storage/innobase/dict/dict0stats.cc +++ b/storage/innobase/dict/dict0stats.cc @@ -449,6 +449,7 @@ dict_stats_table_clone_create( idx->type = index->type; idx->to_be_dropped = 0; + idx->to_be_rebuilt = false; idx->online_status = ONLINE_INDEX_COMPLETE; idx->set_committed(true); diff --git a/storage/innobase/handler/handler0alter.cc b/storage/innobase/handler/handler0alter.cc index 23e7eda8b66..a818eb4eb95 100644 --- a/storage/innobase/handler/handler0alter.cc +++ b/storage/innobase/handler/handler0alter.cc @@ -262,11 +262,10 @@ inline void dict_table_t::prepare_instant(const dict_table_t& old, Therefore columns must have been added at the end, or modified instantly in place. */ DBUG_ASSERT(index.n_fields >= oindex.n_fields); - DBUG_ASSERT(index.n_fields > oindex.n_fields - || !not_redundant()); + // FIXME: +// DBUG_ASSERT(index.n_fields > oindex.n_fields); #ifdef UNIV_DEBUG if (index.n_fields == oindex.n_fields) { - ut_ad(!not_redundant()); for (unsigned i = index.n_fields; i--; ) { ut_ad(index.fields[i].col->same_format( *oindex.fields[i].col)); @@ -463,9 +462,9 @@ inline void dict_index_t::instant_add_field(const dict_index_t& instant) /* Instant conversion from NULL to NOT NULL is not allowed. */ DBUG_ASSERT(!fields[i].col->is_nullable() || instant.fields[i].col->is_nullable()); - DBUG_ASSERT(fields[i].col->is_nullable() - == instant.fields[i].col->is_nullable() - || !table->not_redundant()); + // FIXME: +// DBUG_ASSERT(fields[i].col->is_nullable() +// == instant.fields[i].col->is_nullable()); } #endif n_fields = instant.n_fields; @@ -1453,6 +1452,126 @@ check_v_col_in_order( return(true); } +/** Iterate through array of known size. */ +template +class array_iterator +{ + element_t* array; + element_t* array_end; +public: + typedef std::ptrdiff_t difference_type; + typedef element_t value_type; + typedef value_type* pointer; + typedef value_type& reference; + typedef size_t size_type; + typedef std::forward_iterator_tag iterator_category; + typedef array_iterator iterator_type; + + array_iterator(element_t* buffer, uint count) + : array(buffer), array_end(&buffer[count]) {} + array_iterator& operator++ () + { + ut_ad(array < array_end); + ++array; + return *this; + } + value_type operator* () const + { + ut_ad(array < array_end); + return *array; + } + operator bool () const { return array == array_end; } + bool operator==(const iterator_type& rhs) const + { + return array == rhs.array; + } + bool operator!=(const iterator_type& rhs) const + { + return !(*this == rhs); + } + iterator_type end() const { return array_iterator(array_end, 0); } +}; + +/** Iterate through table secondary indexes with altered-nullable columns. */ +class altered_nullable_index_iterator : public dict_index_iterator +{ + List_iterator_fast create_it; +protected: + altered_nullable_index_iterator() : dict_index_iterator() {} +public: + typedef altered_nullable_index_iterator iterator_type; + + altered_nullable_index_iterator(const dict_table_t& table, + const Alter_inplace_info* ha_alter_info) + : dict_index_iterator(table), + create_it(ha_alter_info->alter_info->create_list) { ++(*this); } + iterator_type& operator++() + { + auto &it = static_cast(*this); + List_iterator_fast &cf_it = create_it; + ++it; + it = std::find_if(it, it.end(), [&cf_it](decltype(*it) index) { + cf_it.rewind(); + while (const Create_field* cf = cf_it++) { + Field *f = cf->field; + if (!(cf->altered && f && f->stored_in_db() + && (f->flags & NOT_NULL_FLAG) + && !(cf->flags & NOT_NULL_FLAG))) { + continue; + } + unsigned col_no = innodb_col_no(cf->field); + if (index->contains_col_or_prefix(col_no, false)) { + return true; + } + } + return false; + }); + return *this; + } + bool operator==(const iterator_type& rhs) const + { + return static_cast(*this) + == static_cast(rhs); + } + bool operator!=(const iterator_type& rhs) const + { + return !(*this == rhs); + } + static iterator_type end() { return iterator_type(); } +}; + +/** Check if all nullable-affected table indexes are already in DROP INDEX clause. +@param[in] ib_table InnoDB table definition +@param[in] ha_alter_info the ALTER TABLE operation */ +static inline +bool +instant_alter_indexes_possible(const dict_table_t& ib_table, + Alter_inplace_info* ha_alter_info) +{ + altered_nullable_index_iterator it(ib_table, ha_alter_info); + array_iterator keys(ha_alter_info->index_drop_buffer, + ha_alter_info->index_drop_count); + bool all_in_drop_buf = true; + for(; *it; ++it) + { + dict_index_t *index = *it; + bool in_drop_buf = keys.end() != std::find_if( + keys, keys.end(), + [&index](decltype(*keys) key) + { + return 0 == strcmp(index->name, key->name.str); + }); + if (!in_drop_buf) { + ++ha_alter_info->index_rebuild_count; + index->to_be_rebuilt = true; + all_in_drop_buf = false; + } else { + index->to_be_rebuilt = false; + } + }; + return all_in_drop_buf; +} + /** Determine if an instant operation is possible for altering columns. @param[in] ib_table InnoDB table definition @param[in] ha_alter_info the ALTER TABLE operation @@ -1462,7 +1581,7 @@ static bool instant_alter_column_possible( const dict_table_t& ib_table, - const Alter_inplace_info* ha_alter_info, + Alter_inplace_info* ha_alter_info, const TABLE* table, const TABLE* altered_table) { @@ -1561,7 +1680,13 @@ instant_alter_column_possible( if (ha_alter_info->handler_flags & ALTER_COLUMN_NULLABLE) { if (ib_table.not_redundant()) { - return false; + if (!instant_alter_indexes_possible(ib_table, + ha_alter_info)) { + ha_alter_info->handler_flags + |= ALTER_ADD_NON_UNIQUE_NON_PRIM_INDEX + | ALTER_DROP_NON_UNIQUE_NON_PRIM_INDEX; + return false; + } } Field** af = altered_table->field; @@ -3819,6 +3944,30 @@ ha_innobase_inplace_ctx::create_key_defs( indexdef++; } + + if (ha_alter_info->index_rebuild_count) { + for (dict_index_iterator it(*old_table, true); *it; ++it) { + dict_index_t *index = *it; + if (!index->to_be_rebuilt) { + continue; + } + for (uint i = 0; i < ha_alter_info->key_count; ++i) { + if (strcmp(index->name, key_info[i].name.str)) { + continue; + } + innobase_create_index_def( + altered_table, key_info, i, + false, false, indexdef, heap); + n_add++; + + if (indexdef->ind_type & DICT_FTS) { + n_fts_add++; + } + indexdef++; + break; + } + } + } } DBUG_ASSERT(indexdefs + n_add == indexdef); @@ -7711,7 +7860,8 @@ ha_innobase::prepare_inplace_alter_table( drop_fk = NULL; } - if (ha_alter_info->index_drop_count) { + if (ha_alter_info->index_drop_count + || ha_alter_info->index_rebuild_count) { dict_index_t* drop_primary = NULL; DBUG_ASSERT(ha_alter_info->handler_flags @@ -7721,7 +7871,8 @@ ha_innobase::prepare_inplace_alter_table( /* Check which indexes to drop. */ drop_index = static_cast( mem_heap_alloc( - heap, (ha_alter_info->index_drop_count + 1) + heap, (ha_alter_info->index_drop_count + + ha_alter_info->index_rebuild_count + 1) * sizeof *drop_index)); for (uint i = 0; i < ha_alter_info->index_drop_count; i++) { @@ -7748,6 +7899,13 @@ ha_innobase::prepare_inplace_alter_table( } } + for (dict_index_iterator it(*indexed_table, true); *it; ++it) { + if ((*it)->to_be_rebuilt) { + ut_ad(!(*it)->is_primary()); + drop_index[n_drop_index++] = *it; + } + } + /* If all FULLTEXT indexes were removed, drop an internal FTS_DOC_ID_INDEX as well, unless it exists in the table. */ @@ -7804,7 +7962,8 @@ ha_innobase::prepare_inplace_alter_table( for (uint i = 0; i < n_drop_index; i++) { dict_index_t* index = drop_index[i]; - if (innobase_check_foreign_key_index( + if (!index->to_be_rebuilt + && innobase_check_foreign_key_index( ha_alter_info, index, indexed_table, col_names, m_prebuilt->trx, drop_fk, n_drop_fk)) { diff --git a/storage/innobase/include/dict0mem.h b/storage/innobase/include/dict0mem.h index 11e150c4f78..294f3b49b15 100644 --- a/storage/innobase/include/dict0mem.h +++ b/storage/innobase/include/dict0mem.h @@ -955,6 +955,9 @@ struct dict_index_t { unsigned to_be_dropped:1; /*!< TRUE if the index is to be dropped; protected by dict_operation_lock */ + unsigned to_be_rebuilt:1; + /*!< TRUE if the index is to be rebuilt in + the process of inplace ALTER */ unsigned online_status:2; /*!< enum online_index_status. Transitions from ONLINE_INDEX_COMPLETE (to @@ -2151,6 +2154,43 @@ struct dict_table_t { dict_vcol_templ_t* vc_templ; }; +/** STL-compatible iterator over dict_index_t list. */ +class dict_index_iterator +{ + dict_index_t* index; +protected: + dict_index_iterator() : index(nullptr) {} +public: + typedef std::ptrdiff_t difference_type; + typedef dict_index_t* value_type; + typedef value_type* pointer; + typedef value_type& reference; + typedef size_t size_type; + typedef std::forward_iterator_tag iterator_category; + typedef dict_index_iterator iterator_type; + dict_index_iterator(const dict_table_t& table, bool secondary = false) + : index(UT_LIST_GET_FIRST(table.indexes)) { + if (secondary) { + ++(*this); + } + } + iterator_type& operator++() + { + index = UT_LIST_GET_NEXT(indexes, index); + return *this; + } + value_type operator* () const { return index; } + bool operator==(const iterator_type& rhs) const + { + return index == rhs.index; + } + bool operator!=(const iterator_type& rhs) const + { + return !(*this == rhs); + } + static iterator_type end() { return iterator_type(); } +}; + inline void dict_index_t::set_modified(mtr_t& mtr) const { mtr.set_named_space(table->space);