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

ddl, meta: allow increasing auto_random bits #17423

Merged
merged 28 commits into from
Jun 10, 2020

Conversation

tangenta
Copy link
Contributor

@tangenta tangenta commented May 26, 2020

What problem does this PR solve?

Issue Number: close #17422

Problem Summary:

What is changed and how it works?

Proposal: xxx

What's Changed:

  • Relax the restriction "Disallow modifying auto_random bits".
  • Add a DDL job argument in the case of ModifyColumn: newAutoRandBits.
  • Add an error message: "decreasing auto_random bits is not supported".
  • Add tests.

How it Works:

To make sure the new auto_random bits number does not affect the correctness of implicit allocation, the check is as follows:

If the length of the incremental bit in the new layout is greater than those in the origin layout, it is safe to switch to the new layout.

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test

Side effects

Release note

  • The number of auto_random bits is allowed to increase by ALTER TABLE if possible.

@tangenta tangenta added the sig/sql-infra SIG: SQL Infra label May 26, 2020
@tangenta tangenta requested review from AilinKid and bb7133 May 26, 2020 11:19
@codecov
Copy link

codecov bot commented May 27, 2020

Codecov Report

Merging #17423 into master will decrease coverage by 0.3084%.
The diff coverage is 84.3137%.

@@               Coverage Diff                @@
##             master     #17423        +/-   ##
================================================
- Coverage   79.7675%   79.4591%   -0.3085%     
================================================
  Files           525        524         -1     
  Lines        144184     141362      -2822     
================================================
- Hits         115012     112325      -2687     
+ Misses        19993      19958        -35     
+ Partials       9179       9079       -100     

Copy link
Contributor

@AilinKid AilinKid left a comment

Choose a reason for hiding this comment

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

Rest LGTM

ddl/column.go Show resolved Hide resolved
@AilinKid AilinKid requested a review from djshow832 May 27, 2020 03:17
ddl/column.go Show resolved Hide resolved
ddl/column.go Show resolved Hide resolved
@zimulala zimulala added the require-LGT3 Indicates that the PR requires three LGTM. label Jun 2, 2020
Copy link
Contributor

@AilinKid AilinKid left a comment

Choose a reason for hiding this comment

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

LGTM

meta/autoid/errors.go Outdated Show resolved Hide resolved
@tangenta tangenta requested review from bb7133 and djshow832 June 4, 2020 04:57
Copy link
Contributor

@djshow832 djshow832 left a comment

Choose a reason for hiding this comment

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

LGTM

ddl/ddl_api.go Outdated Show resolved Hide resolved
@@ -585,17 +588,20 @@ func (w *worker) onModifyColumn(t *meta.Meta, job *model.Job) (ver int64, _ erro
oldColName := &model.CIStr{}
pos := &ast.ColumnPosition{}
var modifyColumnTp byte
err := job.DecodeArgs(newCol, oldColName, pos, &modifyColumnTp)
var updatedAutoRandomBits uint64
err := job.DecodeArgs(newCol, oldColName, pos, &modifyColumnTp, &updatedAutoRandomBits)
Copy link
Contributor

Choose a reason for hiding this comment

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

The DDL owner is an old-version TiDB: auto_random shard bits remain unchanged(but DDL successfully executed).

It will not cause data inconsistency, but it will confuse the user, the execution is successful, but the function is not successful.

ddl/column.go Show resolved Hide resolved
@@ -585,17 +588,20 @@ func (w *worker) onModifyColumn(t *meta.Meta, job *model.Job) (ver int64, _ erro
oldColName := &model.CIStr{}
pos := &ast.ColumnPosition{}
var modifyColumnTp byte
err := job.DecodeArgs(newCol, oldColName, pos, &modifyColumnTp)
var updatedAutoRandomBits uint64
err := job.DecodeArgs(newCol, oldColName, pos, &modifyColumnTp, &updatedAutoRandomBits)
Copy link
Contributor

Choose a reason for hiding this comment

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

At the moment, I don’t seem to have any good methods other than the newly added types. Or you ask if the DBA can accept this situation

@zimulala zimulala added the compatibility-breaker Violation of forwards/backwards compatibility in a design-time piece. label Jun 9, 2020
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 Jun 9, 2020
@tangenta
Copy link
Contributor Author

tangenta commented Jun 9, 2020

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 9, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jun 9, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 9, 2020

@tangenta merge failed.

@tangenta
Copy link
Contributor Author

tangenta commented Jun 9, 2020

/run-common-test
/run-unit-test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility-breaker Violation of forwards/backwards compatibility in a design-time piece. require-LGT3 Indicates that the PR requires three LGTM. sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support increasing auto_random bits if possible
8 participants