Hi, Thirunarayanan! On Jan 28, Thirunarayanan Balathandayuthapani wrote:

revision-id: 322301e2434 (mariadb-10.2.21-2-g322301e2434) parent(s): fe4c65db1dc author: Thirunarayanan Balathandayuthapani committer: Thirunarayanan Balathandayuthapani timestamp: 2019-01-01 11:01:03 +0530 message:

MDEV-16849 Extending indexed VARCHAR column should be instantaneous

- Addressed Serg's review comments.

--- diff --git a/mysql-test/suite/innodb/r/alter_varchar_change.result b/mysql-test/suite/innodb/r/alter_varchar_change.result index 5d2c6403aab..2f412aecbf1 100644 --- a/mysql-test/suite/innodb/r/alter_varchar_change.result +++ b/mysql-test/suite/innodb/r/alter_varchar_change.result @@ -316,6 +316,137 @@ t1 CREATE TABLE `t1` ( DROP TABLE t1; CREATE TABLE t1(f1 INT NOT NULL, f2 VARCHAR(100), +INDEX idx(f2(10)), +INDEX idx1(f1))ENGINE=InnoDB; +CALL get_table_id("test/t1", @tbl_id); +ALTER TABLE t1 MODIFY f2 VARCHAR(50), DROP INDEX idx1; +CALL get_table_id("test/t1", @tbl1_id); +SELECT @tbl1_id = @tbl_id; +@tbl1_id = @tbl_id +0 +SELECT @idx1_id = @idx_id; +@idx1_id = @idx_id +0

Why `DROP INDEX idx1` ? As far as I remember, the point was "make VARCHAR shorter, but still longer than a prefix". In this case there is no need to rebuild the index, why was it rebuilt?

+SHOW CREATE TABLE t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `f1` int(11) NOT NULL, + `f2` varchar(50) DEFAULT NULL, + KEY `idx` (`f2`(10)) +) ENGINE=InnoDB DEFAULT CHARSET=latin1 +DROP TABLE t1; +CREATE TABLE t1(f1 INT NOT NULL, +f2 VARCHAR(100), +INDEX idx(f2(10)), +INDEX idx1(f1))ENGINE=InnoDB; +CALL get_table_id("test/t1", @tbl_id); +ALTER TABLE t1 MODIFY f2 VARCHAR(5), DROP INDEX idx1; +CALL get_table_id("test/t1", @tbl1_id); +SELECT @tbl1_id = @tbl_id; +@tbl1_id = @tbl_id +0 +SELECT @idx1_id = @idx_id; +@idx1_id = @idx_id +0

Correct. "VARCHAR is changed to be shorter than a prefix" means that the index must be rebuilt.

+SHOW CREATE TABLE t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `f1` int(11) NOT NULL, + `f2` varchar(5) DEFAULT NULL, + KEY `idx` (`f2`) +) ENGINE=InnoDB DEFAULT CHARSET=latin1 +DROP TABLE t1; ... diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 2864474a8e8..7eca33812b9 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -6629,6 +6629,7 @@ static bool fill_alter_inplace_info(THD *thd, Go through keys and check if the original ones are compatible with new table. */ + uint old_field_len= 0; KEY *table_key; KEY *table_key_end= table->key_info + table->s->keys; KEY *new_key; @@ -6698,8 +6699,16 @@ static bool fill_alter_inplace_info(THD *thd, Key definition has changed if we are using a different field or if the user key part length is different. */ - if (key_part->length <= new_part->length && - old_field->pack_length() < new_field->pack_length && + old_field_len= old_field->pack_length(); + + if (old_field->type() == MYSQL_TYPE_VARCHAR) + { + old_field_len= (old_field->pack_length() + - ((Field_varstring*) old_field)->length_bytes); + } + + if (key_part->length == old_field_len && + key_part->length <= new_part->length &&

Why <= here? Doesn't it mean that if key_part->length == new_part->length then you'll still set ALTER_COLUMN_INDEX_LENGTH even if _nothing_ has changed?

(key_part->field->is_equal((Create_field*) new_field) == IS_EQUAL_PACK_LENGTH)) {

Regards, Sergei Chief Architect MariaDB and security@mariadb.org