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

feat(postgres): allow running multiple statements in no tx migrations #3694

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions sqlx-postgres/src/migrate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -276,10 +276,19 @@ async fn execute_migration(
conn: &mut PgConnection,
migration: &Migration,
) -> Result<(), MigrateError> {
let _ = conn
.execute(&*migration.sql)
.await
.map_err(|e| MigrateError::ExecuteMigration(e, migration.version))?;
let sql = migration.sql.trim();
// note: this would _not_ match the split if the file starts with `-- split-migration`
// because it requires a new line prefix, but that doesn't really make sense anyway so it's fine
let split_migrations = sql.split("\n-- split-migration\n");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Bikeshedding: I don't think -migration adds any information here. If I encountered this without context while reading through a migration file and didn't already know what it did, I'd just have a lot more questions:

  • How is it splitting the migration? Is it generating a separate record in the migrations table?
  • What does splitting the migration mean?
  • Why would you want to split the migration?

I think we could choose a much more self-descriptive name here. I suggested -- statement-break in the original issue but I'm not sure that's helpful, either. "Aren't statements already broken up by semicolons?" I would ask.

Here's some ideas:

  • -- split-execute
  • -- execute-separately
  • -- transaction-break
  • -- autocommit-break
    • I like the idea of working "autocommit" in there somewhere because it could be helpful to remind the user why this exists.
  • -- split-autocommit
  • -- transactional-ddl-split

Copy link
Author

Choose a reason for hiding this comment

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

I'm happy with any of them, feel free to pick your favorite and I'll edit the PR 😄

Copy link
Author

Choose a reason for hiding this comment

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

Hey @abonander any thoughts on which marker we should choose? Of those options I personally like -- transaction-break but again no strong opinion

Copy link
Collaborator

@abonander abonander Jan 30, 2025

Choose a reason for hiding this comment

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

Yeah, transaction-break seems to be the front-runner, but my biggest concern is someone might look at a migration file and see something like this:

-- no-transaction
Statement;
Statement;
Statement;

-- transaction-break
Command;
-- transaction-break

Statement;

and be really confused. "Wtf? I thought this was already not executing in a transaction? What does -- transaction-break mean if it's not supposed to be in a transaction in the first place?"

However, I'm now realizing that this functionality actually can supercede -- no-transaction as long as we make sure to handle the migration as if it started with -- no-transaction.

This actually has great potential value to the user because we can execute everything before the first -- transaction-break in a transaction block, so if there's an error in that part of the migration, it can be completely rolled back and the user isn't stuck with a partially applied migration.

And also, if the only statement in the migration is one that can't happen in a transaction, -- no-transaction and -- transaction-break are effectively equivalent.

There is probably still value in keeping -- no-transaction in that situation because I believe there's "Postgres-compatible" third-party databases that don't support any DDL in a transaction, and -- no-transaction kind of makes more sense as a whole-file flag than -- transaction-break.

Alternatively, we could just roll this functionality into -- no-transaction since it's essentially a superset of its behavior. But I think seeing multiple -- no-transaction lines in a migration would also be confusing; it'd be like, "did it not get the message the first time?"

Maybe... roll it together, support -- transaction-break as an alias of -- no-transaction and leave it up to the user as a stylistic choice? But it's probably better to just pick one label and stick with it.

Copy link
Collaborator

@abonander abonander Jan 30, 2025

Choose a reason for hiding this comment

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

Eh, I would probably leave -- no-transaction as it is. But use of -- transaction-break would make it optional.

for part in split_migrations {
if part.trim().is_empty() {
continue;
}
let _ = conn
.execute(&*part.trim())
.await
.map_err(|e| MigrateError::ExecuteMigration(e, migration.version))?;
}

// language=SQL
let _ = query(
Expand Down
20 changes: 20 additions & 0 deletions tests/postgres/migrate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,26 @@ async fn no_tx(mut conn: PoolConnection<Postgres>) -> anyhow::Result<()> {
Ok(())
}


#[sqlx::test(migrations = false)]
async fn split(mut conn: PoolConnection<Postgres>) -> anyhow::Result<()> {
clean_up(&mut conn).await?;
let migrator = Migrator::new(Path::new("tests/postgres/migrations_split")).await?;

// run migration
migrator.run(&mut conn).await?;

// check outcome
let res: i32 = conn
.fetch_one("SELECT * FROM test_table")
.await?
.get(0);

assert_eq!(res, 1);

Ok(())
}

/// Ensure that we have a clean initial state.
async fn clean_up(conn: &mut PgConnection) -> anyhow::Result<()> {
conn.execute("DROP DATABASE IF EXISTS test_db").await.ok();
Expand Down
8 changes: 8 additions & 0 deletions tests/postgres/migrations_split/0_create_index.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
-- no-transaction

CREATE TABLE test_table (x int);
-- split-migration
CREATE INDEX CONCURRENTLY test_table_x_idx ON test_table (x);
-- split-migration
INSERT INTO test_table (x) VALUES (1);
-- prove that you can have a comment that won't split -- split-migration DROP TABLE does_not_exist;
Copy link
Author

Choose a reason for hiding this comment

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

I confirmed that if I remove the \ns from the string split this gets split such that DROP TABLE does_not_exist; becomes runnable SQL that then fails to run.

Loading