Support running multiple statements per migrations file

[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.

[titaniumtraveler]

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]

I'm fine with any name 😄

[adriangb]

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]

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]

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

[titaniumtraveler]

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.