Exception during migration creates some tables about migratus

[yogthos]

You have to explicitly separate multiple statements using the --;; notation as described here.

[theronic]

I am using the separator as specified in the docs, e.g.:

create table users (
  id serial primary key not null,
  created_on timestamp not null default now(),
  full_name text not null,
  email text not null,
  is_active boolean not null default true
)
--;;
create table identities (
  id serial primary key not null,
  created_on timestamp not null default now(),
  user_id int references users(id) not null,
  provider text not null,
  username text not null,
  password_hash text null,
  password_salt text null,
  is_active boolean not null default true
)
--;;
create table sessions (
  id serial primary key not null,
  user_id int references users(id) not null,
  created_on timestamp not null default now(),
  expires_on timestamp not null,
  is_active boolean not null default true
)

[yogthos]

What version of the library are you using. There was a fix to rethrow the exception in this commit 29c4753 and running up/down should be transaction. So, if you're on a version before 0.8.15 you'd see this issue.

[theronic]

I'm on 0.8.16.

[yogthos]

Hmm another question is what db you're running against.

[adambaker]

I'm seeing this problem too, using migratus 0.8.25 with postgresql version 9.4.8, jdbc driver version 9.4-1206-jdbc4. Here's a pretty minimal script that fails:

create table users (
  id serial primary key
);
--;;
create table adults (
  id integer references users primary key,
  email text not null unique
);
--;;
create table students (
create table students (
  id integer references users primary key,
  email text unique,
  grade smallint not null
);

Running this generates the expected sql syntax error, but rather than rollback a transaction it leaves the adults and users tables in the database.

[theronic]

@yogthos, I'm running Postgres 9.5.0.0.

[theronic]

From skimming the code, it seems that the only code that runs in a transaction, is each individual up/down statement:

migratus/src/migratus/database.clj

Line 68 in 7aedc8b

(sql/with-db-transaction

You probably want to wrap the entire sequence of migrations in a transaction, like here:

migratus/src/migratus/core.clj

Line 53 in 7aedc8b

(doseq [migration migrations]

[yogthos]

The core is data store agnostic, so it wouldn't be the place to wrap JDBC specific transactions. However, each transaction is meant to be independent in any case. It might make sense to stop and as soon as an up statement returns false though, since at that point it wouldn't make sense to apply the rest of transactions.

[yogthos]

This would be my proposed solution here. The migrations will stop being applied as soon as any migration fails.

[theronic]

Ah yes, multiple statements in a migration should run in a transaction - not all migrations (as I badly suggested earlier). And if any statement fails, the transaction for that migration should be rolled back.

[yogthos]

So, that should be the current behavior. I do think it makes sense to stop further when migration does fail. That's the purpose of the new update.

[samroberton]

We've also seen this behaviour: a migration script where the second statement fails (causing an exception which propagates back out of migratus to our calling code) somehow commits the effect of the first statement to the database (but with no entry in schema_migrations for the partially-applied migration).

We're running migratus 0.8.20, on Postgres 9.5, with version 9.4.1208 of the Postgres JDBC driver, and clojure.java.jdbc 0.5.8.

[samroberton]

Found the error: it was incorrect use of sql/with-db-transaction (opening the transaction, but then proceeding to use the db-spec itself as the DB connection parameter, instead of the transaction), which was fixed (for up migrations at least) by this commit: f24349a. Ie for up migrations, at least, this is fixed in 0.8.28.

Down migrations still incorrectly use db instead of t-con as the DB connection parameter. PR incoming.

[yogthos]

Thanks for the pr, just pushed out a new version with the fix.