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: fix index length calculation #13727

Merged
merged 4 commits into from
Nov 27, 2019
Merged

Conversation

tangenta
Copy link
Contributor

@tangenta tangenta commented Nov 25, 2019

What problem does this PR solve?

In MySQL 8.0:

create table t12 (a varchar(1000) primary key);
Error 1071: Specified key was too long; max key length is 3072 bytes

In TiDB master:

create table t12 (a varchar(1000) primary key);
Query OK, 0 rows affected

The default charset is utf8mb4 with at most 4 bytes per character. However, TiDB assumes 1 byte per character by default, which is incorrect.

What is changed and how it works?

  • Rewrite getIndexColumnLength, taking charsets into consideration.
  • Add more tests.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test

Code changes

N/A

Side effects

  • Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Release note

  • ddl: fix the index length calculation logic.

@codecov
Copy link

codecov bot commented Nov 25, 2019

Codecov Report

Merging #13727 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #13727   +/-   ##
===========================================
  Coverage   80.4209%   80.4209%           
===========================================
  Files           474        474           
  Lines        118269     118269           
===========================================
  Hits          95113      95113           
  Misses        15740      15740           
  Partials       7416       7416

ddl/index.go Outdated Show resolved Hide resolved
ddl/index.go Outdated Show resolved Hide resolved
ddl/index.go Outdated Show resolved Hide resolved
ddl/index.go Outdated Show resolved Hide resolved
@tangenta tangenta force-pushed the index-length-calc branch 2 times, most recently from 388ea3f to f4df915 Compare November 26, 2019 06:28
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

Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

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

LGTM

@bb7133 bb7133 added the status/LGT2 Indicates that a PR has LGTM 2. label Nov 26, 2019
ddl/index.go Show resolved Hide resolved
ddl/index.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

@wjhuang2016 wjhuang2016 added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Nov 26, 2019
@tangenta tangenta added the status/can-merge Indicates a PR has been approved by a committer. label Nov 26, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 26, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Nov 26, 2019

@tangenta merge failed.

@tangenta
Copy link
Contributor Author

/run-all-tests

@tangenta
Copy link
Contributor Author

/run-common-test -tidb-test=pr/956
/run-integration-common-test -tidb-test=pr/956

@tangenta tangenta merged commit 755fd19 into pingcap:master Nov 27, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 27, 2019

cherry pick to release-3.0 in PR #13779

@sre-bot
Copy link
Contributor

sre-bot commented Nov 27, 2019

cherry pick to release-2.1 failed

@sre-bot
Copy link
Contributor

sre-bot commented Apr 7, 2020

It seems that, not for sure, we failed to cherry-pick this commit to release-2.1. Please comment '/run-cherry-picker' to try to trigger the cherry-picker if we did fail to cherry-pick this commit before. @tangenta PTAL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

9 participants