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 #1962

Merged
merged 13 commits into from Mar 27, 2017

Conversation

Projects
None yet
3 participants
@itaysabato
Contributor

itaysabato commented Mar 12, 2017

A suggested fix for #1950.

If no transactions are disabled (neither in the general migration configuration nor in any individual migration file) then the whole migration to the latest version (including the acquisition of the lock and its release) is done in the same transaction which either completely succeeds or completely fails.

Among other benefits, this makes migrating to latest in parallel possible (e.g. in the bootstrap of multiple servers) since another process will automatically wait while another is migrating instead of failing with a lock error and crashing.

I had to fix some existing tests to adjust to this new behavior and I've also added a couple of new ones which test parallel migrations.

Please review :)
Thanks.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Mar 15, 2017

Note to myself for the release. This is breaking change.... and thanks @itaysabato I'll try to review this soon

@elhigu

elhigu approved these changes Mar 22, 2017

Some indentations were a bit strange leading to:

.then(() => {
})
  .then(() => {
  })
    .then(() => {
    })

drifting of indentation level even that promise chain is not nesting... Code looks really clean and I couldn't find anything to complain 👍

Looks like this is already fine for merging. Thanks a lot for the effort you've been putting to this!

I added couple of questions to comments in parts that I didn't understand why it was done like that...

t.increments();
t.string('name');
}),
knex.schema

This comment has been minimized.

@elhigu

elhigu Mar 22, 2017

Collaborator

strange indentation here :)

This comment has been minimized.

@itaysabato

itaysabato Mar 23, 2017

Contributor

This whole file was actually copied as-is with the same indentation.

});
if (knex.client.dialect === 'postgres') {
it("is able to run two migrations in parallel in postgres", function () {

This comment has been minimized.

@elhigu

elhigu Mar 22, 2017

Collaborator

Why is this postgres only test?

This comment has been minimized.

@itaysabato

itaysabato Mar 23, 2017

Contributor

The default transaction modes in the other databases either do auto-commit (e.g. MySQL) or don't support for share locking (e.g. SQLite). Therefore (by default) concurrent migrations are expected to fail in the same way as before for the non-Postgres databases.

This comment has been minimized.

@elhigu

elhigu Mar 23, 2017

Collaborator

Umm... I don't understand why auto-commit would be a problem here? If each migration is runnings are done in their own transactions, auto-commit is automatically disabled in mysql? I'm a bit worried about the fact that this functionality ended up working only for postgresql.

This comment has been minimized.

@itaysabato

itaysabato Mar 23, 2017

Contributor

From what I learned (and tested) auto-commit is enabled by default in MySQL. If the knex user will change this in his database then I expect it will behave the same as Postgres.

Otherwise, it means that one transaction will see that the other transaction has locked and it will cause it to fail (instead of waiting for it to finish). This is consistent with the current behavior of knex migrations.

This comment has been minimized.

@itaysabato

itaysabato Mar 23, 2017

Contributor

Reference: https://dev.mysql.com/doc/refman/5.7/en/innodb-locking-reads.html

Locking of rows for update using SELECT FOR UPDATE only applies when autocommit is disabled (either by beginning transaction with START TRANSACTION or by setting autocommit to 0. If autocommit is enabled, the rows matching the specification are not locked.

This comment has been minimized.

@elhigu

elhigu Mar 27, 2017

Collaborator

Fair enough. Looks like only postgres and mssql are not doing implicit commits and oracle + mysql are https://massimotinelli.wordpress.com/2016/06/23/implicit-commit-ddl-oracle-vs-ms-sql-server/. Would be good to document that difference of functionality of different DB engines it is not obvious for many of the users.

This comment has been minimized.

@elhigu

elhigu Mar 27, 2017

Collaborator

We are not running integration tests yet for mssql on CI but eventually we will. So for this pull request would be enough to add also mssql to be tested in addition to postgres.

This comment has been minimized.

@itaysabato

itaysabato Mar 27, 2017

Contributor

You mean I should include mssql in the if condition, though it would have no effect on the current CI test, yes?

This comment has been minimized.

@elhigu

elhigu Mar 27, 2017

Collaborator

yes please.

This comment has been minimized.

@itaysabato

itaysabato Mar 27, 2017

Contributor

Done. Merge? :)

return knex.migrate.status({directory: 'test/integration/migrate/test'}).then(function(migrationLevel) {
expect(migrationLevel).to.equal(3);
})
.then(function() {

This comment has been minimized.

@elhigu

elhigu Mar 22, 2017

Collaborator

here indentation is also drifting

This comment has been minimized.

@itaysabato

itaysabato Mar 23, 2017

Contributor

Right, WebStorm does that sometimes, should I fix it?

knex.migrate.latest({directory: 'test/integration/migrate/test'}),
knex.migrate.latest({directory: 'test/integration/migrate/test'})
])
.then(function () {

This comment has been minimized.

@elhigu

elhigu Mar 22, 2017

Collaborator

indentation drifting...

This comment has been minimized.

@itaysabato

itaysabato Mar 23, 2017

Contributor

WebStorm again, fixable.

// bad migrations that don't get loaded. This
// will simulate the DB and source being in sync.
return Promise.all([
knex('knex_migrations').returning('id').insert({

This comment has been minimized.

@elhigu

elhigu Mar 22, 2017

Collaborator

Lots of code was removed here, is test still checking everything fine (sorry I'm a bit tired and don't understand why code was ok to be removed... need to read this again tomorrow)?

This comment has been minimized.

@itaysabato

itaysabato Mar 23, 2017

Contributor

This code was removed since it added mock data that is no longer required - this migration directory no longer has invalid migrations, they were moved to test_with_invalid.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Mar 27, 2017

@itaysabato awesome and thanks !

@elhigu elhigu merged commit a5f4c39 into tgriesser:master Mar 27, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@elhigu

This comment has been minimized.

Collaborator

elhigu commented Mar 27, 2017

I think I will release 0.13 pretty soon.

@AlSherif

This comment has been minimized.

AlSherif commented Jul 26, 2017

@elhigu @itaysabato I'm running migrations on a replicated postgres DB system and the migration will fail quite often (on the first migration only) between the creation of the knex lock table and the select on that table, due to both requests going to different DB nodes.

latest(config) {
    this.config = this.setConfig(config);
    return this._migrationData()
      .tap(validateMigrationList)
      .spread((all, completed) => {
        const migrations = difference(all, completed);

        const transactionForAll = !this.config.disableTransactions
          && isEmpty(filter(migrations, name => {
            const migration = require(path.join(this._absoluteConfigDir(), name));
            return !this._useTransaction(migration);
          }));

        if (transactionForAll) {
          return this.knex.transaction(trx => this._runBatch(migrations, 'up', trx));
        }
        else {
          return this._runBatch(migrations, 'up');
        }
      })
  }

The check, creation and repeated check for the knex tables are all happening in '_migrationData()' more specifically in '_listCompleted(trx = this.knex)'. This does in our case break a migration that runs over multiple databases (synchronously).
I hacked a fix doing this:

_listCompleted(trx = this.knex) {
    const { tableName } = this.config
    return trx.transaction((rlTrx) => {
      return this._ensureTable(rlTrx)
        .then(() => rlTrx.from(tableName).orderBy('id').select('name'))
        .then((migrations) => map(migrations, 'name'))
    })
  }

Maybe one of you guys has a better solution?

@itaysabato

This comment has been minimized.

Contributor

itaysabato commented Jul 26, 2017

@AlSherif An example of how your migration files and config look like might help to understand what is wrong. What exception/error message are you getting?

Did you try disabling transactions?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jul 26, 2017

If error happens before migration_lock table even exist so that 2 different instances are trying to create it at the same time, failing makes sense. I would suggest that you initialize your DB so that migrations / lock table are there from the beginning, before even starting the app that runs migrations. If that is somehow possible in your architecture.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jul 26, 2017

Another way around the problem might be to create advisory lock before running migrations so that only one transaction tries to create the initial tables https://www.linkedin.com/pulse/postgresql-locking-revealed-petar-partlov

With advisory locks you should be able to create lock that is not related to any table, so it doesn't matter that there is no tables / rows for creating that kind of lock.

@AlSherif

This comment has been minimized.

AlSherif commented Jul 26, 2017

The error that I'm getting is a failed select on the 'knex_migration_lock' table, 'relationship does not exist'.
As I said, I am pretty sure that this is caused due to the 'create knex_migration_lock' and 'select knex_migration_lock' calls going to different nodes in the DB replication pool. Those calls are not in a transaction. I' am not using any asyncronous calls, I just iterate over a list of databases and execute the migration.
This is not a critical problem for us since the migration only has to run once, afterwards the knex tables exist.
Are there any possible issues with my fix? If not, I would tend to go with that for the sake of speed and simplicity. I'll check out the advisory locks though.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jul 26, 2017

@AlSherif ah ok. What kind of replication setup are you using? If its postgres native streaming replication is it in async or sync mode? If problem is the one you mentioned advisory locks wont help, since select still can go to node which is out of sync...

Do you have multiple application instances running migrations at the same time (I just assumed you were since this change fixed that behaviour to block instead of failing)?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jul 26, 2017

If you are running just on application process trying to run migrations at the same time, then you might be able to make all the queries to go to the same connection by creating Migration instance and passing trx object to it as a constructor argument. Trx object is quite compatible with base knex instance, but it won't use connection pool, but sends all the queries to the same connection.

https://github.com/tgriesser/knex/blob/master/src/migrate/index.js

If you have multiple processes running migrations at the same time and your replication setup is not synchronous I have no idea how to make it work :)

@AlSherif

This comment has been minimized.

AlSherif commented Jul 26, 2017

@elhigu Yes its only one application process, I just posted it here because I assumed that this fix will run all migration related calls in a transaction, which it doesn't.
I actually use a knex instance and call the migration on it, so it should be possible to use a transaction instance instead. Thanks, that should work then :)

@itaysabato

This comment has been minimized.

Contributor

itaysabato commented Jul 26, 2017

@elhigu Perhaps I am mistaken, but I think the migrator will have to be rewritten for that to work because it often uses syntax that is not supported on a transaction instance. Specifically, the case of calling .transaction on the transaction instance should probably be avoided.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jul 26, 2017

@itaysabato creating nested transactions (transaction inside transactions) is implemented with save points and is supported by knex. But yeah, I don't remember from top of my head, which parts of knex API cannot be used through trx object. There might be some.

@AlSherif

This comment has been minimized.

AlSherif commented Jul 26, 2017

@itaysabato @elhigu Seems to run well so far, calling migrations on a transaction object.

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