1berry/MariaDB-server
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fixed deadlock with LOCK TABLES and ALTER TABLE

Browse Source 
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)
This commit is contained in:
Monty 2020-05-22 18:02:24 +03:00
parent 6462af1c2e
commit be647ff14d
15 changed files with 136 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
mysql-test/r/lock.result
View File

@ -505,3 +505,17 @@ disconnect con1;
connection default;
UNLOCK TABLES;
DROP TABLE t1, t2;
#
# MDEV-21398 Deadlock (server hang) or assertion failure in
# Diagnostics_area::set_error_status upon ALTER under lock
#
CREATE TABLE t1 (a INT) ENGINE=MyISAM;
LOCK TABLE t1 WRITE, t1 AS t1a READ;
ALTER TABLE t1 CHANGE COLUMN IF EXISTS x xx INT;
Warnings:
Note 1054 Unknown column 'x' in 't1'
UNLOCK TABLES;
DROP TABLE t1;
#
# End of 10.2 tests
#

14
mysql-test/t/lock.test
View File

@ -619,3 +619,17 @@ UNLOCK TABLES;
UNLOCK TABLES;
DROP TABLE t1, t2;
--echo #
--echo # MDEV-21398 Deadlock (server hang) or assertion failure in
--echo # Diagnostics_area::set_error_status upon ALTER under lock
--echo #
CREATE TABLE t1 (a INT) ENGINE=MyISAM;
LOCK TABLE t1 WRITE, t1 AS t1a READ;
ALTER TABLE t1 CHANGE COLUMN IF EXISTS x xx INT;
UNLOCK TABLES;
DROP TABLE t1;
--echo #
--echo # End of 10.2 tests
--echo #

4
sql/sql_admin.cc
View File

@ -1087,7 +1087,7 @@ send_result_message:
}
/* Make sure this table instance is not reused after the operation. */
if (table->table)
table->table->m_needs_reopen= true;
table->table->mark_table_for_reopen();
}
result_code= result_code ? HA_ADMIN_FAILED : HA_ADMIN_OK;
table->next_local= save_next_local;
@ -1212,7 +1212,7 @@ err:
trans_rollback(thd);
if (table && table->table)
{
table->table->m_needs_reopen= true;
table->table->mark_table_for_reopen();
table->table= 0;
}
close_thread_tables(thd); // Shouldn't be needed

77
sql/sql_base.cc
View File

@ -2161,9 +2161,9 @@ Locked_tables_list::init_locked_tables(THD *thd)
in reopen_tables(). reopen_tables() is a critical
path and we don't want to complicate it with extra allocations.
*/
m_reopen_array= (TABLE**)alloc_root(&m_locked_tables_root,
sizeof(TABLE*) *
(m_locked_tables_count+1));
m_reopen_array= (TABLE_LIST**)alloc_root(&m_locked_tables_root,
sizeof(TABLE_LIST*) *
(m_locked_tables_count+1));
if (m_reopen_array == NULL)
{
reset();
@ -2273,6 +2273,7 @@ void Locked_tables_list::reset()
m_locked_tables_last= &m_locked_tables;
m_reopen_array= NULL;
m_locked_tables_count= 0;
some_table_marked_for_reopen= 0;
}
@ -2368,7 +2369,7 @@ unlink_all_closed_tables(THD *thd, MYSQL_LOCK *lock, size_t reopen_count)
in reopen_tables() always links the opened table
to the beginning of the open_tables list.
*/
DBUG_ASSERT(thd->open_tables == m_reopen_array[reopen_count]);
DBUG_ASSERT(thd->open_tables == m_reopen_array[reopen_count]->table);
thd->open_tables->pos_in_locked_tables->table= NULL;
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)
}
/*
Mark all instances of the table to be reopened
This is only needed when LOCK TABLES is active
*/
void Locked_tables_list::mark_table_for_reopen(THD *thd, TABLE *table)
{
TABLE_SHARE *share= table->s;
for (TABLE_LIST *table_list= m_locked_tables;
table_list; table_list= table_list->next_global)
{
if (table_list->table->s == share)
table_list->table->internal_set_needs_reopen(true);
}
/* This is needed in the case where lock tables where not used */
table->internal_set_needs_reopen(true);
some_table_marked_for_reopen= 1;
}
/**
Reopen the tables locked with LOCK TABLES and temporarily closed
by a DDL statement or FLUSH TABLES.
@param need_reopen If set, reopen open tables that are marked with
for reopen.
If not set, reopen tables that where closed.
@note This function is a no-op if we're not under LOCK TABLES.
@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)
MYSQL_LOCK *merged_lock;
DBUG_ENTER("Locked_tables_list::reopen_tables");
DBUG_ASSERT(some_table_marked_for_reopen || !need_reopen);
/* Reset flag that some table was marked for reopen */
some_table_marked_for_reopen= 0;
for (TABLE_LIST *table_list= m_locked_tables;
table_list; table_list= table_list->next_global)
{
@ -2442,24 +2475,32 @@ Locked_tables_list::reopen_tables(THD *thd, bool need_reopen)
else
{
if (table_list->table) /* The table was not closed */
continue;
continue;
}
/* Links into thd->open_tables upon success */
if (open_table(thd, table_list, &ot_ctx))
{
unlink_all_closed_tables(thd, 0, reopen_count);
DBUG_RETURN(TRUE);
}
table_list->table->pos_in_locked_tables= table_list;
/* See also the comment on lock type in init_locked_tables(). */
table_list->table->reginfo.lock_type= table_list->lock_type;
DBUG_ASSERT(reopen_count < m_locked_tables_count);
m_reopen_array[reopen_count++]= table_list->table;
m_reopen_array[reopen_count++]= table_list;
}
if (reopen_count)
{
TABLE **tables= (TABLE**) my_alloca(reopen_count * sizeof(TABLE*));
for (uint i= 0 ; i < reopen_count ; i++)
{
TABLE_LIST *table_list= m_reopen_array[i];
/* Links into thd->open_tables upon success */
if (open_table(thd, table_list, &ot_ctx))
{
unlink_all_closed_tables(thd, 0, i);
my_afree((void*) tables);
DBUG_RETURN(TRUE);
}
tables[i]= table_list->table;
table_list->table->pos_in_locked_tables= table_list;
/* See also the comment on lock type in init_locked_tables(). */
table_list->table->reginfo.lock_type= table_list->lock_type;
}
thd->in_lock_tables= 1;
/*
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)
works fine. Patching legacy code of thr_lock.c is risking to
break something else.
*/
lock= mysql_lock_tables(thd, m_reopen_array, reopen_count,
lock= mysql_lock_tables(thd, tables, reopen_count,
MYSQL_OPEN_REOPEN);
thd->in_lock_tables= 0;
if (lock == NULL || (merged_lock=
@ -2481,9 +2522,11 @@ Locked_tables_list::reopen_tables(THD *thd, bool need_reopen)
unlink_all_closed_tables(thd, lock, reopen_count);
if (! thd->killed)
my_error(ER_LOCK_DEADLOCK, MYF(0));
my_afree((void*) tables);
DBUG_RETURN(TRUE);
}
thd->lock= merged_lock;
my_afree((void*) tables);
}
DBUG_RETURN(FALSE);
}

10
sql/sql_class.h
View File

@ -1823,20 +1823,23 @@ private:
TABLE_LIST *m_locked_tables;
TABLE_LIST **m_locked_tables_last;
/** An auxiliary array used only in reopen_tables(). */
TABLE **m_reopen_array;
TABLE_LIST **m_reopen_array;
/**
Count the number of tables in m_locked_tables list. We can't
rely on thd->lock->table_count because it excludes
non-transactional temporary tables. We need to know
an exact number of TABLE objects.
*/
size_t m_locked_tables_count;
uint m_locked_tables_count;
public:
bool some_table_marked_for_reopen;
Locked_tables_list()
:m_locked_tables(NULL),
m_locked_tables_last(&m_locked_tables),
m_reopen_array(NULL),
m_locked_tables_count(0)
m_locked_tables_count(0),
some_table_marked_for_reopen(0)
{
init_sql_alloc(&m_locked_tables_root, MEM_ROOT_BLOCK_SIZE, 0,
MYF(MY_THREAD_SPECIFIC));
@ -1859,6 +1862,7 @@ public:
bool restore_lock(THD *thd, TABLE_LIST *dst_table_list, TABLE *table,
MYSQL_LOCK *lock);
void add_back_last_deleted_lock(TABLE_LIST *dst_table_list);
void mark_table_for_reopen(THD *thd, TABLE *table);
};

3
sql/sql_parse.cc
View File

@ -5998,7 +5998,8 @@ finish:
lex->unit.cleanup();
/* close/reopen tables that were marked to need reopen under LOCK TABLES */
if (! thd->lex->requires_prelocking())
if (unlikely(thd->locked_tables_list.some_table_marked_for_reopen) &&
!thd->lex->requires_prelocking())
thd->locked_tables_list.reopen_tables(thd, true);
if (! thd->in_sub_stmt)

8
sql/sql_partition.cc
View File

@ -4592,7 +4592,7 @@ uint prep_alter_part_table(THD *thd, TABLE *table, Alter_info *alter_info,
{
*fast_alter_table= true;
/* Force table re-open for consistency with the main case. */
table->m_needs_reopen= true;
table->mark_table_for_reopen();
}
else
{
@ -4640,7 +4640,7 @@ uint prep_alter_part_table(THD *thd, TABLE *table, Alter_info *alter_info,
must be reopened.
*/
*fast_alter_table= true;
table->m_needs_reopen= true;
table->mark_table_for_reopen();
}
else
{
@ -6418,7 +6418,7 @@ void handle_alter_part_error(ALTER_PARTITION_PARAM_TYPE *lpt,
THD *thd= lpt->thd;
TABLE *table= lpt->table;
DBUG_ENTER("handle_alter_part_error");
DBUG_ASSERT(table->m_needs_reopen);
DBUG_ASSERT(table->needs_reopen());
if (close_table)
{
@ -6637,7 +6637,7 @@ uint fast_alter_partition_table(THD *thd, TABLE *table,
bool frm_install= FALSE;
MDL_ticket *mdl_ticket= table->mdl_ticket;
DBUG_ENTER("fast_alter_partition_table");
DBUG_ASSERT(table->m_needs_reopen);
DBUG_ASSERT(table->needs_reopen());
part_info= table->part_info;
lpt->thd= thd;

2
sql/sql_plugin.cc
View File

@ -1844,7 +1844,7 @@ static void plugin_load(MEM_ROOT *tmp_root)
sql_print_error(ER_THD(new_thd, ER_GET_ERRNO), my_errno,
table->file->table_type());
end_read_record(&read_record_info);
table->m_needs_reopen= TRUE; // Force close to free memory
table->mark_table_for_reopen();
close_mysql_tables(new_thd);
end:
delete new_thd;

6
sql/sql_table.cc
View File

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

4
sql/sql_udf.cc
View File

@ -255,7 +255,9 @@ void udf_init()
if (error > 0)
sql_print_error("Got unknown error: %d", my_errno);
end_read_record(&read_record_info);
table->m_needs_reopen= TRUE; // Force close to free memory
// Force close to free memory
table->mark_table_for_reopen();
end:
close_mysql_tables(new_thd);

11
sql/table.cc
View File

@ -8647,3 +8647,14 @@ void TABLE::initialize_quick_structures()
bzero(quick_costs, sizeof(quick_costs));
bzero(quick_n_ranges, sizeof(quick_n_ranges));
}
/*
Mark table to be reopened after query
*/
void TABLE::mark_table_for_reopen()
{
THD *thd= in_use;
DBUG_ASSERT(thd);
thd->locked_tables_list.mark_table_for_reopen(thd, this);
}

12
sql/table.h
View File

@ -1290,8 +1290,8 @@ public:
bool insert_or_update; /* Can be used by the handler */
bool alias_name_used; /* true if table_name is alias */
bool get_fields_in_item_tree; /* Signal to fix_field */
bool m_needs_reopen;
private:
bool m_needs_reopen;
bool created; /* For tmp tables. TRUE <=> tmp table was actually created.*/
public:
#ifdef HAVE_REPLICATION
@ -1401,6 +1401,16 @@ public:
/** Should this instance of the table be reopened? */
inline bool needs_reopen()
{ return !db_stat || m_needs_reopen; }
/*
Mark that all current connection instances of the table should be
reopen at end of statement
*/
void mark_table_for_reopen();
/* Should only be called from Locked_tables_list::mark_table_for_reopen() */
void internal_set_needs_reopen(bool value)
{
m_needs_reopen= value;
}
bool alloc_keys(uint key_count);
bool check_tmp_key(uint key, uint key_parts,

2
sql/temporary_tables.cc
View File

@ -1065,7 +1065,7 @@ TABLE *THD::find_temporary_table(const char *key, uint key_length,
case TMP_TABLE_ANY: found= true; break;
}
}
if (table && unlikely(table->m_needs_reopen))
if (table && unlikely(table->needs_reopen()))
{
share->all_tmp_tables.remove(table);
free_temporary_table(table);

2
sql/tztime.cc
View File

@ -1699,7 +1699,7 @@ my_tz_init(THD *org_thd, const char *default_tzname, my_bool bootstrap)
{
tl->table->use_all_columns();
/* Force close at the end of the function to free memory. */
tl->table->m_needs_reopen= TRUE;
tl->table->mark_table_for_reopen();
}
/*

2
storage/innobase/handler/ha_innodb.cc
View File

@ -3468,7 +3468,7 @@ ha_innobase::reset_template(void)
/* Force table to be freed in close_thread_table(). */
DBUG_EXECUTE_IF("free_table_in_fts_query",
if (m_prebuilt->in_fts_query) {
table->m_needs_reopen = true;
table->mark_table_for_reopen();
}
);