DDL: fix a bug in column charset and collate when create table and modify column #11300

AilinKid · 2019-07-17T13:21:31Z

What problem does this PR solve?

This PR tries to fix the following bugs when creating a table and modify the columns:

tidb> create table t (a char(1) collate utf8_bin) charset utf8mb4 collate utf8mb4_bin;
Query OK, 0 rows affected (0.13 sec)

tidb> show create table t;
+-------+--------------------------------------------------------------------------------------------------------------------------------------------------+
| Table | Create Table                                                                                                                                     |
+-------+--------------------------------------------------------------------------------------------------------------------------------------------------+
| t     | CREATE TABLE `t` (
  `a` char(1) CHARACTER SET utf8mb4 COLLATE utf8_bin DEFAULT NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin |
+-------+--------------------------------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.00 sec)

Column a should use utf8_bin as collate and derive utf8 as it's charset as we expected.

tidb> create table t (a char(1) charset utf8 collate utf8_bin) charset utf8mb4 collate utf8mb4_bin;
Query OK, 0 rows affected (0.12 sec)

tidb> alter table t modify column a char(1) collate utf8_general_ci;
ERROR 1115 (42000): Unknown character set: 'utf8mb4', collation: 'utf8_general_ci'

The reason for the unexpected error is that: the second sql will use the table's charset and collate, the new col is consist of utf8mb4 charset from the table and utf8_general_ci collate from the specified one before it is assigned to old col.

What is changed and how it works?

When creating tables and modifying columns, the specified collate info is in colDef.Option rather colDef.collate.
So we should take this collate info into consideration when we set them or judge conflict or derive the corresponding charset.

Check List

Tests

Unit test
Integration test

Code changes

Add some logic code to handle collate info in colDef.Option and remove the option when we done to prevent double check.

codecov · 2019-07-17T13:26:40Z

Codecov Report

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

@@             Coverage Diff             @@
##             master     #11300   +/-   ##
===========================================
  Coverage   82.1169%   82.1169%           
===========================================
  Files           424        424           
  Lines         93636      93636           
===========================================
  Hits          76891      76891           
  Misses        11441      11441           
  Partials       5304       5304

winkyao · 2019-07-20T13:14:26Z

ddl/ddl_api.go

- if err := setCharsetCollationFlenDecimal(colDef.Tp, tblCharset, dbCharset); err != nil {
+ // The specified collate is in colDef.Options, but the charset is in colDef.Tp.
+ specifiedCollate := ""
+ for _, op := range colDef.Options {


The colDef.options is iterated in the https://github.com/pingcap/tidb/blob/master/ddl/ddl_api.go#L2062, Could we eliminate the iteration here?

The colDef.options is iterated in the https://github.com/pingcap/tidb/blob/master/ddl/ddl_api.go#L2062, Could we eliminate the iteration here?

I've checked it!
The buildColumnAndConstraint func is called by two different logic: one is create table, the other is modify column.
For the later there is an option loop indeed before call this func, if we try to make use of loop, we need pass parameter, it's ok. Consequently, for the former that's means we have no option loop neither in the func or before call it, it will cause an error.
So we still need the loop here.

winkyao · 2019-07-20T13:14:41Z

Rest LGTM

crazycs520 · 2019-07-22T05:11:00Z

ddl/ddl_api.go

}
+

remove this line.

remove this line.

Roger that!

AilinKid · 2019-07-22T09:07:26Z

/run-all-tests

AilinKid · 2019-07-22T09:21:14Z

/run-common-test

bb7133 · 2019-07-22T11:49:18Z

ddl/ddl_api.go

+func extractCollateFromOption(def *ast.ColumnDef) string {
+ specifiedCollate := ""
+ collateIndex := -1
+ for i, op := range def.Options {


As you may see, it is possible that we encounter multiple ast.ColumnOptionCollate, we should choose the last one as the final result:

For example, in MySQL 8.0:

mysql> create table tm(a varchar(20) collate utf8_unicode_ci collate utf8_general_ci) default charset=utf8mb4; Query OK, 0 rows affected, 2 warnings (0.08 sec) mysql> show create table tm; +-------+------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | Table | Create Table | +-------+------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | tm | CREATE TABLE `tm` ( `a` varchar(20) CHARACTER SET utf8 COLLATE utf8_general_ci DEFAULT NULL ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci | +-------+------------------------------------------------------------------------------------------------------------------------------------------------------------------+ 1 row in set (0.01 sec)

Notice that collation of column a is utf8_general_ci.

Please also notice that the when we resolve multiple collations, their charsets must be the same:

mysql> create table tm(a varchar(20) collate ascii_bin collate utf8_general_ci) default charset=utf8mb4; ERROR 1253 (42000): COLLATION 'utf8_general_ci' is not valid for CHARACTER SET 'ascii'

You can follow similar existing logics to handle multiple ColumnOptionCollate:

tidb/ddl/ddl_api.go

Line 109 in b3f7186

case ast.DatabaseOptionCollate:

You can follow similar existing logics to handle multiple ColumnOptionCollate:

tidb/ddl/ddl_api.go

Line 109 in b3f7186

case ast.DatabaseOptionCollate:

That's great !!！
From mysql syntax, charset is unique specified, but collate can be multiple.
That's means I need to follow mysql's behavior when handling multiple collate.

And this reference code do me a great favor!!!

bb7133 · 2019-07-22T14:02:23Z

executor/ddl_test.go

+
+ // Test charset and multiple collate modification in column
+ tk.MustExec("drop table if exists modify_column_multiple_collate;")
+ tk.MustExec("create table modify_column_multiple_collate (a char(1) collate utf8_bin collate utf8_general_ci) charset utf8mb4 collate utf8mb4_bin")


Please add some cases for ErrCollationCharsetMismatch

ddl/ddl_api.go

crazycs520

LGTM

winkyao · 2019-07-24T01:59:01Z

ddl/ddl_api.go

+// When handle charset and collate of a column, we should take collates(may multiple) in option into
+// consideration rather than handling it separately. Then the following option processing logic should
+// ignores this cause we will delete them from option here
+func extractCollateFromOption(def *ast.ColumnDef) []string {


the golang comment needs start with the function name, // extractCollateFromOption ....

the golang comment needs start with the function name, // extractCollateFromOption ....

ok. got it.

…column

winkyao

LGTM

bb7133 · 2019-07-24T07:34:04Z

/run-all-tests

bb7133

LGTM

ddl/ddl_api.go

winkyao · 2019-07-24T09:28:51Z

/run-cherry-picker

sre-bot · 2019-07-24T09:30:52Z

cherry pick to release-3.0 failed

sre-bot · 2019-07-24T09:33:03Z

cherry pick to release-2.1 failed

…dify column pingcap#11300

…dify column #11300 (#11423) All tests passed, auto merged by Bot

…dify column (pingcap#11300)

winkyao reviewed Jul 20, 2019

View reviewed changes

crazycs520 reviewed Jul 22, 2019

View reviewed changes

AilinKid force-pushed the fix_column_charset_and_collate branch from 00d307c to 7d52034 Compare July 22, 2019 09:02

AilinKid force-pushed the fix_column_charset_and_collate branch from 7d52034 to a396a72 Compare July 22, 2019 09:47

zimulala added the component/DDL-need-LGT3 label Jul 22, 2019

bb7133 reviewed Jul 22, 2019

View reviewed changes

AilinKid force-pushed the fix_column_charset_and_collate branch from a396a72 to 3866f0c Compare July 22, 2019 13:57

bb7133 reviewed Jul 22, 2019

View reviewed changes

crazycs520 reviewed Jul 23, 2019

View reviewed changes

ddl/ddl_api.go Show resolved Hide resolved

crazycs520 reviewed Jul 23, 2019

View reviewed changes

winkyao reviewed Jul 24, 2019

View reviewed changes

AilinKid added 6 commits July 24, 2019 10:21

fix a bug in column charset and collate when create table and modify …

95d27a9

…column

extract the fix code logic to a func

1511961

fix multiple collate specified in column

dc2bfb8

add more test for multiple column collate

f33f25d

add more Err case for multiple column collate

4347300

fix comment format

a730428

AilinKid force-pushed the fix_column_charset_and_collate branch from 900cfba to a730428 Compare July 24, 2019 02:22

winkyao reviewed Jul 24, 2019

View reviewed changes

winkyao added the status/LGT2 Indicates that a PR has LGTM 2. label Jul 24, 2019

bb7133 reviewed Jul 24, 2019

View reviewed changes

ddl/ddl_api.go Outdated Show resolved Hide resolved

bb7133 approved these changes Jul 24, 2019

View reviewed changes

Merge branch 'master' into fix_column_charset_and_collate

8040ca6

bb7133 added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Jul 24, 2019

AilinKid merged commit 29970ca into pingcap:master Jul 24, 2019

winkyao added needs-cherry-pick-3.0 labels Jul 24, 2019

AilinKid added a commit to AilinKid/tidb that referenced this pull request Jul 24, 2019

DDL: fix a bug in column charset and collate when create table and mo…

0553ba9

…dify column pingcap#11300

This was referenced Jul 24, 2019

DDL: fix a bug in column charset and collate when create table and modify column(#11300) #11423

Merged

DDL: fix a bug in column charset and collate when create table and modify column (#11300) #11424

Closed

sre-bot pushed a commit that referenced this pull request Jul 26, 2019

DDL: fix a bug in column charset and collate when create table and mo…

13ba8b2

…dify column #11300 (#11423) All tests passed, auto merged by Bot

AilinKid added a commit to AilinKid/tidb that referenced this pull request Jul 29, 2019

DDL: fix a bug in column charset and collate when create table and mo…

4189e2a

…dify column (pingcap#11300)

AilinKid mentioned this pull request Jul 29, 2019

DDL: fix a bug in column charset and collate when create table and modify column (#11300) #11492

Merged

you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DDL: fix a bug in column charset and collate when create table and modify column #11300

DDL: fix a bug in column charset and collate when create table and modify column #11300

AilinKid commented Jul 17, 2019 •

edited by bb7133

Loading

codecov bot commented Jul 17, 2019 •

edited

Loading

winkyao Jul 20, 2019

AilinKid Jul 22, 2019 •

edited

Loading

winkyao commented Jul 20, 2019

crazycs520 Jul 22, 2019

AilinKid Jul 22, 2019

AilinKid commented Jul 22, 2019

AilinKid commented Jul 22, 2019

bb7133 Jul 22, 2019

bb7133 Jul 22, 2019

AilinKid Jul 22, 2019

bb7133 Jul 22, 2019

crazycs520 left a comment

winkyao Jul 24, 2019

AilinKid Jul 24, 2019

winkyao left a comment

bb7133 commented Jul 24, 2019

bb7133 left a comment

winkyao commented Jul 24, 2019

sre-bot commented Jul 24, 2019

sre-bot commented Jul 24, 2019

DDL: fix a bug in column charset and collate when create table and modify column #11300

DDL: fix a bug in column charset and collate when create table and modify column #11300

Conversation

AilinKid commented Jul 17, 2019 • edited by bb7133 Loading

What problem does this PR solve?

What is changed and how it works?

Check List

codecov bot commented Jul 17, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

AilinKid Jul 22, 2019 • edited Loading

Choose a reason for hiding this comment

winkyao commented Jul 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AilinKid commented Jul 22, 2019

AilinKid commented Jul 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazycs520 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winkyao left a comment

Choose a reason for hiding this comment

bb7133 commented Jul 24, 2019

bb7133 left a comment

Choose a reason for hiding this comment

winkyao commented Jul 24, 2019

sre-bot commented Jul 24, 2019

sre-bot commented Jul 24, 2019

AilinKid commented Jul 17, 2019 •

edited by bb7133

Loading

codecov bot commented Jul 17, 2019 •

edited

Loading

AilinKid Jul 22, 2019 •

edited

Loading