Fix use of transactions to apply migrations #86
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
This patch fixes an issue that has apparently existed for a very long time: application of migrations in a transaction has not been working as intended! Currently, if you apply a migration like the following: ```sql -- +micrate Up CREATE TABLE foo (x int); invalid_sql; -- +micrate Down DROP TABLE foo; ``` This will: 1. Create table `foo` 2. Crash on parsing `invalid_sql` When this happens, the database will be left in a partially migrated state, with `foo` created and the migration marked as applied, essentially "bricking" micrate until you manually resolve it. - The migration will not be marked as applied - If you try to run `up` again, it will complain `foo` already exists, so you need to add `IF NOT EXISTS` or manually `DROP TABLE foo;`. - Hopefully there are no more errors, or you'll have to do it again! The reason this happens was just because of the misuse of the transaction API. Opening a tx with `db.transaction` checks out a connection from the pool, and issues `BEGIN` for you. From there, you must issue queries against `tx.connection` - where the tx is taking place - in order for them to be applied. Instead, `db` is used again, which instead pull *another* connection from the DB pool; meaning the tx does not get *any* of the migration statements. With this fix, failed migrations will be fully rolled back as intended, allowing you to make fixes & run `up` again without issue.
drujensen-happymoney
approved these changes
Jan 31, 2023
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.
This patch fixes an issue that has apparently existed for a very long time: application of migrations in a transaction has not been working as intended!
Currently, if you apply a migration like the following:
This will:
fooinvalid_sqlWhen this happens, the database will be left in a partially migrated state, with
foocreated and the migration marked as applied, essentially "bricking" micrate until you manually resolve it.upagain, it will complainfooalready exists, so you need to addIF NOT EXISTSor manuallyDROP TABLE foo;.The reason this happens was just because of the misuse of the transaction API. Opening a tx with
db.transactionchecks out a connection from the pool, and issuesBEGINfor you. From there, you must issue queries againsttx.connection- where the tx is taking place - in order for them to be applied.Instead,
dbis used again, which instead pull another connection from the DB pool; meaning the tx does not get any of the migration statements.With this fix, failed migrations will be fully rolled back as intended, allowing you to make fixes & run
upagain without issue.