Migrations should happen in a transaction

[rjmunro]

This is intentionally a reopening of sequelize/sequelize#1882 which was closed automatically by a bot. I think it's a fundamental feature for a migration engine.

What you are doing?

There was a problem applying a migration, due to a issue in the code of the migrations.

What do you expect to happen?

I expect the whole migration to be rolled back.

Nearly all database engines support DDL in transactions. It should be the default way updates are done, unless the user chooses not to.

Even if the database does not fully support transactions for DDL (MySQL) sometimes migrations can make updates to data e.g. adding records to lookup tables, and it should be the default to roll these back.

What is actually happening?

The database ends up in an inconsistent state, with half the changes of the migration applied, but not the other half. I can't re-run the migration, I can't easily go back. I have to manually finish the migration and then manually update the meta tables.

[rjmunro]

@Stale This is one of the top 5% of requested features by upvotes - I don't think it should be closed.

[iamjochem]

@rjmunro - I happened upon your issue whilst scanning for transaction-related issues (anything that might explain why I'm seeing sporadic transaction-related failure under load ... in my case it seems as if a given transaction option is being ignored) ...

Regarding MySQL, it does not support transactions for DDL queries at all (maybe the latest version does, or some flavour like MariaDB, but not the version I'm using at). This means automatic transaction support in the sequelize migration tool will not help you (in the case of MySQL).

Having run into pretty much the same problem as you I can up with a couple of pragmatic rules regarding development of migration files:

1. only 1 query (1 up & 1 down) per migration file
2. only use raw queries. (less abstraction, clearer intent)

you end up with potentially a lot of migration files but it's also bullet-proof in terms of rollback.

you could argue that you need to have multiple queries per migration file in order to keep deployments in sync with migrations (i.e. that every release has just one migration). I believe that that is almost impossible to police when there are multiple features being developed by multiple developers, especially when you throw CI & automation in the mix, not to mention that it is likely that most deployments do not have a corresponding migration.

Actually from a rollback POV the 'sequelize-meta' table should be storing batch-numbers along side the migrations that represent which migrations were run for any given migrate up ... additionally I think that it would be smart to store the source-code of the migrations files a. so that there is a record of what was run bundled in the actual DB instance and b. so that it is [technically] possible to perform rollbacks even when the latest migrations are not available (e.g. because you have checked-out/deployed some previous version of your codebase).

[rjmunro]

@iamjochem This is good advice for people stuck on MySQL or it's forks. For users of virtually any other databases, transactions are supported for DDL statements, and we should use them.

Transactions even reduce downtime because while all the DDL is going on in the transaction, the old data can still be read and updated.

Splitting migrations into one query each doesn't help you if you make a mistake and the query fails. A transaction will just put the database back to exactly how it was before you started.

[iamjochem]

@rjmunro - forcing migrations to contain only one query mimics transactions ... because the migration becomes all-or-nothing, failure results in a determinate state for the DB and the migrations (if a migration with multiple queries, that does not use a transaction, fails then you are stuck having to manually fix the DB because you can no longer perform an up or a down migration) ... with a failing single-query migration the DB will not be mutated so you have the ability to fix the query and try the up again. obviously this is only pertinant to MySQL & it's derivatives, any backend that supports transactions for DDL mutations should have them forcibly employed by the migration code on a per-migration basis.

[jacqt]

From a cursory look at the sequelize codebase and some following experiments, the following works for in Postgres.

exports.up = function (queryInterface) {
  return queryInterface.sequelize.transaction(function (t) { return Promise.all([
    queryInterface.createTable('audit_logs', {
      ...
    }, {transaction: t})

    queryInterface.createTable('audit_logs2', {
      ...
    }, {transaction: t})
  ]); });
};

I haven't taken a complete look at all the queryInterface methods, but most seem to be passing the options arguments straight to the dialect specific query method (again for Postgres only), so it seems like this would work.

Can any of the maintainers (@sushantdhiman ?) confirm whether this is behavior that we can rely on to remain consistent (and potentially documented)?

[sushantdhiman]

Any thing that touches sequelize.query which includes all query interface api and model public api can pass options.transaction, it will include that query to given transaction, I think its a well known feature, may be docs need clarification.

But its not a correct solution as operation on sequelize_meta wont be covered with this transaction

[jacqt]

Thank you for the quick response and for confirming! Makes sense how it is not a complete solution. I took a deeper look at how it might be possible to share this transaction with the migrator, and I think I have an inkling why this issue is still open :)

Off the top of my head, maybe one way to support this is to modify the cli here (https://github.com/sequelize/cli/blob/master/src/core/migrator.js#L49) to add a 3rd parameter with an unmanaged transaction that the (un)logMigration (https://github.com/sequelize/umzug/blob/master/src/storages/SequelizeStorage.js#L100) would use (and commit if the up method succeeds or rollback if the up method fails). Then it is up to to the developer to use the transaction argument as he wishes.

E.g.

exports.up = function (queryInterface, Sequelize, transaction) {
  return transaction.then((t) { return Promise.all([
    queryInterface.createTable('audit_logs', {
      ...
    }, {transaction: t})

    queryInterface.createTable('audit_logs2', {
      ...
    }, {transaction: t})
  ]); });
};

Some care will need to be done to make sure the transaction is eventually closed or rolled back.

If you think this is worth exploring I'd be happy to tinker with it if I have time or to contribute a bounty for this.

Lastly - I wanted to thank you for maintaining Sequelize! At minimum I would be happy to help write docs to clarify the behavior RE: the options parameter you mentioned in your comment.

[sushantdhiman]

Sounds good

[RoelRoel]

Sequalize migrations and seeders are one big deception. It should not even be released without transactions. How can seeders be useful without it?

[marlonkjoseph]

This caught me off guard.

[vitorsilvalima]

I think sequelize community really deserves to have this feature

[klarkc]

I had assumed that sequelize was correctly supporting transactions with mysql in successful migrations. Should not sequelize, at least, warn about transactions not being made?

[Restuta]

@sushantdhiman

I think its a well known feature

Idk about that, I only discovered it via this issue, reading https://sequelize.org/master/manual/migrations.html or https://sequelize.org/master/class/lib/query-interface.js~QueryInterface.html section didn't help, nobody on my team of 12 new about this as well. That this can be applied to queryInterface functions, specifically.

I also assumed, as well as my team, that migrations are by default run in transactions 🤷‍♂

[aat2703]

Is it correctly understood that MySQL dialect atm does not support transactions in migrations?

[papb]

@aat2703 Every dialect supports transactions in migrations. The only matter here is that they are not set by default.

[iamjochem]

@papb - MySQL does not allow you to run multiple DDL statements in a single transaction - each DDL statement implicitly commits/rollsback any active transaction - which is why I suggested the 1 query per migration strategy for MySQL users.

[papb]

@iamjochem Ouch, I didn't know that, and apparently that is true for most dialects (not only MySQL). Postgres seems to be the only exception...

But then it is no longer a Sequelize problem :)

[nicoabie]

@iamjochem @papb

In the context of a migration ran against MySQL I was renaming a column and then adding another.
(please note how because of the column names I need to run one first and then the other)

up: function (queryInterface, Sequelize) {
  return queryInterface.renameColumn('TABLE', 'COLUMNA', 'COLUMNB')
  .then(() => queryInterface.addColumn('TABLE', 'COLUMNA', Sequelize.STRING))
}

That failed throwing

ERROR: Deadlock found when trying to get lock; try restarting transaction

The migration was partially applied and I had to fix it manually as previously explained by @iamjochem

But if I wrap it up inside a transaction manually

up: function (queryInterface, Sequelize) {
  return queryInterface.sequelize.transaction( t => {
    return queryInterface.renameColumn('TABLE', 'COLUMNA', 'COLUMNB', {transaction: t})
    .then(() => queryInterface.addColumn('TABLE', 'COLUMNA', Sequelize.STRING, {transaction: t}))
  });
}

It works as expected

I'd like if you please can explain why it DOES work. I've read in this thread that MySQL does not support DDL transactions.
In that case the first attempt should have been able to run OK but it DIDN'T
And in the second case the transaction wouldn't have changed anything because MySQL doesn't support them but it DID work.

What I've learned from this experience is that any migration that has more than one call to the query interface should perform its work under a manually created transaction.

Maybe the docs should make that point clearer if the library cannot do it automatically behind the scenes

Looking forward to your opinions, thanks in advance and take care of the coronavirus.

[igordeoliveirasa]

Thank you for the quick response and for confirming! Makes sense how it is not a complete solution. I took a deeper look at how it might be possible to share this transaction with the migrator, and I think I have an inkling why this issue is still open :)

Off the top of my head, maybe one way to support this is to modify the cli here (https://github.com/sequelize/cli/blob/master/src/core/migrator.js#L49) to add a 3rd parameter with an unmanaged transaction that the (un)logMigration (https://github.com/sequelize/umzug/blob/master/src/storages/SequelizeStorage.js#L100) would use (and commit if the up method succeeds or rollback if the up method fails). Then it is up to to the developer to use the transaction argument as he wishes.

E.g.

exports.up = function (queryInterface, Sequelize, transaction) {
  return transaction.then((t) { return Promise.all([
    queryInterface.createTable('audit_logs', {
      ...
    }, {transaction: t})

    queryInterface.createTable('audit_logs2', {
      ...
    }, {transaction: t})
  ]); });
};

Some care will need to be done to make sure the transaction is eventually closed or rolled back.

If you think this is worth exploring I'd be happy to tinker with it if I have time or to contribute a bounty for this.

Lastly - I wanted to thank you for maintaining Sequelize! At minimum I would be happy to help write docs to clarify the behavior RE: the options parameter you mentioned in your comment.

I really would like to have a global transaction...

[sans-clue]

I'm confused, do transactions in migration files not work in these conditions?

Ubuntu 20.04
Mysql 8.0.27
Node: 14.17.0
CLI: 6.2.0
ORM: 6.9.0

I tried creating a similar migration as shown on the docs.

'use strict';

module.exports = {
  up: async (queryInterface, Sequelize) => {
    const transaction = await queryInterface.sequelize.transaction()
    try {
      await queryInterface.addColumn(
        'Users',
        'nick',
        { type: Sequelize.STRING },
        { transaction },
      )

      await queryInterface.addIndex(
        'Users',
        'nick',
        {
          fields: 'nick',
          unique: true,
          transaction,
        }
      );

      await transaction.commit()
    } catch (err) {
      await transaction.rollback()
      throw err
    }
  },

  down: async (queryInterface, Sequelize) => {
    const transaction = await queryInterface.sequelize.transaction()
    try {
      await queryInterface.removeColumn('Users', 'nick', { transaction })
      await transaction.commit()
    } catch (err) {
      await transaction.rollback()
      throw err
    }
  }
};

Running the migration throws an error (which is what I need for this test).

ERROR: Cannot create property 'fields' on string 'nick'

But what is relevant here is that nick still exists on the table and now the database is in an unstable state.
Running db:migrate:undo undoes the migration before it (in my case create-user-table) and running migrate again understandably throws a duplicate error.

Can we get some clarification on where this is supposed to be working?

[jvasseur]

That's because MySQL actually does support mixing modifying the database structure inside migrations (if I recall correctly it automatically commits the transaction before modifying the table).

The code in the example only works correctly with databases that support it like PostgreSQL (on databases that doesn't you end up with the transaction being ignored).

[sans-clue]

That's because MySQL actually does support mixing modifying the database structure inside migrations (if I recall correctly it automatically commits the transaction before modifying the table).

Can you tell me what "mixing modifying the database structure" means? Sorry English is not my native language and I think I'm just not understanding your comment right.

The code in the example only works correctly with databases that support it like PostgreSQL (on databases that doesn't you end up with the transaction being ignored).

Ah, thank you. My confusion stemmed from one of the earlier comments where they claimed it worked on MySQL.

[ccmetz]

That's because MySQL actually does support mixing modifying the database structure inside migrations (if I recall correctly it automatically commits the transaction before modifying the table).

The code in the example only works correctly with databases that support it like PostgreSQL (on databases that doesn't you end up with the transaction being ignored).

@jvasseur So is this only an issue when using transactions during migrations on MySQL databases? I'm currently using sequelize with a MySQL database and transactions work in regular queries from my API backend. Do transactions just not work within the context of doing a migration?

[jvasseur]

It's not the context of migrations, but ALTER query that poses problems with MySQL transactions, so yes most of the time your transactions won't work in the context of migrations but if you create a migration that only uses UPDATE queries, transactions would work perfectly with them.

[mschipperheyn]

I've recently noticed that migrations that partially fail actually commit the partially successful parts of the migration, leading to broken databases. I've been working with Sequelize for quite a while and I'm pretty sure this didn't use to be the case. What I would expect is that if I catch an error in promise chain of a migration and throw it, the entire migration transaction would be rolled back. I would not expect to have to manually specify a transaction. That is not the case (anymore). What is the best way to get this behavior (back)? And isn't this simply a bug?

[ephys]

Migrations have never run in a transaction as far as I know

[mltsy]

@mschipperheyn - are you by any chance using a database other than Postgres? It seems PostgreSQL is the only database server that we have confirmed has full support for running DDL statements (i.e. CREATE, DROP, ALTER) inside a transaction, and discarding or rolling them back if the transaction fails. If you're using another database, perhaps previous migrations you had run that failed partway through and were successfully rolled back were not using DDL statements? (maybe they were data manipulation migrations?)

[nicoabie]

IMHO It is not sequelize job to fix that.

Just keep DML and DDL migrations in separate files. We do that and works fine, you can even configure a linter or some tooling to enforce the rule