add rejectReadOnly option (to fix AWS Aurora failover) (go-sql-driver#604)

* add RejectReadOnly

* update README.md

* close connection explicitly before returning ErrBadConn for 1792 (#2)

* add test and improve doc

* doc/comment changes

Author: songgao

AUTHORS

@@ -61,5 +61,6 @@ Zhenye Xie <xiezhenye at gmail.com>
 
 Barracuda Networks, Inc.
 Google Inc.
+Keybase Inc.
 Pivotal Inc.
 Stripe Inc.

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`
 
 ```

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")
+	})
+}

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

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]