jackc / tern

The SQL Fan's Migrator
MIT License
925 stars 68 forks source link

Allow migrations inside of a transaction #24

Closed tynor closed 3 years ago

tynor commented 3 years ago

The way we run tests involves creating a transaction, running migrations, using save points during the test, then rolling back to leave the database empty again. This works really nicely except that if the version table does not exist yet, migrate.NewMigratorEx fails because the call to Migrator.GetCurrentVersion with a missing table aborts the transaction.

We currently work around this by duplicating Migrator.ensureSchemaVersionTableExists with this patch applied and calling that before migrate.NewMigratorEx.

This PR is in case you're interested in supporting this use case in tern itself. It replaces the method of checking for a missing version table with a query against the pg_catalog.pg_tables view. This leaves open transactions active, even if the version table does not exist.

jackc commented 3 years ago

The use case seems reasonable. I have few possible changes though.

splitSchemaTable makes the assumption that if the the version table is not schema qualified then it should exist in the public schema. While usually true, this is not always the case. I think it would be better to use pg_table_is_visible instead of assuming a particular schema.

But there is another approach as well. ensureSchemaVersionTableExists could check if it is already in a transaction. If so create a save point before the GetCurrentVersion call and release it after. Only trick there would be you would need to get the current tx status with m.conn.PgConn().TxStatus() and manually execute the SQL for creating and releasing the save point.

tynor commented 3 years ago

I agree assuming public is not ideal. It looks like it wouldn't be hard to integrate pg_table_is_visible into the query: select count(*) from pg_catalog.pg_class where relname=$1 and relkind='r' and pg_table_is_visible(oid).

I'm open to arguments to the contrary, but I think my bias would be toward keeping the single query rather than branching on transaction status.

In the mean time I can test with the modified query.

jackc commented 3 years ago

Thanks. LGTM.