Skip to content

Commit

Permalink
Fix a deadlock in thd_report_wait_for()
Browse files Browse the repository at this point in the history
Unlike commit a54abf0 claimed,
the caller of THD::awake() may actually hold the InnoDB lock_sys->mutex.
That commit introduced a deadlock of threads in the replication slave
when running the test rpl.rpl_parallel_optimistic_nobinlog.

lock_trx_handle_wait(): Expect the callers to acquire and release
lock_sys->mutex and trx->mutex.

innobase_kill_query(): Restore the logic for conditionally acquiring
and releasing the mutexes. THD::awake() can be called from inside
InnoDB while holding one or both mutexes, via thd_report_wait_for() and
via wsrep_innobase_kill_one_trx().
  • Loading branch information
dr-m committed Mar 16, 2018
1 parent dbb3960 commit ca40330
Show file tree
Hide file tree
Showing 6 changed files with 70 additions and 30 deletions.
23 changes: 23 additions & 0 deletions storage/innobase/handler/ha_innodb.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4915,8 +4915,31 @@ static void innobase_kill_query(handlerton*, THD* thd, enum thd_kill_levels)

if (trx_t* trx = thd_to_trx(thd)) {
ut_ad(trx->mysql_thd == thd);

switch (trx->abort_type) {
case TRX_WSREP_ABORT:
break;
case TRX_SERVER_ABORT:
if (!wsrep_thd_is_BF(trx->mysql_thd, FALSE)) {
lock_mutex_enter();
}
/* fall through */
case TRX_REPLICATION_ABORT:
trx_mutex_enter(trx);
}
/* Cancel a pending lock request if there are any */
lock_trx_handle_wait(trx);
switch (trx->abort_type) {
case TRX_WSREP_ABORT:
break;
case TRX_SERVER_ABORT:
if (!wsrep_thd_is_BF(trx->mysql_thd, FALSE)) {
lock_mutex_exit();
}
/* fall through */
case TRX_REPLICATION_ABORT:
trx_mutex_exit(trx);
}
}

DBUG_VOID_RETURN;
Expand Down
23 changes: 8 additions & 15 deletions storage/innobase/lock/lock0lock.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7977,26 +7977,19 @@ lock_trx_handle_wait(
/*=================*/
trx_t* trx) /*!< in/out: trx lock state */
{
dberr_t err;

lock_mutex_enter();

trx_mutex_enter(trx);
ut_ad(lock_mutex_own());
ut_ad(trx_mutex_own(trx));

if (trx->lock.was_chosen_as_deadlock_victim) {
err = DB_DEADLOCK;
} else if (trx->lock.wait_lock != NULL) {
lock_cancel_waiting_and_release(trx->lock.wait_lock);
err = DB_LOCK_WAIT;
} else {
return DB_DEADLOCK;
}
if (!trx->lock.wait_lock) {
/* The lock was probably granted before we got here. */
err = DB_SUCCESS;
return DB_SUCCESS;
}

lock_mutex_exit();
trx_mutex_exit(trx);

return(err);
lock_cancel_waiting_and_release(trx->lock.wait_lock);
return DB_LOCK_WAIT;
}

/*********************************************************************//**
Expand Down
4 changes: 4 additions & 0 deletions storage/innobase/row/row0sel.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4615,7 +4615,11 @@ row_search_for_mysql(
a deadlock and the transaction had to wait then
release the lock it is waiting on. */

lock_mutex_enter();
trx_mutex_enter(trx);
err = lock_trx_handle_wait(trx);
lock_mutex_exit();
trx_mutex_exit(trx);

switch (err) {
case DB_SUCCESS:
Expand Down
23 changes: 23 additions & 0 deletions storage/xtradb/handler/ha_innodb.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5511,8 +5511,31 @@ static void innobase_kill_query(handlerton*, THD* thd, enum thd_kill_levels)
#endif /* WITH_WSREP */
if (trx_t* trx = thd_to_trx(thd)) {
ut_ad(trx->mysql_thd == thd);

switch (trx->abort_type) {
case TRX_WSREP_ABORT:
break;
case TRX_SERVER_ABORT:
if (!wsrep_thd_is_BF(trx->mysql_thd, FALSE)) {
lock_mutex_enter();
}
/* fall through */
case TRX_REPLICATION_ABORT:
trx_mutex_enter(trx);
}
/* Cancel a pending lock request if there are any */
lock_trx_handle_wait(trx);
switch (trx->abort_type) {
case TRX_WSREP_ABORT:
break;
case TRX_SERVER_ABORT:
if (!wsrep_thd_is_BF(trx->mysql_thd, FALSE)) {
lock_mutex_exit();
}
/* fall through */
case TRX_REPLICATION_ABORT:
trx_mutex_exit(trx);
}
}

DBUG_VOID_RETURN;
Expand Down
23 changes: 8 additions & 15 deletions storage/xtradb/lock/lock0lock.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8087,26 +8087,19 @@ lock_trx_handle_wait(
/*=================*/
trx_t* trx) /*!< in/out: trx lock state */
{
dberr_t err;

lock_mutex_enter();

trx_mutex_enter(trx);
ut_ad(lock_mutex_own());
ut_ad(trx_mutex_own(trx));

if (trx->lock.was_chosen_as_deadlock_victim) {
err = DB_DEADLOCK;
} else if (trx->lock.wait_lock != NULL) {
lock_cancel_waiting_and_release(trx->lock.wait_lock);
err = DB_LOCK_WAIT;
} else {
return DB_DEADLOCK;
}
if (!trx->lock.wait_lock) {
/* The lock was probably granted before we got here. */
err = DB_SUCCESS;
return DB_SUCCESS;
}

lock_mutex_exit();
trx_mutex_exit(trx);

return(err);
lock_cancel_waiting_and_release(trx->lock.wait_lock);
return DB_LOCK_WAIT;
}

/*********************************************************************//**
Expand Down
4 changes: 4 additions & 0 deletions storage/xtradb/row/row0sel.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4627,7 +4627,11 @@ row_search_for_mysql(
a deadlock and the transaction had to wait then
release the lock it is waiting on. */

lock_mutex_enter();
trx_mutex_enter(trx);
err = lock_trx_handle_wait(trx);
lock_mutex_exit();
trx_mutex_exit(trx);

switch (err) {
case DB_SUCCESS:
Expand Down

0 comments on commit ca40330

Please sign in to comment.