Bug #96578 `Insert on duplicate key update` updates the wrong row
Submitted: 16 Aug 22:08 Modified: 23 Aug 15:26
Reporter: Zhicheng Zhu Email Updates:
Status: Verified 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

[16 Aug 22:08] Zhicheng Zhu
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.
[17 Aug 6:59] Umesh Shastry
Hello Zhicheng Zhu,

Thank you for the report.

regards,
Umesh
[17 Aug 7:00] Umesh Shastry
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.

[20 Aug 16:17] Zhicheng Zhu
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
[22 Aug 11:12] Umesh Shastry
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
[22 Aug 16:07] Zhicheng Zhu
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
[23 Aug 6:57] Umesh Shastry
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
[23 Aug 6:57] Umesh Shastry
Test results - 8.0.17

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

[23 Aug 15:26] Zhicheng Zhu
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
[23 Aug 15:26] Zhicheng Zhu
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