From a3bdfa27b9af97167975bf7ea7526fbe642c0558 Mon Sep 17 00:00:00 2001 From: Song Gao Date: Tue, 23 May 2017 15:31:50 -0700 Subject: [PATCH 1/5] add RejectReadOnly --- AUTHORS | 1 + dsn.go | 32 ++++++++++++++++++++++++++++++++ packets.go | 5 +++++ 3 files changed, 38 insertions(+) diff --git a/AUTHORS b/AUTHORS index 1928dac89..e91065adc 100644 --- a/AUTHORS +++ b/AUTHORS @@ -59,4 +59,5 @@ Zhenye Xie Barracuda Networks, Inc. Google Inc. +Keybase Inc. Stripe Inc. diff --git a/dsn.go b/dsn.go index 5c828bf90..8c01c697e 100644 --- a/dsn.go +++ b/dsn.go @@ -54,6 +54,21 @@ type Config struct { MultiStatements bool // Allow multiple statements in one query ParseTime bool // Parse time values to time.Time Strict bool // Return warnings as errors + + // RejectreadOnly causes mysql driver to reject read-only connections. This + // is specifically for AWS Aurora: During a failover, there seems to be a + // race condition on Aurora, where we get connected to the [old master + // before failover], i.e. the [new read-only slave after failover]. + // + // Note that this should be a fairly rare case, as automatic failover + // normally happens when master is down, and the race condition shouldn't + // happen unless it comes back up online as soon as the failover is kicked + // off. But it's pretty easy to reproduce using a manual failover. In case + // this happens, we should reconnect to the Aurora cluster by returning a + // driver.ErrBadConnection. + // + // tl;dr: Set this if you are using Aurora. + RejectReadOnly bool } // FormatDSN formats the given Config into a DSN string which can be passed to @@ -195,6 +210,15 @@ func (cfg *Config) FormatDSN() string { buf.WriteString(cfg.ReadTimeout.String()) } + if cfg.RejectReadOnly { + if hasParam { + buf.WriteString("&rejectReadOnly=true") + } else { + hasParam = true + buf.WriteString("?rejectReadOnly=true") + } + } + if cfg.Strict { if hasParam { buf.WriteString("&strict=true") @@ -472,6 +496,14 @@ func parseDSNParams(cfg *Config, params string) (err error) { return } + // Reject read-only connections + case "rejectReadOnly": + var isBool bool + cfg.RejectReadOnly, isBool = readBool(value) + if !isBool { + return errors.New("invalid bool value: " + value) + } + // Strict mode case "strict": var isBool bool diff --git a/packets.go b/packets.go index cb21397a2..65a1a2cfc 100644 --- a/packets.go +++ b/packets.go @@ -551,6 +551,11 @@ func (mc *mysqlConn) handleErrorPacket(data []byte) error { // Error Number [16 bit uint] errno := binary.LittleEndian.Uint16(data[1:3]) + // 1792: ER_CANT_EXECUTE_IN_READ_ONLY_TRANSACTION + if errno == 1792 && mc.cfg.RejectReadOnly { + return driver.ErrBadConn + } + pos := 3 // SQL State [optional: # + 5bytes string] From 6b8624ce29957a9cda06ca4eaa169cbdc661bb69 Mon Sep 17 00:00:00 2001 From: Song Gao Date: Tue, 23 May 2017 15:35:09 -0700 Subject: [PATCH 2/5] update README.md --- README.md | 23 +++++++++++++++++++++++ dsn.go | 16 +--------------- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index a060e3cfd..12f49bd0d 100644 --- a/README.md +++ b/README.md @@ -267,6 +267,29 @@ Default: 0 I/O read timeout. The value must be a decimal number with a unit suffix (*"ms"*, *"s"*, *"m"*, *"h"*), such as *"30s"*, *"0.5m"* or *"1m30s"*. +##### `rejectReadOnly` + +``` +Type: bool +Valid Values: true, false +Default: false +``` + + +RejectreadOnly causes mysql driver to reject read-only connections. This is +specifically for AWS Aurora: During a failover, there seems to be a race +condition on Aurora, where we get connected to the [old master before +failover], i.e. the [new read-only slave after failover]. + +Note that this should be a fairly rare case, as automatic failover normally +happens when master is down, and the race condition shouldn't happen unless it +comes back up online as soon as the failover is kicked off. But it's pretty +easy to reproduce using a manual failover. In case this happens, we should +reconnect to the Aurora cluster by returning a driver.ErrBadConnection. + +tl;dr: Set this if you are using Aurora. + + ##### `strict` ``` diff --git a/dsn.go b/dsn.go index 8c01c697e..f811124eb 100644 --- a/dsn.go +++ b/dsn.go @@ -53,22 +53,8 @@ type Config struct { InterpolateParams bool // Interpolate placeholders into query string MultiStatements bool // Allow multiple statements in one query ParseTime bool // Parse time values to time.Time + RejectReadOnly bool // Reject read-only connections; see README.md Strict bool // Return warnings as errors - - // RejectreadOnly causes mysql driver to reject read-only connections. This - // is specifically for AWS Aurora: During a failover, there seems to be a - // race condition on Aurora, where we get connected to the [old master - // before failover], i.e. the [new read-only slave after failover]. - // - // Note that this should be a fairly rare case, as automatic failover - // normally happens when master is down, and the race condition shouldn't - // happen unless it comes back up online as soon as the failover is kicked - // off. But it's pretty easy to reproduce using a manual failover. In case - // this happens, we should reconnect to the Aurora cluster by returning a - // driver.ErrBadConnection. - // - // tl;dr: Set this if you are using Aurora. - RejectReadOnly bool } // FormatDSN formats the given Config into a DSN string which can be passed to From 83d56b7ce7e37d6ad77408d8b4d47f22b62c39d7 Mon Sep 17 00:00:00 2001 From: Song Gao Date: Mon, 29 May 2017 13:21:47 -0700 Subject: [PATCH 3/5] close connection explicitly before returning ErrBadConn for 1792 (#2) --- packets.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packets.go b/packets.go index 65a1a2cfc..a9250d841 100644 --- a/packets.go +++ b/packets.go @@ -553,6 +553,16 @@ func (mc *mysqlConn) handleErrorPacket(data []byte) error { // 1792: ER_CANT_EXECUTE_IN_READ_ONLY_TRANSACTION if errno == 1792 && mc.cfg.RejectReadOnly { + // Oops; we are connected to a read-only connection, and won't be able + // to issue any write statements. Since RejectReadOnly is configured, + // we throw away this connection hoping this one would have write + // permission. This is specifically for an AWS Aurora behavior during + // failover. See README.md for more. + // + // We explicitly close the connection before returning + // driver.ErrBadConn to ensure that `database/sql` purges this + // connection and initiates a new one for next statement next time. + mc.Close() return driver.ErrBadConn } From d2fefe86e9f3126ca81221a1ccbea1a56266ad6f Mon Sep 17 00:00:00 2001 From: Song Gao Date: Mon, 29 May 2017 15:08:20 -0700 Subject: [PATCH 4/5] add test and improve doc --- README.md | 26 +++++++++++++------------ driver_test.go | 52 +++++++++++++++++++++++++++++++++++++++++++++----- packets.go | 4 ++-- 3 files changed, 63 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index 12f49bd0d..552e5d89b 100644 --- a/README.md +++ b/README.md @@ -276,18 +276,20 @@ Default: false ``` -RejectreadOnly causes mysql driver to reject read-only connections. This is -specifically for AWS Aurora: During a failover, there seems to be a race -condition on Aurora, where we get connected to the [old master before -failover], i.e. the [new read-only slave after failover]. - -Note that this should be a fairly rare case, as automatic failover normally -happens when master is down, and the race condition shouldn't happen unless it -comes back up online as soon as the failover is kicked off. But it's pretty -easy to reproduce using a manual failover. In case this happens, we should -reconnect to the Aurora cluster by returning a driver.ErrBadConnection. - -tl;dr: Set this if you are using Aurora. +RejectreadOnly causes the driver to reject read-only connections. This is for a +possible race condition during an automatic failover, where the mysql client +gets connected to a read-only replica after the failover. + +Note that this should be a fairly rare case, as an automatic failover normally +happens when the primary is down, and the race condition shouldn't happen +unless it comes back up online as soon as the failover is kicked off. On the +other hand, when this happens, an mysql application can get stuck on a +read-only connection until restarted. It is however fairly easy to reproduce, +for example, using a manual failover on AWS Aurora's MySQL-compatible cluster. + +If you are not relying on read-only transactions to reject writes that aren't +supposed to happen, setting this on some MySQL providers (such as AWS Aurora) +is safer for failovers. ##### `strict` diff --git a/driver_test.go b/driver_test.go index 07f7e79b9..6ca5434a9 100644 --- a/driver_test.go +++ b/driver_test.go @@ -171,6 +171,17 @@ func (dbt *DBTest) mustQuery(query string, args ...interface{}) (rows *sql.Rows) return rows } +func maybeSkip(t *testing.T, err error, skipErrno uint16) { + mySQLErr, ok := err.(*MySQLError) + if !ok { + return + } + + if mySQLErr.Number == skipErrno { + t.Skipf("skipping test for error: %v", err) + } +} + func TestEmptyQuery(t *testing.T) { runTests(t, dsn, func(dbt *DBTest) { // just a comment, no query @@ -1168,11 +1179,9 @@ func TestStrict(t *testing.T) { if conn != nil { conn.Close() } - if me, ok := err.(*MySQLError); ok && me.Number == 1231 { - // Error 1231: Variable 'sql_mode' can't be set to the value of 'ALLOW_INVALID_DATES' - // => skip test, MySQL server version is too old - return - } + // Error 1231: Variable 'sql_mode' can't be set to the value of + // 'ALLOW_INVALID_DATES' => skip test, MySQL server version is too old + maybeSkip(t, err, 1231) runTests(t, relaxedDsn, func(dbt *DBTest) { dbt.mustExec("CREATE TABLE test (a TINYINT NOT NULL, b CHAR(4))") @@ -1949,3 +1958,36 @@ func TestColumnsReusesSlice(t *testing.T) { t.Fatalf("expected columnNames to be set, got nil") } } + +func TestRejectReadOnly(t *testing.T) { + runTests(t, dsn, func(dbt *DBTest) { + // Create Table + dbt.mustExec("CREATE TABLE test (value BOOL)") + // Set the session to read-only. We didn't set the `rejectReadOnly` + // option, so any writes after this should fail. + _, err := dbt.db.Exec("SET SESSION TRANSACTION READ ONLY") + // Error 1193: Unknown system variable 'TRANSACTION' => skip test, + // MySQL server version is too old + maybeSkip(t, err, 1193) + if _, err := dbt.db.Exec("DROP TABLE test"); err == nil { + t.Fatalf("writing to DB in read-only session without " + + "rejectReadOnly did not error") + } + // Set the session back to read-write so runTests() can properly clean + // up the table `test`. + dbt.mustExec("SET SESSION TRANSACTION READ WRITE") + }) + + // Enable the `rejectReadOnly` option. + runTests(t, dsn+"&rejectReadOnly=true", func(dbt *DBTest) { + // Create Table + dbt.mustExec("CREATE TABLE test (value BOOL)") + // Set the session to read only. Any writes after this should error on + // a driver.ErrBadConn, and cause `database/sql` to initiate a new + // connection. + dbt.mustExec("SET SESSION TRANSACTION READ ONLY") + // This would error, but `database/sql` should automatically retry on a + // new connection which is not read-only, and eventually succeed. + dbt.mustExec("DROP TABLE test") + }) +} diff --git a/packets.go b/packets.go index a9250d841..303405a17 100644 --- a/packets.go +++ b/packets.go @@ -556,8 +556,8 @@ func (mc *mysqlConn) handleErrorPacket(data []byte) error { // Oops; we are connected to a read-only connection, and won't be able // to issue any write statements. Since RejectReadOnly is configured, // we throw away this connection hoping this one would have write - // permission. This is specifically for an AWS Aurora behavior during - // failover. See README.md for more. + // permission. This is specifically for a possible race condition + // during failover (e.g. on AWS Aurora). See README.md for more. // // We explicitly close the connection before returning // driver.ErrBadConn to ensure that `database/sql` purges this From bf068b4ba49a265de0236a150124692bab5180eb Mon Sep 17 00:00:00 2001 From: Song Gao Date: Tue, 30 May 2017 11:41:18 -0700 Subject: [PATCH 5/5] doc/comment changes --- README.md | 8 ++++---- dsn.go | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 552e5d89b..76a35d32e 100644 --- a/README.md +++ b/README.md @@ -276,14 +276,14 @@ Default: false ``` -RejectreadOnly causes the driver to reject read-only connections. This is for a -possible race condition during an automatic failover, where the mysql client -gets connected to a read-only replica after the failover. +`rejectreadOnly=true` causes the driver to reject read-only connections. This +is for a possible race condition during an automatic failover, where the mysql +client gets connected to a read-only replica after the failover. Note that this should be a fairly rare case, as an automatic failover normally happens when the primary is down, and the race condition shouldn't happen unless it comes back up online as soon as the failover is kicked off. On the -other hand, when this happens, an mysql application can get stuck on a +other hand, when this happens, a MySQL application can get stuck on a read-only connection until restarted. It is however fairly easy to reproduce, for example, using a manual failover on AWS Aurora's MySQL-compatible cluster. diff --git a/dsn.go b/dsn.go index 25ade564d..199c9b177 100644 --- a/dsn.go +++ b/dsn.go @@ -53,7 +53,7 @@ type Config struct { InterpolateParams bool // Interpolate placeholders into query string MultiStatements bool // Allow multiple statements in one query ParseTime bool // Parse time values to time.Time - RejectReadOnly bool // Reject read-only connections; see README.md + RejectReadOnly bool // Reject read-only connections Strict bool // Return warnings as errors }