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, table: allow using SHARD_ROW_ID_BITS with auto_incremental columns #10759

Merged
merged 5 commits into from
Jun 12, 2019
Merged

ddl, table: allow using SHARD_ROW_ID_BITS with auto_incremental columns #10759

merged 5 commits into from
Jun 12, 2019

Conversation

bb7133
Copy link
Member

@bb7133 bb7133 commented Jun 11, 2019

What problem does this PR solve?

This PR allows using SHARD_ROW_ID_BITS for tables with AUTO_INCREMENT columns.

Before this PR, SHARD_ROW_ID_BITS cannot work with AUTO_INCREMENT:

tidb> CREATE TABLE T5 (`A` INT UNIQUE KEY AUTO_INCREMENT);
Query OK, 0 rows affected (0.01 sec)
tidb> ALTER TABLE T5 SHARD_ROW_ID_BITS = 4;
ERROR 1105 (HY000): unsupported shard_row_id_bits for table with auto_increment column.

This restriction leads to inconveniences to many TiDB users, this PR gives a quick way to remove it by:

  • For row_id(or handle), TiDB still tries to make sharding if SHARD_ROW_ID_BITS is defined.
  • For auto_increment_id(or autoid), TiDB doesn't make sharding to keep it incremental always.

What is changed and how it works?

A new AllocHandle() is added to table.Table, which is used for the allocation of handle. Now AllocAutoID() will not try to do sharding for the allocated ids.

Check List

Tests

  • Integration test

Code changes

  • Has exported function/method change

Side effects

  • Increased code complexity

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation
  • Need to be included in the release note

@bb7133 bb7133 force-pushed the bb7133/shard_auto_incr branch from dbcfbb1 to 22f42fc Compare June 11, 2019 03:29
@codecov
Copy link

codecov bot commented Jun 11, 2019

Codecov Report

Merging #10759 into master will increase coverage by 0.0385%.
The diff coverage is 42.8571%.

@@               Coverage Diff                @@
##             master     #10759        +/-   ##
================================================
+ Coverage   79.8572%   79.8958%   +0.0385%     
================================================
  Files           415        415                
  Lines         88270      88310        +40     
================================================
+ Hits          70490      70556        +66     
+ Misses        12572      12548        -24     
+ Partials       5208       5206         -2

@codecov
Copy link

codecov bot commented Jun 11, 2019 •
edited
Loading

Codecov Report

Merging #10759 into master will decrease coverage by 0.0304%.
The diff coverage is 57.1428%.

@@               Coverage Diff               @@
##             master    #10759        +/-   ##
===============================================
- Coverage   80.3175%   80.287%   -0.0305%     
===============================================
  Files           416       416                
  Lines         88251     88282        +31     
===============================================
- Hits          70881     70879         -2     
- Misses        12178     12205        +27     
- Partials       5192      5198         +6

@bb7133 bb7133 added component/DDL-need-LGT3 type/enhancement The issue or PR belongs to an enhancement. labels Jun 11, 2019
@bb7133 bb7133 requested review from tiancaiamao and coocood June 11, 2019 07:06
crazycs520
crazycs520 reviewed Jun 11, 2019
View reviewed changes
Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

LGTM

@crazycs520
Copy link
Contributor

/run-all-tests

coocood
coocood reviewed Jun 11, 2019
View reviewed changes
table/tables/tables.go Outdated
@@ -916,6 +916,11 @@ func GetColDefaultValue(ctx sessionctx.Context, col *table.Column, defaultVals [

// AllocAutoID implements table.Table AllocAutoID interface.
func (t *tableCommon) AllocAutoID(ctx sessionctx.Context) (int64, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe AllocAutoIncrementValue is more accurate.

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed, thanks! @coocood

@bb7133 bb7133 force-pushed the bb7133/shard_auto_incr branch from 10781d1 to 78b6c2a Compare June 11, 2019 11:13
@bb7133 bb7133 force-pushed the bb7133/shard_auto_incr branch from 78b6c2a to 872256d Compare June 11, 2019 11:16
@coocood
Copy link
Member

coocood commented Jun 11, 2019

@bb7133
We should check that the table's PK is not handle.

@bb7133
Copy link
Member Author

bb7133 commented Jun 11, 2019

@bb7133
We should check that the table's PK is not handle.

I didn't get your point: If the PK is handle, AllocHandle() is not called... the check is there already

@coocood
Copy link
Member

coocood commented Jun 12, 2019

@bb7133
We should check that the table's PK is not handle.

I didn't get your point: If the PK is handle, AllocHandle() is not called... the check is there already

I mean check it in the create table phase.

@bb7133
Copy link
Member Author

bb7133 commented Jun 12, 2019

addressed, PTAL @coocood @crazycs520

coocood
coocood reviewed Jun 12, 2019
View reviewed changes
ddl/ddl_api.go Outdated
@@ -1219,6 +1219,9 @@ func buildTableInfoWithCheck(ctx sessionctx.Context, d *ddl, s *ast.CreateTableS
if err = handleTableOptions(s.Options, tbInfo); err != nil {
return nil, errors.Trace(err)
}
if tbInfo.ShardRowIDBits > 0 && tbInfo.PKIsHandle {
Copy link
Member

Choose a reason for hiding this comment

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

Why not put it in handleTableOptions?

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed, thanks!

@bb7133 bb7133 force-pushed the bb7133/shard_auto_incr branch from 239da3a to 7f7ad56 Compare June 12, 2019 10:24
@coocood
Copy link
Member

coocood commented Jun 12, 2019

LGTM

@coocood coocood added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 12, 2019
crazycs520
crazycs520 reviewed Jun 12, 2019
View reviewed changes
Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

LGTM

@crazycs520 crazycs520 added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 12, 2019
@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added require-LGT3 Indicates that the PR requires three LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Jun 12, 2019
@tiancaiamao
Copy link
Contributor

/run-all-tests

@tiancaiamao tiancaiamao merged commit 7d27fa6 into pingcap:master Jun 12, 2019
bb7133 added a commit to bb7133/tidb that referenced this pull request Jun 12, 2019
@bb7133
ddl, table: allow using SHARD_ROW_ID_BITS with auto_incremental colum…
9873d40 
…ns (pingcap#10759)
@bb7133 bb7133 mentioned this pull request Jun 12, 2019
Merged
bb7133 added a commit to bb7133/tidb that referenced this pull request Jun 13, 2019
@bb7133
ddl, table: allow using SHARD_ROW_ID_BITS with auto_incremental colum…
fe00b95 
…ns (pingcap#10759)
@bb7133 bb7133 mentioned this pull request Jun 13, 2019
Merged
winkyao pushed a commit that referenced this pull request Jun 14, 2019
@bb7133 @winkyao
ddl, table: allow using SHARD_ROW_ID_BITS with auto_incremental colum…
09fb804 
…ns (#10759) (#10794)
@you06 you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020
@bb7133 bb7133 deleted the bb7133/shard_auto_incr branch December 29, 2023 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crazycs520 crazycs520 crazycs520 left review comments

@coocood coocood coocood left review comments

@tiancaiamao tiancaiamao tiancaiamao approved these changes

Assignees
No one assigned
Labels
require-LGT3 Indicates that the PR requires three LGTM. sig/sql-infra SIG: SQL Infra type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bb7133 @crazycs520 @coocood @tiancaiamao @you06