-
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
[Direct PR] [release-18.0] Remove sql changes from schemacopy
#17050
[Direct PR] [release-18.0] Remove sql changes from schemacopy
#17050
Conversation
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
|
I would appreciate reviews from both of you, @shlomi-noach and @arthurschreiber, as the original PR reviewer and creators! 🥺 |
I agree with Manan's analysis, but we'll wait for the other reviewers. |
Without the schema change the detect query fails -
|
I don't remember the reason behind it, but can't we use utf8mb4 based collations for the query? 🤔 |
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 inline with schema_migrations.sql
and other tables.
toQueries: []string{ | ||
"create table t1 (id int primary key, foo varchar(64) collate utf8mb3_bin)", | ||
}, | ||
// This isn't strictly correct. We have a diff even though there shouldn't be one. |
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.
It is, if we assume the default charset is utf8mb4
, right?
This is potentially related? #16670 |
I looked into this in more detail, and here is what I found. In release-17.0, the schemacopy schema we had was -
and then when we upgrade to release-18.0, the schema becomes -
this makes schema diff generate this diff -
However, when we apply this, the
this creates schema diff to continue to generate the following diff with the desired schema -
The problem is the default charset added in the end. We could change the schemacopy table to have utf8mb4 collations and charsets to fix this problem on MySQL 8.0, but then MySQL 5.7 would break because the default charset there would be different! I think this was fixed properly by #14930 (I'm not sure, I didn't confirm), but its only available release-19.0 onwards. Since there is no good way of fixing this easily, and it doesn't actually cause any catastrophic issue, we're just gonna not do anything for now. |
Description
This PR is fixing an issue we saw while running v18 in production. With the changes to the SQL made in #15859, we are seeing that ever iteration of syncSideCarDB is finding a schema diff and running it -
This is because the schema diff code doesn't handle collations and charsets correctly. I've added a test for that assertion too. Moreover, in the PR #15859, we already changed how we read the data from
schemacopy
so it doesn't look like the SQL changes there are required. They are being reverted in this PR.Related Issue(s)
Checklist
Deployment Notes