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

Prevent multiple connections from running migrations at the same time #1094

Merged
merged 7 commits into from Dec 15, 2015

Conversation

Projects
None yet
3 participants
@elhigu
Copy link
Collaborator

elhigu commented Dec 13, 2015

  • Adds locking when migrations are started
  • forceFreeMigrationsLock function to allow removing the lock manually

@elhigu elhigu referenced this pull request Dec 13, 2015

Closed

Add migration locking #1067

@elhigu elhigu force-pushed the elhigu:migration-lock branch from 51b10f2 to 184e8ca Dec 13, 2015

@elhigu

This comment has been minimized.

Copy link
Collaborator Author

elhigu commented Dec 13, 2015

@rhys-vdw could you give this the final review. @dustinmartin made this almost ready, but didn't have time to give it a last touch in #1067.

@elhigu elhigu force-pushed the elhigu:migration-lock branch from 184e8ca to 7e274f5 Dec 13, 2015

@rhys-vdw

This comment has been minimized.

Copy link
Collaborator

rhys-vdw commented Dec 14, 2015

@elhigu Yep, will review this evening.

return this._isLocked(trx)
.then(isLocked => {
if (isLocked) {
throw new Error("Migration table is are already locked");

This comment has been minimized.

@rhys-vdw

rhys-vdw Dec 15, 2015

Collaborator

"is are" already locked.

This comment has been minimized.

@elhigu

elhigu Dec 15, 2015

Author Collaborator

will fix.


_getLock() {
return this.knex.transaction(trx => {
return this._isLocked(trx)

This comment has been minimized.

@rhys-vdw

rhys-vdw Dec 15, 2015

Collaborator

Am I correct in thinking you could still have a problem here unless specifying 'repeatable reads' as the isolation level?

This comment has been minimized.

@elhigu

elhigu Dec 15, 2015

Author Collaborator

What kind of problem? When the transaction of the first connection is has ran select ... for update; and then the transaction of the second connection tries to run the same select ... for update; the second connection will wait until the first transaction ends before returning result for that query.

This comment has been minimized.

@rhys-vdw

rhys-vdw Dec 15, 2015

Collaborator

I believe that is only the case if the isolation level is set to 'repeatable reads' or higher.~~

Wikipedia on 'read committed' isolation level or lower:

...read committed is an isolation level that guarantees that any data read is committed at the moment it is read. It simply restricts the reader from seeing any intermediate, uncommitted, 'dirty' read. It makes no promise whatsoever that if the transaction re-issues the read, it will find the same data; data is free to change after it is read.

So, if I understand correctly, at this isolation it is possible to have two transaction that issue a select before either transaction updates the lock table. This means they would each redundantly update the table and try to execute the migrations. Of course the chance of this happening would be greatly reduced since they would need to line up perfectly.

I'm not sure exactly how this would be configured since Knex doesn't have an isolation level interface - so perhaps this is a moot point.

This comment has been minimized.

@rhys-vdw

rhys-vdw Dec 15, 2015

Collaborator

@elhigu Oops, missed forUpdate. Don't mind me.

})
.catch((error) => {
return this._getLock()
.catch(err => {

This comment has been minimized.

@rhys-vdw

rhys-vdw Dec 15, 2015

Collaborator

This catch statement could be moved into _getLock (after transaction handler) - feels more sensible to fire the LockError from there. (or perhaps I'm just bike shedding)

This comment has been minimized.

@elhigu

elhigu Dec 15, 2015

Author Collaborator

Agreed :) I'll change this.

@rhys-vdw

This comment has been minimized.

Copy link
Collaborator

rhys-vdw commented Dec 15, 2015

@elhigu Looking good.

@elhigu elhigu force-pushed the elhigu:migration-lock branch from 7e274f5 to 3dd5ac8 Dec 15, 2015

elhigu added a commit that referenced this pull request Dec 15, 2015

Merge pull request #1094 from elhigu/migration-lock
Prevent multiple connections from running migrations at the same time

@elhigu elhigu merged commit d105924 into tgriesser:master Dec 15, 2015

1 check passed

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

This comment has been minimized.

Copy link
Contributor

dustinmartin commented Dec 16, 2015

@elhigu Thanks or finishing this up. Very much appreciated.

When do you think the next release of knex will be that will include these changes?

@rhys-vdw

This comment has been minimized.

Copy link
Collaborator

rhys-vdw commented Dec 16, 2015

@dustinmartin Should do one soon. @elhigu is there anything else you'd like to see in the next release?

@elhigu

This comment has been minimized.

Copy link
Collaborator Author

elhigu commented Dec 17, 2015

@rhys-vdw there is still that one pull request about sqlite3 dialect escaping which I would like to get in. It fixes also some regressions from postgres string literal escaping from getting to release.

@elhigu elhigu deleted the elhigu:migration-lock branch Jan 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.