-
-
Notifications
You must be signed in to change notification settings - Fork 520
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
Atomic migration #1379
Atomic migration #1379
Conversation
Just to clear some questions in my mind, so the updated default behaviour will be: wrapping the entire |
Hey @tyt2y3, the new behaviour is: The Then, inside each method, it will start a transaction to execute the migrations only if it's a Postgres connection. For non-Postgres connection, it just use connection as-is and execute the migrations as usual. |
sea-orm-migration/tests/main.rs
Outdated
println!("\nRoll back changes when encounter errors"); | ||
|
||
// Set a flag to throw error inside `m20230109_000001_seed_cake_table.rs` | ||
std::env::set_var("ABOARD_MIGRATION", "YES"); |
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.
I suspect this should be abort
?
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.
Oppps, what a typo
async fn up(&self, manager: &SchemaManager) -> Result<(), DbErr> { | ||
let db = manager.get_connection(); | ||
|
||
let transaction = db.begin().await?; |
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.
Why open a transaction here? Just want to confirm, the behaviour should be no different whether we open a transaction or not, right? Because it's already inside a transaction (on Postgres)?
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.
-
I want to show it's possible to open a transaction inside migrate
up
&down
. -
For Postgres, yes, it's no difference. Because as you said you're already inside a transaction. However, the story is different for MySQL and SQLite. Opening a transaction is vital because the migration itself isn't atomic. If you want to seed the database atomically then you need to start a transaction yourself.
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.
Got it, cool
.insert(&transaction) | ||
.await?; | ||
|
||
transaction.commit().await?; |
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.
For any backend, aborting the migration BEFORE committing makes more sense?
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.
Sure! see if both transactions got rolled back at the same time
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.
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.
OK. I also have some adjustments to make
* Update 02-writing-migration.md * Update SeaORM/docs/03-migration/02-writing-migration.md * Support various UUID formats that are available in `uuid::fmt` module (SeaQL/sea-orm#1325) * Casting columns as a different data type on select, insert and update (SeaQL/sea-orm#1304) * Methods of `ActiveModelBehavior` receive db connection as a parameter (SeaQL/sea-orm#1145, SeaQL/sea-orm#1328) * Added `execute_unprepared` method to `DatabaseConnection` and `DatabaseTransaction` (SeaQL/sea-orm#1327) * Added `Select::into_tuple` to select rows as tuples (instead of defining a custom Model) (SeaQL/sea-orm#1311) * Generate `#[serde(skip)]` for hidden columns (SeaQL/sea-orm#1171, SeaQL/sea-orm#1320) * Generate entity with extra derives and attributes for model struct (SeaQL/sea-orm#1124, SeaQL/sea-orm#1321) * Generate entity with extra derives and attributes for model struct (SeaQL/sea-orm#1124, SeaQL/sea-orm#1321) * async_trait * Migrations are now performed inside a transaction for Postgres (SeaQL/sea-orm#1379) * `MockDatabase::append_exec_results()`, `MockDatabase::append_query_results()`, `MockDatabase::append_exec_errors()` and `MockDatabase::append_query_errors()` take any types implemented `IntoIterator` trait (SeaQL/sea-orm#1367) * Cleanup the use of `vec!` macros * Added `DatabaseConnection::close` (SeaQL/sea-orm#1236) * Added `ActiveValue::reset` to convert `Unchanged` into `Set` (SeaQL/sea-orm#1177) * Added `QueryTrait::apply_if` to optionally apply a filter (SeaQL/sea-orm#1415) * Added the `sea-orm-internal` feature flag to expose some SQLx types (SeaQL/sea-orm#1297, SeaQL/sea-orm#1434) * Add `QuerySelect::columns` method - select multiple columns (SeaQL/sea-orm#1264) * Edit * Update SeaORM/docs/02-install-and-config/02-connection.md Co-authored-by: Chris Tsang <chris.2y3@outlook.com> * Update SeaORM/docs/05-basic-crud/03-insert.md Co-authored-by: Chris Tsang <chris.2y3@outlook.com> * fmt * Edit --------- Co-authored-by: Chris Tsang <chris.2y3@outlook.com>
PR Info
New Features
Breaking Changes
SchemaManager::conn
toSchemaManagerConnection
SchemaManager::new()
toIntoSchemaManagerConnection
SchemaManager::get_connection()
to&SchemaManagerConnection<'c>
MigratorTrait
's methodsget_migration_models
,get_migration_with_status
,get_pending_migrations
,get_applied_migrations
,install
andstatus
methods takeConnectionTrait
as the connectionfresh
,refresh
,reset
,up
anddown
methods takeIntoSchemaManagerConnection
as the connectionChanges
SchemaManagerConnection
andIntoSchemaManagerConnection