rdallman
commented
6 years ago 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
dhui
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:
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 :)