diff --git a/README.md b/README.md index 48d12bfa9..e57aaf744 100644 --- a/README.md +++ b/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` diff --git a/connection.go b/connection.go index 35d669ca7..dd3329251 100644 --- a/connection.go +++ b/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 diff --git a/driver_test.go b/driver_test.go index 8430f3850..65a197588 100644 --- a/driver_test.go +++ b/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)") diff --git a/dsn.go b/dsn.go index c3ea608d1..ca8d62d72 100644 --- a/dsn.go +++ b/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": diff --git a/dsn_test.go b/dsn_test.go index 120550cf4..f060801f0 100644 --- a/dsn_test.go +++ b/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. }), }, { diff --git a/packets.go b/packets.go index 88f318e33..a9617f4d4 100644 --- a/packets.go +++ b/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. diff --git a/readonly_test.go b/readonly_test.go new file mode 100644 index 000000000..16925d2c5 --- /dev/null +++ b/readonly_test.go @@ -0,0 +1,216 @@ +// Go MySQL Driver - A MySQL-Driver for Go's database/sql package +// +// Copyright 2026 The Go-MySQL-Driver Authors. All rights reserved. +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at http://mozilla.org/MPL/2.0/. + +package mysql + +import ( + "context" + "database/sql/driver" + "encoding/binary" + "errors" + "strings" + "testing" +) + +// TestRejectReadOnlyDSN covers what the rejectReadOnly parameter does now that +// the behaviour it used to control is unconditional. It is accepted in the +// direction that agrees with the driver and refused in the direction that does +// not, so a DSN carrying the upstream default is a loud failure rather than a +// silent disagreement. +func TestRejectReadOnlyDSN(t *testing.T) { + for _, value := range []string{"true", "1", "TRUE"} { + if _, err := ParseDSN("user:pass@tcp(127.0.0.1:3306)/db?rejectReadOnly=" + value); err != nil { + t.Errorf("rejectReadOnly=%s: %v (a DSN written for upstream should still parse)", value, err) + } + } + + for _, value := range []string{"false", "0", "FALSE"} { + _, err := ParseDSN("user:pass@tcp(127.0.0.1:3306)/db?rejectReadOnly=" + value) + if err == nil { + t.Errorf("rejectReadOnly=%s parsed without error; it states an expectation the driver will not meet", value) + continue + } + if !strings.Contains(err.Error(), "cannot be disabled") { + t.Errorf("rejectReadOnly=%s: error %q does not explain that the option is gone", value, err) + } + } + + if _, err := ParseDSN("user:pass@tcp(127.0.0.1:3306)/db?rejectReadOnly=yes"); err == nil { + t.Error("rejectReadOnly=yes parsed without error; a non-boolean value is still a malformed DSN") + } +} + +// TestReadOnlyErrorsAreBadConn pins the rejection itself: the three read-only +// error numbers must yield driver.ErrBadConn — which is what makes +// database/sql discard the connection and retry — with no configuration +// involved. Upstream gates this on an option that defaults to off. +func TestReadOnlyErrorsAreBadConn(t *testing.T) { + // 1792: ER_CANT_EXECUTE_IN_READ_ONLY_TRANSACTION + // 1290: ER_OPTION_PREVENTS_STATEMENT (returned by Aurora during failover) + // 1836: ER_READ_ONLY_MODE + for _, errno := range []uint16{1792, 1290, 1836} { + _, mc := newRWMockConn(0) + err := mc.handleErrorPacket(errPacket(errno, "read-only")) + if !errors.Is(err, driver.ErrBadConn) { + t.Errorf("errno %d returned %v, want driver.ErrBadConn", errno, err) + } + if !mc.closed.Load() { + t.Errorf("errno %d did not close the connection; database/sql would hand it back out", errno) + } + } + + // An unrelated error must still surface as itself. Widening the rejection + // to errors that are not about read-only would turn a real failure into a + // silent retry loop. + _, mc := newRWMockConn(0) + err := mc.handleErrorPacket(errPacket(1062, "Duplicate entry")) + var myErr *MySQLError + if !errors.As(err, &myErr) || myErr.Number != 1062 { + t.Errorf("errno 1062 returned %v, want a *MySQLError with Number 1062", err) + } + if mc.closed.Load() { + t.Error("errno 1062 closed the connection") + } +} + +// TestReadOnlyTxIsExempt covers the one case that must NOT be rejected: 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 — rejecting would replace a usable *MySQLError +// with a dead transaction. (Upstream's TestContextBeginReadOnly is the +// end-to-end version of this and passes unmodified.) +func TestReadOnlyTxIsExempt(t *testing.T) { + _, mc := newRWMockConn(0) + mc.inReadOnlyTx = true + + err := mc.handleErrorPacket(errPacket(1792, "Cannot execute statement in a READ ONLY transaction")) + var myErr *MySQLError + if !errors.As(err, &myErr) || myErr.Number != 1792 { + t.Errorf("inside an explicit read-only transaction, errno 1792 returned %v, want a *MySQLError", err) + } + if mc.closed.Load() { + t.Error("the connection was closed; the caller's own read-only transaction is not a demoted writer") + } + + // Once the transaction ends the exemption must end with it, or one + // read-only transaction disarms the protection for the rest of the + // connection's life. + mc.inReadOnlyTx = false + if err := mc.handleErrorPacket(errPacket(1792, "read-only")); !errors.Is(err, driver.ErrBadConn) { + t.Errorf("after the transaction, errno 1792 returned %v, want driver.ErrBadConn", err) + } +} + +// TestReadOnlyTxLifecycle drives the exemption flag through begin, Commit, +// Rollback and ResetSession, which is the only place it can actually go wrong +// and the reason the field exists. +// +// TestReadOnlyTxIsExempt above proves handleErrorPacket *reads* the flag +// correctly, but it sets the field itself, so it cannot tell a flag that is +// cleared from one that nobody ever clears. A flag stuck true silently +// disables the read-only rejection for the rest of the connection's life in +// the pool — the whole feature off, with no symptom. +func TestReadOnlyTxLifecycle(t *testing.T) { + // beginTx runs a scripted START TRANSACTION against a mock server and + // returns the connection with the flag in whatever state begin left it. + beginTx := func(t *testing.T, readOnly bool) (*mockConn, *mysqlConn, driver.Tx) { + t.Helper() + conn, mc := newRWMockConn(0) + conn.queuedReplies = [][]byte{okPacket()} + tx, err := mc.begin(readOnly) + if err != nil { + t.Fatalf("begin(%v): %v", readOnly, err) + } + return conn, mc, tx + } + + // rejects reports what a read-only error does on this connection now. + rejects := func(mc *mysqlConn) bool { + return errors.Is(mc.handleErrorPacket(errPacket(1792, "read-only")), driver.ErrBadConn) + } + + t.Run("a read-only transaction sets it", func(t *testing.T) { + _, mc, _ := beginTx(t, true) + if !mc.inReadOnlyTx { + t.Error("begin(true) did not set inReadOnlyTx; the caller's own read-only transaction would be rejected") + } + }) + + t.Run("a read-write transaction does not", func(t *testing.T) { + // This is the direction that disarms the feature: if begin set the + // flag unconditionally, any connection that had ever run a BeginTx + // would stop rejecting read-only errors. + _, mc, _ := beginTx(t, false) + if mc.inReadOnlyTx { + t.Error("begin(false) set inReadOnlyTx; an ordinary transaction would exempt the connection from read-only rejection") + } + if !rejects(mc) { + t.Error("a read-only error inside a read-write transaction was not rejected") + } + }) + + t.Run("Commit clears it", func(t *testing.T) { + conn, mc, tx := beginTx(t, true) + conn.queuedReplies = [][]byte{okPacket()} + if err := tx.Commit(); err != nil { + t.Fatalf("Commit: %v", err) + } + if mc.inReadOnlyTx { + t.Fatal("Commit left inReadOnlyTx set") + } + if !rejects(mc) { + t.Error("after Commit a read-only error was still exempt; one read-only transaction disarmed the connection for good") + } + }) + + t.Run("Rollback clears it", func(t *testing.T) { + conn, mc, tx := beginTx(t, true) + conn.queuedReplies = [][]byte{okPacket()} + if err := tx.Rollback(); err != nil { + t.Fatalf("Rollback: %v", err) + } + if mc.inReadOnlyTx { + t.Fatal("Rollback left inReadOnlyTx set") + } + if !rejects(mc) { + t.Error("after Rollback a read-only error was still exempt") + } + }) + + t.Run("ResetSession clears it", func(t *testing.T) { + // Belt and braces: every sql.Tx ends in Commit or Rollback, so this + // should be unreachable — but it is the point where a pooled + // connection's assumptions are re-established for a new borrower, and + // the stuck direction of this flag is the unsafe one. + _, mc, _ := beginTx(t, true) + if err := mc.ResetSession(context.Background()); err != nil { + t.Fatalf("ResetSession: %v", err) + } + if mc.inReadOnlyTx { + t.Error("ResetSession left inReadOnlyTx set; the next borrower inherits the exemption") + } + }) +} + +// okPacket builds a minimal OK packet: header (length, sequence 1), then the +// 0x00 marker, zero affected rows, zero last-insert-id, autocommit status and +// no warnings. Sequence 1 is what the server answers a freshly written command +// with, which is what mc.exec expects back. +func okPacket() []byte { + return []byte{7, 0, 0, 1, 0, 0, 0, 2, 0, 0, 0} +} + +// errPacket builds the ERR packet body handleErrorPacket expects: the 0xff +// marker, the error number, then a SQL state block and the message. +func errPacket(errno uint16, message string) []byte { + data := []byte{iERR, 0, 0} + binary.LittleEndian.PutUint16(data[1:3], errno) + data = append(data, '#') + data = append(data, []byte("HY000")...) + return append(data, []byte(message)...) +} diff --git a/transaction.go b/transaction.go index 8c502f49e..ad41ea2c4 100644 --- a/transaction.go +++ b/transaction.go @@ -24,6 +24,7 @@ func (tx *mysqlTx) Commit() (err error) { return } err = tx.mc.exec("COMMIT") + tx.mc.inReadOnlyTx = false tx.mc = nil return } @@ -40,6 +41,7 @@ func (tx *mysqlTx) Rollback() (err error) { return } err = tx.mc.exec("ROLLBACK") + tx.mc.inReadOnlyTx = false tx.mc = nil return }