maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Thirunarayanan!

On Dec 13, Thirunarayanan Balathandayuthapani wrote:
> revision-id: c16ad313bc4 (mariadb-10.2.19-4-gc16ad313bc4)
> parent(s): 7e756437789
> author: Thirunarayanan Balathandayuthapani <thiru@xxxxxxxxxxx>
> committer: Thirunarayanan Balathandayuthapani <thiru@xxxxxxxxxxx>
> timestamp: 2018-11-15 15:14:02 +0530
> message:
> 
> MDEV-16849 Extending indexed VARCHAR column should be instantaneous
... 
> diff --git a/mysql-test/suite/innodb/r/alter_varchar_change.result b/mysql-test/suite/innodb/r/alter_varchar_change.result
> new file mode 100644
> index 00000000000..5d2c6403aab
> --- /dev/null
> +++ b/mysql-test/suite/innodb/r/alter_varchar_change.result
> @@ -0,0 +1,335 @@
...
> +CREATE TABLE t1(f1 INT NOT NULL,
> +f2 VARCHAR(100),
> +INDEX idx(f2(10)))ENGINE=InnoDB;
> +CALL get_table_id("test/t1", @tbl_id);
> +CALL get_index_id(@tbl_id, "idx", @idx_id);
> +ALTER TABLE t1 MODIFY f2 VARCHAR(200), DROP INDEX idx, ADD INDEX idx(f2(50));
> +CALL get_table_id("test/t1", @tbl1_id);
> +CALL get_index_id(@tbl1_id, "idx", @idx1_id);
> +SELECT @tbl1_id = @tbl_id;
> +@tbl1_id = @tbl_id
> +1
> +SELECT @idx1_id = @idx_id;
> +@idx1_id = @idx_id
> +1

Is that right? Old index would sort values "aaaaaaaaaa1", "aaaaaaaaaa2",
"aaaaaaaaaa3" as equal, so they could be in the index in any order.
But in the new index they aren't equal.

> +CREATE TABLE t1(f1 INT NOT NULL,
> +f2 VARCHAR(100), FULLTEXT idx(f2))ENGINE=InnoDB;
> +CALL get_table_id("test/t1", @tbl_id);
> +CALL get_index_id(@tbl_id, "idx", @idx_id);
> +ALTER TABLE t1 MODIFY f2 VARCHAR(200);

please, add a case where there's a FULLTEXT index, as above,
but ALTER TABLE makes a column shorter.

The index must be rebuilt.

> +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);
> +CALL get_index_id(@tbl_id, "idx", @idx_id);
> +ALTER TABLE t1 MODIFY f2 VARCHAR(200), DROP INDEX idx1;

Please also add a test case where VARCHAR is made shorter, but still
longer than a prefix:

 ALTER TABLE t1 MODIFY f2 VARCHAR(50)

The index should not be rebuilt.
And shorter than a prefix:

 ALTER TABLE t1 MODIFY f2 VARCHAR(5)

The index must be rebuilt.

Also add a test when ALTER TABLE changes column's charset, or column
type (CHAR/VARCHAR/TEXT). And a test where column's length is changed
over 255. E.g. from 200 to 300.

> diff --git a/sql/sql_table.cc b/sql/sql_table.cc
> --- a/sql/sql_table.cc
> +++ b/sql/sql_table.cc
> @@ -6687,18 +6687,28 @@ static bool fill_alter_inplace_info(THD *thd,
>           key_part < end;
>           key_part++, new_part++)
>      {
> +      new_field= get_field_by_index(alter_info, new_part->fieldnr);
> +      old_field= table->field[key_part->fieldnr - 1];
>        /*
> +        If there is a change in index length due to column expansion
> +        like varchar(X) changed to varchar(X + N) and has a compatible
> +        packed data representation, we mark it for fast/INPLACE change
> +        in index definition. InnoDB supports INPLACE for this cases
> +
>          Key definition has changed if we are using a different field or
> -        if the used key part length is different. It makes sense to
> -        check lengths first as in case when fields differ it is likely
> -        that lengths differ too and checking fields is more expensive
> -        in general case.
> +        if the user key part length is different.
>        */
> -      if (key_part->length != new_part->length)
> +      if (key_part->length <= new_part->length &&
> +          old_field->pack_length() < new_field->pack_length &&

I still don't understand this condition. Why do you care whether a field
length has changed, while you're should interested to know whether an
index length have changed? Field length changes are completely
irrelevant here.

again, the only possible ALTER_COLUMN_INDEX_LENGTH case:
 * key part length was the same as column length, and it was not decreased.

So, your condition should, probably, be something like

        if (key_part->length == old_field->pack_length() &&
            key_part->length < new_part->length)

> +	  (key_part->field->is_equal((Create_field*) new_field)
> +           == IS_EQUAL_PACK_LENGTH))
> +      {
> +        ha_alter_info->handler_flags |=
> +           Alter_inplace_info::ALTER_COLUMN_INDEX_LENGTH;
> +      }
> +      else if (key_part->length != new_part->length)
>          goto index_changed;
>  
> -      new_field= get_field_by_index(alter_info, new_part->fieldnr);
> -
>        /*
>          For prefix keys KEY_PART_INFO::field points to cloned Field
>          object with adjusted length. So below we have to check field

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx