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.

Sign up for GitHub

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

Jump to bottom

Ensure collation is set to database default when changing column to binary #45792

Merged
merged 1 commit into from
Aug 9, 2022
Merged

Ensure collation is set to database default when changing column to binary #45792

merged 1 commit into from
Aug 9, 2022

Conversation

ahoglund
Copy link
Contributor

@ahoglund ahoglund commented Aug 9, 2022 •
edited

Binary types are not being set to an incompatible collation type when the column is changed. See this comment for details: #45744 (comment)

This PR adds a failing test which demonstrates a problem introduced in #45742, and a fix to the implementation to pass the test.

byroot
byroot approved these changes Aug 9, 2022
View reviewed changes
Copy link
Member

@byroot byroot left a comment

Choose a reason for hiding this comment

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

Thanks, the fix looks good to me. However please squash your commits.

@ahoglund ahoglund force-pushed the ahoglund/binary-collation-fix branch 2 times, most recently from 6b48798 to abcc9ee Compare August 9, 2022 15:00
@ahoglund ahoglund changed the title change_column switches to binary collation when type is binary Ensure collation is set to database default when changing column Aug 9, 2022
@ahoglund
Copy link
Contributor Author

ahoglund commented Aug 9, 2022

I see the buildkite/rails build is failing, but it looks unrelated to my change. Is there a way to rerun it?

@ahoglund
Ensure collation is set to database default when changing column
be0a58b 
A recent change made it so that when you perform a change column
it will preserve collation on the column rather than nil it out·
which would select the database default. This works fine except in
the case of a binary type. In this case the collation should be
set to nil in the options so that the database default collation
type can be used on the column.
@ahoglund ahoglund force-pushed the ahoglund/binary-collation-fix branch from abcc9ee to be0a58b Compare August 9, 2022 17:42
@ahoglund ahoglund changed the title Ensure collation is set to database default when changing column Ensure collation is set to database default when changing column to binary Aug 9, 2022
@byroot byroot merged commit 9b138de into rails:main Aug 9, 2022
@yahonda
Copy link
Member

yahonda commented Aug 9, 2022

Restarted the failing build.

@ahoglund ahoglund deleted the ahoglund/binary-collation-fix branch August 10, 2022 02:42
@chrisbloom7
Copy link
Contributor

chrisbloom7 commented Oct 14, 2022 •
edited

I'm running into this while upgrading from 7.0.3.1 to 7.0.4 and running db:migrate. The migrations fail when trying to create a varbinary column on an older mysql 5.7 database. It looks like this fix did not make it into the 7.0.4 release. Is there any plans to backport it? At the moment the only way I can seem to get around it is to load a previous schema dump that was generated after that migration was run.

Edit: Setting collation: "binary" on each of the add_column/change_column commands in any old migration files that referenced a varbinary column does not seem to be an equivalent solution as it leads to a different error: Mysql2::Error: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'binary NOT NULL' at line 1

byroot added a commit that referenced this pull request Nov 1, 2022
@byroot
Merge pull request #45792 from ahoglund/ahoglund/binary-collation-fix
89a1f6e 
Ensure collation is set to database default when changing column to binary
@skipkayhil
Copy link
Member

I'm running into this while upgrading from 7.0.3.1 to 7.0.4 and running db:migrate.

This should be fixed on 7-0-stable now. Feel free to open a new issue if you are still seeing errors!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@byroot byroot byroot approved these changes

Assignees
No one assigned
Labels
activerecord
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ahoglund @yahonda @chrisbloom7 @skipkayhil @byroot