Skip to content

Commit 841a620

Browse files
jhauglidjhauglid
committed
Fix for bug #14516798 "PREPARE_INPLACE_ALTER_TABLE NOT UNDER EXCLUSIVE
MDL WHEN LOCK=SHARED". ALTER TABLE executed using in-place algorithm could trigger an InnoDB assertion if during prepare phase of ALTER TABLE, the table being altered happened to be used in more than one connection. The problem was that for types of in-place ALTER TABLE which require strong lock during the prepare phase, the SQL-layer acquired only SNW metadata lock, thus allowing concurrent reads during this phase (as per WL specification). At the same time InnoDB assumed that during the prepare phase the table is used only by the connection executing ALTER TABLE. This mismatch led to assertion failure in InnoDB. This fix addresses the problem by ensuring that for in-place ALTER TABLE variants which require strong lock during the prepare phase, the SQL-layer acquires an exclusive (X) metadata lock. To support this change we had to add new variant of locking requirements for in-place ALTER TABLE - HA_ALTER_INPLACE_SHARED_LOCK_AFTER_PREPARE, to be used by InnoDB instead of HA_ALTER_INPLACE_SHARED_LOCK (since InnoDB requires X lock during prepare phase and the latter acquires only SNW lock for both prepare and main phase). Commit on behalf of Dmitry Lenev.
1 parent c76147f commit 841a620

14 files changed

Lines changed: 152 additions & 61 deletions

‎mysql-test/r/innodb_mysql_sync.result‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -244,17 +244,14 @@ a b
244244
INSERT INTO t1 VALUES (3,3);
245245
SET DEBUG_SYNC= 'now SIGNAL continue1';
246246
SET DEBUG_SYNC= 'now WAIT_FOR upgraded';
247-
# Now writes should be blocked, reads still allowed.
247+
# Now both reads and writes should be blocked
248248
SELECT * FROM t1;
249-
a b
250-
1 1
251-
2 2
252-
3 3
249+
ERROR HY000: Lock wait timeout exceeded; try restarting transaction
253250
INSERT INTO t1 VALUES (4,4);
254251
ERROR HY000: Lock wait timeout exceeded; try restarting transaction
255252
SET DEBUG_SYNC= 'now SIGNAL continue2';
256253
SET DEBUG_SYNC= 'now WAIT_FOR beforecommit';
257-
# Now both reads and writes should be blocked.
254+
# Still both reads and writes should be blocked.
258255
SELECT * FROM t1;
259256
ERROR HY000: Lock wait timeout exceeded; try restarting transaction
260257
INSERT INTO t1 VALUES (5,5);

‎mysql-test/t/innodb_mysql_sync.test‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -401,14 +401,15 @@ INSERT INTO t1 VALUES (3,3);
401401

402402
SET DEBUG_SYNC= 'now SIGNAL continue1';
403403
SET DEBUG_SYNC= 'now WAIT_FOR upgraded';
404-
--echo # Now writes should be blocked, reads still allowed.
404+
--echo # Now both reads and writes should be blocked
405+
--error ER_LOCK_WAIT_TIMEOUT
405406
SELECT * FROM t1;
406407
--error ER_LOCK_WAIT_TIMEOUT
407408
INSERT INTO t1 VALUES (4,4);
408409

409410
SET DEBUG_SYNC= 'now SIGNAL continue2';
410411
SET DEBUG_SYNC= 'now WAIT_FOR beforecommit';
411-
--echo # Now both reads and writes should be blocked.
412+
--echo # Still both reads and writes should be blocked.
412413
--error ER_LOCK_WAIT_TIMEOUT
413414
SELECT * FROM t1;
414415
--error ER_LOCK_WAIT_TIMEOUT

‎sql/handler.h‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ enum enum_alter_inplace_result {
6464
HA_ALTER_ERROR,
6565
HA_ALTER_INPLACE_NOT_SUPPORTED,
6666
HA_ALTER_INPLACE_EXCLUSIVE_LOCK,
67+
HA_ALTER_INPLACE_SHARED_LOCK_AFTER_PREPARE,
6768
HA_ALTER_INPLACE_SHARED_LOCK,
6869
HA_ALTER_INPLACE_NO_LOCK_AFTER_PREPARE,
6970
HA_ALTER_INPLACE_NO_LOCK
@@ -2691,8 +2692,9 @@ class handler :public Sql_alloc
26912692
*) As the first step, we acquire a lock corresponding to the concurrency
26922693
level which was returned by handler::check_if_supported_inplace_alter()
26932694
and requested by the user. This lock is held for most of the
2694-
duration of in-place ALTER (if HA_ALTER_INPLACE_NO_LOCK_AFTER_PREPARE
2695-
was returned we acquire a write lock for duration of the next step only).
2695+
duration of in-place ALTER (if HA_ALTER_INPLACE_SHARED_LOCK_AFTER_PREPARE
2696+
or HA_ALTER_INPLACE_NO_LOCK_AFTER_PREPARE were returned we acquire an
2697+
exclusive lock for duration of the next step only).
26962698
*) After that we call handler::ha_prepare_inplace_alter_table() to give the
26972699
storage engine a chance to update its internal structures with a higher
26982700
lock level than the one that will be used for the main step of algorithm.
@@ -2735,11 +2737,15 @@ class handler :public Sql_alloc
27352737
@retval HA_ALTER_ERROR Unexpected error.
27362738
@retval HA_ALTER_INPLACE_NOT_SUPPORTED Not supported, must use copy.
27372739
@retval HA_ALTER_INPLACE_EXCLUSIVE_LOCK Supported, but requires X lock.
2740+
@retval HA_ALTER_INPLACE_SHARED_LOCK_AFTER_PREPARE
2741+
Supported, but requires SNW lock
2742+
during main phase. Prepare phase
2743+
requires X lock.
27382744
@retval HA_ALTER_INPLACE_SHARED_LOCK Supported, but requires SNW lock.
27392745
@retval HA_ALTER_INPLACE_NO_LOCK_AFTER_PREPARE
27402746
Supported, concurrent reads/writes
27412747
allowed. However, prepare phase
2742-
requires SNW lock.
2748+
requires X lock.
27432749
@retval HA_ALTER_INPLACE_NO_LOCK Supported, concurrent
27442750
reads/writes allowed.
27452751
@@ -2796,8 +2802,9 @@ class handler :public Sql_alloc
27962802
/**
27972803
Allows the storage engine to update internal structures with concurrent
27982804
writes blocked. If check_if_supported_inplace_alter() returns
2799-
HA_ALTER_INPLACE_NO_LOCK_AFTER_PREPARE, this function is called with
2800-
writes blocked, otherwise the same level of locking as for
2805+
HA_ALTER_INPLACE_NO_LOCK_AFTER_PREPARE or
2806+
HA_ALTER_INPLACE_SHARED_AFTER_PREPARE, this function is called with
2807+
exclusive lock otherwise the same level of locking as for
28012808
inplace_alter_table() will be used.
28022809
28032810
@note Storage engines are responsible for reporting any errors by

‎sql/sql_admin.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ static int prepare_for_repair(THD *thd, TABLE_LIST *table_list,
166166
*/
167167
if (wait_while_table_is_used(thd, table, HA_EXTRA_FORCE_REOPEN))
168168
goto end;
169-
close_all_tables_for_name(thd, table_list->table->s, FALSE);
169+
close_all_tables_for_name(thd, table_list->table->s, false, NULL);
170170
table_list->table= 0;
171171
}
172172
/*

‎sql/sql_base.cc‎

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -976,7 +976,7 @@ bool close_cached_tables(THD *thd, TABLE_LIST *tables,
976976
result= TRUE;
977977
goto err_with_reopen;
978978
}
979-
close_all_tables_for_name(thd, table->s, FALSE);
979+
close_all_tables_for_name(thd, table->s, false, NULL);
980980
}
981981
}
982982

@@ -1247,13 +1247,16 @@ static void close_open_tables(THD *thd)
12471247
In that case the documented behaviour is to
12481248
implicitly remove the table from LOCK TABLES
12491249
list.
1250+
@param[in] skip_table
1251+
TABLE instance that should be kept open.
12501252
12511253
@pre Must be called with an X MDL lock on the table.
12521254
*/
12531255

12541256
void
12551257
close_all_tables_for_name(THD *thd, TABLE_SHARE *share,
1256-
bool remove_from_locked_tables)
1258+
bool remove_from_locked_tables,
1259+
TABLE *skip_table)
12571260
{
12581261
char key[MAX_DBKEY_LENGTH];
12591262
uint key_length= share->table_cache_key.length;
@@ -1268,7 +1271,8 @@ close_all_tables_for_name(THD *thd, TABLE_SHARE *share,
12681271
TABLE *table= *prev;
12691272

12701273
if (table->s->table_cache_key.length == key_length &&
1271-
!memcmp(table->s->table_cache_key.str, key, key_length))
1274+
!memcmp(table->s->table_cache_key.str, key, key_length) &&
1275+
table != skip_table)
12721276
{
12731277
thd->locked_tables_list.unlink_from_list(thd,
12741278
table->pos_in_locked_tables,
@@ -1281,7 +1285,8 @@ close_all_tables_for_name(THD *thd, TABLE_SHARE *share,
12811285
mysql_lock_remove(thd, thd->lock, table);
12821286

12831287
/* Inform handler that table will be dropped after close */
1284-
if (table->db_stat) /* Not true for partitioned tables. */
1288+
if (table->db_stat && /* Not true for partitioned tables. */
1289+
skip_table == NULL)
12851290
table->file->extra(HA_EXTRA_PREPARE_FOR_DROP);
12861291
close_thread_table(thd, prev);
12871292
}
@@ -1291,9 +1296,12 @@ close_all_tables_for_name(THD *thd, TABLE_SHARE *share,
12911296
prev= &table->next;
12921297
}
12931298
}
1294-
/* Remove the table share from the cache. */
1295-
tdc_remove_table(thd, TDC_RT_REMOVE_ALL, db, table_name,
1296-
FALSE);
1299+
if (skip_table == NULL)
1300+
{
1301+
/* Remove the table share from the cache. */
1302+
tdc_remove_table(thd, TDC_RT_REMOVE_ALL, db, table_name,
1303+
FALSE);
1304+
}
12971305
}
12981306

12991307

@@ -9136,6 +9144,15 @@ void tdc_flush_unused_tables()
91369144
instances (if there are no
91379145
used instances will also
91389146
remove TABLE_SHARE).
9147+
TDC_RT_REMOVE_NOT_OWN_KEEP_SHARE -
9148+
remove all TABLE instances
9149+
except those that belong to
9150+
this thread, but don't mark
9151+
TABLE_SHARE as old. There
9152+
should be no TABLE objects
9153+
used by other threads and
9154+
caller should have exclusive
9155+
metadata lock on the table.
91399156
@param db Name of database
91409157
@param table_name Name of table
91419158
@param has_lock If TRUE, LOCK_open is already acquired
@@ -9179,11 +9196,15 @@ void tdc_remove_table(THD *thd, enum_tdc_remove_table_type remove_type,
91799196
TDC does not contain old shares which don't have any tables
91809197
used.
91819198
*/
9182-
share->version= 0;
9199+
if (remove_type != TDC_RT_REMOVE_NOT_OWN_KEEP_SHARE)
9200+
share->version= 0;
91839201
table_cache_manager.free_table(thd, remove_type, share);
91849202
}
91859203
else
9204+
{
9205+
DBUG_ASSERT(remove_type != TDC_RT_REMOVE_NOT_OWN_KEEP_SHARE);
91869206
(void) my_hash_delete(&table_def_cache, (uchar*) share);
9207+
}
91879208
}
91889209

91899210
if (! has_lock)

‎sql/sql_base.h‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,8 @@ enum find_item_error_report_type {REPORT_ALL_ERRORS, REPORT_EXCEPT_NOT_FOUND,
6060
IGNORE_EXCEPT_NON_UNIQUE};
6161

6262
enum enum_tdc_remove_table_type {TDC_RT_REMOVE_ALL, TDC_RT_REMOVE_NOT_OWN,
63-
TDC_RT_REMOVE_UNUSED};
63+
TDC_RT_REMOVE_UNUSED,
64+
TDC_RT_REMOVE_NOT_OWN_KEEP_SHARE};
6465

6566
/* bits for last argument to remove_table_from_cache() */
6667
#define RTFC_NO_FLAG 0x0000
@@ -280,7 +281,8 @@ bool close_cached_tables(THD *thd, TABLE_LIST *tables,
280281
bool wait_for_refresh, ulong timeout);
281282
bool close_cached_connection_tables(THD *thd, LEX_STRING *connect_string);
282283
void close_all_tables_for_name(THD *thd, TABLE_SHARE *share,
283-
bool remove_from_locked_tables);
284+
bool remove_from_locked_tables,
285+
TABLE *skip_table);
284286
OPEN_TABLE_LIST *list_open_tables(THD *thd, const char *db, const char *wild);
285287
void tdc_remove_table(THD *thd, enum_tdc_remove_table_type remove_type,
286288
const char *db, const char *table_name,

‎sql/sql_partition.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6535,7 +6535,7 @@ static void alter_partition_lock_handling(ALTER_PARTITION_PARAM_TYPE *lpt)
65356535
/*
65366536
Remove all instances of the table and its locks and other resources.
65376537
*/
6538-
close_all_tables_for_name(thd, lpt->table->s, false);
6538+
close_all_tables_for_name(thd, lpt->table->s, false, NULL);
65396539
}
65406540
lpt->table= 0;
65416541
lpt->table_list->table= 0;
@@ -6623,7 +6623,7 @@ void handle_alter_part_error(ALTER_PARTITION_PARAM_TYPE *lpt,
66236623
}
66246624
/* Ensure the share is destroyed and reopened. */
66256625
part_info= lpt->part_info->get_clone();
6626-
close_all_tables_for_name(thd, table->s, false);
6626+
close_all_tables_for_name(thd, table->s, false, NULL);
66276627
}
66286628
else
66296629
{

‎sql/sql_partition_admin.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -619,8 +619,8 @@ bool Sql_cmd_alter_table_exchange_partition::
619619

620620
DEBUG_SYNC(thd, "swap_partition_after_wait");
621621

622-
close_all_tables_for_name(thd, swap_table->s, FALSE);
623-
close_all_tables_for_name(thd, part_table->s, FALSE);
622+
close_all_tables_for_name(thd, swap_table->s, false, NULL);
623+
close_all_tables_for_name(thd, part_table->s, false, NULL);
624624

625625
DEBUG_SYNC(thd, "swap_partition_before_rename");
626626

0 commit comments

Comments
 (0)