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.

trx_sys.trx_list: Use a plain ilist<trx_t> that is protected by lock_sys.latch, which was already protecting some traversal of trx_sys.trx_list. All traversal will now use range for, instead of callback functions.

fetch_data_into_cache_low(): Return whether we ran out of memory.

fetch_data_into_cache(): Reduce the hold time of lock_sys.latch and do not wrongly reset the cache->is_truncated flag at the end.

trx_sys_t::clone_oldest_view(): Protect the list traversal with shared lock_sys.latch.

trx_sys_t::register_trx(), trx_sys_t::deregister_trx(): Protect trx_sys.trx_list with an exclusive lock_sys.latch.

trx_t::disconnect_prepared(): Replaces trx_disconnect_prepared(). Protect the data members with trx_t::mutex. trx_sys.trx_list: Use a plain ilist<trx_t> that is protected by lock_sys.latch, which was already protecting some traversal of trx_sys.trx_list. All traversal will now use range for, instead of callback functions. fetch_data_into_cache_low(): Return whether we ran out of memory. fetch_data_into_cache(): Reduce the hold time of lock_sys.latch and do not wrongly reset the cache->is_truncated flag at the end. trx_sys_t::clone_oldest_view(): Protect the list traversal with shared lock_sys.latch. trx_sys_t::register_trx(), trx_sys_t::deregister_trx(): Protect trx_sys.trx_list with an exclusive lock_sys.latch.

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.

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

		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

Uh oh!

CLAassistant commented May 19, 2026

Uh oh!

dr-m May 19, 2026

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!

dr-m May 19, 2026

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!

dr-m May 19, 2026

Choose a reason for hiding this comment

Uh oh!

dr-m May 19, 2026

Choose a reason for hiding this comment

Uh oh!

svoj left a comment

Choose a reason for hiding this comment

Uh oh!

svoj May 20, 2026

Choose a reason for hiding this comment

Uh oh!

svoj May 20, 2026

Choose a reason for hiding this comment

Uh oh!

svoj May 20, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants