diff --git a/AUTHORS b/AUTHORS index 4601d08f2..0a22d46e7 100644 --- a/AUTHORS +++ b/AUTHORS @@ -61,5 +61,6 @@ Zhenye Xie Barracuda Networks, Inc. Google Inc. +Keybase Inc. Pivotal Inc. Stripe Inc. diff --git a/README.md b/README.md index a060e3cfd..76a35d32e 100644 --- a/README.md +++ b/README.md @@ -267,6 +267,31 @@ 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=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, 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. + +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/dsn.go b/dsn.go index c49827f5d..199c9b177 100644 --- a/dsn.go +++ b/dsn.go @@ -53,6 +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 Strict bool // Return warnings as errors } @@ -195,6 +196,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 +482,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..303405a17 100644 --- a/packets.go +++ b/packets.go @@ -551,6 +551,21 @@ 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 { + // 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 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 + // connection and initiates a new one for next statement next time. + mc.Close() + return driver.ErrBadConn + } + pos := 3 // SQL State [optional: # + 5bytes string]