fixes mysql lock failure

[rdallman]

I believe this closes #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 :)

[rdallman]

Have done some research since running into the CI build failure on go1.7. It appears that sql.Conn was introduced in go1.9, so one option to resolving this is to simply require via // +build go1.9 the mysql package here -- this seems reasonable to me, as the changes to the sql package in go1.9 are desirable for many reasons (namely, prepared statement caching as well as support for *Context methods). This also would include updating CI to use go1.9 for builds in order for tests to pass.

It is indeed possible to get a conn in a < go1.9 way, however, it will require significant updates to the queries used in this package. the driver.Conn interface is limited to being able to start transactions and prepare statements. this means that all calls to Query, QueryRow and Exec will need to be calling statements, which will need to be prepared in advance to avoid overflowing the number of prepared statements in the database (i.e. we'll need a prepared statement cache added onto the *MySql struct). Notably, it is also a little hairy to get a conn via the driver.Driver interface.

I will await further guidance on this issue as to your preference. thanks!

[mattes]

THANK YOU @rdallman for this PR. Great work!

Notably, it is also a little hairy to get a conn via the driver.Driver interface.
agreed

What if we just drop support for < go1.9?

[rdallman]

What if we just drop support for < go1.9?

Sounds great to me, I will update the PR to reflect this (today, I hope)

[rdallman]

Have hit a snag here in dependency hell :( I did manage to update CI to use go1.9.1 and added a build tag.

It appears that go-sql-driver/mysql#657 is blocking in order for this to work with the latest mysql driver, as it stands I imagine that master of this library doesn't work consistently due to this, either. We are locked in to go-sql-driver/mysql@21d7e97 of that driver in order for mysql to work for us, without regard to this library (seems a pretty hairy bug) -- if I use the linked commit version of the mysql driver to run this library's test suite I get passing tests, but with the latest mysql driver it consistently fails. I tracked it down to this line https://github.com/go-sql-driver/mysql/blob/master/packets.go#L38 which in the linked commit https://github.com/go-sql-driver/mysql/blob/21d7e97c9f760ca685a01ecea202e1c84276daa1/packets.go#L38 will work since ErrBadConn is returned (the sql driver understands this). With the latest, it appears connections immediately go awry after performing the first action, which is undesirable for reasons stretching beyond this patch. In any event, since an external interface is calling Lock() and Unlock(), we need behind the scenes to be able to reuse the same connection (as highlighted in parent), so this is blocking here, since just getting a new connection won't work for this use case.

I don't see glide or dep files in repo, simply using go get, and there's not a clear way to resolve this. We can check out the above linked version of the mysql driver in CI in order for tests to pass, but I'm reluctant to not have a more prescriptive way for importers of this library to not step on the same bug simply by innocently using the latest version of the mysql driver, granted that they will soon learn that the tip of that driver will not work well if they use it to do things other than use this migration library anyway. We could optionally merge this in with some kind of loud warning about using a newer version of the mysql library (sadly), as it will work great if using the linked commit version, but this is a little hairy. I'd be happy to add dep or glide to this library, if you see fit, or perhaps you can think of another solution that is better than what I can come up with.

Sorry for this being such a pill!

[rdallman]

Hi @mattes, happy new year, I have some good news.

Have managed to fix the issue in the mysql driver and landed it in go-sql-driver/mysql#736 -- this no longer causes a conn that was acquired via the db.Conn() interface to return an invalid connection error. With this, the tests here for mysql also pass.

Quick summary:

• mysql driver relies on >=go1.9 now
• mysql driver now uses same conn for migrations to fix GET_LOCK & RELEASE_LOCK behavior
• 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.

and the bad news:

CI apt-get does not seem happy today. I'm unable to kick rebuilds off myself (it appears). the tests pass for me with the latest mysql driver locally and don't see many changes in master so expect this to go green.

thanks again!

[rdallman]

Had some spare time today, I fixed the build 8383637 yay. it seems other branches are facing this, too, feel free to steal if this is still blocked.

[dhui]

Fixed in forked release v3.2.0