MDEV-40995 Fix AUTO_INCREMENT race condition in partitioned tables - #5627
greypilgrim-083 wants to merge 2 commits into
Conversation
|
I took a look at the recent CI failures and they appear to be unrelated flaky tests. The specific tests that failed are: main.start_slave_until (replication parser test) Could a maintainer please re-trigger the failed Buildbot jobs for me when they have a chance? Thanks! |
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
Please rebase your fix to 10.11: every bug should be fixed in the lowest affected version.
Also, please make sure there is a commit comment and it is compliant with the contribution guidelines.
235cbf5 to
23d715b
Compare
|
@gkodinov I changed the base branch to 10.11 and added commit message according to contribution guidelines of mariadb |
23d715b to
405c5c2
Compare
gkodinov
left a comment
There was a problem hiding this comment.
LGTM.
One suggestion: since this is only reproducible by a probability multi-threaded run, please consider working on a regression test using the dbug tools (DBUG_SYNC/DBUG_EXECUTE_IF etc).
Please also stand by for the final review.
|
FYI: According to our development cycle we work on bugs In the following periods 15 Mar-30 Apr, 15 Jun-30 Jul, 15 Sep-30 Oct and 15 Dec-31 Jan. So, please, expect to get a review somewhere between these two dates and the goal is to have your PR merged before the second date |
… can leave the table-level AUTO_INCREMENT counter behind MAX(pk), so later inserts are handed ids that already exist and fail with ER_DUP_ENTRY on the PRIMARY KEY For a partitioned InnoDB table with AUTO_INCREMENT as the leftmost PK column, values are issued by the partitioning layer from the shared counter Partition_share::next_auto_inc_val, reserved in doubling blocks (1, 2, 4, 8 ... values per reservation). At end-of-statement, ha_partition::release_auto_increment() returns unused tail values to the shared counter. When a row fails mid-statement, handler::restore_auto_increment() rolls next_insert_id back to the boundary of an earlier block. The guard in release_auto_increment() inspects only the last interval, so it passes even when the returned value is already in use by concurrent sessions. Fix: extend the guard to also verify that next_insert_id is within the statement's own reservation interval, rejecting a lowering caused by restore_auto_increment() rolling back across a block boundary.#
c026db0 to
b033809
Compare
gkodinov
left a comment
There was a problem hiding this comment.
Still LGTM. Thanks for working out a test case! Please stand by for the final review.
|
. |
gkodinov
left a comment
There was a problem hiding this comment.
Still looking good to me. Please be patient about the final review. These are usually done by the senior developers and usually do take a little bit more time as these people have quite a lot on their hands.
b033809 to
b97670f
Compare
holyfoot
left a comment
There was a problem hiding this comment.
The bugfix looks good.
The test requires some work though. The present one just doesn't work as the CREATE TABLE there fails (UNIQUE is not in the partition key).
Also it has to be placed inside already existing suite/parts directory.
- Fix UNIQUE key in CREATE TABLE to include partition key - Move test to suite/parts - Add EXECUTE 1 to DEBUG_SYNC to avoid timeout - Change con2 to pk2=2 to avoid gap lock - Add mdev_40995-master.opt to use innodb-autoinc-lock-mode=2 to avoid table lock
b97670f to
282a343
Compare
gkodinov
left a comment
There was a problem hiding this comment.
LGTM. Thanks. FYI: no need to re-request my review every time you change something that the final reviewer asks for.
Fixes https://jira.mariadb.org/browse/MDEV-40995
The Problem:
When a multi-row insert (or
INSERT ... SELECT) fails on the first row of a newly fetched auto-increment block,restore_auto_incrementrolls the ID pointer backward into an older block. During cleanup,ha_partition::release_auto_incrementblindly trusted this pointer because it only checked the.maximum()boundary. This allowed the global counter to be forcefully lowered into already-used territory, resulting inER_DUP_ENTRYPRIMARY key collisions under concurrent load.The Fix:
Added a
.minimum()boundary check (next_insert_id >= auto_inc_interval_for_cur_row.minimum()) insideha_partition::release_auto_increment. This ensures the global counter is never rolled back below the starting ID of the currently reserved block.Testing Note:
A regression
.testfile is not included because this is a strict multi-threaded race condition. Reliably interleaving the auto-increment block fetches across multiple connections to force the collision would require injecting explicitDEBUG_SYNCpoints into the C++ source. The fix has been thoroughly validated locally using the reporter's02_run.shconcurrent stress test.Please let me know if you would like me to wire up the
DEBUG_SYNCsync points and write the MTR test for this, or if the C++ fix is sufficient as-is!for more detailed analysis of problem please check my comment under https://jira.mariadb.org/browse/MDEV-40995