Skip to content

MDEV-39706 Assertion `!thd || !coordinator_thd' failed#5254

Open
Thirunarayanan wants to merge 1 commit into
10.11from
MDEV-39706
Open

MDEV-39706 Assertion `!thd || !coordinator_thd' failed#5254
Thirunarayanan wants to merge 1 commit into
10.11from
MDEV-39706

Conversation

@Thirunarayanan

Copy link
Copy Markdown
Member

Problem:

  • This assert was introduced in commit 0152c61 (MDEV-39261), which sets coordinator_thd in clone_oldest_view() and resets it to nullptr in batch_cleanup() at the end of the batch.
    When innodb_trx_purge_view_update_only_debug is enabled, InnoDB fail to reset coordinator_thd. As a result, InnoDB fails with assert in next batch

Solution:

trx_purge(): Reset coordinator_thd to nullptr when innodb_trx_purge_view_update_only_debug is enabled.

Note:

Avoided adding test case because cannot --source wait_all_purged.inc with
innodb_trx_purge_view_update_only_debug=1 purge never removes history, So the history list
never reaches 0 and the wait would hang forever.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a debug-only helper method set_coordinator_thd to the purge_sys_t class and updates trx_purge to reset the coordinator thread to nullptr when the debug flag srv_purge_view_update_only_debug is active. The review feedback suggests improving code readability and style by formatting multi-statement blocks across multiple lines and adding standard spacing around the assignment operator.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread storage/innobase/trx/trx0purge.cc Outdated
Comment on lines +1475 to +1476
ut_d(if (srv_purge_view_update_only_debug)
{ purge_sys.set_coordinator_thd(nullptr); return 0; });

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Putting multiple statements on a single line inside a block, especially within a macro invocation like ut_d(...), reduces readability and makes debugging (e.g., setting breakpoints or stepping through code) more difficult. Formatting the block across multiple lines is much cleaner and conforms better to standard C++ readability guidelines.

  ut_d(if (srv_purge_view_update_only_debug) {
         purge_sys.set_coordinator_thd(nullptr);
         return 0;
       });

Comment on lines +510 to +515
#ifdef UNIV_DEBUG
void set_coordinator_thd(THD *thd) noexcept
{
coordinator_thd= thd;
}
#endif /* UNIV_DEBUG */

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

For better readability and adherence to standard C++ style conventions (such as the Google C++ Style Guide), binary operators like the assignment operator = should have spaces on both sides.

#ifdef UNIV_DEBUG
  void set_coordinator_thd(THD *thd) noexcept
  {
    coordinator_thd = thd;
  }
#endif /* UNIV_DEBUG */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Heh, this rather reasonable piece of advice conflicts with our CODING_STANDARDS.md.

@dr-m

dr-m commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Can you write a test case with the following, or with some other nonzero value?

SET GLOBAL innodb_max_purge_lag_wait=1;

@dr-m dr-m left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you please add a test case that executes for example the following (which should avoid a hang):

SET GLOBAL innodb_max_purge_lag_wait=1;

Comment thread storage/innobase/include/trx0purge.h
Comment thread storage/innobase/trx/trx0purge.cc Outdated
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
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.

Problem:
========
  - This assert was introduced in commit
0152c61 (MDEV-39261), which sets
coordinator_thd in clone_oldest_view() and resets it to nullptr in
batch_cleanup() at the end of the batch.
When innodb_trx_purge_view_update_only_debug is enabled, InnoDB fail
to reset coordinator_thd. As a result, InnoDB fails with assert in
next batch

Solution:
========
trx_purge(): Reset coordinator_thd to nullptr when
innodb_trx_purge_view_update_only_debug is enabled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants