MySQL Bugs: #96578: `Insert on duplicate key update` updates the wrong row

Bug #96578	`Insert on duplicate key update` updates the wrong row
Submitted:	16 Aug 2019 22:08	Modified:	4 Feb 2020 19:16
Reporter:	Zhicheng Zhu	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: DML	Severity:	S2 (Serious)
Version:	8.0.16, 5.7.27, 8.0.17	OS:	Any
Assigned to:		CPU Architecture:	Any

Description:
Long story short, as the title said, if we run multiple "insert on duplicated key update" concurrently, then one transaction may update the wrong row which belongs to another transaction. For details, please look at the repro steps. This happens in innodb.  

percona also reported a similar bug with same root cause in myrocks.
https://jira.percona.com/browse/PS-5675

How to repeat:
Repro Script:
# change innodb_autoinc_lock_mode to 2
source include/have_debug.inc;
source include/have_debug_sync.inc;

SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED;
connect (con1, localhost, root,,);
SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED;
connection default;

create table temp1 (a varchar(36) NOT NULL, b varchar(128) NOT NULL, c varchar(10240) NOT NULL);
insert into temp1 values  (1, 'b1', 'c1'), (2,'b2','c2'),(3,'b3','c3'),(4,'b4','c4'),(5,'b5','c5');
create table temp2 (a varchar(36) NOT NULL, b varchar(128) NOT NULL, c varchar(10240) NOT NULL);
insert into temp2 values (2,'b2','c2'),(3,'b3','c3'),(4,'b4','c4'),(5,'b5','c5');

CREATE TABLE extra (
  id bigint(20) unsigned NOT NULL AUTO_INCREMENT,
  a varchar(36) NOT NULL,
  b varchar(128) NOT NULL,
  c varchar(10240) NOT NULL,
  PRIMARY KEY (id),
  UNIQUE KEY uniq_idx (a,b)
) engine = innodb;
set DEBUG_SYNC = "ha_write_row_end WAIT_FOR flushed EXECUTE 1";
send insert into extra(a, b, c) select * from temp1 on duplicate key update c=values(c); 

connection con1;
insert into extra(a, b, c) select * from temp2 on duplicate key update c=values(c); 
set DEBUG_SYNC = "now SIGNAL flushed";

connection default;
reap;
select id, a, b, c from extra;

drop table extra;

Expect result:
id      a       b       c
1       1       b1      c1
2       2       b2      c2
3       3       b3      c3
4       4       b4      c4
5       5       b5      c5

Actual Result(Debug Mode):
mysqld: /data/users/mung/5.6/sql/handler.cc:3650: int handler::update_auto_increment(): Assertion `next_insert_id >= auto_inc_interval_for_cur_row.minimum()' failed.
Actual Result(Release Mode):
Unfortunately you have to leverage the DEBUG_SYNC to get a consistent repro. To simulate the release mode, just remove the Assertion `next_insert_id >= auto_inc_interval_for_cur_row.minimum()' in handler::update_auto_increment. You can see, the data is different from the expected result. 

Suggested fix:
When restoring the prev_insert_id, if the prev_insert_id has already been taken by other transaction, then we should no restore that insert_id and create a new one.

Hello Zhicheng Zhu,

Thank you for the report.

regards,
Umesh

Test results - 8.0.17, 5.7.27

Attachment: 96578_5.6.45_5.7.27_8.0.17.results (application/octet-stream, text), 200.80 KiB.

Hello Umesh, 
Thanks for investigating this! I just looked at the result file. Just curious have you setup the DEBUT_SYNC point? This is actual the key step to create a consistent repro

Hi Zhicheng Zhu,

I haven't changed anything from your repro case while running on debug build.
While simulating on release build, as suggested by you - removed the Assertion `next_insert_id >= auto_inc_interval_for_cur_row.minimum()' in handler::update_auto_increment and commented out DEBUG_SYNC references from the mtr test case.

regards,
Umesh

Hello Umesh, 
I guess there might be some confusion in my comments. To mimic release, just remove Assertion `next_insert_id >= auto_inc_interval_for_cur_row.minimum()', but keep DEBUG_SYNC. Keeping the DEBUG_SYNC is the key, otherwise you need to kept running two queries concurrently for multiple times to repo the issue, which isn't very convenient. **Please don't comment out the DEBUG_SYNC**
Thanks,
Zhicheng

Hi Zhicheng Zhu,

Thank you for the clarification, removed assertion but left the DEBUG_SYNC**.
Please note that this again requires debug build as release builds will complain "ERROR 1193 (HY000): Unknown system variable 'DEBUG_SYNC'".
Joining the test results shortly which confirms the reported issue now.

regards,
Umesh

Test results - 8.0.17

Attachment: 96578_8.0.17.results (application/octet-stream, text), 341.48 KiB.

Hello Umesh,
Yes. "ERROR 1193 (HY000): Unknown system variable 'DEBUG_SYNC'" is expected if you run it under release mode. This is why we have to **remove assert in Debug mode to mimic the behavior in release mode** :-)
Let me know if you need anything
Thanks,
Zhicheng

Hello Umesh,
Yes. "ERROR 1193 (HY000): Unknown system variable 'DEBUG_SYNC'" is expected if you run it under release mode. This is why we have to **remove assert in Debug mode to mimic the behavior in release mode** :-)
Let me know if you need anything
Thanks,
Zhicheng

Posted by developer:
 
Fixed in 5.6.47, 5.7.29, 8.0.19.

With multiple sessions executing concurrent INSERT ... ON DUPLICATE
KEY UPDATE statements into a table with an AUTO_INCREMENT column but
not specifying the AUTO_INCREMENT value, inserts could fail with a
unique index violation.

Thanks for fixing this! Is there a way I can look at this fix and back port this to our system?

https://github.com/mysql/mysql-server/commit/cbd87693248cb4d2a217a8f1c13108e3eca1ecff

Awesome! Thanks for fixing this!