-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fix character set mapping for utf8
and use it in schema-tracking tables
#15130
Conversation
Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -145,7 +152,7 @@ var globalVersionInfo = map[ID]struct { | |||
80: {alias: []collalias{{0b01111111, "cp850_bin", "cp850"}}, isdefault: 0b00000000}, | |||
81: {alias: []collalias{{0b01111111, "cp852_bin", "cp852"}}, isdefault: 0b00000000}, | |||
82: {alias: []collalias{{0b01111111, "swe7_bin", "swe7"}}, isdefault: 0b00000000}, | |||
83: {alias: []collalias{{0b01111111, "utf8_bin", "utf8"}, {0b01111111, "utf8mb3_bin", "utf8mb3"}}, isdefault: 0b00000000}, | |||
83: {alias: []collalias{{0b01111111, "utf8_bin", "utf8"}, {0b01111111, "utf8mb3_bin", "utf8mb3"}, {0b01111111, "utf8mb4_bin", "utf8mb4"}}, isdefault: 0b00000000}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a generated file based on the actual MySQL collations. So this change is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that PR, we used
utf8mb3
as the character set andutf8mb3_bin
for the collation of schema tracking tables since we wanted them to be case-sensitive. However, we realized thatutf8mb3
is deprecated in MySQL 5.7.
I don't understand this. We didn't then afterwards realize. It was already known that it's deprecated, but since MySQL itself uses internally still utf8mb3
for all table information (see information_schema
usage and in Vitess all usage of
vitess/go/mysql/collations/env.go
Lines 200 to 208 in e9c513e
// SystemCollation is the default collation for the system tables | |
// such as the information schema. This is still utf8mb3 to match | |
// MySQLs behavior. This means that you can't use utf8mb4 in table | |
// names, column names, without running into significant issues. | |
var SystemCollation = TypedCollation{ | |
Collation: CollationUtf8mb3ID, | |
Coercibility: CoerceCoercible, | |
Repertoire: RepertoireUnicode, | |
} |
So there's no need for any follow up that I know of.
In this PR, we change the schema definitions to use
utf8
instead ofutf8mb3
and also make changes to Vitess to replicate the MySQL behavior whereutf8
maps toutf8mb3
in MySQL 5.7 andutf8mb4
in MySQL 8.0.
This is not true. utf8
is still an alias for utf8mb3
in MySQL 8.0. And given MySQL's new versioning scheme, I don't think it will ever change for 8.0 and any changes will happen in a newer version.
Yeah I also don't think it's correct to change the encoding for these schema tables. The |
Thank-you both for looking at the PR! I have closed it in light of it not being a change that makes sense. |
Description
This PR builds on the changes made in #14904.
In that PR, we used
utf8mb3
as the character set andutf8mb3_bin
for the collation of schema tracking tables since we wanted them to be case-sensitive. However, we realized thatutf8mb3
is deprecated in MySQL 5.7.In this PR, we change the schema definitions to use
utf8
instead ofutf8mb3
and also make changes to Vitess to replicate the MySQL behavior whereutf8
maps toutf8mb3
in MySQL 5.7 andutf8mb4
in MySQL 8.0.Tests have been added to verify that the schema-diff creates the correct tables in MySQL 5.7 and MySQL 8.0 and the upgrades and downgrades generate the correct alter statements too.
Related Issue(s)
Checklist
Deployment Notes