Bug #42230 during add index, cannot do queries on storage engines that implement add_index
Submitted: 20 Jan 2009 22:02 Modified: 29 Jan 2011 23:05
Reporter: Zardosht Kasheff (OCA) Email Updates:
Status: Closed Impact on me:
None 
Category:MySQL Server Severity:S3 (Non-critical)
Version:5.1.30 OS:Any
Assigned to: Jon Olav Hauglid
Tags: add_index, Contribution
Triage: Triaged: D3 (Medium) / R2 (Low) / E2 (Low)

[20 Jan 2009 22:02] Zardosht Kasheff
Description:
For storage engines that do not implement add_index, one can do queries on a table while a new index is being created. For storage engines that cannot, such as the latest InnoDB downloaded off of their website, one cannot do queries.

How to repeat:
download the latest InnoDB from their website and compile it with MySQL. Add an index to some table. While the index is being added, try to do some query on the table. The query hangs until the index creation is completed.

Suggested fix:
The query should still execute.
[20 Jan 2009 22:50] Miguel Solorzano
Thank you for the bug report. Verified on Windows 64-bit:

c:\5.1\bin>mysqld --standalone --console
090120 20:30:32  InnoDB: highest supported file format is Barracuda.
090120 20:30:33 InnoDB Plugin 1.0.2 started; log sequence number 6970293
090120 20:30:35 [Note] Event Scheduler: Loaded 0 events
090120 20:30:35 [Note] mysqld: ready for connections.
Version: '5.1.31-nt'  socket: ''  port: 3306  Source distribution

c:\5.1\bin>mysql -uroot w1
Welcome to the MySQL monitor.  Commands end with ; or \g.
Your MySQL connection id is 2
Server version: 5.1.31-nt Source distribution

Type 'help;' or '\h' for help. Type '\c' to clear the buffer.

mysql> select * from tb limit 1;
+----+----------------------------------------------------+
| id | col1                                               |
+----+----------------------------------------------------+
|  1 | aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa |
+----+----------------------------------------------------+
1 row in set (0.02 sec)

mysql> select * from tb limit 1;
+----+----------------------------------------------------+
| id | col1                                               |
+----+----------------------------------------------------+
|  1 | aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa |
+----+----------------------------------------------------+
1 row in set (2 min 39.87 sec)

mysql> show variables like "innodb_version";
+----------------+-------+
| Variable_name  | Value |
+----------------+-------+
| innodb_version | 1.0.2 |
+----------------+-------+
1 row in set (0.00 sec)

mysql> show create table tb;
+-------+-----------------------------------------------------------
--------------------------------------------------------------------
| Table | Create Table

+-------+-----------------------------------------------------------
--------------------------------------------------------------------
| tb    | CREATE TABLE `tb` (
  `id` bigint(20) unsigned NOT NULL AUTO_INCREMENT,
  `col1` varchar(50) DEFAULT NULL,
  UNIQUE KEY `id` (`id`),
  KEY `col1` (`col1`)
) ENGINE=InnoDB AUTO_INCREMENT=2490316 DEFAULT CHARSET=latin1 |
+-------+-----------------------------------------------------------
--------------------------------------------------------------------
1 row in set (0.00 sec)
[27 Jan 2009 19:08] Sergey Vojtovich
Regression was caused by fix for BUG#24395. Unless there is other code relying on the incorrect behaviour (there seem to be none), it will be a one line fix. An effort and risk to fix are rather low.

The problem is that the code around ENABLE/DISABLE KEYS acquires exclusive table lock unconditionally, whereas the lock should be acquired only if ENABLE/DISABLE KEYS are requested.

6.0 doesn't seem to be affected.
[21 Jul 2010 16:02] Zardosht Kasheff
Out of curiousity, what is the one line fix?
[24 Jul 2010 1:31] Zardosht Kasheff
I think the following patch solves the issue. Based of Sergey's comment

Attachment: 55528.diff (application/octet-stream, text), 1.14 KiB.

[24 Jul 2010 15:40] Zardosht Kasheff
patch as suggested by Davi Arnaut on internals alias

Attachment: 55528-version2.diff (application/octet-stream, text), 484 bytes.

[24 Jul 2010 15:41] Zardosht Kasheff
I just added 55528-version2.diff, based off of discussion on internals alias. Please disregard 55528.diff and consider this patch. The relevant discussion on the internals alias is http://lists.mysql.com/internals/37995
[26 Jul 2010 7:02] Sveta Smirnova
Bug #55528 was marked as duplicate of this one.
[19 Oct 2010 9:46] Sergey Vojtovich
Back almost two years ago I concluded: "Unless there is other code relying on the incorrect behaviour (there seem to be none), it will be a one line fix."

The one-line fix I was talking about is:
=== modified file 'sql/sql_table.cc'
--- sql/sql_table.cc	2010-10-01 13:59:07 +0000
+++ sql/sql_table.cc	2010-10-19 07:16:30 +0000
@@ -6343,7 +6343,7 @@
                                     alter_info->keys_onoff,
                                     alter_info->error_if_not_empty);
   }
-  else
+  else if (alter_info->flags & ALTER_KEYS_ONOFF)
   {
     if (!table->s->tmp_table &&
         wait_while_table_is_used(thd, table, HA_EXTRA_FORCE_REOPEN))

Unfortunately my assumption was wrong and there is code relying on this incorrect behaviour.

The story is rather tricky. Some relevant events, hopefully in proper order:
- "fast" alter table was implemented (probably WL#1563) with proper locking context;
- fix for BUG#24395 introduced exclusive lock;
- "fast" alter table code was re-shuffled and lost it's locking context. But it still works, because of fix for BUG#24395;
- mdl was introduced and we're further loosing add/drop index locking context;
- WL#3020 was created, but seem to have never been implemented.

Before applying this one-line fix, we must restore proper add/drop index mdl context.

Even having this all fixed, InnoDB may have to be adjusted as well.
[19 Oct 2010 15:41] Zardosht Kasheff
To be clear, this one line fix works in 5.1, correct? It is in 5.5 where this additional work needs to be taken into account?
[20 Oct 2010 7:05] Sergey Vojtovich
The one-line fix seem to open race condition hole even in 5.1.
[20 Oct 2010 7:14] Sergey Vojtovich
The one-line fix seem to open race condition hole even in 5.1.
[21 Oct 2010 21:06] Zardosht Kasheff
What is the race condition? Is it in InnoDB or in all storage engines?
[22 Oct 2010 13:25] Davi Arnaut
Sergey,

> Unfortunately my assumption was wrong and there is code relying
> on this incorrect behaviour.

Which code is this?

> Before applying this one-line fix, we must restore proper add/drop
> index mdl context.

What do you mean?
[22 Oct 2010 13:27] Zardosht Kasheff
Another thing I would like to understand is: what does it take to fix this in 5.1 v. 5.5? The metadata lock is new to 5.5, so I imagine the work to fix this in 5.1 is less than 5.5, correct?
[22 Oct 2010 13:28] Davi Arnaut
Before discussing fix and such, I would like to see a concise description of the problem. There are only superficial conjectures  here..
[25 Oct 2010 10:32] Sergey Vojtovich
Locking context that was lost seem to be relevant to WL#3020 (online alter table), sorry for confusion. Server seem to conform to design described in WL#1563 (fast alter table).

This extra problem seem to be InnoDB specific. With the one-line fix innodb-index test is failing (an error in error log). No concurrency involved, for some reason InnoDB seem to expect table reopen before ::add_index(). It is pretty suspicious expectation - I'd foresee more problems with concurrency.

So the question, that I think run-time should answer, is if locks and reopens are done properly. If answer is positive - the bug should go to InnoDB.

Q&A
---
In 5.1, why is this fix not sufficient?
Because relevant logics in 5.1 is in no way different from 5.5. MDL just makes things slightly more difficult to track.

Why is open_n_lock_single_table not sufficient for locking?
According to specification (WL#1563) it seem to be sufficient.

Which code is this?
Likely InnoDB.

What do you mean?
Disregard.
[25 Oct 2010 10:34] Davi Arnaut
Thanks Sergey, I'll take it further with the InnoDB team.
[10 Nov 2010 15:05] Davi Arnaut
Response from Marko when asked about index management operations under a shared metadata lock:

"For dropping or creating secondary indexes, a shared lock is fine. InnoDB already acquires internal InnoDB table locks according to these rules." [..] "It is safe for dropping indexes or creating secondary indexes, but not for creating a primary key, which requires a table rebuild. It probably would not crash when you create a primary key, but it could lead to a long lock wait or a lock wait timeout, until the table has been rebuilt."
[25 Nov 2010 14:51] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/125036

3395 Jon Olav Hauglid	2010-11-25
      Bug #42230 during add index, cannot do queries on storage engines
                 that implement add_index
      
      The problem was that ALTER TABLE blocked reads on an InnoDB table
      while adding a secondary index, even if this was not needed. It is
      only needed for the final step where the .frm file is updated.
      
      The reason queries were blocked, was that ALTER TABLE upgraded the
      metadata lock from MDL_SHARED_NO_WRITE (which blocks writes) to
      MDL_EXCLUSIVE (which blocks all accesses) before index creation.
      
      The way the server handles index creation, is that storage engines
      publish their capabilities to the server and the server determines
      which of the following three ways this can be handled: 1) build a
      new version of the table; 2) change the existing table but with
      exclusive metadata lock; 3) change the existing table but without
      metadata lock upgrade.
      
      For InnoDB and secondary index creation, option 3) should have been
      selected. However this failed for two reasons. First, InnoDB did
      not publish this capability properly. This patch fixes the problem
      by adding HA_ONLINE_ADD_INDEX to InnoDB's list of alter table flags.
      (The already published HA_ONLINE_ADD_INDEX_NO_WRITES flag only
      indicates that InnoDB support option 2).
      
      Second, the ALTER TABLE code failed to made proper use of the
      information supplied by the storage engine. A variable
      need_lock_for_indexes was set accordingly, but was not later used.
      This patch fixes this problem by only doing metadata lock upgrade
      before index creation/deletion if this variable has been set.
      
      Test case added to innodb_mysql_sync.test.
[17 Dec 2010 11:13] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/127171

3209 Jon Olav Hauglid	2010-12-17
      Bug #42230 during add index, cannot do queries on storage engines
                 that implement add_index
      
      The problem was that ALTER TABLE blocked reads on an InnoDB table
      while adding a secondary index, even if this was not needed. It is
      only needed for the final step where the .frm file is updated.
      
      The reason queries were blocked, was that ALTER TABLE upgraded the
      metadata lock from MDL_SHARED_NO_WRITE (which blocks writes) to
      MDL_EXCLUSIVE (which blocks all accesses) before index creation.
      
      The way the server handles index creation, is that storage engines
      publish their capabilities to the server and the server determines
      which of the following three ways this can be handled: 1) build a
      new version of the table; 2) change the existing table but with
      exclusive metadata lock; 3) change the existing table but without
      metadata lock upgrade.
      
      For InnoDB and secondary index creation, option 3) should have been
      selected. However this failed for two reasons. First, InnoDB did
      not publish this capability properly.
      
      Second, the ALTER TABLE code failed to made proper use of the
      information supplied by the storage engine. A variable
      need_lock_for_indexes was set accordingly, but was not later used.
      This patch fixes this problem by only doing metadata lock upgrade
      before index creation/deletion if this variable has been set.
      
      Test case added to innodb_mysql_sync.test.
[20 Dec 2010 17:38] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/127333

3217 Jon Olav Hauglid	2010-12-20
      Bug #42230 during add index, cannot do queries on storage engines
                 that implement add_index
      
      The problem was that ALTER TABLE blocked reads on an InnoDB table
      while adding a secondary index, even if this was not needed. It is
      only needed for the final step where the .frm file is updated.
      
      The reason queries were blocked, was that ALTER TABLE upgraded the
      metadata lock from MDL_SHARED_NO_WRITE (which blocks writes) to
      MDL_EXCLUSIVE (which blocks all accesses) before index creation.
      
      The way the server handles index creation, is that storage engines
      publish their capabilities to the server and the server determines
      which of the following three ways this can be handled: 1) build a
      new version of the table; 2) change the existing table but with
      exclusive metadata lock; 3) change the existing table but without
      metadata lock upgrade.
      
      For InnoDB and secondary index creation, option 3) should have been
      selected. However this failed for two reasons. First, InnoDB did
      not publish this capability properly.
      
      Second, the ALTER TABLE code failed to made proper use of the
      information supplied by the storage engine. A variable
      need_lock_for_indexes was set accordingly, but was not later used.
      This patch fixes this problem by only doing metadata lock upgrade
      before index creation/deletion if this variable has been set.
      
      This patch also changes some of the related terminology used 
      in the code. Specifically the use of "fast" and "online" with
      respect to ALTER TABLE. "Fast" was used to indicate that an
      ALTER TABLE operation could be done without involving a
      temporary table. "Fast" has been renamed "in-place" to more
      accurately describe the behavior.
      
      "Online" meant that the operation could be done without taking
      a table lock. However, in the current implementation writes
      are always prohibited during ALTER TABLE and an exclusive
      metadata lock is held while updating the .frm, so ALTER TABLE
      was not completely online. This patch replaces "online" with 
      "in-place", with additional comments indicating if concurrent
      reads are allowed during index creation/deletion or not.
      
      An important part of this update of terminology is renaming
      of the handler flags used by handlers to indicate if index
      creation/deletion can be done in-place and if concurrent reads
      are allowed. For example, the HA_ONLINE_ADD_INDEX_NO_WRITES
      flag has been renamed HA_INPLACE_ADD_INDEX_NO_READ_WRITE, while
      HA_ONLINE_ADD_INDEX is now HA_INPLACE_ADD_INDEX_NO_WRITE.
      Note that this is a rename to clarify current behavior, the
      flag values have not changed and no flags have been removed or
      added.
      
      Test case added to innodb_mysql_sync.test.
[22 Dec 2010 10:04] Marko Mäkelä
The patch looks OK as far as InnoDB is concerned, and the description looks OK too. I would like to request another test case to innodb_mysql_sync.test: Attempt to create a primary key and ensure that access to the table is blocked. Just in case, do the primary key creation both implicitly and explicitly.

I would suggest that you adapt this (commented) excerpt from innodb-index.test and interleave it with the DML connection. The first two ALTER statements should lock the table exclusively, and the last one should not (it is just creating a unique secondary index on a table that already has a user-supplied primary key).

-- creates a table with gen_clust_index (DB_ROW_ID as the 'primary key')
create table t1(a int not null, b int, c char(10) not null, d varchar(20)) engine = innodb;
-- creates a clustered index (PRIMARY KEY) on c, because it is declared NOT NULL
alter table t1 add unique index (c), add index (d);
-- drops the old clustered index and creates a new one (copies the table too)
alter table t1 add primary key (a), drop index c;
-- this should create a secondary index (non-blocking)
create unique index c on t1 (c);
[12 Jan 2011 10:25] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/128494

3237 Jon Olav Hauglid	2011-01-12
      Bug #42230 during add index, cannot do queries on storage engines
                 that implement add_index
      
      The problem was that ALTER TABLE blocked reads on an InnoDB table
      while adding a secondary index, even if this was not needed. It is
      only needed for the final step where the .frm file is updated.
      
      The reason queries were blocked, was that ALTER TABLE upgraded the
      metadata lock from MDL_SHARED_NO_WRITE (which blocks writes) to
      MDL_EXCLUSIVE (which blocks all accesses) before index creation.
      
      The way the server handles index creation, is that storage engines
      publish their capabilities to the server and the server determines
      which of the following three ways this can be handled: 1) build a
      new version of the table; 2) change the existing table but with
      exclusive metadata lock; 3) change the existing table but without
      metadata lock upgrade.
      
      For InnoDB and secondary index creation, option 3) should have been
      selected. However this failed for two reasons. First, InnoDB did
      not publish this capability properly.
      
      Second, the ALTER TABLE code failed to made proper use of the
      information supplied by the storage engine. A variable
      need_lock_for_indexes was set accordingly, but was not later used.
      This patch fixes this problem by only doing metadata lock upgrade
      before index creation/deletion if this variable has been set.
      
      This patch also changes some of the related terminology used 
      in the code. Specifically the use of "fast" and "online" with
      respect to ALTER TABLE. "Fast" was used to indicate that an
      ALTER TABLE operation could be done without involving a
      temporary table. "Fast" has been renamed "in-place" to more
      accurately describe the behavior.
      
      "Online" meant that the operation could be done without taking
      a table lock. However, in the current implementation writes
      are always prohibited during ALTER TABLE and an exclusive
      metadata lock is held while updating the .frm, so ALTER TABLE
      is not completely online. This patch replaces "online" with 
      "in-place", with additional comments indicating if concurrent
      reads are allowed during index creation/deletion or not.
      
      An important part of this update of terminology is renaming
      of the handler flags used by handlers to indicate if index
      creation/deletion can be done in-place and if concurrent reads
      are allowed. For example, the HA_ONLINE_ADD_INDEX_NO_WRITES
      flag has been renamed HA_INPLACE_ADD_INDEX_NO_READ_WRITE, while
      HA_ONLINE_ADD_INDEX is now HA_INPLACE_ADD_INDEX_NO_WRITE.
      Note that this is a rename to clarify current behavior, the
      flag values have not changed and no flags have been removed or
      added.
      
      Test case added to innodb_mysql_sync.test.
[26 Jan 2011 12:10] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/129650

3277 Jon Olav Hauglid	2011-01-26
      Bug #42230 during add index, cannot do queries on storage engines
                 that implement add_index
      
      The problem was that ALTER TABLE blocked reads on an InnoDB table
      while adding a secondary index, even if this was not needed. It is
      only needed for the final step where the .frm file is updated.
      
      The reason queries were blocked, was that ALTER TABLE upgraded the
      metadata lock from MDL_SHARED_NO_WRITE (which blocks writes) to
      MDL_EXCLUSIVE (which blocks all accesses) before index creation.
      
      The way the server handles index creation, is that storage engines
      publish their capabilities to the server and the server determines
      which of the following three ways this can be handled: 1) build a
      new version of the table; 2) change the existing table but with
      exclusive metadata lock; 3) change the existing table but without
      metadata lock upgrade.
      
      For InnoDB and secondary index creation, option 3) should have been
      selected. However this failed for two reasons. First, InnoDB did
      not publish this capability properly.
      
      Second, the ALTER TABLE code failed to made proper use of the
      information supplied by the storage engine. A variable
      need_lock_for_indexes was set accordingly, but was not later used.
      This patch fixes this problem by only doing metadata lock upgrade
      before index creation/deletion if this variable has been set.
      
      This patch also changes some of the related terminology used 
      in the code. Specifically the use of "fast" and "online" with
      respect to ALTER TABLE. "Fast" was used to indicate that an
      ALTER TABLE operation could be done without involving a
      temporary table. "Fast" has been renamed "in-place" to more
      accurately describe the behavior.
      
      "Online" meant that the operation could be done without taking
      a table lock. However, in the current implementation writes
      are always prohibited during ALTER TABLE and an exclusive
      metadata lock is held while updating the .frm, so ALTER TABLE
      is not completely online. This patch replaces "online" with 
      "in-place", with additional comments indicating if concurrent
      reads are allowed during index creation/deletion or not.
      
      An important part of this update of terminology is renaming
      of the handler flags used by handlers to indicate if index
      creation/deletion can be done in-place and if concurrent reads
      are allowed. For example, the HA_ONLINE_ADD_INDEX_NO_WRITES
      flag has been renamed to HA_INPLACE_ADD_INDEX_NO_READ_WRITE,
      while HA_ONLINE_ADD_INDEX is now HA_INPLACE_ADD_INDEX_NO_WRITE.
      Note that this is a rename to clarify current behavior, the
      flag values have not changed and no flags have been removed or
      added.
      
      Test case added to innodb_mysql_sync.test.
[26 Jan 2011 13:24] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/129664

3278 Jon Olav Hauglid	2011-01-26
      Bug #42230 during add index, cannot do queries on storage engines
                 that implement add_index
      
      The problem was that ALTER TABLE blocked reads on an InnoDB table
      while adding a secondary index, even if this was not needed. It is
      only needed for the final step where the .frm file is updated.
      
      The reason queries were blocked, was that ALTER TABLE upgraded the
      metadata lock from MDL_SHARED_NO_WRITE (which blocks writes) to
      MDL_EXCLUSIVE (which blocks all accesses) before index creation.
      
      The way the server handles index creation, is that storage engines
      publish their capabilities to the server and the server determines
      which of the following three ways this can be handled: 1) build a
      new version of the table; 2) change the existing table but with
      exclusive metadata lock; 3) change the existing table but without
      metadata lock upgrade.
      
      For InnoDB and secondary index creation, option 3) should have been
      selected. However this failed for two reasons. First, InnoDB did
      not publish this capability properly.
      
      Second, the ALTER TABLE code failed to made proper use of the
      information supplied by the storage engine. A variable
      need_lock_for_indexes was set accordingly, but was not later used.
      This patch fixes this problem by only doing metadata lock upgrade
      before index creation/deletion if this variable has been set.
      
      This patch also changes some of the related terminology used 
      in the code. Specifically the use of "fast" and "online" with
      respect to ALTER TABLE. "Fast" was used to indicate that an
      ALTER TABLE operation could be done without involving a
      temporary table. "Fast" has been renamed "in-place" to more
      accurately describe the behavior.
      
      "Online" meant that the operation could be done without taking
      a table lock. However, in the current implementation writes
      are always prohibited during ALTER TABLE and an exclusive
      metadata lock is held while updating the .frm, so ALTER TABLE
      is not completely online. This patch replaces "online" with 
      "in-place", with additional comments indicating if concurrent
      reads are allowed during index creation/deletion or not.
      
      An important part of this update of terminology is renaming
      of the handler flags used by handlers to indicate if index
      creation/deletion can be done in-place and if concurrent reads
      are allowed. For example, the HA_ONLINE_ADD_INDEX_NO_WRITES
      flag has been renamed to HA_INPLACE_ADD_INDEX_NO_READ_WRITE,
      while HA_ONLINE_ADD_INDEX is now HA_INPLACE_ADD_INDEX_NO_WRITE.
      Note that this is a rename to clarify current behavior, the
      flag values have not changed and no flags have been removed or
      added.
      
      Test case added to innodb_mysql_sync.test.
[26 Jan 2011 13:52] Jon Olav Hauglid
Bug#49730 was closed as a duplicate of this bug.
[26 Jan 2011 13:58] Bugs System
Pushed into mysql-trunk 5.6.2 (revid:jon.hauglid@oracle.com-20110126135704-m2mgl5x1zjz7ojif) (version source revid:jon.hauglid@oracle.com-20110126135704-m2mgl5x1zjz7ojif) (merge vers: 5.6.2) (pib:24)
[26 Jan 2011 14:00] Bugs System
Pushed into mysql-5.5 5.5.10 (revid:jon.hauglid@oracle.com-20110126132329-r1wyh235a3pi2n71) (version source revid:jon.hauglid@oracle.com-20110126132329-r1wyh235a3pi2n71) (merge vers: 5.5.10) (pib:24)
[10 Mar 2011 20:33] Sveta Smirnova
Bug #60388 was marked as duplicate of this one.
[26 May 2011 16:12] Paul Dubois
Noted in 5.5.10, 5.6.2 changelogs.

InnoDB now allows concurrent reads on a table while creating
nonprimary nonunique indexes. (This was found to create problems and
was reverted in 5.5.12.)