MDEV-38839 Assertion (thd->state_flags & Open_tables_state::BACKUPS_A…#5027
MDEV-38839 Assertion (thd->state_flags & Open_tables_state::BACKUPS_A…#5027pranavktiwari wants to merge 1 commit into12.3from
Conversation
pranavktiwari
force-pushed
the
12.3-MDEV-38839
branch
4 times, most recently
from
58ff35c to
afea455
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes binlog table-access classification for CREATE TABLE ... SELECT when the source is a temporary table, aiming to avoid incorrectly treating read-only temp-table access as a write and thereby prevent MIXED-mode binlog cache state from getting stuck and triggering the reported assertion.
Changes:
- Add a dedicated
stmt_writes_to_non_trans_temp_table()helper and use it when marking non-transactional temp-table modifications in binlog write paths. - Adjust
THD::decide_logging_format()soCREATE TABLE ... SELECTcan classify temporary source-table access as reads instead of writes. - Add an MTR regression test covering several temp-table engine combinations around
CREATE TABLE ... SELECT ... FOR UPDATE.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
sql/sql_lex.h |
Adds a helper to distinguish non-transactional temp-table writes from general access. |
sql/sql_class.cc |
Changes table-access flag assignment during logging-format analysis for CREATE TABLE. |
sql/log.cc |
Narrows the condition that marks a non-transactional temp table as modified in transactional cache paths. |
mysql-test/main/binlog_mdev38839.test |
Adds a regression test for the reported assertion scenario. |
mysql-test/main/binlog_mdev38839.result |
Records the expected output for the new regression test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pranavktiwari
force-pushed
the
12.3-MDEV-38839
branch
from
afea455 to
9bd571d
Compare
|
|
||
| --echo # | ||
| --echo # Test case 4: InnoDB temp source, InnoDB target - should not crash | ||
| --echo # |
There was a problem hiding this comment.
Why there are so many test cases? What is the difference in the patch for them?
There was a problem hiding this comment.
I made these 5 different cases ti understand the existing behavior before start fixing.
Once I was sure why this particular test case is failing I started with my changes.
I did not remove them because they should also pass after the code changes.
There is no difference. It just to make sure changes did not affect other scenarios.
There was a problem hiding this comment.
Please, do not make redundant test cases if they don't cover any unique parts of the code.
| LEX::STMT_WRITES_TEMP_NON_TRANS_TABLE); | ||
| { | ||
| if (lex->sql_command == SQLCOM_CREATE_TABLE && | ||
| !lex->tmp_table() && !found_first_not_own_table) |
There was a problem hiding this comment.
These conditions should be explained in comment. I think you aim for table in SELECT. There should be select_lex in TABLE_LIST, so maybe it is worth checking its existence.
| !lex->tmp_table() && !found_first_not_own_table) | ||
| lex->set_stmt_accessed_table(trans ? | ||
| LEX::STMT_READS_TEMP_TRANS_TABLE : | ||
| LEX::STMT_READS_TEMP_NON_TRANS_TABLE); |
There was a problem hiding this comment.
Maybe it is better to set READS flag in another if() below vvvv. This if (tbl->lock_type >= TL_FIRST_WRITE) should be completely excluded for such tables? But really I'm not sure if not setting WRITES is a good idea. How SELECT .. FOR UPDATE behaves now on slave? Does it lose FOR UPDATE locking? This at least should be explained in commit message if it is not important for slave.
There was a problem hiding this comment.
@pranavktiwari I would still like to get these answers:
How SELECT .. FOR UPDATE behaves now on slave? Does it lose FOR UPDATE locking?
If it is temporary table, does it make sense at all? Worth mentioning in commit message.
pranavktiwari
force-pushed
the
12.3-MDEV-38839
branch
from
f380b5e to
10c48d3
Compare
|
|
||
| --echo # | ||
| --echo # Test case 2: MyISAM temp source, InnoDB target - should not crash | ||
| --echo # |
| lex->set_stmt_accessed_table(trans ? LEX::STMT_WRITES_TEMP_TRANS_TABLE : | ||
| { | ||
| if (tbl->updating) | ||
| lex->set_stmt_accessed_table(trans ? LEX::STMT_WRITES_TEMP_TRANS_TABLE : |
There was a problem hiding this comment.
Looks better. Why updating check is only for tmp_table?
| !lex->tmp_table() && !found_first_not_own_table) | ||
| lex->set_stmt_accessed_table(trans ? | ||
| LEX::STMT_READS_TEMP_TRANS_TABLE : | ||
| LEX::STMT_READS_TEMP_NON_TRANS_TABLE); |
There was a problem hiding this comment.
@pranavktiwari I would still like to get these answers:
How SELECT .. FOR UPDATE behaves now on slave? Does it lose FOR UPDATE locking?
If it is temporary table, does it make sense at all? Worth mentioning in commit message.
…LECT FOR UPDATE with MyISAM temp table in MIXED binlog mode Cause: FOR UPDATE on MyISAM temp tables uses a write lock (no row-level locking), incorrectly marking a read as a write. This blocks binlog_truncate_trx_cache() and triggers an assertion. Fix: In decide_logging_format(), check tbl->updating to detect real writes. For read-only access, mark as STMT_READS_TEMP_NON_TRANS_TABLE instead of write, preventing incorrect behavior.
pranavktiwari
force-pushed
the
12.3-MDEV-38839
branch
from
10c48d3 to
5c4fd9b
Compare
MDEV-38839: Fix assertion in close_thread_tables on CREATE TABLE...SELECT FOR UPDATE with MyISAM temp table in MIXED binlog mode
Cause: FOR UPDATE on MyISAM temp tables uses a write lock (no row-level locking), incorrectly marking a read as a write. This blocks binlog_truncate_trx_cache() and triggers an assertion.
Fix: In decide_logging_format(), check tbl->updating to detect real writes. For read-only access, mark as STMT_READS_TEMP_NON_TRANS_TABLE instead of write, preventing incorrect behavior.