MySQL Bugs: #94383: simple ALTER cause unnecessary InnoDB index rebuilds, 5.7.23 or later 5.7 rlses

Bug #94383	simple ALTER cause unnecessary InnoDB index rebuilds, 5.7.23 or later 5.7 rlses
Submitted:	18 Feb 2019 15:49	Modified:	1 Apr 2019 17:49
Reporter:	Mikhail Izioumtchenko (OCA)	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: InnoDB storage engine	Severity:	S3 (Non-critical)
Version:	5.7.23, 5.7.25	OS:	Any
Assigned to:		CPU Architecture:	Any

Description:
If a table is an InnoDB table created in 5.7.22 AND the table contains KEYs on VARCHAR columns, then after upgrade to 5.7.23 or later 5.7 releases, those KEYs will be rebuilt on any simple LATER such as adding COMMENT to the table.

How to repeat:
see further comments

to reproduce:
first, we need to create a table on 5.7.22:
 create table test_varchar(i int primary key, v varchar(40), key (v));
then we upgrade to 5.7.23 or later. In this example I'm using 5.7.25.
We need a diagnostics patch that I will post in the next comment. 
In particular it asserts in the routine that InnoDB uses to rebuild an index:

diff -u -r mysql-5.7.25-orig/storage/innobase/row/row0merge.cc mysql-5.7.25/storage/innobase/row/row0merge.cc
--- mysql-5.7.25-orig/storage/innobase/row/row0merge.cc>2019-02-17 08:17:13.399279574 +0100
+++ mysql-5.7.25/storage/innobase/row/row0merge.cc>·2019-02-18 13:11:36.937551594 +0100
@@ -4430,6 +4433,8 @@
·
 >··/* Read clustered index of the table and create files for
 >··secondary index entries for merge sort */
+printf("#debug 25 finds %lu indexes\n", n_indexes);
+ut_a(!"BUG25"); //#debug BUG25
 >··error = row_merge_read_clustered_index(
 >··>···trx, table, old_table, new_table, online, indexes,
 >··>···fts_sort_idx, psort_info, merge_files, key_numbers,

Then we try to run an ALTER:

... [test]> alter table test_varchar comment 'test-5.7.25';
ERROR 2013 (HY000): Lost connection to MySQL server during query

and indeed 

InnoDB: Failing assertion: !"BUG25"

The bug is that for ALTER TABLE ... COMMENT '...' there is no need to call row_merge_read_clustered_index(), and this is not happening in MySQL 5.7.22

Diagnostic patches:
patch for 5.7.22 I used to prove the keys are not rebuilt in 5.7.22:

diff -u -r mysql-5.7.22-orig/storage/innobase/row/row0merge.cc mysql-5.7.22/storage/innobase/row/row0merge.cc
--- mysql-5.7.22-orig/storage/innobase/row/row0merge.cc 2019-02-17 08:17:36.193419315 +0100
+++ mysql-5.7.22/storage/innobase/row/row0merge.cc      2019-02-18 12:25:45.249163115 +0100
@@ -4424,6 +4424,7 @@

        /* Read clustered index of the table and create files for
        secondary index entries for merge sort */
+ut_a(!"BUG25"); //#debug BUG25
        error = row_merge_read_clustered_index(
                trx, table, old_table, new_table, online, indexes,
                fts_sort_idx, psort_info, merge_files, key_numbers,

and complete giagnostics patch for 5.7.25:

diff -u -r mysql-5.7.25-orig/sql/sql_table.cc mysql-5.7.25/sql/sql_table.cc
--- mysql-5.7.25-orig/sql/sql_table.cc  2019-02-17 08:17:13.373279414 +0100
+++ mysql-5.7.25/sql/sql_table.cc       2019-02-18 13:12:41.839963667 +0100
@@ -6278,8 +6278,13 @@
   if ((table_key->algorithm != new_key->algorithm) ||
       ((table_key->flags & HA_KEYFLAG_MASK) !=
        (new_key->flags & HA_KEYFLAG_MASK)) ||
-      (table_key->user_defined_key_parts != new_key->user_defined_key_parts))
+      (table_key->user_defined_key_parts != new_key->user_defined_key_parts)) {
+printf("#debug 25 in has_index_def_changed 1 ret True for %s\n",table_key->name);
+printf("#debug 25 in has_index_def_changed alg %d vs %d\n", table_key->algorithm, new_key->algorithm);
+printf("#debug 25 in has_index_def_changed parts %u vs %u\n", table_key->user_defined_key_parts, new_key->user_defined_key_parts);
+printf("#debug 25 in has_index_def_changed flags %lu vs %lu and mask %u\n", (table_key->flags & HA_KEYFLAG_MASK), (new_key->flags & HA_KEYFLAG_MASK), HA_KEYFLAG_MASK);
     return true;
+}

   /*
     If an index comment is added/dropped/changed, then mark it for a
@@ -6815,6 +6820,7 @@
     else if (has_index_def_changed(ha_alter_info, table_key, new_key))
     {
       /* Key was modified. */
+printf("#debug 25 in fill_alter_inplace_info need modify key %s to %s\n",table_key->name,new_key->name);
       ha_alter_info->add_modified_key(table_key, new_key);
     }
   }
@@ -6886,12 +6892,14 @@
       }
       else
       {
+printf("#debug 25 fill_alter_inplace_info drop unique index %s BUG\n",table_key->name);
         ha_alter_info->handler_flags|= Alter_inplace_info::DROP_UNIQUE_INDEX;
         if (is_candidate_key(table_key))
           candidate_key_count--;
       }
     }
     else
+printf("#debug 25 drop fill_alter_inplace_info non unique %s index BUG\n",table_key->name);
       ha_alter_info->handler_flags|= Alter_inplace_info::DROP_INDEX;
   }

diff -u -r mysql-5.7.25-orig/storage/innobase/handler/handler0alter.cc mysql-5.7.25/storage/innobase/handler/handler0alter.cc
--- mysql-5.7.25-orig/storage/innobase/handler/handler0alter.cc 2019-02-17 08:17:13.386279494 +0100
+++ mysql-5.7.25/storage/innobase/handler/handler0alter.cc      2019-02-18 13:11:58.583689017 +0100
@@ -6230,8 +6230,13 @@

        DEBUG_SYNC(m_user_thd, "innodb_inplace_alter_table_enter");

+printf("#debug BUG25-enter-ha_innobase::inplace_alter_table\n");fflush(stdout);
+
+printf("#debug BUG25-p0-ha_innobase::inplace_alter_table flags %llu and %llu, expect not\n",ha_alter_info->handler_flags,INNOBASE_ALTER_DATA);fflush(stdout);
        if (!(ha_alter_info->handler_flags & INNOBASE_ALTER_DATA)) {
+printf("#debug BUG25-p1-ha_innobase::inplace_alter_table\n");fflush(stdout);
 ok_exit:
+printf("#debug BUG25-ok-exit-ha_innobase::inplace_alter_table\n");fflush(stdout);
                DEBUG_SYNC(m_user_thd, "innodb_after_inplace_alter_table");
                DBUG_RETURN(false);
        }
diff -u -r mysql-5.7.25-orig/storage/innobase/row/row0merge.cc mysql-5.7.25/storage/innobase/row/row0merge.cc
--- mysql-5.7.25-orig/storage/innobase/row/row0merge.cc 2019-02-17 08:17:13.399279574 +0100
+++ mysql-5.7.25/storage/innobase/row/row0merge.cc      2019-02-18 13:11:36.937551594 +0100
@@ -4334,6 +4334,7 @@
        int64_t                 sig_count = 0;
        bool                    fts_psort_initiated = false;
        DBUG_ENTER("row_merge_build_indexes");
+printf("BUG25-enter-row_merge_build_indexes\n");//#debug

        ut_ad(!srv_read_only_mode);
        ut_ad((old_table == new_table) == !col_map);
@@ -4395,6 +4396,8 @@
                merge_files[i].fd = -1;
        }

+printf("#debug 25 n_indexes is now %lu\n",n_indexes);
+
        for (i = 0; i < n_indexes; i++) {
                if (indexes[i]->type & DICT_FTS) {
                        ibool   opt_doc_id_size = FALSE;
@@ -4430,6 +4433,8 @@

        /* Read clustered index of the table and create files for
        secondary index entries for merge sort */
+printf("#debug 25 finds %lu indexes\n", n_indexes);
+ut_a(!"BUG25"); //#debug BUG25
        error = row_merge_read_clustered_index(
                trx, table, old_table, new_table, online, indexes,
                fts_sort_idx, psort_info, merge_files, key_numbers,

what is happening:
consider what we see in the 5.7.25 log after running
alter table test_varchar_prefix comment 'test-5.7.25';
with the diagnostics patch above

#debug 25 in has_index_def_changed 1 ret True for v
#debug 25 in has_index_def_changed alg 0 vs 0
#debug 25 in has_index_def_changed parts 1 vs 1
#debug 25 in has_index_def_changed flags 32 vs 0 and mask 11699
#debug 25 in fill_alter_inplace_info need modify key v to v
#debug 25 drop fill_alter_inplace_info non unique v index BUG
#debug BUG25-enter-ha_innobase::inplace_alter_table
#debug BUG25-p0-ha_innobase::inplace_alter_table flags 16777219 and 412334097589, expect not
InnoDB: Failing assertion: !"BUG25"
BUG25-enter-row_merge_build_indexes
#debug 25 n_indexes is now 2
#debug 25 finds 2 indexes

the root of the problem is this:
#debug 25 in has_index_def_changed flags 32 vs 0 and mask 11699

so MySQL believes the BEFORE version of the key has a flag of 32 which is HA_BINARY_PACK_KEY, and it makes the decision to rebuild the index. 
I believe anything PACK related is not relevant to InnoDB so there is no need whatsoever to rebuild any index. 

One possible fix is to remove HA_BINARY_PACK_KEY from the definition of HA_KEYFLAG_MASK in include/my_base.h. I'm sure it will break at least MyISAM tests so the real fix will be slightly more complicated.

Final observations:
* the bug applies to all ALTER's that are nearly instantaneous in 5.7.22: column level comments, column defaults etc.
* the bug is a pain when the table is big
* it is especially unpleasant when the replication master is on 5.7.22 or lower and the slaves are on 5.7.23 or higher, then a fast ALTER on master may result in multihour replication delay
* upgrade scripts and CHECK ... FOR UPGRADE do not catch this
* the bug applies also to prefix indexes on VARCHAR's
* it also applies to multicolumn indexes as long as there is a VARCHAR column
* after one index rebuild, the table is 'fixed' so subsequent ALTER's of the kind do not cause the rebuild

A candidate for the culprit (just a guess), a quote from 5.7.23 release notes:

attempts to increase the length of a VARCHAR column of an InnoDB table using ALTER TABLE with the INPLACE algorithm, the attempt failed if the column was indexed.

If an index size exceeded the InnoDB limit of 767 bytes for COMPACT or REDUNDANT row format, CREATE TABLE and ALTER TABLE did not report an error (in strict SQL mode) or a warning (in nonstrict mode). (Bug #26848813)

Hello Mikhail,

Thank you for the report.

regards,
Umesh

test results

Attachment: 94383_5722_5725.results (application/octet-stream, text), 53.57 KiB.

Posted by developer:
 
Fixed in 5.7.27, 8.0.17.

For InnoDB tables that contained an index on a VARCHAR column and
were created prior to MySQL 5.7.23, some simple ALTER TABLE
statements that should have been done in place were performed with a
table rebuild after an upgrade to MySQL 5.7.23 or higher.

Posted by developer:
 
Correction: Bug occurs in 5.7 only, so it is fixed in 5.7.27 only, not 8.0.17.