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 alter table charset bug and compatibility #9790

Merged
merged 25 commits into from
Apr 4, 2019

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Mar 19, 2019

What problem does this PR solve?

  1. Change table charset should change all column charset too.

In MySQL

mysql > create table t1 (a varchar(10) character set ascii);
Query OK, 0 rows affected
Time: 0.021s
mysql > show  create table t1
+-------+----------------------------------------------------+
| Table | Create Table                                       |
+-------+----------------------------------------------------+
| t1    | CREATE TABLE `t1` (                                |
|       |   `a` varchar(10) CHARACTER SET ascii DEFAULT NULL |
|       | ) ENGINE=InnoDB DEFAULT CHARSET=latin1             |
+-------+----------------------------------------------------+
1 row in set
Time: 0.007s
mysql > alter table t1 convert to charset utf8mb4;
Query OK, 0 rows affected
Time: 0.028s
mysql > show  create table t1
+-------+-----------------------------------------+
| Table | Create Table                            |
+-------+-----------------------------------------+
| t1    | CREATE TABLE `t1` (                     |
|       |   `a` varchar(10) DEFAULT NULL          |
|       | ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 |
+-------+-----------------------------------------+
1 row in set

In TiDB

tidb > create table t1 (a varchar(10) character set ascii) charset=utf8
Query OK, 0 rows affected
Time: 0.007s
tidb > show create table t1
+-------+----------------------------------------------------------------------+
| Table | Create Table                                                         |
+-------+----------------------------------------------------------------------+
| t1    | CREATE TABLE `t1` (                                                  |
|       |   `a` varchar(10) CHARACTER SET ascii COLLATE ascii_bin DEFAULT NULL |
|       | ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin                |
+-------+----------------------------------------------------------------------+
1 row in set
Time: 0.006s
tidb > alter table t1 convert to charset utf8mb4;
Query OK, 0 rows affected
Time: 0.007s
tidb > show create table t1
+-------+----------------------------------------------------------------------+
| Table | Create Table                                                         |
+-------+----------------------------------------------------------------------+
| t1    | CREATE TABLE `t1` (                                                  |
|       |   `a` varchar(10) CHARACTER SET ascii COLLATE ascii_bin DEFAULT NULL |
|       | ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin          |
+-------+----------------------------------------------------------------------+
1 row in set
Time: 0.006s
  1. The table charset maybe "" cause create in the old TiDB version(such as 2.0.8), then execute alter table charset will return error, But it should execute successfully.

  2. Some behaver is not compatible with MySQL.

In MySQL

create table t (a varchar(10) character set utf8) charset=utf8;
mysql > alter table t charset latin1 charset utf8 charset utf8mb4 collate utf8_bin;
(1302, u"Conflicting declarations: 'CHARACTER SET latin1' and 'CHARACTER SET utf8'")
mysql > alter table t charset utf8mb4 collate "" collate utf8mb4_bin;
(1273, u"Unknown collation: ''")
mysql > alter table t charset utf8mb4 collate "";
(1273, u"Unknown collation: ''")
mysql > alter table t collate "";
(1273, u"Unknown collation: ''")

In TiDB v2.0.8

create table t1 (a varchar(10));
$ curl "http://$IP:10080/schema/test/t1"
{
    ...
    "charset": "",    #table charset.
    "collate": "",
    "cols": [
        {
            ...
            "id": 1,
            "name": {
                "L": "a",
                "O": "a"
            },
            ...
            "type": {
                "Charset": "utf8",  #column charset.
                "Collate": "utf8_bin",
                 ...

In TiDB master,

tidb > alter table t1 convert to charset utf8mb4;
(1105, u'unsupported modify charset from  to utf8mb4')

What is changed and how it works?

Change AlterTableCharsetAndCollate func.

Check List

Tests

  • Integration test

Code changes

  • Has exported function/method change

Side effects

Related changes

  • Need to cherry-pick to the release branch 2.1

ddl/ddl_api.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #9790 into master will decrease coverage by 0.0124%.
The diff coverage is 86.7469%.

@@               Coverage Diff                @@
##             master      #9790        +/-   ##
================================================
- Coverage   78.0083%   77.9959%   -0.0125%     
================================================
  Files           404        404                
  Lines         81954      82012        +58     
================================================
+ Hits          63931      63966        +35     
- Misses        13329      13341        +12     
- Partials       4694       4705        +11

@crazycs520
Copy link
Contributor Author

/run-all-tests

@crazycs520
Copy link
Contributor Author

/run-unit-test

ddl/ddl_api.go Outdated Show resolved Hide resolved
ddl/ddl_api.go Outdated Show resolved Hide resolved
ddl/ddl_api.go Outdated Show resolved Hide resolved
ddl/ddl_api.go Outdated Show resolved Hide resolved
@crazycs520
Copy link
Contributor Author

/run-all-tests

Copy link
Contributor

@xiekeyi98 xiekeyi98 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
ddl/table.go Outdated Show resolved Hide resolved
ddl/ddl_api.go Outdated Show resolved Hide resolved
ddl/ddl_api.go Outdated Show resolved Hide resolved
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

ddl/schema.go Outdated Show resolved Hide resolved
ddl/ddl_api.go Show resolved Hide resolved
ddl/ddl_api.go Outdated Show resolved Hide resolved
ddl/ddl_api.go Show resolved Hide resolved
ddl/db_integration_test.go Outdated Show resolved Hide resolved
@bb7133
Copy link
Member

bb7133 commented Mar 28, 2019

LGTM

ddl/ddl_api.go Outdated
if collates[i] == "" {
return "", "", ErrUnknownCollation.GenWithStackByArgs("")
}
}
collate = collates[len(collates)-1]
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that we need to check all collates.

mysql> alter table t charset utf8 collate utf8_bin collate utf8_bin collate utf8mb4_bin;
ERROR 1253 (42000): COLLATION 'utf8mb4_bin' is not valid for CHARACTER SET 'utf8'
mysql> alter table t charset utf8 collate utf8_bin collate utf8mb4_bin collate utf8_bin;
ERROR 1253 (42000): COLLATION 'utf8mb4_bin' is not valid for CHARACTER SET 'utf8'

@crazycs520 crazycs520 added the status/LGT2 Indicates that a PR has LGTM 2. label Apr 4, 2019
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
Copy link
Contributor

zimulala commented Apr 4, 2019

/run-all-tests

@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 4, 2019
@crazycs520 crazycs520 merged commit b2c7f08 into pingcap:master Apr 4, 2019
@crazycs520 crazycs520 deleted the fix-alter-table-charset branch April 4, 2019 11:53
crazycs520 added a commit to crazycs520/tidb that referenced this pull request Jun 5, 2019
@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
Labels
sig/sql-infra SIG: SQL Infra status/LGT3 The PR has already had 3 LGTM. type/bugfix This PR fixes a bug. type/compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants