Skip to content

MDEV-39344: trx_disconnect_prepared() uses wrong mutex #5094

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dr-m wants to merge 1 commit into
base: 10.6
Choose a base branch
from
Open

MDEV-39344: trx_disconnect_prepared() uses wrong mutex #5094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 26 additions & 40 deletions storage/innobase/buf/buf0buf.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1534,42 +1534,6 @@ inline void buf_pool_t::page_hash_table::write_unlock_all() noexcept
}


namespace
{

struct find_interesting_trx
{
void operator()(const trx_t &trx)
{
if (!trx.is_started())
return;
if (trx.mysql_thd == nullptr)
return;
if (withdraw_started <= trx.start_time_micro)
return;

if (!found)
{
sql_print_warning("InnoDB: The following trx might hold "
"the blocks in buffer pool to "
"be withdrawn. Buffer pool "
"resizing can complete only "
"after all the transactions "
"below release the blocks.");
found= true;
}

lock_trx_print_wait_and_mvcc_state(stderr, &trx, current_time);
}

bool &found;
/** microsecond_interval_timer() */
const ulonglong withdraw_started;
const my_hrtime_t current_time;
};

} // namespace

/** Resize from srv_buf_pool_old_size to srv_buf_pool_size. */
inline void buf_pool_t::resize()
{
Expand Down Expand Up @@ -1656,15 +1620,37 @@ inline void buf_pool_t::resize()
message_interval *= 2;
}

bool found= false;
find_interesting_trx f
{found, withdraw_started, my_hrtime_coarse()};
withdraw_started = current_time;
const my_hrtime_t current_hrtime{my_hrtime_coarse()};
bool found{false};
Comment on lines -1659 to +1625
Copy link
Copy Markdown
Contributor Author

@dr-m dr-m May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to this file buf0buf.cc are only applicable to the 10.6 branches that do not include #3107. We’re only replacing callback-based traversal with an inline range for loop here.


/* This is going to exceed the maximum size of a
memory transaction. */
LockMutexGuard g{SRW_LOCK_CALL};
trx_sys.trx_list.for_each(f);
for (const trx_t& trx: trx_sys.trx_list) {
if (!trx.is_started() || !trx.mysql_thd
|| withdraw_started <= trx.start_time_micro) {
Comment on lines +1631 to +1632
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Accessing trx.mysql_thd and trx.is_started() without holding trx.mutex is a data race. These fields are modified in trx_t::disconnect_prepared() under the protection of trx.mutex. While the impact in this context (buffer pool resizing diagnostics) is likely limited to incorrect warnings, it is technically undefined behavior. Consider acquiring the transaction mutex before checking these fields.

continue;
}

if (!found) {
sql_print_warning("InnoDB: The following "
"trx might hold "
"the blocks in "
"buffer pool to "
"be withdrawn. "
"Buffer pool "
"resizing can "
"complete only "
"after all "
"the transactions "
"below release the blocks.");
found = true;
}

lock_trx_print_wait_and_mvcc_state(stderr, &trx,
current_hrtime);
}
}

if (should_retry_withdraw) {
Expand Down
2 changes: 1 addition & 1 deletion storage/innobase/handler/ha_innodb.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2953,7 +2953,7 @@ static int innobase_close_connection(handlerton *, THD *thd) noexcept
case TRX_STATE_PREPARED:
if (trx->has_logged_persistent())
{
trx_disconnect_prepared(trx);
trx->disconnect_prepared();
return 0;
}
/* fall through */
Expand Down
81 changes: 7 additions & 74 deletions storage/innobase/include/trx0sys.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -781,58 +781,6 @@ class rw_trx_hash_t
}
};

class thread_safe_trx_ilist_t
{
public:
void create() { mysql_mutex_init(trx_sys_mutex_key, &mutex, nullptr); }
void close() { mysql_mutex_destroy(&mutex); }
Comment on lines -787 to -788
Copy link
Copy Markdown
Contributor Author

@dr-m dr-m May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I forgot to remove the trx_sys_mutex_key. If it turns out that this change will reduce performance due to increased contention on lock_sys.latch, I’ll put back the mutex, preferrably as srw_mutex or srw_lock_low to reduce overhead.


bool empty() const
{
mysql_mutex_lock(&mutex);
auto result= trx_list.empty();
mysql_mutex_unlock(&mutex);
return result;
}

void push_front(trx_t &trx)
{
mysql_mutex_lock(&mutex);
trx_list.push_front(trx);
mysql_mutex_unlock(&mutex);
}

void remove(trx_t &trx)
{
mysql_mutex_lock(&mutex);
trx_list.remove(trx);
mysql_mutex_unlock(&mutex);
}

template <typename Callable> void for_each(Callable &&callback) const
{
mysql_mutex_lock(&mutex);
for (const auto &trx : trx_list)
callback(trx);
mysql_mutex_unlock(&mutex);
}

template <typename Callable> void for_each(Callable &&callback)
{
mysql_mutex_lock(&mutex);
for (auto &trx : trx_list)
callback(trx);
mysql_mutex_unlock(&mutex);
}

void freeze() const { mysql_mutex_lock(&mutex); }
void unfreeze() const { mysql_mutex_unlock(&mutex); }

private:
alignas(CPU_LEVEL1_DCACHE_LINESIZE) mutable mysql_mutex_t mutex;
alignas(CPU_LEVEL1_DCACHE_LINESIZE) ilist<trx_t> trx_list;
};

/** The transaction system central memory data structure. */
class trx_sys_t
{
Expand All @@ -858,9 +806,10 @@ class trx_sys_t
bool m_initialised;

public:
/** List of all transactions. */
thread_safe_trx_ilist_t trx_list;
/** List of all transactions; protected by lock_sys.latch */
ilist<trx_t> trx_list;

alignas(CPU_LEVEL1_DCACHE_LINESIZE)
/** Temporary rollback segments */
trx_rseg_t temp_rsegs[TRX_SYS_N_RSEGS];

Expand Down Expand Up @@ -1102,7 +1051,7 @@ class trx_sys_t
void close();

/** @return total number of active (non-prepared) transactions */
size_t any_active_transactions(size_t *prepared= nullptr);
size_t any_active_transactions(size_t *prepared= nullptr) noexcept;


/**
Expand Down Expand Up @@ -1179,21 +1128,15 @@ class trx_sys_t

@param trx transaction
*/
void register_trx(trx_t *trx)
{
trx_list.push_front(*trx);
}
inline void register_trx(trx_t *trx) noexcept;


/**
Deregisters transaction in trx_sys.

@param trx transaction
*/
void deregister_trx(trx_t *trx)
{
trx_list.remove(*trx);
}
inline void deregister_trx(trx_t *trx) noexcept;


/**
Expand All @@ -1207,17 +1150,7 @@ class trx_sys_t


/** @return the number of active views */
size_t view_count() const
{
size_t count= 0;

trx_list.for_each([&count](const trx_t &trx) {
if (trx.read_view.is_open())
++count;
});

return count;
}
size_t view_count() const noexcept;

/** Disable further allocation of transactions in a rollback segment
that are subject to innodb_undo_log_truncate=ON
Expand Down
8 changes: 3 additions & 5 deletions storage/innobase/include/trx0trx.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,6 @@ trx_t *trx_create();
/** At shutdown, frees a transaction object. */
void trx_free_at_shutdown(trx_t *trx);

/** Disconnect a prepared transaction from MySQL.
@param[in,out] trx transaction */
void trx_disconnect_prepared(trx_t *trx);

/** Initialize (resurrect) transactions at startup. */
dberr_t trx_lists_init_at_db_start();

Expand Down Expand Up @@ -730,7 +726,7 @@ struct trx_t : ilist_node<>
This field is accessed by the thread that owns the transaction,
without holding any mutex.
There is only one foreign-thread access in trx_print_low()
and a possible race condition with trx_disconnect_prepared(). */
and a possible race condition with disconnect_prepared(). */
bool is_recovered;
const char* op_info; /*!< English text describing the
current operation, or an empty
Expand Down Expand Up @@ -968,6 +964,8 @@ struct trx_t : ilist_node<>
public:
/** Commit the transaction. */
void commit() noexcept;
/** Disconnect a prepared transaction */
void disconnect_prepared() noexcept;

/** Try to drop a persistent table.
@param table persistent table
Expand Down
41 changes: 14 additions & 27 deletions storage/innobase/lock/lock0lock.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5239,43 +5239,30 @@ lock_trx_print_locks(
}
}

/** Functor to display all transactions */
struct lock_print_info
/*********************************************************************//**
Prints info of locks for each transaction. This function will release
lock_sys.latch, which the caller must be holding in exclusive mode.
@param file output stream */
void lock_print_info_all_transactions(FILE *file)
{
lock_print_info(FILE* file, my_hrtime_t now) :
file(file), now(now),
purge_trx(purge_sys.query ? purge_sys.query->trx : nullptr)
{}
fprintf(file, "LIST OF TRANSACTIONS FOR EACH SESSION:\n");

void operator()(const trx_t &trx) const
const trx_t *const purge_trx= purge_sys.query
? purge_sys.query->trx : nullptr;
const my_hrtime_t now{my_hrtime_coarse()};

for (const trx_t &trx : trx_sys.trx_list)
{
if (UNIV_UNLIKELY(&trx == purge_trx))
return;
continue;
lock_trx_print_wait_and_mvcc_state(file, &trx, now);

if (trx.will_lock && srv_print_innodb_lock_monitor)
lock_trx_print_locks(file, &trx);
}

FILE* const file;
const my_hrtime_t now;
const trx_t* const purge_trx;
};

/*********************************************************************//**
Prints info of locks for each transaction. This function will release
lock_sys.latch, which the caller must be holding in exclusive mode. */
void
lock_print_info_all_transactions(
/*=============================*/
FILE* file) /*!< in/out: file where to print */
{
fprintf(file, "LIST OF TRANSACTIONS FOR EACH SESSION:\n");

trx_sys.trx_list.for_each(lock_print_info(file, my_hrtime_coarse()));
lock_sys.wr_unlock();

ut_d(lock_validate());
lock_sys.wr_unlock();
ut_d(lock_validate());
}

#ifdef UNIV_DEBUG
Expand Down
8 changes: 5 additions & 3 deletions storage/innobase/read/read0read.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ Created 2/16/1997 Heikki Tuuri
#include "srv0srv.h"
#include "trx0sys.h"
#include "trx0purge.h"
#include "lock0lock.h"

/*
-------------------------------------------------------------------------------
Expand Down Expand Up @@ -259,7 +260,8 @@ void trx_sys_t::clone_oldest_view(ReadViewBase *view) const
{
view->snapshot(nullptr);
/* Find oldest view. */
trx_list.for_each([view](const trx_t &trx) {
trx.read_view.append_to(view);
});
lock_sys.rd_lock(SRW_LOCK_CALL);
for (const trx_t &trx : trx_list)
trx.read_view.append_to(view);
lock_sys.rd_unlock();
}
27 changes: 22 additions & 5 deletions storage/innobase/srv/srv0srv.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -695,6 +695,20 @@ static void srv_refresh_innodb_monitor_stats(time_t current_time)
mysql_mutex_unlock(&srv_innodb_monitor_mutex);
}

size_t trx_sys_t::view_count() const noexcept
{
ut_ad(lock_sys.is_holder());

size_t count{0};

for (const trx_t &trx : trx_list)
if (trx.read_view.is_open())
++count;

return count;
}


/******************************************************************//**
Outputs to a file the output of the InnoDB Monitor.
@return FALSE if not all information printed
Expand Down Expand Up @@ -760,6 +774,7 @@ srv_printf_innodb_monitor(
ut_copy_file(file, dict_foreign_err_file);
}

size_t view_count{0};
mysql_mutex_unlock(&dict_foreign_err_mutex);

/* Only if lock_print_info_summary proceeds correctly,
Expand All @@ -778,6 +793,8 @@ srv_printf_innodb_monitor(
}
}

view_count = trx_sys.view_count();

/* NOTE: The following function will release the lock_sys.latch
that lock_print_info_summary() acquired. */

Expand Down Expand Up @@ -848,11 +865,11 @@ srv_printf_innodb_monitor(

buf_print_io(file);

fputs("--------------\n"
"ROW OPERATIONS\n"
"--------------\n", file);
fprintf(file, ULINTPF " read views open inside InnoDB\n",
trx_sys.view_count());
fprintf(file,
"--------------\n"
"ROW OPERATIONS\n"
"--------------\n"
"%zu read views open inside InnoDB\n", view_count);
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The view_count variable may incorrectly report 0 here if the InnoDB lock monitor is disabled. It is initialized to 0 at line 777 and only updated within the conditional block at line 796. To ensure accurate reporting in the monitor output, view_count should be fetched regardless of whether the lock monitor is active, while ensuring that lock_sys.latch is held during the call to trx_sys.view_count().

Copy link
Copy Markdown
Contributor Author

@dr-m dr-m May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was intentional, but not mentioned in the commit message. If we need to protect trx_sys.trx_list with something else than lock_sys.latch then this problem would go away; we’d unconditionally read the view_count at this spot.


if (ulint n_reserved = fil_system.sys_space->n_reserved_extents) {
fprintf(file,
Expand Down
Loading