Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: support auto_increment_increment & auto_increment_offset. #14301

Merged
merged 19 commits into from
Jan 8, 2020

Conversation

AilinKid
Copy link
Contributor

@AilinKid AilinKid commented Dec 31, 2019

What problem does this PR solve?

Fix #14245
Support auto_increment_increment & auto_increment_offset.

What is changed and how it works?

  1. Add increment&offset variable modify support.
  2. Take increment & offset into allocate consideration.
  3. Check MySQL doc and valid the range of these two variables.
  4. Attach more tests to increment&offset allocation logic.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change
    func CalcNeededIDs(base, n, increment, offset int64, isUnsigned bool) int64
  • Has interface methods change
    Alloc(tableID int64, n uint64, increment, offset int64) (int64, int64, error)

Related changes

  • Need to update the documentation

Release note

  • Support system variable auto_increment_increment & auto_increment_offset.

@AilinKid
Copy link
Contributor Author

AilinKid commented Jan 1, 2020

/run-unit-test

@shenli shenli changed the title Meta : Support auto_increment_increment & auto_increment_offset. Meta: Support auto_increment_increment & auto_increment_offset. Jan 2, 2020
executor/insert_common.go Outdated Show resolved Hide resolved
executor/insert_test.go Show resolved Hide resolved
executor/insert_common.go Outdated Show resolved Hide resolved
meta/autoid/autoid.go Outdated Show resolved Hide resolved
meta/autoid/autoid.go Outdated Show resolved Hide resolved
meta/autoid/autoid_test.go Outdated Show resolved Hide resolved
sessionctx/variable/session.go Outdated Show resolved Hide resolved
sessionctx/variable/session.go Outdated Show resolved Hide resolved
meta/autoid/autoid.go Outdated Show resolved Hide resolved
meta/autoid/autoid.go Outdated Show resolved Hide resolved
meta/autoid/autoid.go Outdated Show resolved Hide resolved
@AilinKid
Copy link
Contributor Author

AilinKid commented Jan 2, 2020

/bench

@sre-bot
Copy link
Contributor

sre-bot commented Jan 2, 2020

Benchmark Report

Run Sysbench Performance Test on VMs

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: 5d76de903a6d2c95f83da365e5d757dad9fb9e7b
+++ tidb: 6dba49685cc9595d930c78a202b90b657bcd3cc1
tikv: 267a5ebf7edb229cc53f5a8faa71975225bc6d8e
pd: ea974c9488b8d304a72916d8cc50962f5ab99c5d
================================================================================
oltp_update_index:
    * QPS: 6369.24 ± 0.65% (std=29.06) delta: -0.21% (p=0.743)
    * Latency p50: 20.03 ± 0.35% (std=0.07) delta: -0.10%
    * Latency p99: 37.23 ± 0.90% (std=0.34) delta: -0.92%
            
oltp_insert:
    * QPS: 4702.86 ± 0.32% (std=11.54) delta: 0.16% (p=0.395)
    * Latency p50: 27.21 ± 0.33% (std=0.07) delta: -0.17%
    * Latency p99: 45.42 ± 6.42% (std=1.89) delta: -3.46%
            
oltp_read_write:
    * QPS: 15845.50 ± 0.26% (std=29.71) delta: 0.21% (p=0.875)
    * Latency p50: 162.33 ± 0.84% (std=0.84) delta: 0.08%
    * Latency p99: 322.04 ± 8.78% (std=16.97) delta: 1.48%
            
oltp_point_select:
    * QPS: 43423.43 ± 0.93% (std=247.68) delta: 1.61% (p=0.012)
    * Latency p50: 2.95 ± 0.23% (std=0.00) delta: -1.39%
    * Latency p99: 9.60 ± 2.21% (std=0.14) delta: 0.44%
            
oltp_update_non_index:
    * QPS: 4746.72 ± 0.23% (std=6.78) delta: 0.23% (p=0.163)
    * Latency p50: 26.96 ± 0.23% (std=0.04) delta: -0.24%
    * Latency p99: 40.86 ± 1.19% (std=0.34) delta: -1.09%
            

meta/autoid/errors.go Show resolved Hide resolved
meta/autoid/autoid.go Outdated Show resolved Hide resolved
meta/autoid/autoid.go Outdated Show resolved Hide resolved
meta/autoid/autoid.go Outdated Show resolved Hide resolved
meta/autoid/autoid.go Outdated Show resolved Hide resolved
executor/insert_test.go Outdated Show resolved Hide resolved
executor/insert_common.go Outdated Show resolved Hide resolved
sessionctx/variable/session.go Outdated Show resolved Hide resolved
meta/autoid/autoid_test.go Outdated Show resolved Hide resolved
@AilinKid
Copy link
Contributor Author

AilinKid commented Jan 3, 2020

/build

@AilinKid
Copy link
Contributor Author

AilinKid commented Jan 3, 2020

/run-unit-test

1 similar comment
@AilinKid
Copy link
Contributor Author

AilinKid commented Jan 3, 2020

/run-unit-test

@AilinKid AilinKid requested review from bb7133 and tangenta January 6, 2020 03:52
meta/autoid/autoid.go Outdated Show resolved Hide resolved
meta/autoid/autoid.go Outdated Show resolved Hide resolved
table/table.go Outdated Show resolved Hide resolved
table/table.go Outdated Show resolved Hide resolved
meta/autoid/autoid.go Outdated Show resolved Hide resolved
meta/autoid/autoid.go Outdated Show resolved Hide resolved
if e.handleErr(col, &autoDatum, cnt, err) != nil {
return nil, err
}
// It's compatible with mysql setting the first allocated autoID to lastInsertID.
// Cause autoID may be specified by user, judge only the first row is not suitable.
if e.lastInsertID == 0 {
e.lastInsertID = uint64(min) + 1
e.lastInsertID = uint64(min)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the previous code is uint64(min) + 1 but now it can be uint64(min)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already derive the firstID and increment from the allocated range.
AllocBatchAutoIncrementValue now return firstID and increment.

meta/autoid/autoid.go Outdated Show resolved Hide resolved
Copy link
Member

@wjhuang2016 wjhuang2016 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AilinKid AilinKid added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 8, 2020
@AilinKid AilinKid changed the title Meta: Support auto_increment_increment & auto_increment_offset. *: Support auto_increment_increment & auto_increment_offset. Jan 8, 2020
@bb7133
Copy link
Member

bb7133 commented Jan 8, 2020

LGTM

@bb7133 bb7133 changed the title *: Support auto_increment_increment & auto_increment_offset. *: support auto_increment_increment & auto_increment_offset. Jan 8, 2020
@bb7133 bb7133 added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. and removed status/LGT1 Indicates that a PR has LGTM 1. component/DDL-need-LGT3 labels Jan 8, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jan 8, 2020

/run-all-tests

@sre-bot sre-bot merged commit 870d4b1 into pingcap:master Jan 8, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jan 8, 2020

cherry pick to release-3.0 failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/new-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

autoid: support system variables auto_increment_increment and auto_increment_offset
6 participants