Skip to content

Commit be647ff

Browse files
montywimontywi
committed
Fixed deadlock with LOCK TABLES and ALTER TABLE
MDEV-21398 Deadlock (server hang) or assertion failure in Diagnostics_area::set_error_status upon ALTER under lock This failure could only happen if one locked the same table multiple times and then did an ALTER TABLE on the table. Major change is to change all instances of table->m_needs_reopen= true; to table->mark_table_for_reopen(); The main fix for the problem was to ensure that we mark all instances of the table in the locked_table_list and when we reopen the tables, we first close all tables before reopening and locking them. Other things: - Don't call thd->locked_tables_list.reopen_tables if there are no tables marked for reopen. (performance)
1 parent 6462af1 commit be647ff

File tree

15 files changed

+136
-35
lines changed

15 files changed

+136
-35
lines changed

‎mysql-test/r/lock.result

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -505,3 +505,17 @@ disconnect con1;
505505
connection default;
506506
UNLOCK TABLES;
507507
DROP TABLE t1, t2;
508+
#
509+
# MDEV-21398 Deadlock (server hang) or assertion failure in
510+
# Diagnostics_area::set_error_status upon ALTER under lock
511+
#
512+
CREATE TABLE t1 (a INT) ENGINE=MyISAM;
513+
LOCK TABLE t1 WRITE, t1 AS t1a READ;
514+
ALTER TABLE t1 CHANGE COLUMN IF EXISTS x xx INT;
515+
Warnings:
516+
Note 1054 Unknown column 'x' in 't1'
517+
UNLOCK TABLES;
518+
DROP TABLE t1;
519+
#
520+
# End of 10.2 tests
521+
#

‎mysql-test/t/lock.test

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -619,3 +619,17 @@ UNLOCK TABLES;
619619
UNLOCK TABLES;
620620
DROP TABLE t1, t2;
621621

622+
--echo #
623+
--echo # MDEV-21398 Deadlock (server hang) or assertion failure in
624+
--echo # Diagnostics_area::set_error_status upon ALTER under lock
625+
--echo #
626+
627+
CREATE TABLE t1 (a INT) ENGINE=MyISAM;
628+
LOCK TABLE t1 WRITE, t1 AS t1a READ;
629+
ALTER TABLE t1 CHANGE COLUMN IF EXISTS x xx INT;
630+
UNLOCK TABLES;
631+
DROP TABLE t1;
632+
633+
--echo #
634+
--echo # End of 10.2 tests
635+
--echo #

‎sql/sql_admin.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1087,7 +1087,7 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables,
10871087
}
10881088
/* Make sure this table instance is not reused after the operation. */
10891089
if (table->table)
1090-
table->table->m_needs_reopen= true;
1090+
table->table->mark_table_for_reopen();
10911091
}
10921092
result_code= result_code ? HA_ADMIN_FAILED : HA_ADMIN_OK;
10931093
table->next_local= save_next_local;
@@ -1212,7 +1212,7 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables,
12121212
trans_rollback(thd);
12131213
if (table && table->table)
12141214
{
1215-
table->table->m_needs_reopen= true;
1215+
table->table->mark_table_for_reopen();
12161216
table->table= 0;
12171217
}
12181218
close_thread_tables(thd); // Shouldn't be needed

‎sql/sql_base.cc

Lines changed: 60 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2161,9 +2161,9 @@ Locked_tables_list::init_locked_tables(THD *thd)
21612161
in reopen_tables(). reopen_tables() is a critical
21622162
path and we don't want to complicate it with extra allocations.
21632163
*/
2164-
m_reopen_array= (TABLE**)alloc_root(&m_locked_tables_root,
2165-
sizeof(TABLE*) *
2166-
(m_locked_tables_count+1));
2164+
m_reopen_array= (TABLE_LIST**)alloc_root(&m_locked_tables_root,
2165+
sizeof(TABLE_LIST*) *
2166+
(m_locked_tables_count+1));
21672167
if (m_reopen_array == NULL)
21682168
{
21692169
reset();
@@ -2273,6 +2273,7 @@ void Locked_tables_list::reset()
22732273
m_locked_tables_last= &m_locked_tables;
22742274
m_reopen_array= NULL;
22752275
m_locked_tables_count= 0;
2276+
some_table_marked_for_reopen= 0;
22762277
}
22772278

22782279

@@ -2368,7 +2369,7 @@ unlink_all_closed_tables(THD *thd, MYSQL_LOCK *lock, size_t reopen_count)
23682369
in reopen_tables() always links the opened table
23692370
to the beginning of the open_tables list.
23702371
*/
2371-
DBUG_ASSERT(thd->open_tables == m_reopen_array[reopen_count]);
2372+
DBUG_ASSERT(thd->open_tables == m_reopen_array[reopen_count]->table);
23722373

23732374
thd->open_tables->pos_in_locked_tables->table= NULL;
23742375
thd->open_tables->pos_in_locked_tables= 0;
@@ -2398,10 +2399,36 @@ unlink_all_closed_tables(THD *thd, MYSQL_LOCK *lock, size_t reopen_count)
23982399
}
23992400

24002401

2402+
/*
2403+
Mark all instances of the table to be reopened
2404+
2405+
This is only needed when LOCK TABLES is active
2406+
*/
2407+
2408+
void Locked_tables_list::mark_table_for_reopen(THD *thd, TABLE *table)
2409+
{
2410+
TABLE_SHARE *share= table->s;
2411+
2412+
for (TABLE_LIST *table_list= m_locked_tables;
2413+
table_list; table_list= table_list->next_global)
2414+
{
2415+
if (table_list->table->s == share)
2416+
table_list->table->internal_set_needs_reopen(true);
2417+
}
2418+
/* This is needed in the case where lock tables where not used */
2419+
table->internal_set_needs_reopen(true);
2420+
some_table_marked_for_reopen= 1;
2421+
}
2422+
2423+
24012424
/**
24022425
Reopen the tables locked with LOCK TABLES and temporarily closed
24032426
by a DDL statement or FLUSH TABLES.
24042427
2428+
@param need_reopen If set, reopen open tables that are marked with
2429+
for reopen.
2430+
If not set, reopen tables that where closed.
2431+
24052432
@note This function is a no-op if we're not under LOCK TABLES.
24062433
24072434
@return TRUE if an error reopening the tables. May happen in
@@ -2419,6 +2446,12 @@ Locked_tables_list::reopen_tables(THD *thd, bool need_reopen)
24192446
MYSQL_LOCK *merged_lock;
24202447
DBUG_ENTER("Locked_tables_list::reopen_tables");
24212448

2449+
DBUG_ASSERT(some_table_marked_for_reopen || !need_reopen);
2450+
2451+
2452+
/* Reset flag that some table was marked for reopen */
2453+
some_table_marked_for_reopen= 0;
2454+
24222455
for (TABLE_LIST *table_list= m_locked_tables;
24232456
table_list; table_list= table_list->next_global)
24242457
{
@@ -2442,24 +2475,32 @@ Locked_tables_list::reopen_tables(THD *thd, bool need_reopen)
24422475
else
24432476
{
24442477
if (table_list->table) /* The table was not closed */
2445-
continue;
2446-
}
2447-
2448-
/* Links into thd->open_tables upon success */
2449-
if (open_table(thd, table_list, &ot_ctx))
2450-
{
2451-
unlink_all_closed_tables(thd, 0, reopen_count);
2452-
DBUG_RETURN(TRUE);
2478+
continue;
24532479
}
2454-
table_list->table->pos_in_locked_tables= table_list;
2455-
/* See also the comment on lock type in init_locked_tables(). */
2456-
table_list->table->reginfo.lock_type= table_list->lock_type;
24572480

24582481
DBUG_ASSERT(reopen_count < m_locked_tables_count);
2459-
m_reopen_array[reopen_count++]= table_list->table;
2482+
m_reopen_array[reopen_count++]= table_list;
24602483
}
24612484
if (reopen_count)
24622485
{
2486+
TABLE **tables= (TABLE**) my_alloca(reopen_count * sizeof(TABLE*));
2487+
2488+
for (uint i= 0 ; i < reopen_count ; i++)
2489+
{
2490+
TABLE_LIST *table_list= m_reopen_array[i];
2491+
/* Links into thd->open_tables upon success */
2492+
if (open_table(thd, table_list, &ot_ctx))
2493+
{
2494+
unlink_all_closed_tables(thd, 0, i);
2495+
my_afree((void*) tables);
2496+
DBUG_RETURN(TRUE);
2497+
}
2498+
tables[i]= table_list->table;
2499+
table_list->table->pos_in_locked_tables= table_list;
2500+
/* See also the comment on lock type in init_locked_tables(). */
2501+
table_list->table->reginfo.lock_type= table_list->lock_type;
2502+
}
2503+
24632504
thd->in_lock_tables= 1;
24642505
/*
24652506
We re-lock all tables with mysql_lock_tables() at once rather
@@ -2472,7 +2513,7 @@ Locked_tables_list::reopen_tables(THD *thd, bool need_reopen)
24722513
works fine. Patching legacy code of thr_lock.c is risking to
24732514
break something else.
24742515
*/
2475-
lock= mysql_lock_tables(thd, m_reopen_array, reopen_count,
2516+
lock= mysql_lock_tables(thd, tables, reopen_count,
24762517
MYSQL_OPEN_REOPEN);
24772518
thd->in_lock_tables= 0;
24782519
if (lock == NULL || (merged_lock=
@@ -2481,9 +2522,11 @@ Locked_tables_list::reopen_tables(THD *thd, bool need_reopen)
24812522
unlink_all_closed_tables(thd, lock, reopen_count);
24822523
if (! thd->killed)
24832524
my_error(ER_LOCK_DEADLOCK, MYF(0));
2525+
my_afree((void*) tables);
24842526
DBUG_RETURN(TRUE);
24852527
}
24862528
thd->lock= merged_lock;
2529+
my_afree((void*) tables);
24872530
}
24882531
DBUG_RETURN(FALSE);
24892532
}

‎sql/sql_class.h

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1823,20 +1823,23 @@ class Locked_tables_list
18231823
TABLE_LIST *m_locked_tables;
18241824
TABLE_LIST **m_locked_tables_last;
18251825
/** An auxiliary array used only in reopen_tables(). */
1826-
TABLE **m_reopen_array;
1826+
TABLE_LIST **m_reopen_array;
18271827
/**
18281828
Count the number of tables in m_locked_tables list. We can't
18291829
rely on thd->lock->table_count because it excludes
18301830
non-transactional temporary tables. We need to know
18311831
an exact number of TABLE objects.
18321832
*/
1833-
size_t m_locked_tables_count;
1833+
uint m_locked_tables_count;
18341834
public:
1835+
bool some_table_marked_for_reopen;
1836+
18351837
Locked_tables_list()
18361838
:m_locked_tables(NULL),
18371839
m_locked_tables_last(&m_locked_tables),
18381840
m_reopen_array(NULL),
1839-
m_locked_tables_count(0)
1841+
m_locked_tables_count(0),
1842+
some_table_marked_for_reopen(0)
18401843
{
18411844
init_sql_alloc(&m_locked_tables_root, MEM_ROOT_BLOCK_SIZE, 0,
18421845
MYF(MY_THREAD_SPECIFIC));
@@ -1859,6 +1862,7 @@ class Locked_tables_list
18591862
bool restore_lock(THD *thd, TABLE_LIST *dst_table_list, TABLE *table,
18601863
MYSQL_LOCK *lock);
18611864
void add_back_last_deleted_lock(TABLE_LIST *dst_table_list);
1865+
void mark_table_for_reopen(THD *thd, TABLE *table);
18621866
};
18631867

18641868

‎sql/sql_parse.cc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5998,7 +5998,8 @@ mysql_execute_command(THD *thd)
59985998
lex->unit.cleanup();
59995999

60006000
/* close/reopen tables that were marked to need reopen under LOCK TABLES */
6001-
if (! thd->lex->requires_prelocking())
6001+
if (unlikely(thd->locked_tables_list.some_table_marked_for_reopen) &&
6002+
!thd->lex->requires_prelocking())
60026003
thd->locked_tables_list.reopen_tables(thd, true);
60036004

60046005
if (! thd->in_sub_stmt)

‎sql/sql_partition.cc

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4592,7 +4592,7 @@ uint prep_alter_part_table(THD *thd, TABLE *table, Alter_info *alter_info,
45924592
{
45934593
*fast_alter_table= true;
45944594
/* Force table re-open for consistency with the main case. */
4595-
table->m_needs_reopen= true;
4595+
table->mark_table_for_reopen();
45964596
}
45974597
else
45984598
{
@@ -4640,7 +4640,7 @@ uint prep_alter_part_table(THD *thd, TABLE *table, Alter_info *alter_info,
46404640
must be reopened.
46414641
*/
46424642
*fast_alter_table= true;
4643-
table->m_needs_reopen= true;
4643+
table->mark_table_for_reopen();
46444644
}
46454645
else
46464646
{
@@ -6418,7 +6418,7 @@ void handle_alter_part_error(ALTER_PARTITION_PARAM_TYPE *lpt,
64186418
THD *thd= lpt->thd;
64196419
TABLE *table= lpt->table;
64206420
DBUG_ENTER("handle_alter_part_error");
6421-
DBUG_ASSERT(table->m_needs_reopen);
6421+
DBUG_ASSERT(table->needs_reopen());
64226422

64236423
if (close_table)
64246424
{
@@ -6637,7 +6637,7 @@ uint fast_alter_partition_table(THD *thd, TABLE *table,
66376637
bool frm_install= FALSE;
66386638
MDL_ticket *mdl_ticket= table->mdl_ticket;
66396639
DBUG_ENTER("fast_alter_partition_table");
6640-
DBUG_ASSERT(table->m_needs_reopen);
6640+
DBUG_ASSERT(table->needs_reopen());
66416641

66426642
part_info= table->part_info;
66436643
lpt->thd= thd;

‎sql/sql_plugin.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1844,7 +1844,7 @@ static void plugin_load(MEM_ROOT *tmp_root)
18441844
sql_print_error(ER_THD(new_thd, ER_GET_ERRNO), my_errno,
18451845
table->file->table_type());
18461846
end_read_record(&read_record_info);
1847-
table->m_needs_reopen= TRUE; // Force close to free memory
1847+
table->mark_table_for_reopen();
18481848
close_mysql_tables(new_thd);
18491849
end:
18501850
delete new_thd;

‎sql/sql_table.cc

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7783,7 +7783,8 @@ mysql_prepare_alter_table(THD *thd, TABLE *table,
77837783
if (field->default_value)
77847784
field->default_value->expr->walk(&Item::rename_fields_processor, 1,
77857785
&column_rename_param);
7786-
table->m_needs_reopen= 1; // because new column name is on thd->mem_root
7786+
// Force reopen because new column name is on thd->mem_root
7787+
table->mark_table_for_reopen();
77877788
}
77887789

77897790
/* Check if field is changed */
@@ -8195,7 +8196,8 @@ mysql_prepare_alter_table(THD *thd, TABLE *table,
81958196
{
81968197
check->expr->walk(&Item::rename_fields_processor, 1,
81978198
&column_rename_param);
8198-
table->m_needs_reopen= 1; // because new column name is on thd->mem_root
8199+
// Force reopen because new column name is on thd->mem_root
8200+
table->mark_table_for_reopen();
81998201
}
82008202
new_constraint_list.push_back(check, thd->mem_root);
82018203
}

‎sql/sql_udf.cc

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -255,7 +255,9 @@ void udf_init()
255255
if (error > 0)
256256
sql_print_error("Got unknown error: %d", my_errno);
257257
end_read_record(&read_record_info);
258-
table->m_needs_reopen= TRUE; // Force close to free memory
258+
259+
// Force close to free memory
260+
table->mark_table_for_reopen();
259261

260262
end:
261263
close_mysql_tables(new_thd);

0 commit comments

Comments
 (0)