Skip to content
This repository has been archived by the owner on Jun 28, 2018. It is now read-only.

Can't acquire lock on "manual" migrations #297

Open
josephbuchma opened this issue Oct 19, 2017 · 6 comments
Open

Can't acquire lock on "manual" migrations #297

josephbuchma opened this issue Oct 19, 2017 · 6 comments

Comments

@josephbuchma
Copy link

josephbuchma commented Oct 19, 2017

Using MySQL
This code (cut from test):

        // ...
        t.Log("mg.Version():")
        t.Log(mg.Version())
        err = mg.Migrate(1)
        if err != nil {
                t.Errorf("Migrate(1) err: %s", err)
        }
        t.Log("mg.Version():")
        t.Log(mg.Version())

        // up
        err = mg.Migrate(2)
        if err != nil {
                t.Errorf("Migrate(2): %s", err)
        }
        t.Log("mg.Version():")
        t.Log(mg.Version())

        err = mg.Down()
        if err != nil {
                t.Errorf("Down err: %s", err)
        }

Results in following output:

--- FAIL: TestMigrate (3.12s)
        migrate_test.go:58: mg.Version():
        migrate_test.go:59: 0 false no migration
        migrate_test.go:64: mg.Version():
        migrate_test.go:65: 1 false <nil>
        migrate_test.go:70: Migrate(2): can't acquire lock
        migrate_test.go:72: mg.Version():
        migrate_test.go:73: 1 false <nil>
        migrate_test.go:77: Down err: can't acquire lock

May be related to #220
And is not direclty related to #235 for sure (I do not run migrations concurrently, and in my case it is 100% reproduceable)

@mattes
Copy link
Owner

mattes commented Oct 20, 2017

Might be related to #274 .

How do you run the tests?

@josephbuchma
Copy link
Author

josephbuchma commented Oct 20, 2017

go test -v -run TestMigrate

Migrations are basic create / drop table without transactions.

Complete test code:

func TestMigrate(t *testing.T) {
        migrationsDir, cleanup := setupTestMigrations() // creates few up-down *.sql pairs
        defer cleanup()

        var err error

        db, err = sql.Open("mysql", "root:@tcp(localhost:3306)/migrations_test?multiStatements=true")
        if err != nil || db == nil {
                t.Fatalf("failed to open database connection: %s", err)
        }

        dbdrv, err := mysql.WithInstance(db, &mysql.Config{})
        if err != nil {
                t.Fatalf("mysq.WithInstance error: %s", err)
        }

        fsrc, err := (&file.File{}).Open("file://" + migrationsDir)
        if err != nil {
                t.Fatalf("Failed to open migrations source: %s", err)
        }

        mg, err := migrate.NewWithInstance(
                "file",
                fsrc,
                "mysql",
                dbdrv,
        )
        if err != nil {
                t.Fatalf("Failed to initialize Migrate: %s", err)
        }

        t.Log("mg.Version():")
        t.Log(mg.Version())
        err = mg.Migrate(1)
        if err != nil {
                t.Errorf("Migrate(1) err: %s", err)
        }
        t.Log("mg.Version():")
        t.Log(mg.Version())

        // up
        err = mg.Migrate(2)
        if err != nil {
                t.Errorf("Migrate(2): %s", err)
        }
        t.Log("mg.Version():")
        t.Log(mg.Version())

        err = mg.Down()
        if err != nil {
                t.Errorf("Down err: %s", err)
        }
}

rdallman pushed a commit to rdallman/migrate that referenced this issue Oct 21, 2017
I believe this closes mattes#297 as well.

I have been working on adding testing of migrations and it requires acquiring
the lock in mysql multiple times to go up and down. After nailing this down to
GET_LOCK returning a failure for every subsequent GET_LOCK call after the
first, I decided it was time to rtfm and lo and behold:

https://dev.mysql.com/doc/refman/5.7/en/miscellaneous-functions.html#function_release-lock

RELEASE_LOCK will not work if called from a different thread than GET_LOCK.
The simplest solution using the golang database/sql pkg appears to be to just
get a single conn to use for every operation. since migrations are happening
sequentially, I don't think this will be a performance hit (possible
improvement). now calling Lock() and Unlock() multiple times will work;
prior to this patch, every call to RELEASE_LOCK would fail. this required
minimal changes to use the *sql.Conn methods instead of the *sql.DB methods.

other changes:

* upped time out to 10s on GET_LOCK, 1s timeout can too easily leave us in a
state where we think we have the lock but it has timed out (during the
operation).
* fixes SetVersion which was not using the tx it was intending to, and fixed a
bug where the transaction could have been left open since Rollback or Commit
may never have been called.

I added a test but it does not seem to come up in the previous patch, at least
easily, I tried some shenanigans. At least, this behavior should be fixed with
this patch and hopefully the test / comment is advisory enough.

thank you for maintaining this library, it has been relatively easy to
integrate with and most stuff works (pg works great :)
@ernsheong
Copy link

ernsheong commented Oct 23, 2017

Any help with this? I am getting can't acquire lock error on Postgres when trying to run migrations (could be no new migrations pending) on a new app instance while an existing one is already present (this is for rolling restart).

Killing the current app instance first and then running migrations on the new instance works fine. migrate is being run as a lib.

Possible workarounds?

@Teddy-Schmitz
Copy link

See my issue #296 the postgres driver is unable to unlock properly. What we have done is setup a different db conn just to migrate and close it after migration. Closing the connection releases the lock.

jonathaningram added a commit to govau/cloud.gov.au that referenced this issue Jan 5, 2018
Hopefully fixes "can't acquire lock" error when trying to migrate.

See mattes/migrate#297 (comment)
rdallman pushed a commit to rdallman/migrate that referenced this issue Jan 25, 2018
I believe this closes mattes#297 as well.

I have been working on adding testing of migrations and it requires acquiring
the lock in mysql multiple times to go up and down. After nailing this down to
GET_LOCK returning a failure for every subsequent GET_LOCK call after the
first, I decided it was time to rtfm and lo and behold:

https://dev.mysql.com/doc/refman/5.7/en/miscellaneous-functions.html#function_release-lock

RELEASE_LOCK will not work if called from a different thread than GET_LOCK.
The simplest solution using the golang database/sql pkg appears to be to just
get a single conn to use for every operation. since migrations are happening
sequentially, I don't think this will be a performance hit (possible
improvement). now calling Lock() and Unlock() multiple times will work;
prior to this patch, every call to RELEASE_LOCK would fail. this required
minimal changes to use the *sql.Conn methods instead of the *sql.DB methods.

other changes:

* upped time out to 10s on GET_LOCK, 1s timeout can too easily leave us in a
state where we think we have the lock but it has timed out (during the
operation).
* fixes SetVersion which was not using the tx it was intending to, and fixed a
bug where the transaction could have been left open since Rollback or Commit
may never have been called.

I added a test but it does not seem to come up in the previous patch, at least
easily, I tried some shenanigans. At least, this behavior should be fixed with
this patch and hopefully the test / comment is advisory enough.

thank you for maintaining this library, it has been relatively easy to
integrate with and most stuff works (pg works great :)
@josephbuchma
Copy link
Author

I switched to https://github.com/db-journey/migrate (which is hard fork of this repo), primarily because it supported timestamp-based migrations versioning, where you don't need to ensure that your migration version is higher than current db schema version. But it evolved into major contribution, where I refactored code, removed all redundant complication with channels, simplified Driver interface and added few new features, including database locking during migrations. Now it's much more contribution-friendly, take a look @mattes

rdallman pushed a commit to rdallman/migrate that referenced this issue Feb 12, 2018
I believe this closes mattes#297 as well.

I have been working on adding testing of migrations and it requires acquiring
the lock in mysql multiple times to go up and down. After nailing this down to
GET_LOCK returning a failure for every subsequent GET_LOCK call after the
first, I decided it was time to rtfm and lo and behold:

https://dev.mysql.com/doc/refman/5.7/en/miscellaneous-functions.html#function_release-lock

RELEASE_LOCK will not work if called from a different thread than GET_LOCK.
The simplest solution using the golang database/sql pkg appears to be to just
get a single conn to use for every operation. since migrations are happening
sequentially, I don't think this will be a performance hit (possible
improvement). now calling Lock() and Unlock() multiple times will work;
prior to this patch, every call to RELEASE_LOCK would fail. this required
minimal changes to use the *sql.Conn methods instead of the *sql.DB methods.

other changes:

* upped time out to 10s on GET_LOCK, 1s timeout can too easily leave us in a
state where we think we have the lock but it has timed out (during the
operation).
* fixes SetVersion which was not using the tx it was intending to, and fixed a
bug where the transaction could have been left open since Rollback or Commit
may never have been called.

I added a test but it does not seem to come up in the previous patch, at least
easily, I tried some shenanigans. At least, this behavior should be fixed with
this patch and hopefully the test / comment is advisory enough.

thank you for maintaining this library, it has been relatively easy to
integrate with and most stuff works (pg works great :)
@miguelmota
Copy link

Getting can't acquire lock error as well.

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

Successfully merging a pull request may close this issue.

5 participants