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.

Sign up for GitHub

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

Jump to bottom

ddl: fix alter table share rowid bit problem #9868

Merged
merged 19 commits into from
Apr 9, 2019
Merged

ddl: fix alter table share rowid bit problem #9868

merged 19 commits into from
Apr 9, 2019

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Mar 22, 2019 •
edited
Loading

What problem does this PR solve?

There is some problem in share rowid bits. Think about follow situation.
The current SHARD_ROW_ID_BITS = 6, then, after alter SHARD_ROW_ID_BITS = 4,

100011 1001100110011001100110011001100110011001100110011001100111

is duplicated with:

1000 111001100110011001100110011001100110011001100110011001100111

What is changed and how it works?

Add a MaxShardRowIDBits to tableInfo to record the max ShardRowIDBits has been used.
Then use MaxShardRowIDBits to check autoID overflow.

Then in upper situation, after SHARD_ROW_ID_BITS = 4, TiDB will also use 6 to check allocateAutoID overflow. then the duplicate ID cannot be insert , because the duplicated ID is overflow with MaxShardRowIDBits=6.

Related Parser PR: pingcap/parser#252, Pleaser merge parser first.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change

Side effects

Related changes

  • Need to cherry-pick to the release branch

winkyao
winkyao reviewed Mar 25, 2019
View reviewed changes
ddl/table.go Outdated Show resolved Hide resolved
@crazycs520
Copy link
Contributor Author

Please merge pingcap/parser#252 first.

winkyao
winkyao reviewed Mar 28, 2019
View reviewed changes
ddl/table.go Outdated Show resolved Hide resolved
winkyao
winkyao reviewed Mar 28, 2019
View reviewed changes
ddl/table.go Show resolved Hide resolved
winkyao
winkyao reviewed Apr 4, 2019
View reviewed changes
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

@codecov
Copy link

codecov bot commented Apr 4, 2019 •
edited
Loading

Codecov Report

Merging #9868 into master will decrease coverage by 0.0181%.
The diff coverage is 58.8235%.

@@               Coverage Diff               @@
##             master     #9868        +/-   ##
===============================================
- Coverage   78.1391%   78.121%   -0.0182%     
===============================================
  Files           405       405                
  Lines         82005     82033        +28     
===============================================
+ Hits          64078     64085         +7     
- Misses        13231     13248        +17     
- Partials       4696      4700         +4

zimulala
zimulala reviewed Apr 8, 2019
View reviewed changes
table/tables/tables.go Outdated Show resolved Hide resolved
table/tables/tables.go Outdated Show resolved Hide resolved
table/tables/tables.go Show resolved Hide resolved
@zimulala zimulala added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 8, 2019
@crazycs520
Copy link
Contributor Author

/run-all-tests

1 similar comment
@crazycs520
Copy link
Contributor Author

/run-all-tests

bb7133
bb7133 reviewed Apr 8, 2019
View reviewed changes
ddl/table.go Outdated Show resolved Hide resolved
ddl/db_test.go Outdated Show resolved Hide resolved
ddl/db_test.go Outdated Show resolved Hide resolved
ddl/db_test.go Outdated Show resolved Hide resolved
@tiancaiamao
Copy link
Contributor

LGTM
Please add notes about the compatibility, and maybe you need update the document too.

@tiancaiamao tiancaiamao added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 8, 2019
zimulala
zimulala previously approved these changes Apr 9, 2019
View reviewed changes
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/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Apr 9, 2019
@zimulala
Copy link
Contributor

zimulala commented Apr 9, 2019

/run-all-tests
Please fix the confliction.

@crazycs520
Copy link
Contributor Author

/run-all-tests

@crazycs520
Copy link
Contributor Author

/rebuild

@crazycs520 crazycs520 merged commit ed3e265 into pingcap:master Apr 9, 2019
crazycs520 added a commit to crazycs520/tidb that referenced this pull request May 13, 2019
@crazycs520
ddl: fix alter table share rowid bit problem (pingcap#9868)
c6a90dc
@crazycs520 crazycs520 mentioned this pull request May 13, 2019
Merged
zz-jason pushed a commit that referenced this pull request May 14, 2019
@crazycs520 @zz-jason
ddl: fix alter table share rowid bit problem (#9868) (#10436)
713ed0d
@you06 you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@winkyao winkyao winkyao left review comments

@bb7133 bb7133 bb7133 left review comments

@zimulala zimulala zimulala approved these changes

Assignees
No one assigned
Labels
sig/sql-infra SIG: SQL Infra status/LGT3 The PR has already had 3 LGTM. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@crazycs520 @tiancaiamao @zimulala @bb7133 @winkyao @you06