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

*: add split index region syntax #10203

Merged
merged 30 commits into from
May 6, 2019
Merged

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Apr 19, 2019

What problem does this PR solve?

PR: #10138 support pre-split table region. But it will only split 1 region for per index. Then the index region may still be the hot region and the write bottleneck.

What is changed and how it works?

This PR support split index region by SQL. Syntax like below:

SPLIT TABLE table_name INDEX index_name BY (val1...), (val2...),(val3...) ...

example

SPLIT INDEX t1 INDEX idx1 (100), (1000), (10000);   
//range is : 0~100, 100 ~1000, 1000~ 1000010000 ~ maxInt64

split table t1 idx2 ("a", "2000-01-01 00:00:01"),  ("b", "2019-04-17 14:26:19"),  ("c", "");  
// range is: minIndexValue ~ ("a", "2000-01-01 00:00:01"),
("a", "2000-01-01 00:00:01") ~ ("b", "2019-04-17 14:26:19"),
("b", "2019-04-17 14:26:19") ~ ("c", ""),
("c", "") ~ maxIndexValue.

Please merge #10138 firstly.
Related parser PR: pingcap/parser#297

Check List

Tests

  • Adding test.

Code changes

  • Has exported function/method change

Side effects

Related changes

Copy link
Member

@nolouch nolouch left a comment

Choose a reason for hiding this comment

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

LGTM

regionIDs := make([]uint64, 0, len(e.valueLists))
index := tables.NewIndex(e.table.Meta().ID, e.table.Meta(), e.indexInfo)
for _, values := range e.valueLists {
idxKey, _, err := index.GenIndexKey(e.ctx.GetSessionVars().StmtCtx, values, 1, nil)
Copy link
Member

Choose a reason for hiding this comment

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

1 means min handle id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, Maybe 0 is better?

Copy link
Contributor

Choose a reason for hiding this comment

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

you can use a constant, better than hard code!

Copy link
Contributor

Choose a reason for hiding this comment

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

@winkyao
I think 1 isn't always the min handle. If a table has a PKIsHandle column, then the handle may be a negative value.
So I think we need to use math.MinInt64.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zimulala The key is encoding in binary format, so negative value is always greater than a positive value. @nolouch Right?

Copy link
Contributor Author

@crazycs520 crazycs520 May 6, 2019

Choose a reason for hiding this comment

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

Here is a test:
0 encode to byte is: [128 0 0 0 0 0 0 0]
math.MinInt64 encode to byte is: [0 0 0 0 0 0 0 0]

so, math.MinInt64 is better.

@codecov
Copy link

codecov bot commented Apr 28, 2019

Codecov Report

Merging #10203 into master will decrease coverage by 0.0206%.
The diff coverage is 63.5135%.

@@               Coverage Diff                @@
##             master     #10203        +/-   ##
================================================
- Coverage   77.6772%   77.6566%   -0.0206%     
================================================
  Files           411        412         +1     
  Lines         85460      85511        +51     
================================================
+ Hits          66383      66405        +22     
- Misses        14119      14141        +22     
- Partials       4958       4965         +7

for j, valueItem := range valuesItem {
x, ok := valueItem.(*driver.ValueExpr)
if !ok {
return nil, errors.Trace(errors.New("expect constant values"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we remove this trace? Other places don't use it.
And we can print the value to the error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

regionIDs := make([]uint64, 0, len(e.valueLists))
index := tables.NewIndex(e.table.Meta().ID, e.table.Meta(), e.indexInfo)
for _, values := range e.valueLists {
idxKey, _, err := index.GenIndexKey(e.ctx.GetSessionVars().StmtCtx, values, 1, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

@winkyao
I think 1 isn't always the min handle. If a table has a PKIsHandle column, then the handle may be a negative value.
So I think we need to use math.MinInt64.

planner/core/planbuilder.go Outdated Show resolved Hide resolved
planner/core/planbuilder.go Outdated Show resolved Hide resolved
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

executor/split.go Show resolved Hide resolved
err := s.WaitScatterRegionFinish(regionID)
if err != nil {
logutil.Logger(context.Background()).Warn("wait scatter region failed",
zap.Uint64("regionID", regionID),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add the table and index information to this log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@zimulala zimulala added the status/LGT1 Indicates that a PR has LGTM 1. label May 6, 2019
go.mod Outdated Show resolved Hide resolved
@crazycs520
Copy link
Contributor Author

/run-all-tests

Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 6, 2019
@crazycs520 crazycs520 merged commit 7ecb315 into pingcap:master May 6, 2019
crazycs520 added a commit to crazycs520/tidb that referenced this pull request Jun 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution status/LGT2 Indicates that a PR has LGTM 2. type/usability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants