Support running multiple statements per migrations file #3693

New issue

Open

#3694

Open

Support running multiple statements per migrations file#3693

#3694

Labels

enhancement

adriangb

I have found these related issues/pull requests

#3181 introduced a --no-transaction directive specifically so that things like CREATE INDEX CONCURRENTLY or CREATE DATABASE could be run on Postgres.

Description

But if you include more than one statement in a query Postgres starts an implicit transaction, which means that you can only run a single statement per migration file. This quickly becomes frustrating.

Prefered solution

I propose the addition of a -- sqlx: split migration directive that allows doing something like this:

-- no-transaction
CREATE INDEX CONCURRENTLY ...
-- sqlx: split migration
CREATE INDEX CONCURRENTLY ...

Then sqlx literally calls split() on the migration string and runs multiple queries instead of a single one.

Is this a breaking change? Why or why not?

I don't think it has to be.

added

linked a pull request that will close this issue

on Jan 19, 2025

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

titaniumtraveler

Contributor

But if you include more than one statement in a query Postgres starts an implicit transaction

Here is a link to the docs this is talking about: https://www.postgresql.org/docs/current/protocol-flow.html#PROTOCOL-FLOW-MULTI-STATEMENT

I have been reading up on the internals chapter of the manual lately and had been a bit confused, because I remembered a sentence that suggested differently (and I accidentally had skipped over the section just below that, that explains the thing with the transactions).

I propose the addition of a -- sqlx: split migration directive that allows doing something like this:

I would prefer using sqlx:split-migration instead to avoid whitespace, which might lead to more user errors and the migration not actually being split.
(This is me speaking as someone who literally only contributed one patch to the project to add completions to sqlx-cli via clap_complete)

On that topic, it might be a good idea to rename no-transaction to sqlx:no-transaction?
(And deprecate, but not remove the old one of cause)

If it is expected, or at least possible that more directives are added in the future it would be nice for them to have a consistent format and be clearly attributable to sqlx.

Also it would make it easier to add snippets for them into code editors ...

adriangb

Author

I'm fine with any name 😄

adriangb

Author

Another question is: do we want to support sqlx:no-transaction for each sqlx:split-migration? Technically we could, but that would require more extensive refactors that might be breaking changes.

titaniumtraveler

Contributor

Another question is: do we want to support sqlx:no-transaction for each sqlx:split-migration? Technically we could, but that would require more extensive refactors that might be breaking changes.

I don't think it would be very hard. The way your PR works just isn't good for
that. As far as I could see, your PR currently processes at execution time,
which in my opinion is the wrong way to processes the migration.

Wouldn't it be better to split the migrations as they are read from the
filesystem? (That would also avoid some runtime work when people are executing
the migrator returned by migrate!())

Implementing it on the level of resolve_blocking() (where the migrations are read from the directory) would also have to advantage that all the directives that influence how a migration is executed are implemented in one place!

sqlx/sqlx-core/src/migrate/source.rs

Lines 57 to 145 in f6d2fa3

    
           pub fn resolve_blocking(path: &Path) -> Result<Vec<(Migration, PathBuf)>, ResolveError> { 
        
               let s = fs::read_dir(path).map_err(|e| ResolveError { 
        
                   message: format!("error reading migration directory {}: {e}", path.display()), 
        
                   source: Some(e), 
        
               })?; 
        
               let mut migrations = Vec::new(); 
        
               for res in s { 
        
                   let entry = res.map_err(|e| ResolveError { 
        
                       message: format!( 
        
                           "error reading contents of migration directory {}: {e}", 
        
                           path.display() 
        
                       ), 
        
                       source: Some(e), 
        
                   })?; 
        
                   let entry_path = entry.path(); 
        
                   let metadata = fs::metadata(&entry_path).map_err(|e| ResolveError { 
        
                       message: format!( 
        
                           "error getting metadata of migration path {}", 
        
                           entry_path.display() 
        
                       ), 
        
                       source: Some(e), 
        
                   })?; 
        
                   if !metadata.is_file() { 
        
                       // not a file; ignore 
        
                       continue; 
        
                   } 
        
                   let file_name = entry.file_name(); 
        
                   // This is arguably the wrong choice, 
        
                   // but it really only matters for parsing the version and description. 
        
                   // 
        
                   // Using `.to_str()` and returning an error if the filename is not UTF-8 
        
                   // would be a breaking change. 
        
                   let file_name = file_name.to_string_lossy(); 
        
                   let parts = file_name.splitn(2, '_').collect::<Vec<_>>(); 
        
                   if parts.len() != 2 || !parts[1].ends_with(".sql") { 
        
                       // not of the format: <VERSION>_<DESCRIPTION>.<REVERSIBLE_DIRECTION>.sql; ignore 
        
                       continue; 
        
                   } 
        
                   let version: i64 = parts[0].parse() 
        
                       .map_err(|_e| ResolveError { 
        
                           message: format!("error parsing migration filename {file_name:?}; expected integer version prefix (e.g. `01_foo.sql`)"), 
        
                           source: None, 
        
                       })?; 
        
                   let migration_type = MigrationType::from_filename(parts[1]); 
        
                   // remove the `.sql` and replace `_` with ` ` 
        
                   let description = parts[1] 
        
                       .trim_end_matches(migration_type.suffix()) 
        
                       .replace('_', " ") 
        
                       .to_owned(); 
        
                   let sql = fs::read_to_string(&entry_path).map_err(|e| ResolveError { 
        
                       message: format!( 
        
                           "error reading contents of migration {}: {e}", 
        
                           entry_path.display() 
        
                       ), 
        
                       source: Some(e), 
        
                   })?; 
        
                   // opt-out of migration transaction 
        
                   let no_tx = sql.starts_with("-- no-transaction"); 
        
                   migrations.push(( 
        
                       Migration::new( 
        
                           version, 
        
                           Cow::Owned(description), 
        
                           migration_type, 
        
                           Cow::Owned(sql), 
        
                           no_tx, 
        
                       ), 
        
                       entry_path, 
        
                   )); 
        
               } 
        
               // Ensure that we are sorted by version in ascending order. 
        
               migrations.sort_by_key(|(m, _)| m.version); 
        
               Ok(migrations) 
        
           }

adriangb

Author

I thought of that but we'd have to add a suffix to the version or something

titaniumtraveler

Contributor

I thought of that but we'd have to add a suffix to the version or something

Yeah you are right. And adding it would sadly break SemVer, because Migration is not marked as #[non_exhaustive].

I guess in this case it might genuinely better to wait for 0.9.0.
As I understand it, there is a bunch of work happening in #3383 for that, which also refactors this section of the code anyway.

to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

No one assigned

Labels

enhancement

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

feat(postgres): allow running multiple statements in no tx migrationslaunchbadge/sqlx

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support running multiple statements per migrations file #3693