From 636c911d33f73e5843dc768c0538bd25e0fb200c Mon Sep 17 00:00:00 2001 From: Reed Allman Date: Thu, 19 Oct 2017 06:24:39 -0700 Subject: [PATCH 1/3] fixes mysql lock failure 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 :) --- database/mysql/mysql.go | 41 ++++++++++++++++++++++++------------ database/mysql/mysql_test.go | 34 ++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 14 deletions(-) diff --git a/database/mysql/mysql.go b/database/mysql/mysql.go index 403eec0f0..6c538e3f1 100644 --- a/database/mysql/mysql.go +++ b/database/mysql/mysql.go @@ -1,6 +1,7 @@ package mysql import ( + "context" "crypto/tls" "crypto/x509" "database/sql" @@ -35,7 +36,9 @@ type Config struct { } type Mysql struct { - db *sql.DB + // mysql RELEASE_LOCK must be called from the same conn, so + // just do everything over a single conn anyway. + db *sql.Conn isLocked bool config *Config @@ -67,8 +70,13 @@ func WithInstance(instance *sql.DB, config *Config) (database.Driver, error) { config.MigrationsTable = DefaultMigrationsTable } + conn, err := instance.Conn(context.Background()) + if err != nil { + return nil, err + } + mx := &Mysql{ - db: instance, + db: conn, config: config, } @@ -162,9 +170,9 @@ func (m *Mysql) Lock() error { return err } - query := "SELECT GET_LOCK(?, 1)" + query := "SELECT GET_LOCK(?, 10)" var success bool - if err := m.db.QueryRow(query, aid).Scan(&success); err != nil { + if err := m.db.QueryRowContext(context.Background(), query, aid).Scan(&success); err != nil { return &database.Error{OrigErr: err, Err: "try lock failed", Query: []byte(query)} } @@ -188,10 +196,14 @@ func (m *Mysql) Unlock() error { } query := `SELECT RELEASE_LOCK(?)` - if _, err := m.db.Exec(query, aid); err != nil { + if _, err := m.db.ExecContext(context.Background(), query, aid); err != nil { return &database.Error{OrigErr: err, Query: []byte(query)} } + // NOTE: RELEASE_LOCK could return NULL or (or 0 if the code is changed), + // in which case isLocked should be true until the timeout expires -- synchronizing + // these states is likely not worth trying to do; reconsider the necessity of isLocked. + m.isLocked = false return nil } @@ -203,7 +215,7 @@ func (m *Mysql) Run(migration io.Reader) error { } query := string(migr[:]) - if _, err := m.db.Exec(query); err != nil { + if _, err := m.db.ExecContext(context.Background(), query); err != nil { return database.Error{OrigErr: err, Err: "migration failed", Query: migr} } @@ -211,19 +223,20 @@ func (m *Mysql) Run(migration io.Reader) error { } func (m *Mysql) SetVersion(version int, dirty bool) error { - tx, err := m.db.Begin() + tx, err := m.db.BeginTx(context.Background(), &sql.TxOptions{}) if err != nil { return &database.Error{OrigErr: err, Err: "transaction start failed"} } query := "TRUNCATE `" + m.config.MigrationsTable + "`" - if _, err := m.db.Exec(query); err != nil { + if _, err := tx.ExecContext(context.Background(), query); err != nil { + tx.Rollback() return &database.Error{OrigErr: err, Query: []byte(query)} } if version >= 0 { query := "INSERT INTO `" + m.config.MigrationsTable + "` (version, dirty) VALUES (?, ?)" - if _, err := m.db.Exec(query, version, dirty); err != nil { + if _, err := tx.ExecContext(context.Background(), query, version, dirty); err != nil { tx.Rollback() return &database.Error{OrigErr: err, Query: []byte(query)} } @@ -238,7 +251,7 @@ func (m *Mysql) SetVersion(version int, dirty bool) error { func (m *Mysql) Version() (version int, dirty bool, err error) { query := "SELECT version, dirty FROM `" + m.config.MigrationsTable + "` LIMIT 1" - err = m.db.QueryRow(query).Scan(&version, &dirty) + err = m.db.QueryRowContext(context.Background(), query).Scan(&version, &dirty) switch { case err == sql.ErrNoRows: return database.NilVersion, false, nil @@ -259,7 +272,7 @@ func (m *Mysql) Version() (version int, dirty bool, err error) { func (m *Mysql) Drop() error { // select all tables query := `SHOW TABLES LIKE '%'` - tables, err := m.db.Query(query) + tables, err := m.db.QueryContext(context.Background(), query) if err != nil { return &database.Error{OrigErr: err, Query: []byte(query)} } @@ -281,7 +294,7 @@ func (m *Mysql) Drop() error { // delete one by one ... for _, t := range tableNames { query = "DROP TABLE IF EXISTS `" + t + "` CASCADE" - if _, err := m.db.Exec(query); err != nil { + if _, err := m.db.ExecContext(context.Background(), query); err != nil { return &database.Error{OrigErr: err, Query: []byte(query)} } } @@ -297,7 +310,7 @@ func (m *Mysql) ensureVersionTable() error { // check if migration table exists var result string query := `SHOW TABLES LIKE "` + m.config.MigrationsTable + `"` - if err := m.db.QueryRow(query).Scan(&result); err != nil { + if err := m.db.QueryRowContext(context.Background(), query).Scan(&result); err != nil { if err != sql.ErrNoRows { return &database.Error{OrigErr: err, Query: []byte(query)} } @@ -307,7 +320,7 @@ func (m *Mysql) ensureVersionTable() error { // if not, create the empty migration table query = "CREATE TABLE `" + m.config.MigrationsTable + "` (version bigint not null primary key, dirty boolean not null)" - if _, err := m.db.Exec(query); err != nil { + if _, err := m.db.ExecContext(context.Background(), query); err != nil { return &database.Error{OrigErr: err, Query: []byte(query)} } return nil diff --git a/database/mysql/mysql_test.go b/database/mysql/mysql_test.go index 5fdb75670..ae2c9567f 100644 --- a/database/mysql/mysql_test.go +++ b/database/mysql/mysql_test.go @@ -63,3 +63,37 @@ func Test(t *testing.T) { } }) } + +func TestLockWorks(t *testing.T) { + mt.ParallelTest(t, versions, isReady, + func(t *testing.T, i mt.Instance) { + p := &Mysql{} + addr := fmt.Sprintf("mysql://root:root@tcp(%v:%v)/public", i.Host(), i.Port()) + d, err := p.Open(addr) + if err != nil { + t.Fatalf("%v", err) + } + dt.Test(t, d, []byte("SELECT 1")) + + ms := d.(*Mysql) + + err = ms.Lock() + if err != nil { + t.Fatal(err) + } + err = ms.Unlock() + if err != nil { + t.Fatal(err) + } + + // make sure the 2nd lock works (RELEASE_LOCK is very finicky) + err = ms.Lock() + if err != nil { + t.Fatal(err) + } + err = ms.Unlock() + if err != nil { + t.Fatal(err) + } + }) +} From a8ef2b8cfeee454dd7586cded4a4b6985a86fd68 Mon Sep 17 00:00:00 2001 From: Reed Allman Date: Thu, 19 Oct 2017 16:47:24 -0700 Subject: [PATCH 2/3] update CI and build tags to go1.9 --- .travis.yml | 1 - database/mysql/mysql.go | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 54242e9c7..8712ba3c9 100644 --- a/.travis.yml +++ b/.travis.yml @@ -2,7 +2,6 @@ language: go sudo: required go: - - 1.8 - 1.9 env: diff --git a/database/mysql/mysql.go b/database/mysql/mysql.go index 6c538e3f1..a816d2a24 100644 --- a/database/mysql/mysql.go +++ b/database/mysql/mysql.go @@ -1,3 +1,5 @@ +// +build go1.9 + package mysql import ( From 094bad431e7f820a0da943cb71ac3356564d4a39 Mon Sep 17 00:00:00 2001 From: Reed Allman Date: Fri, 9 Feb 2018 15:14:05 -0800 Subject: [PATCH 3/3] s/db/conn/ --- database/mysql/mysql.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/database/mysql/mysql.go b/database/mysql/mysql.go index a816d2a24..428fcb8b3 100644 --- a/database/mysql/mysql.go +++ b/database/mysql/mysql.go @@ -40,7 +40,7 @@ type Config struct { type Mysql struct { // mysql RELEASE_LOCK must be called from the same conn, so // just do everything over a single conn anyway. - db *sql.Conn + conn *sql.Conn isLocked bool config *Config @@ -78,7 +78,7 @@ func WithInstance(instance *sql.DB, config *Config) (database.Driver, error) { } mx := &Mysql{ - db: conn, + conn: conn, config: config, } @@ -158,7 +158,7 @@ func (m *Mysql) Open(url string) (database.Driver, error) { } func (m *Mysql) Close() error { - return m.db.Close() + return m.conn.Close() } func (m *Mysql) Lock() error { @@ -174,7 +174,7 @@ func (m *Mysql) Lock() error { query := "SELECT GET_LOCK(?, 10)" var success bool - if err := m.db.QueryRowContext(context.Background(), query, aid).Scan(&success); err != nil { + if err := m.conn.QueryRowContext(context.Background(), query, aid).Scan(&success); err != nil { return &database.Error{OrigErr: err, Err: "try lock failed", Query: []byte(query)} } @@ -198,7 +198,7 @@ func (m *Mysql) Unlock() error { } query := `SELECT RELEASE_LOCK(?)` - if _, err := m.db.ExecContext(context.Background(), query, aid); err != nil { + if _, err := m.conn.ExecContext(context.Background(), query, aid); err != nil { return &database.Error{OrigErr: err, Query: []byte(query)} } @@ -217,7 +217,7 @@ func (m *Mysql) Run(migration io.Reader) error { } query := string(migr[:]) - if _, err := m.db.ExecContext(context.Background(), query); err != nil { + if _, err := m.conn.ExecContext(context.Background(), query); err != nil { return database.Error{OrigErr: err, Err: "migration failed", Query: migr} } @@ -225,7 +225,7 @@ func (m *Mysql) Run(migration io.Reader) error { } func (m *Mysql) SetVersion(version int, dirty bool) error { - tx, err := m.db.BeginTx(context.Background(), &sql.TxOptions{}) + tx, err := m.conn.BeginTx(context.Background(), &sql.TxOptions{}) if err != nil { return &database.Error{OrigErr: err, Err: "transaction start failed"} } @@ -253,7 +253,7 @@ func (m *Mysql) SetVersion(version int, dirty bool) error { func (m *Mysql) Version() (version int, dirty bool, err error) { query := "SELECT version, dirty FROM `" + m.config.MigrationsTable + "` LIMIT 1" - err = m.db.QueryRowContext(context.Background(), query).Scan(&version, &dirty) + err = m.conn.QueryRowContext(context.Background(), query).Scan(&version, &dirty) switch { case err == sql.ErrNoRows: return database.NilVersion, false, nil @@ -274,7 +274,7 @@ func (m *Mysql) Version() (version int, dirty bool, err error) { func (m *Mysql) Drop() error { // select all tables query := `SHOW TABLES LIKE '%'` - tables, err := m.db.QueryContext(context.Background(), query) + tables, err := m.conn.QueryContext(context.Background(), query) if err != nil { return &database.Error{OrigErr: err, Query: []byte(query)} } @@ -296,7 +296,7 @@ func (m *Mysql) Drop() error { // delete one by one ... for _, t := range tableNames { query = "DROP TABLE IF EXISTS `" + t + "` CASCADE" - if _, err := m.db.ExecContext(context.Background(), query); err != nil { + if _, err := m.conn.ExecContext(context.Background(), query); err != nil { return &database.Error{OrigErr: err, Query: []byte(query)} } } @@ -312,7 +312,7 @@ func (m *Mysql) ensureVersionTable() error { // check if migration table exists var result string query := `SHOW TABLES LIKE "` + m.config.MigrationsTable + `"` - if err := m.db.QueryRowContext(context.Background(), query).Scan(&result); err != nil { + if err := m.conn.QueryRowContext(context.Background(), query).Scan(&result); err != nil { if err != sql.ErrNoRows { return &database.Error{OrigErr: err, Query: []byte(query)} } @@ -322,7 +322,7 @@ func (m *Mysql) ensureVersionTable() error { // if not, create the empty migration table query = "CREATE TABLE `" + m.config.MigrationsTable + "` (version bigint not null primary key, dirty boolean not null)" - if _, err := m.db.ExecContext(context.Background(), query); err != nil { + if _, err := m.conn.ExecContext(context.Background(), query); err != nil { return &database.Error{OrigErr: err, Query: []byte(query)} } return nil