Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

migration transaction #148

Closed
tkellen opened this issue Jan 8, 2014 · 20 comments
Closed

migration transaction #148

tkellen opened this issue Jan 8, 2014 · 20 comments

Comments

@tkellen
Copy link
Contributor

tkellen commented Jan 8, 2014

Is there presently a way to run a migration in a transaction so if it fails it doesn't partially complete?

@tgriesser
Copy link
Member

I'm actually not sure, though there probably should be... I was under the impression that ddl stuff couldn't be in transactions but after a bit of googling it looks like in postgres it can be and in mysql it can be with some caveats. I'll dig into it a little bit.

@tkellen
Copy link
Contributor Author

tkellen commented Jan 8, 2014

Awesome. Also, here is one of my migrations... is there a more idiomatic way to do this?

var sequence = require('when/sequence');

exports.up = function (knex) {
  // stupid helper method required to run these in sequence
  var create = function(table, fn) {
    return function() {
      return knex.schema.createTable(table, fn);
    }
  };
  return sequence([
    create('account', function (t) {
...
    }),
    create('access_token', function (t) {
...
    }),
    create('oauth_client', function (t) {
...
    }),
    create('password_reset_token', function (t) {
...
    }),
    create('event', function (t) {
...
    }),
    create('test', function (t) {
...
    }),
    create('test_snapshot', function (t) {
....
    }),
    create('test_snapshot_file', function (t) {
...
    }),
  ]);
};

exports.down = function (knex) {
  // stupid helper method required to run these in sequence
  var drop = function(table) {
    return function() {
      return knex.schema.dropTableIfExists(table);
    }
  };
  return sequence([
    drop('test_snapshot_file'),
    drop('test_snapshot'),
    drop('test_snapshot_set'),
    drop('event'),
    drop('password_reset_token'),
    drop('oauth_client'),
    drop('access_token'),
    drop('user')
  ])
};

@tkellen
Copy link
Contributor Author

tkellen commented Jan 8, 2014

PS: I just started working at @bocoup this Monday and I'm ripping sails out of an internal project to replace it with vanilla express and knex/bookshelf. It has been an absolute dream. Thank you SO MUCH for this library. Seriously, I told like 6 people here yesterday that using knex/bookshelf was the first time I didn't hate interacting with a database in node.

@bendrucker
Copy link
Member

The best way to do migrations is to assume the file timestamps determine sequence and not try to build it into the files themselves. Promise.all is what you should be using here and things will run in sequence because, if I'm remembering right, create and alter table operations block each other (and block reads too on MySQL).

That means you're going to need to create tables with primary keys and columns in one migration and then add foreign key columns in a latter migration. Here's an abbreviate version of a pair of mine (written in CS). Obviously this cuts out quite a bit, but hopefully the general gist is clear.

exports.up = (knex, Promise) ->
    Promise.all [
        knex.schema.createTable 'donations', (t) ->
            t.increments('id').primary()
            t.timestamps()
            # Data
            t.decimal 'amount', 12, 2
    ]
exports.up = (knex, Promise) ->
    Promise.all [
        knex.schema.table 'donations', (t) ->
            t.integer('donor_id')
                .unsigned()
                .references('id')
                .inTable('donors')
            t.integer('organization_id')
                .unsigned()
                .references('id')
                .inTable('organizations')
            t.integer('campaign_id')
                .unsigned()
                .references('id')
                .inTable('campaigns')
            t.integer('payment_id')
                .unsigned()
    ]

@bendrucker bendrucker mentioned this issue Jan 8, 2014
@tkellen
Copy link
Contributor Author

tkellen commented Jan 8, 2014

Going to give that a shot now. I'm using PostgreSQL, fyi.

@bendrucker
Copy link
Member

Good man :)

Just so you know, the calls to the up/down functions pass knex and promise (i.e. bluebird). So you can just go ahead and use the promise lib just by including it in the args.

@tkellen
Copy link
Contributor Author

tkellen commented Jan 8, 2014

Handy to know about the Promise lib getting passed in, that simplifies things a bit! I'm going to stick with my ugly wrapper function--I don't like inflating new tables in multiple steps like you've suggested @bendrucker.

@tkellen
Copy link
Contributor Author

tkellen commented Jan 8, 2014

Thanks guys!

@tgriesser
Copy link
Member

@tkellen congrats on @bocoup, glad to hear that you're liking knex/bookshelf - definitely keep the tickets coming as you hit issues, or drop by #bookshelf irc!

Keeping this one open to remind me about looking into schema stuff within transactions.

@ryankask
Copy link

ryankask commented Mar 1, 2014

Has there been any more discussion on how this could be implemented?

@tgriesser
Copy link
Member

@ryankask Yeah, it should be implemented in the 0.6.0-alpha branch actually, I just still have some things I need to touch up with it, and then get Bookshelf working with it, and then it'll be good to go.

Feel free to give it a shot and see if there's anything I missed.

@wbyoung
Copy link
Contributor

wbyoung commented May 10, 2014

Glad to hear this is coming soon!

@tgriesser
Copy link
Member

Released.

@StephanHoyer
Copy link

Awesome to hear.

How can we make use of this? I can't find any hints in the documentation.

@tgriesser
Copy link
Member

Just wrap the transaction as you normally would and use the transacting object as the query (schema) builder:

return knex.transaction(function(trx) {
   return trx.schema.createTable('...')
      .createTable('...')
      .table('...')
})

@rodhoward
Copy link

I'm still having trouble with partial migrations. my code looks like this:

run-file.js:
const readFile = util.promisify(fs.readFile);

module.exports = async (knex, fileName) => {
  const filePath = path.join(__dirname, '../migrations/sqls', fileName);
  const data = await readFile(filePath, { encoding: 'utf-8' });
  return knex.raw(data);
};
migration:
exports.up = async knex => {
  await knex.transaction(async tran => {
    await runFile(tran, '20190125090040-add-tags-to-roles-up/tags.sql');
    await runFile(tran, '20190125090040-add-tags-to-roles-up/roles_tags.sql');
  });
};

The tags sql is applied and the stays in the database even after the roles_tags sql fails.

I'm guessing there is either an issue with the way I'm using transactions or transaction.raw() isn't actually executed in a transaction. I'm connecting to mysql:
'innodb_version', '8.0.13'
'protocol_version', '10'
'tls_version', 'TLSv1,TLSv1.1,TLSv1.2'
'version', '8.0.13'
'version_comment', 'MySQL Community Server - GPL'
'version_compile_machine', 'x86_64'
'version_compile_os', 'Linux'

Because this is a migration should this already be in a transaction anyway? How does that work?
my knex version is "^0.16.3"

Cheers
Rod

@elhigu
Copy link
Member

elhigu commented Jan 25, 2019

@rodhoward you cannot run DDL queries in transaction and then try to roll then back on error with mysql. https://stackoverflow.com/questions/54179669/transaction-issue-with-knexjs-typescript-and-mariadb/54180024#54180024

@zardilior
Copy link

@tgriesser you solution has problems as alterTable and createTable cant be rolled back in mySql as according to @rodhoward

@mxkxf
Copy link

mxkxf commented May 7, 2020

I'm using knex 0.21.1 and running the following migration:

exports.up = async knex => {
  await knex.schema.alterTable('test', table => {
    table.string('some_column');
  });

  // Statement which deliberately fails
  await knex.schema.alterTable('table_that_does_not_exist', table => {
    table.increments('id').primary();
  });
};

exports.down = knex => {
  // ...
};

And if migrations were transacted by default, I wouldn't expect test.some_column to exist as the statement straight afterwards for table_that_does_not_exist fails - but test.some_column is there.

Am I missing some config or something?

@elhigu
Copy link
Member

elhigu commented May 8, 2020

@mikefrancis if you are using mysql / mariadb transactions really does not work with schema changing queries. See my earlier comment #148 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants