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.

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

Use DELETE FROM instead of TRUNCATE for MySQL #656

Merged
merged 2 commits into from Dec 18, 2021

Conversation

antigremlin
Copy link
Contributor

Resolves issue #584

In MySQL, TRUNCATE statements cannot be rolled back, at least for 5.7:

This is another attempt at fixing, as the previous PR #585 has stalled. The credit goes to @martinarrieta

@antigremlin
Copy link
Contributor Author

The first PR mentioned above accidentally rearranged imports. This PR only changes the one line necessary. I'd be happy to see this merged soon as this is a critical bug. It nearly did damage to one of my databases.

@coveralls
Copy link

coveralls commented Nov 17, 2021

Pull Request Test Coverage Report for Build 1560897359

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 12 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.06%) to 57.8%

Files with Coverage Reduction New Missed Lines %
source/migration.go 12 13.64%
Totals Coverage Status
Change from base Build 1424470324: 0.06%
Covered Lines: 3731
Relevant Lines: 6455

💛 - Coveralls

Copy link
Member

@dhui dhui left a comment

Choose a reason for hiding this comment

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

Per #584 (comment), the transaction needs to be serializable

@antigremlin
Copy link
Contributor Author

Per #584 (comment), the transaction needs to be serializable

This PR fixes a very dangerous bug: If the migration process gets killed at the wrong moment, it leaves the version table empty (the truncate was successful, but the insert was interrupted). Next thing that happens is that the tool tries to reapply all the migrations from the beginning, which can be disastrous.

I can mark it as serializable. But why do you think it changes anything? According to https://dev.mysql.com/doc/refman/5.7/en/innodb-transaction-isolation-levels.html serializable only affects SELECT statements, not DELETE and INSERT ones.

@dhui
Copy link
Member

dhui commented Dec 8, 2021

I can mark it as serializable. But why do you think it changes anything? According to https://dev.mysql.com/doc/refman/5.7/en/innodb-transaction-isolation-levels.html serializable only affects SELECT statements, not DELETE and INSERT ones.

Please do. For safety, we should use the most strict isolation level. There's nearly no downside and setting the migration version doesn't need to be performant. e.g. we're happy to pay for any overhead between REPEATABLE READ and SERIALIZABLE for the extra safety.
This protects against other instances of migrate reading the migration version, which may later update the version.

@obalunenko
Copy link

@antigremlin Can you rebase your branch on base branch and update the PR? 🙏🏿

@dhui dhui merged commit 9f5ed82 into golang-migrate:master Dec 18, 2021
@antigremlin
Copy link
Contributor Author

@antigremlin Can you rebase your branch on base branch and update the PR? 🙏🏿

My apologies. Was going to do that and didn't notice the PR is already merged.
Great to see this in. Thank you @dhui!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants