* : Multiple rows insert in a statement should have consecutive autoID if needed. #11876

AilinKid · 2019-08-26T12:09:24Z

What problem does this PR solve?

fix issue 11696
This comes from JDBC getGeneratedKeys error in Mybatis framework

create table t (a int not null auto_increment, b int, primary key(a));

concurrent case: 
A: insert into t(b) values(...),(...),(...)...;  
B: insert into t(b) values(...),(...),(...)...;
...

consequence: 
If N rows inserted After A, when you use LAST_INSERT_ID() = lid, it follows:
You cannot guarantee lid、lid+1、lid+2、...、lid+N is the auto id  sequence in A. (cause alloc func will be called by other case)

Attention Please:
This PR guarantees the consecutive autoID in a batch. (That means users should use JDBC getGeneratedKeys feature under the circumstance that the insert rows in a statement should less than the batch size. For most case, it's adequate. )

When doing multiple batch insert, keeping the consecutive autoID will cache all rows in memory for lazy autoID batch allocation.

What is changed and how it works?

1：let allocator support AllocN(num) to allocate consecutive sequence.
2：when do insert rows, use AllocN(num) to make sure consecutive id allocation.

Check List

Tests

Unit test
Integration test

Code changes

Add a func AllocN(tableID, Num) in Allocator interface.

Related changes

Need to cherry-pick to the release branch

Release note

Support multiple rows insert in a statement have consecutive autoID if needed.

codecov · 2019-08-26T12:15:21Z

Codecov Report

Merging #11876 into master will decrease coverage by 0.0863%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             master     #11876        +/-   ##
================================================
- Coverage   79.9776%   79.8912%   -0.0864%     
================================================
  Files           460        460                
  Lines        103609     103527        -82     
================================================
- Hits          82864      82709       -155     
- Misses        14697      14770        +73     
  Partials       6048       6048

jackysp · 2019-09-04T07:08:43Z

Good job!
/run-all-tests

bb7133 · 2019-09-05T05:16:11Z

executor/insert_common.go

+		case AutoIDNull:
+			// Find consecutive num.
+			var cnt int
+			var start int


Suggested change

var start int

var start int

Simply start := i is enough

Cool Done! >.<

bb7133 · 2019-09-05T05:24:35Z

executor/insert_common.go

+			var cnt int
+			var start int
+			start = i
+			for i < length {


How about:

cnt := 1 for i+1 < length && e.cache[i+1].kind == AutoIDNull { cnt++ i++ }

So you don't need to fix the index i

bb7133 · 2019-09-05T05:26:04Z

executor/insert_common.go

+			if e.filterErr(err) != nil {
+				return nil, err
+			}
+			// Distribute autoIDs to rows.


Suggested change

// Distribute autoIDs to rows.

// Assign autoIDs to rows.

bb7133 · 2019-09-05T05:28:25Z

executor/insert_common.go

+			// Cause d may changed in HandleBadNull, do assignment here
+			rows[rowIdx][colIdx] = d
+		case AutoIDZero:
+			// Don't change value 0 to auto id, if NoAutoValueOnZero SQL mode is set.


Suggested change

// Don't change value 0 to auto id, if NoAutoValueOnZero SQL mode is set.

// Don't change value 0 to auto id, if `NO_AUTO_VALUE_ON_ZERO` SQL mode is set.

Is this a TODO?

No, that means fill 0 to autoID column, just let it be.

bb7133 · 2019-09-05T05:55:03Z

executor/insert_common.go

@@ -530,6 +675,98 @@ func (e *InsertValues) fillRow(ctx context.Context, row []types.Datum, hasValue
 	return row, nil
 }

+// fillRowLazy is quite same to fillRow() except it will cache auto increment datum for lazy batch allocation of autoID.
+// This case is used in insert|replace into values (row),(row),(row)...
+func (e *InsertValues) fillRowLazy(ctx context.Context, row []types.Datum, hasValue []bool, rowIdx int) ([]types.Datum, error) {


This function shares some codes with fillRow, and the two functions are with same signature, can we use a flag variable of InsertValues(for example, lazyFillAutoID) to indicate if the filling logic is lazy, and try to combine two functions?

Nice idea！

AilinKid · 2019-09-09T06:59:34Z

/run-all-tests

AilinKid · 2019-09-09T12:13:00Z

/run-all-tests

AilinKid · 2019-09-09T12:30:32Z

/rebuild

AilinKid · 2019-09-09T14:05:22Z

Attention！This PR need to modify mysql-test

AilinKid · 2019-09-17T02:35:20Z

/run-all-tests

AilinKid · 2019-09-17T03:58:20Z

/run-all-tests

AilinKid · 2019-09-17T06:17:29Z

@jackysp hi，shuaipeng，this PR is ready for review now, PTAL

jackysp · 2019-09-17T08:45:41Z

The PR is really big, is there anyone else to review together?

meta/autoid/autoid.go

sre-bot · 2019-10-10T02:20:34Z

/run-all-tests

sre-bot · 2019-10-10T02:24:42Z

/run-all-tests

sre-bot · 2019-10-10T02:30:32Z

@AilinKid merge failed.

AilinKid · 2019-10-10T02:36:08Z

/run-unit-test

sre-bot · 2019-10-10T02:40:51Z

cherry pick to release-2.1 failed

sre-bot · 2019-10-10T02:46:24Z

cherry pick to release-3.0 failed

sre-bot · 2019-10-10T02:50:38Z

cherry pick to release-3.1 failed

AilinKid · 2019-10-10T02:51:36Z

Clicking the update branch button triggers the merge? (bug?)
integration-common-test & integration-ddl-test has passed!
unit-tests with a decimal test fail! I've tried update branch and run-all-tests in other PR with all passed. (it seems random fail, maybe the environment factor)

…oID if needed. (pingcap#11876)

…D if needed. (#11876) (#12602)

…D if needed. (#11876) (#12593)

…D if needed. (#11876) (#12619)

AilinKid added needs-cherry-pick-2.1 labels Aug 26, 2019

AilinKid changed the title ~~ddl : Multiple rows insert in a statement should have consecutive autoID if needed. [DNM]~~ ddl : Multiple rows insert in a statement should have consecutive autoID if needed. Aug 29, 2019

AilinKid removed the status/DNM label Aug 29, 2019

winkyao added the status/WIP label Aug 29, 2019

AilinKid removed the status/WIP label Sep 1, 2019

AilinKid requested review from jackysp, bb7133 and francis0407 and removed request for francis0407 September 4, 2019 04:47

bb7133 reviewed Sep 5, 2019

View reviewed changes

AilinKid force-pushed the consecutive-auto-id branch 3 times, most recently from 25dbe2d to 2e4497f Compare September 9, 2019 05:33

AilinKid force-pushed the consecutive-auto-id branch from 5a78aaf to 8bc9a07 Compare September 9, 2019 08:32

AilinKid force-pushed the consecutive-auto-id branch from e131b79 to 10a32e3 Compare September 17, 2019 03:14

jackysp requested review from crazycs520 and cfzjywxk September 17, 2019 08:46

bb7133 reviewed Oct 10, 2019

View reviewed changes

meta/autoid/autoid.go Show resolved Hide resolved

add comment for alloc N

5eebe61

bb7133 approved these changes Oct 10, 2019

View reviewed changes

bb7133 added status/can-merge Indicates a PR has been approved by a committer. needs-cherry-pick-3.1 labels Oct 10, 2019

Merge branch 'master' into consecutive-auto-id

abd4704

Merge branch 'master' into consecutive-auto-id

54a3e2e

AilinKid changed the title ~~ddl : Multiple rows insert in a statement should have consecutive autoID if needed.~~ dml : Multiple rows insert in a statement should have consecutive autoID if needed. Oct 10, 2019

AilinKid merged commit 01cddf6 into pingcap:master Oct 10, 2019

AilinKid changed the title ~~dml : Multiple rows insert in a statement should have consecutive autoID if needed.~~ * : Multiple rows insert in a statement should have consecutive autoID if needed. Oct 10, 2019

AilinKid added a commit to AilinKid/tidb that referenced this pull request Oct 10, 2019

dml : Multiple rows insert in a statement should have consecutive aut…

6a01b63

…oID if needed. (pingcap#11876)

AilinKid mentioned this pull request Oct 10, 2019

* : Multiple rows insert in a statement should have consecutive autoID if needed. (#11876) #12593

Merged

AilinKid added a commit to AilinKid/tidb that referenced this pull request Oct 10, 2019

dml : Multiple rows insert in a statement should have consecutive aut…

8095760

…oID if needed. (pingcap#11876)

AilinKid mentioned this pull request Oct 10, 2019

* : Multiple rows insert in a statement should have consecutive autoID if needed. (#11876) #12602

Merged

AilinKid added a commit to AilinKid/tidb that referenced this pull request Oct 11, 2019

dml : Multiple rows insert in a statement should have consecutive aut…

624df64

…oID if needed. (pingcap#11876)

AilinKid mentioned this pull request Oct 11, 2019

* : Multiple rows insert in a statement should have consecutive autoID if needed. (#11876) #12619

Merged

sre-bot pushed a commit that referenced this pull request Oct 13, 2019

* : Multiple rows insert in a statement should have consecutive autoI…

6596890

…D if needed. (#11876) (#12602)

sre-bot pushed a commit that referenced this pull request Oct 13, 2019

* : Multiple rows insert in a statement should have consecutive autoI…

e5e2263

…D if needed. (#11876) (#12593)

ngaut pushed a commit that referenced this pull request Oct 14, 2019

* : Multiple rows insert in a statement should have consecutive autoI…

df41474

…D if needed. (#11876) (#12619)

you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020

bb7133 mentioned this pull request Mar 10, 2020

getGeneratedKeys from batch insert return wrong ids #11696

Closed

AilinKid deleted the consecutive-auto-id branch August 14, 2024 07:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

* : Multiple rows insert in a statement should have consecutive autoID if needed. #11876

* : Multiple rows insert in a statement should have consecutive autoID if needed. #11876

AilinKid commented Aug 26, 2019 •

edited by zz-jason

Loading

codecov bot commented Aug 26, 2019 •

edited

Loading

jackysp commented Sep 4, 2019

bb7133 Sep 5, 2019

AilinKid Sep 6, 2019

bb7133 Sep 5, 2019

AilinKid Sep 6, 2019

bb7133 Sep 5, 2019

AilinKid Sep 9, 2019

bb7133 Sep 5, 2019

AilinKid Sep 6, 2019

bb7133 Sep 5, 2019

AilinKid Sep 6, 2019

AilinKid commented Sep 9, 2019

AilinKid commented Sep 9, 2019

AilinKid commented Sep 9, 2019

AilinKid commented Sep 9, 2019 •

edited

Loading

AilinKid commented Sep 17, 2019

AilinKid commented Sep 17, 2019

AilinKid commented Sep 17, 2019 •

edited

Loading

jackysp commented Sep 17, 2019

sre-bot commented Oct 10, 2019

sre-bot commented Oct 10, 2019

sre-bot commented Oct 10, 2019

AilinKid commented Oct 10, 2019

sre-bot commented Oct 10, 2019

sre-bot commented Oct 10, 2019

sre-bot commented Oct 10, 2019

AilinKid commented Oct 10, 2019 •

edited

Loading

	// Don't change value 0 to auto id, if NoAutoValueOnZero SQL mode is set.
	// Don't change value 0 to auto id, if `NO_AUTO_VALUE_ON_ZERO` SQL mode is set.

* : Multiple rows insert in a statement should have consecutive autoID if needed. #11876

* : Multiple rows insert in a statement should have consecutive autoID if needed. #11876

Conversation

AilinKid commented Aug 26, 2019 • edited by zz-jason Loading

What problem does this PR solve?

What is changed and how it works?

Check List

codecov bot commented Aug 26, 2019 • edited Loading

Codecov Report

jackysp commented Sep 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AilinKid commented Sep 9, 2019

AilinKid commented Sep 9, 2019

AilinKid commented Sep 9, 2019

AilinKid commented Sep 9, 2019 • edited Loading

AilinKid commented Sep 17, 2019

AilinKid commented Sep 17, 2019

AilinKid commented Sep 17, 2019 • edited Loading

jackysp commented Sep 17, 2019

sre-bot commented Oct 10, 2019

sre-bot commented Oct 10, 2019

sre-bot commented Oct 10, 2019

AilinKid commented Oct 10, 2019

sre-bot commented Oct 10, 2019

sre-bot commented Oct 10, 2019

sre-bot commented Oct 10, 2019

AilinKid commented Oct 10, 2019 • edited Loading

AilinKid commented Aug 26, 2019 •

edited by zz-jason

Loading

codecov bot commented Aug 26, 2019 •

edited

Loading

AilinKid commented Sep 9, 2019 •

edited

Loading

AilinKid commented Sep 17, 2019 •

edited

Loading

AilinKid commented Oct 10, 2019 •

edited

Loading