Always reject read-only connections

README.md

@@ -45,7 +45,8 @@ partition's own bundle to verify them.
 
 ## What this fork changes
 
-Two things, both for packaging reasons only. Neither alters protocol behaviour.
+Three things. The first two are packaging; neither alters protocol
+behaviour. The third changes a default, deliberately.
 
 **The module path is `github.com/block/mysql`.** Upstream's path plus a
 `replace` directive would work for a binary, but `replace` is not inherited
@@ -66,6 +67,14 @@ connections with:
 db, err := sql.Open("block-mysql", dsn)
 ```
 
+**Read-only connections are always rejected.** Upstream's `rejectReadOnly`
+option defaults to off; here the behaviour is unconditional and
+`Config.RejectReadOnly` is gone. RDS and Aurora fail over by moving DNS, so a
+pooled connection to the demoted writer stays open and every write on it fails
+until the process restarts — with nothing in the DSN or the error to say the
+connection is the problem. See the `rejectReadOnly` parameter below for what
+happens to a DSN that still sets it.
+
 The DSN format, `Config`, and the rest of the API are upstream's.
 
 ## Linking both drivers
@@ -97,11 +106,15 @@ git fetch upstream
 git merge upstream/master
 ```
 
-Edits to upstream files are confined to three things: the module path and
+Edits to upstream files are confined to four things: the module path and
 driver name (`go.mod`, `driver.go`, plus doc comments and test call sites that
-spell either one out), the CI matrix (see below), and a one-line call in
-`Config.normalize` that hands off to `rds.go`. The capabilities above live in
-files upstream does not have, which is what keeps merges near-mechanical.
+spell either one out), the CI matrix (see below), a one-line call in
+`Config.normalize` that hands off to `rds.go`, and the read-only rejection (one
+condition in `packets.go`, the parameter in `dsn.go`, and the
+read-only-transaction flag in `connection.go`/`transaction.go`). The
+capabilities above live in files upstream does not have, which is what keeps
+merges near-mechanical.
+
 Additions are cheapest when they follow the same shape: new files, or new
 methods on existing types, in preference to reworking an upstream code path.
 
@@ -522,30 +535,56 @@ I/O read timeout. The value must be a decimal number with a unit suffix (*"ms"*,
 
 ```
 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.
-
-Note that ERROR 1290 can be returned for a `read-only` server and this option will
-cause a retry for that error. However the same error number is used for some
-other cases. You should ensure your application will never cause an ERROR 1290
-except for `read-only` mode when enabling this option.
+Valid Values:   true
+Default:        (always on; see below)
+```
+
+**Changed in this fork.** Upstream makes this an option, defaulting to off.
+Here the driver always rejects read-only connections, `Config.RejectReadOnly`
+is gone, and the parameter survives only so a DSN written for upstream keeps
+parsing: `rejectReadOnly=true` is accepted and does nothing, while
+`rejectReadOnly=false` is an error rather than a silent no-op, because it
+states an expectation the driver will not meet.
+
+Rejecting means that when a statement fails with a read-only error (1792, 1290
+or 1836), the driver closes that connection and returns `driver.ErrBadConn`, so
+`database/sql` retries the statement on a new one.
+
+It is not optional because the failure it prevents is silent and the mistake
+that causes it is invisible. RDS and Aurora fail over by moving DNS: a pooled
+connection to the demoted writer stays open and stays usable, and every write
+on it fails for as long as the pool keeps it — until the process restarts.
+Nothing in the DSN or in the error says the connection is the problem, and a
+deployment that forgot the option does not find out until a failover.
+
+One exception: a transaction opened with `sql.TxOptions{ReadOnly: true}` is
+exempt. There the read-only error is the answer the caller asked for, and
+`database/sql` does not retry inside a transaction anyway, so rejecting would
+replace a usable `*MySQLError` with a dead transaction.
+
+Two consequences worth knowing:
+
+* A session made read-only by the application's own `SET SESSION TRANSACTION
+  READ ONLY` is *not* exempt — nothing distinguishes it from a demoted writer.
+  Writes on such a session are retried on a new connection instead of failing.
+  Use privileges, or the `ReadOnly` transaction option, to express that intent.
+* ERROR 1290 is also raised for conditions unrelated to failover — the ones you
+  are likely to meet are `secure_file_priv` (a `SELECT … INTO OUTFILE` or `LOAD
+  DATA` outside the permitted directory), `super_read_only`, `innodb_read_only`
+  and `--skip-grant-tables`. All of these persist rather than clear, so the
+  statement is retried, the connection churned, and the caller finally sees
+  `driver.ErrBadConn` rather than the message that named the problem. The
+  driver logs the server's own error before discarding it, so the condition is
+  still identifiable — look for `closing read-only connection` in the log.
+
+**If your target is deliberately read-only** — an Aurora reader endpoint, a
+replica, a source you only ever read from — this fork is a poor fit for that
+connection, and there is no longer an option to turn it off. Wrap reads in
+`sql.TxOptions{ReadOnly: true}` and they are exempt; anything on autocommit
+that the server rejects will still be retried and churned. If that is not
+workable, use a driver that lets you disable the behaviour for that connection.
+The trade is deliberate: the population this fork serves writes to RDS
+primaries, where the silent failure is the more expensive one.
 
 
 ##### `serverPubKey`

connection.go

@@ -41,6 +41,13 @@ type mysqlConn struct {
 	parseTime        bool
 	compress         bool
 
+	// inReadOnlyTx is set while a transaction the caller explicitly opened
+	// with driver.TxOptions.ReadOnly is in flight. Fork addition: it is the
+	// one case where a read-only error is the answer the caller asked for
+	// rather than a sign of a demoted writer, so handleErrorPacket must not
+	// turn it into ErrBadConn. See packets.go.
+	inReadOnlyTx bool
+
 	// for context support (Go 1.8+)
 	watching bool
 	watcher  chan<- context.Context
@@ -165,6 +172,7 @@ func (mc *mysqlConn) begin(readOnly bool) (driver.Tx, error) {
 	}
 	err := mc.exec(q)
 	if err == nil {
+		mc.inReadOnlyTx = readOnly
 		return &mysqlTx{mc}, err
 	}
 	return nil, mc.markBadConn(err)
@@ -795,6 +803,14 @@ func (mc *mysqlConn) ResetSession(ctx context.Context) error {
 		return driver.ErrBadConn
 	}
 
+	// Fork addition: re-establish the read-only-transaction exemption for the
+	// new borrower. Commit and Rollback both clear it and every sql.Tx ends in
+	// one of them, so this is hardening rather than a fix — but the direction
+	// it can get stuck in, true, silently disables the read-only rejection for
+	// the rest of this connection's life, and a pooled connection's
+	// assumptions belong here. See packets.go.
+	mc.inReadOnlyTx = false
+
 	// Perform a stale connection check. We only perform this check for
 	// the first query on a connection that has been checked out of the
 	// connection pool: a fresh connection from the pool is more likely

driver_test.go

@@ -2293,26 +2293,29 @@ func TestColumnsReusesSlice(t *testing.T) {
 	}
 }
 
+// TestRejectReadOnly exercises the read-only rejection, which this fork
+// applies unconditionally. Upstream's version of this test also asserted the
+// behaviour with the option off; that is no longer a state the driver can be
+// in, so the first case now covers a DSN that says nothing.
 func TestRejectReadOnly(t *testing.T) {
+	// No parameter: the rejection happens anyway. This is the case upstream's
+	// default gets wrong.
 	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.
+		// 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.
 		_, 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")
+		// 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")
 	})
 
-	// Enable the `rejectReadOnly` option.
+	// rejectReadOnly=true still parses, for a DSN written against upstream.
 	runTests(t, dsn+"&rejectReadOnly=true", func(dbt *DBTest) {
 		// Create Table
 		dbt.mustExec("CREATE TABLE test (value BOOL)")

dsn.go

@@ -72,7 +72,6 @@ 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
 
 	// unexported fields. new options should be come here.
 	// boolean first. alphabetical order.
@@ -407,10 +406,6 @@ func (cfg *Config) FormatDSN() string {
 		writeDSNParam(&buf, &hasParam, "readTimeout", cfg.ReadTimeout.String())
 	}
 
-	if cfg.RejectReadOnly {
-		writeDSNParam(&buf, &hasParam, "rejectReadOnly", "true")
-	}
-
 	if len(cfg.ServerPubKey) > 0 {
 		writeDSNParam(&buf, &hasParam, "serverPubKey", url.QueryEscape(cfg.ServerPubKey))
 	}
@@ -677,13 +672,23 @@ func parseDSNParams(cfg *Config, params string) (err error) {
 				return
 			}
 
-		// Reject read-only connections
+		// Reject read-only connections.
+		//
+		// Fork change: this is unconditional here, so the parameter carries no
+		// information. It is still accepted, because a DSN written for upstream
+		// should not stop parsing — but only in the direction that agrees with
+		// what the driver does. rejectReadOnly=false is refused rather than
+		// ignored: it states an expectation the driver will not meet, and a
+		// silent no-op is exactly the kind of quiet disagreement this change
+		// exists to remove. See packets.go.
 		case "rejectReadOnly":
-			var isBool bool
-			cfg.RejectReadOnly, isBool = readBool(value)
+			on, isBool := readBool(value)
 			if !isBool {
 				return errors.New("invalid bool value: " + value)
 			}
+			if !on {
+				return errors.New("rejectReadOnly=false: this driver always rejects read-only connections; the option cannot be disabled")
+			}
 
 		// Server public key
 		case "serverPubKey":

dsn_test.go

@@ -129,7 +129,9 @@ var testDSNs = []struct {
 			cfg.AllowOldPasswords = true
 			cfg.ClientFoundRows = true
 			cfg.ParseTime = true
-			cfg.RejectReadOnly = true
+			// rejectReadOnly=true is still in the DSN above: it parses and
+			// sets nothing, because this driver always rejects read-only
+			// connections. See dsn.go.
 		}),
 	},
 	{

packets.go

@@ -598,13 +598,44 @@ func (mc *mysqlConn) handleErrorPacket(data []byte) error {
 	// 1792: ER_CANT_EXECUTE_IN_READ_ONLY_TRANSACTION
 	// 1290: ER_OPTION_PREVENTS_STATEMENT (returned by Aurora during failover)
 	// 1836: ER_READ_ONLY_MODE
-	if (errno == 1792 || errno == 1290 || errno == 1836) && mc.cfg.RejectReadOnly {
+	if (errno == 1792 || errno == 1290 || errno == 1836) && !mc.inReadOnlyTx {
 		// 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.
+		// to issue any write statements. We throw away this connection hoping
+		// the next 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.
 		//
+		// Fork change: upstream gates this on the rejectReadOnly option, which
+		// defaults to off. It is unconditional here. On a provider that fails
+		// over by moving DNS — RDS and Aurora both do — an application that
+		// leaves it off keeps a pooled connection to the demoted writer and
+		// every write on it fails, indefinitely, with no error that says the
+		// connection is the problem. Nothing about the DSN reveals the
+		// omission, so it is not a mistake a deployment discovers except
+		// during a failover. See dsn.go for what happens to the option.
+		//
+		// The one exception is a transaction the caller opened with
+		// driver.TxOptions.ReadOnly: there the read-only error is the answer
+		// they asked for, and database/sql does not retry inside a
+		// transaction anyway, so rejecting would replace a usable
+		// *MySQLError with a dead transaction. A session made read-only by
+		// the application's own SET is not exempt — nothing distinguishes it
+		// from a demoted writer.
+		//
+		// Log before discarding it. On the failover this is written for the
+		// caller never sees anything — database/sql retries onto a healthy
+		// connection — so one line is the whole trace. On a target that is
+		// *persistently* read-only (a reader endpoint, a cluster with no
+		// writer, super_read_only left on after maintenance) database/sql
+		// burns its retry budget and hands the caller a bare
+		// driver.ErrBadConn; without this, the server's own message — the only
+		// text that names the actual problem — is assembled nowhere.
+		msg := data[3:]
+		if len(msg) > 6 && msg[0] == 0x23 {
+			msg = msg[6:] // skip the "#HY000" SQL-state marker, as below
+		}
+		mc.log("closing read-only connection, errno ", errno, ": ", string(msg))
+
 		// 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.