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

infoschema, domain, ddl: fix upper cased charset names #10272

Merged
merged 4 commits into from
Apr 29, 2019

Conversation

bb7133
Copy link
Member

@bb7133 bb7133 commented Apr 25, 2019

What problem does this PR solve?

This version aims to deal with upper-cased charset name in TableInfo stored by previous versions of TiDB.

TiDB always supposes that all charsets / collations are lower-cased and try to convert them if they're not. However, the convert is missed in some scenarios, some upper-cased charsets / collations are kept and stored, leading to some potential issues:

mysql> create table t1(a varchar(20)) default charset=utf8;
Query OK, 0 rows affected (0.03 sec)
mysql> insert into t1 values ('特克斯和凯科斯群岛');
Query OK, 1 row affected (0.01 sec)

mysql> create table t2(a varchar(20)) default charset=UTF8;
Query OK, 0 rows affected (0.03 sec)
mysql> insert into t2 values ('特克斯和凯科斯群岛');
ERROR 1406 (22001): Data too long for column 'a' at row 1

mysql> select version();
+------------------------------------------+
| version()                                |
+------------------------------------------+
| 5.7.25-TiDB-v3.0.0-beta.1-104-g0a9569af4 |
+------------------------------------------+
1 row in set (0.00 sec)

What is changed and how it works?

With the help of pingcap/parser#301, a new TableInfoVersion3 is added as CurrLatestTableInfoVersion. All tables with version prior to TableInfoVersion3 will do the charset / collation name convert when loaded.

Also, pingcap/parser#301 validates all charset / collation names and convert them to lower-case in parser.

Check List

Tests

  • Integration test

Code changes

  • Has exported function/method change

Side effects

  • (Potentially) Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch

@mahjonp
Copy link
Contributor

mahjonp commented Apr 25, 2019

/rebuild

3 similar comments
@zhouqiang-cl
Copy link
Contributor

/rebuild

@zhouqiang-cl
Copy link
Contributor

/rebuild

@mahjonp
Copy link
Contributor

mahjonp commented Apr 26, 2019

/rebuild

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 29, 2019

Codecov Report

Merging #10272 into master will increase coverage by 0.0154%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master     #10272        +/-   ##
================================================
+ Coverage   77.6568%   77.6723%   +0.0154%     
================================================
  Files           411        411                
  Lines         85431      85441        +10     
================================================
+ Hits          66343      66364        +21     
+ Misses        14124      14119         -5     
+ Partials       4964       4958         -6

Copy link
Contributor

@lonng lonng left a comment

Choose a reason for hiding this comment

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

LGTM

@lonng lonng added the status/LGT2 Indicates that a PR has LGTM 2. label Apr 29, 2019
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

@bb7133
Copy link
Member Author

bb7133 commented Apr 29, 2019

/run-unit-test

@bb7133 bb7133 merged commit 7cc688c into pingcap:master Apr 29, 2019
bb7133 added a commit to bb7133/tidb that referenced this pull request May 5, 2019
@bb7133 bb7133 deleted the bb7133/fix_charset_case branch December 29, 2023 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants