tx rollback doesn't unreserve the migrations table

[leblowl]

Testing locally with an H2 db, I found that if one of my migrations failed, then migratus would not unreserve the schema migrations table. This led to a weird state for me, because when I tried to re-run the migration, it told me something like "table reserved", but when I tried to rollback it pretty much left me without any constructive message - it looked like it was rolling back, but it really didn't do anything. I think the messaging can be brought up in a separate issue, but I just wanted to mention it as it relates to finding this bug. So I think the issue is with tx vs no-tx. When the migration has tx enabled, the entire migration code is wrapped in a transaction, including the mark-unreserved call:

migratus/src/migratus/database.clj

Lines 230 to 234 in 19c0fa9

	(migrate-up [this migration]
	(if (proto/tx? migration :up)
	(sql/with-db-transaction [t-con @connection]
	(migrate-up* t-con config migration))
	(migrate-up* (:db config) config migration)))

migratus/src/migratus/database.clj

Lines 54 to 72 in 19c0fa9

	(defn migrate-up* [db config {:keys [name] :as migration}]
	(let [id (proto/id migration)
	table-name (migration-table-name config)]
	(if (mark-reserved db table-name)
	(try
	(when-not (complete? db table-name id)
	(proto/up migration (assoc config :conn db))
	(mark-complete db table-name name id)
	:success)
	(catch Throwable up-e
	(log/error (format "Migration %s failed because %s backing out" name (.getMessage up-e)))
	(try
	(proto/down migration (assoc config :conn db))
	(catch Throwable down-e
	(log/debug down-e (format "As expected, one of the statements failed in %s while backing out the migration" name))))
	(throw up-e))
	(finally
	(mark-unreserved db table-name)))
	:ignore)))

I added some tests that should hilight this behavior. Thought, I'm not really sure how to best structure tests like this that share the same structure but slightly different data...maybe you have a suggestion.

[leblowl]

Also not sure how best to share code like this, open to suggestions. Thanks

[yogthos]

I think the issue here is that h2 has quirky support for transactions. So, not sure how much can be done to address that within migratus.

[leblowl]

ok, makes sense. H2 is the only DB I have tested. I can double check it with another store like PostgreSQL

[nakiya]

Same happens on MySql too

[eoinmcq]

[Edit: having thought about this I am not convinced it is the same problem as the reserved id would never have successfully been written due to an error in the transaction and it being rolled back - I will leave the comment here in case people have read it. Thanks!)

Same happens on Postgres.

The unreserve is being done within the same transaction block which failed so will always fail with a transaction aborted. (In the case of migrate-up* all being done in one transaction block obviously.)

The manifestation of the problem I have/saw was the the SQL error code was coming back as 25P02 (transaction aborted) because the exception in the finally block was superseding the exception in the catch block so no matter what the original error was it would always get overwritten.

In my understanding of this (I am very new to Clojure...) the unreserve can never work in failed cases because the transaction block it is trying to run in will always be in a failed state before the unreserve is ran.