schemadiff: normalize table options case #10200
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shlomi-noach
added
Type: Enhancement
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Bug fixes
Non-trivial changes
New/Existing features
Backward compatibility
|
dbussink
force-pushed
the
schemadiff-normalize-table-options
branch
from
c97ceb5
to
9cfb746
Compare
shlomi-noach
commented
May 3, 2022
go/vt/schemadiff/table.go
Outdated
Comment on lines
192
to
220
| var integralTypes = map[string]bool{ | ||
| "tinyint": true, | ||
| "smallint": true, | ||
| "mediumint": true, | ||
| "int": true, | ||
| "bigint": true, | ||
| } |
There was a problem hiding this comment.
Let's move these to mysql.go. There's other stuff I will move there.
|
Tracking issue: #10203 |
|
We have a legitimate |
dbussink
approved these changes
May 3, 2022
go/vt/schemadiff/mysql.go
Outdated
| "set": true, | ||
| } | ||
|
|
||
| // This is the default charset for MySQL 8.0 |
There was a problem hiding this comment.
This means that right now schemadiff is opinionated in the sense that it targets specifically how MySQL 8.0 works.
| // a timestamp defaults to `NOT NULL`. | ||
| // | ||
| // We opt here to instead remove that difference and always then add `NULL` and treat | ||
| // `explicit_defaults_for_timestamp` as always enabled in the context of DDL for diffing. |
There was a problem hiding this comment.
This is another case where the MySQL 8.0 behavior is considered the standard and the behavior we assume.
vmg
requested changes
May 3, 2022
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
This adds a number of additional normalization rules. First it enforces lowercasing for a number of column level options, namely the type, charset & collate. It then also maps utf8 always to utf8mb3. Additionally, we drop any length definitions for integral types. Another import one is that we drop any charset / collate definitions that match what the default table level option already is. This ensures a smaller diff and no conflicts on differences there. Different MySQL versions behavior differently there and this creates a consistent situation. Lastly it also fixes ENGINE references in PARTITION clauses to be handled consistently. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
When a column is defined with an explicit charset, but no collation, it defaults to the default collation for that charset, not the collation defined at the table level. This means that we need to handle this case in normalization. This is solved by solving the kind of "reverse" situation. What we do is still remove the charset definition, but if it is specified and diverges from the default collation at the table level, we add the collation explicitly at the column level. This is more clear for the end user what is happening and they are using a different collation on the column (even though the CREATE syntax kinda implicitly had that side effect in MySQL). Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
Timestamps have a fun runtime behavior that's dependent on the explicit_defaults_for_timestamp setting in MySQL. By default that's off in 5.7 and turned on in 8.0. With that flag disabled, MySQL will create timestamp fields with `NOT NULL` by default. In our normalization, we always opt for the behavior that 8.0 has from a DDL perspective when we parse things, assuming a `NULL` that is missing means `NULL` as documented. To ensure things work consistently then with 5.7 as well, in the normalized form we do add `NULL` to the DDL always to ensure we create things as expected. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
Depend on the collation package for all needed collation information. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
dbussink
force-pushed
the
schemadiff-normalize-table-options
branch
from
f5eba03
to
9e6ccc3
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Follow up to #10188, this PR normalizes values of table options, upon reading table definition.
for example, the engine name for InnoDB is always
InnoDBat that specific case. Even thoughINNODBis valid, we will always transform it toInnoDB.Character sets and collations are always lower cased.
ROW_FORMATvalues are always upper cased.We also now explicitly mutate
utf8toutf8mb3.Related Issue(s)
Checklist