MDEV-39344: trx_disconnect_prepared() uses wrong mutex by dr-m · Pull Request #5094 · MariaDB/server

dr-m · 2026-05-19T08:28:02Z

trx_t::disconnect_prepared(): Replaces trx_disconnect_prepared(). Protect the data members with trx_t::mutex.

fetch_data_into_cache(): Fix a correctness regression that had been introduced in 5cbf4e3 when the INFORMATION_SCHEMA views innodb_trx, innodb_locks, innodb_lock_waits were introduced in the InnoDB Plugin for MySQL 5.1, which later became the built-in InnoDB in MySQL 5.5 and MariaDB Server 5.5.

CLAassistant · 2026-05-19T08:28:13Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

dr-m · 2026-05-19T08:29:37Z

-		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};


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.

gemini-code-assist

Code Review

This pull request refactors the InnoDB transaction system by replacing the thread_safe_trx_ilist_t wrapper with a standard ilist protected by the global lock_sys.latch and converting functor-based iterations into manual loops. Feedback points out a logic error in the InnoDB monitor where view_count might be incorrectly reported as zero, a bug in the Information Schema cache regarding truncation signaling, and potential data races when accessing transaction fields. Additionally, there is a concern that using the central lock_sys.latch for transaction registration could create a scalability bottleneck.

gemini-code-assist · 2026-05-19T08:31:22Z

+		"--------------\n"
+		"ROW OPERATIONS\n"
+		"--------------\n"
+		"%zu read views open inside InnoDB\n", view_count);


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().

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.

gemini-code-assist · 2026-05-19T08:31:23Z

+			if (!trx.is_started() || !trx.mysql_thd
+			    || withdraw_started <= trx.start_time_micro) {


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.

gemini-code-assist · 2026-05-19T08:31:23Z

+  lock_sys.wr_lock(SRW_LOCK_CALL);
+  trx_list.push_front(*trx);
+  lock_sys.wr_unlock();


Protecting the global transaction list with the central lock_sys.latch during register_trx and deregister_trx may introduce a scalability bottleneck. These functions are called at the start and end of every transaction. Moving from a dedicated list mutex to an exclusive lock on the heavily-contended lock_sys.latch could increase contention in high-concurrency environments.

Yes, exactly this we need to find out by running some performance tests.

dr-m · 2026-05-19T08:37:11Z

-  void create() { mysql_mutex_init(trx_sys_mutex_key, &mutex, nullptr); }
-  void close() { mysql_mutex_destroy(&mutex); }


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.

svoj

I don't feel I like the idea of moving trx_sys.trx_list under the lock_sys.latch protection. I had hard time trying to follow the updated code. I feel like it increases maintenance cost. And it doesn't directly relate to the reported problem, it was about trx members protection rather than trx_sys.trx_list.

I'm less concerned about extra pressure on the lock_sys.latch, but I agree that it should be tested and we need to suggest correct benchmarks for verification that'd cover worst case scenario. E.g. combination of connect + oltp_read_write?

Why would we want to have this change in 10.6, which is EOL in 1.5 months? Shouldn't such refactoring be targeted for the main?

Also if #5043 is going to be pushed, this list is becoming somewhat redundant. We can usr rw_trx_ids.trxs instead.

svoj · 2026-05-20T16:47:16Z

  /* todo/fixme: suggest to do it at innodb prepare */
-  trx->will_lock= false;
-  trx_sys.rw_trx_hash.put_pins(trx);
+  will_lock= false;


Previously will_lock was updated out of critical section. IIUC it is always accessed by the owner thread, except for 2 occurrences in lock0lock.cc.

check_trx_state() can be called with either trx->mutex or trx->rw_trx_hash_element->mutex locked, there's one case when both are locked

lock_print_info doesn't lock trx->mutex

will_lock is updated in lots of places without holding trx->mutex

This looks like a very inconsistent locking pattern. I don't see much value in moving this specific will_lock update under trx->mutex. I suggest that we should reevaluate its locking pattern separately.

svoj · 2026-05-20T16:50:10Z

+  ut_ad(mysql_thd);
+  ut_ad(!mysql_log_file_name);
+  read_view.close();
+  mutex.wr_lock();


Why not mutex_lock()? Because of assert()?

svoj · 2026-05-20T19:27:46Z

  size_t total_trx= 0, prepared_trx= 0;

-  trx_sys.trx_list.for_each([&](const trx_t &trx) {
+  for (const trx_t &trx : trx_list)


Don't you need a lock here? IIUC it can be executed in multithreaded environment.

Right, this function trx_sys.any_active_transactions() is being invoked from two places during shutdown to check if any transactions are still active. During that time, trx_sys.trx_list may be concurrently modified, and we’d have to protect the access.

dr-m · 2026-05-27T05:37:21Z

I don't feel I like the idea of moving trx_sys.trx_list under the lock_sys.latch protection. I had hard time trying to follow the updated code. I feel like it increases maintenance cost. And it doesn't directly relate to the reported problem, it was about trx members protection rather than trx_sys.trx_list.

I'm less concerned about extra pressure on the lock_sys.latch, but I agree that it should be tested and we need to suggest correct benchmarks for verification that'd cover worst case scenario. E.g. combination of connect + oltp_read_write?

Why would we want to have this change in 10.6, which is EOL in 1.5 months? Shouldn't such refactoring be targeted for the main?

Also if #5043 is going to be pushed, this list is becoming somewhat redundant. We can usr rw_trx_ids.trxs instead.

Thank you. I will revise this to only fix the reported problem and not refactor the trx_sys at this point. A separate ticket may be filed for the trx_sys refactoring.

For 10.6, the simpler patch to fix the reported problem should be good enough. This more extensive patch would be applicable to any later major version branch; only the find_interesting_trx is something that was removed in MDEV-29445.

svoj

I think this change is a step towards the right direction. It keeps issue described in c0817da fixed.

However c0817da unintentionally fixed similar issue in lock_print_info_all_transactions(). This fix reopens it. The monitor code looks quite fragile, so I leave it up to you to decide if you want to dig into this further.

svoj · 2026-05-27T19:57:08Z

+  /* these are protected by cache->rw_lock.wr_lock() */
  trx_i_s_cache_clear(cache);
+  /* this flag may be set by fetch_data_into_cache_low() below */
+  cache->is_truncated= false;


This change is correct. But locking pattern of is_truncated is broken. It is subject of data race as well as race condition.

thr1: trx_i_s_cache_start_write(trx_i_s_cache); thr1: trx_i_s_possibly_fetch_data_into_cache(trx_i_s_cache); // all is good, not truncated thr1: trx_i_s_cache_end_write(trx_i_s_cache); thr1: if (trx_i_s_cache_is_truncated(trx_i_s_cache)) // data race thr2: trx_i_s_cache_start_write(trx_i_s_cache); thr2: trx_i_s_possibly_fetch_data_into_cache(trx_i_s_cache); // truncated thr2: trx_i_s_cache_end_write(trx_i_s_cache); thr1: starts reading truncated data with no warning issued

Thank you. The probability of this TOCTOU race could be reduced by the timestamp logic.

I ended up folding all this logic to a single function call that returns whether the cache was truncated. Also, there is no need for pointer indirection or for passing a pointer to a singleton static object across compilation unit boundaries.

I like this change. It solves data race, but it doesn't solve reported TOCTOU. :)

I’m also not sure which approach is preferable here: keeping trx_i_s_cache_t hidden as a static class inside trx0i_s.cc, or making it a globally visible class with private members. I suspect the former is mostly a remnant of the pre-C++ era.

trx_t::disconnect_prepared(): Replaces trx_disconnect_prepared(). Protect is_recovered, mysql_thd with mutex, to be consistent with protecting fetch_data_into_cache_low(). This fixes up commit c0817da (MDEV-29575). Suggested by: Sergey Vojtovich

There is a memory limit implemented for the INFORMATION_SCHEMA tables innodb_trx, innodb_locks, innodb_lock_waits. The predicate trx_i_s_cache_is_truncated() would never hold in trx_i_s_common_fill_table() due to a logic bug that had been introduced in a refactoring. fetch_data_into_cache(): Shrink the critical section of lock_sys.latch and do not unconditionally reset cache->is_truncated when leaving the function. This fixes a correctness regression that had been introduced back in 2007 by commit 5cbf4e3 for the InnoDB Plugin for MySQL 5.1, which later became the built-in InnoDB in MySQL 5.5 and MariaDB Server 5.5. Reviewed by: Sergey Vojtovich

trx_i_s_possibly_fetch_data_into_cache(): Fetch into the cache and return whether the cache was truncated. Previously, we checked this flag without holding any latch. trx_i_s_cache_start_write(), trx_i_s_cache_end_write(), trx_i_s_cache_is_truncated(): Remove. These are now part of trx_i_s_possibly_fetch_data_into_cache(). trx_i_s_cache: Remove. The singleton cache object can be a static object with no pointer indirection.

dr-m requested a review from svoj May 19, 2026 08:28

dr-m self-assigned this May 19, 2026

dr-m added the MariaDB Corporation label May 19, 2026

dr-m commented May 19, 2026

View reviewed changes

gemini-code-assist Bot reviewed May 19, 2026

View reviewed changes

dr-m commented May 19, 2026

View reviewed changes

svoj reviewed May 20, 2026

View reviewed changes

dr-m force-pushed the 10.6-MDEV-39344 branch from ae744e4 to 6bc94e0 Compare May 27, 2026 08:11

dr-m requested a review from svoj May 27, 2026 08:11

dr-m force-pushed the 10.6-MDEV-39344 branch from 5395628 to cb6b5e2 Compare May 27, 2026 08:47

svoj approved these changes May 27, 2026

View reviewed changes

dr-m added 3 commits May 28, 2026 09:23

dr-m force-pushed the 10.6-MDEV-39344 branch from cb6b5e2 to 1606951 Compare May 28, 2026 06:24

dr-m merged commit 1606951 into 10.6 May 28, 2026
12 of 14 checks passed

dr-m deleted the 10.6-MDEV-39344 branch May 28, 2026 07:13

		if (!trx.is_started() \|\| !trx.mysql_thd
		\|\| withdraw_started <= trx.start_time_micro) {

		void create() { mysql_mutex_init(trx_sys_mutex_key, &mutex, nullptr); }
		void close() { mysql_mutex_destroy(&mutex); }

Uh oh!

Conversation

dr-m commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented May 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gemini-code-assist Bot May 19, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svoj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dr-m commented May 27, 2026

Uh oh!

svoj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

dr-m commented May 19, 2026 •

edited

Loading