mysqltopo: use the block-mysql driver

[morgo]

Part of the block/mysql#3 wave. Draft — the dependency is pinned to that PR's branch and must be re-pointed at a master commit before merging.

Why

strata links github.com/block/mysql, Block's fork of go-sql-driver/mysql, for capabilities upstream doesn't carry (QueryResultContext, Warnings()). That fork is moving from a replace directive to its own module path, because replace isn't inherited across module boundaries and so can't reach consumers of a library built on it.

Once the module path differs, a binary linking both packages has two distinct *mysql.MySQLError types, and errors.As across that boundary returns false silently rather than failing loudly.

That matters here specifically. convertError depends on exactly that assertion:

• 1062 → NodeExists
• 1213 / 1205 → Timeout

And strata's own test helpers assert on errors this package returns. If mysqltopo stays on upstream while strata moves to the fork, those mappings quietly stop matching — the failure mode is a retry loop that never fires, not a compile error.

Scope

Deliberately mysqltopo only:

• server.go, notification.go — import path and sql.Open driver name
• main_test.go, notification_test.go, server_error_test.go, server_resolve_test.go

mysqltopo is Block-added, so editing it costs nothing when merging upstream vitess. The remaining go-sql-driver/mysql imports in this repo are all in upstream-owned files (go/test/endtoend/…, go/vt/vtgate/semantics/info_schema_gen_test.go); touching those would create permanent merge conflicts, so they keep using upstream. The module stays in go.mod and both coexist.

Note the two files alias the import differently — server.go takes it as plain mysql, notification.go as mysqldriver, because there mysql is vitess's own go/mysql. Also RegisterTLSConfig("rds-topo", …) and the sql.Open it feeds have to move together, since the TLS registry is package-level state; both are in server.go, so they did.

Verification

go/vt/topo/mysqltopo tests pass against MySQL 8.0.44.

🤖 Generated with Claude Code

[aparajon]

🤖 Adversarial correctness review — 3cb54ef7 (+22/-15, 8 files)

The mechanical part is complete and I checked it rather than counting on the diff being uniform: all five sql.Open sites in mysqltopo now name block-mysql, the four import rewrites cover every file in the package that touched the driver, and after the change no non-test file anywhere in the repo imports upstream go-sql-driver — the one remaining go-sql-driver/mysql string outside tests is the new comment in server.go:54 itself.

The check that matters most for a rename like this is what happens to a site you miss, and here it fails loudly. I probed the package's test binary directly:

PROBE drivers=[block-mysql]
PROBE open_mysql_err=sql: unknown driver "mysql" (forgotten import?)

Nothing links upstream into this package, so a stray sql.Open("mysql", …) can't silently bind to the wrong driver — it fails at the call. And tools/unit_test_runner.sh selects packages with go list ./go/... | grep -v endtoend, so go/vt/topo/mysqltopo is in the unit-test job and would surface it.

#	Sev	Where	What
1	low	go/vt/topo/mysqltopo/server.go:54	The new comment justifies the import with a whole-binary property that this repo doesn't have and convertError doesn't need
2	low	go/vt/topo/mysqltopo/server.go:546	The comment directly above the *mysql.MySQLError assertion still says "go-sql-driver returns", contradicting the new comment 490 lines up in the same file

---

1 — the comment picks a global premise where a local one is both true and sufficient (low)

// Block's fork of go-sql-driver/mysql, registered as "block-mysql". strata
// links the fork for capabilities upstream does not carry; using it here
// too keeps one driver — and so one *mysql.MySQLError type — in the binary,
// which is what convertError below depends on.
"github.com/block/mysql"

Adding a comment here is right — an import whose path doesn't match the package name it binds deserves one. Two things about this particular justification.

It isn't true of this repo. Eight test files still import upstream, and go.mod:23 still carries github.com/go-sql-driver/mysql v1.9.3 in the direct require block because of them:

go/vt/vtgate/semantics/info_schema_gen_test.go
go/test/endtoend/preparestmt/main_test.go
go/test/endtoend/vtcombo/vttest_sample_test.go
go/test/endtoend/vtcombo/onlineddl/onlineddl_test.go
go/test/endtoend/mysqlserver/mysql_server_test.go
go/test/endtoend/vtgate/{keyspace_watches,queries/misc,foreignkey}/*_test.go

Production binaries do link only the fork, which is the useful half of the claim — but "one driver in the binary" as stated is a property no one is maintaining, and mysqltopo is registered as a topo plugin across vtgate, vttablet, vtctld, vtorc and topo2topo, so a future test that pairs one of those with an upstream import would falsify it without anyone noticing.

And it isn't what convertError needs. convertError has 63 call sites, all inside this package, all on errors from handles opened by connect / newNotificationSystem — both of which now open block-mysql. That is a local invariant: nothing in this package imports upstream, so every error reaching the errors.As came from the fork's driver. It holds regardless of what the rest of the binary links, it's checkable in one directory, and it's the thing that would actually be violated if someone later opened a connection here with a different driver. Stating that instead makes the comment both correct and enforceable — something like "every *sql.DB in this package is opened with this driver, so convertError's *mysql.MySQLError assertion always sees the fork's type."

2 — the other comment about the same type wasn't updated (low)

// Handle MySQL-specific errors. go-sql-driver returns *mysql.MySQLError,
// which carries the server error number directly; ...
var driverErr *mysql.MySQLError

This is the comment attached to the assertion that the new comment at :54 exists to protect, and it still attributes the type to go-sql-driver. After this PR the file says both things: that the import is deliberately not go-sql-driver, and that the error type comes from go-sql-driver. A reader chasing the errors.As lands on the second one. block-mysql returns *mysql.MySQLError is the whole fix.

Same category, not worth its own row: "block-mysql" is now a literal in five places in this package. A package-level const driverName = "block-mysql" gives the next edit one place to be wrong instead of five. Weak as merge-drift protection — mysqltopo is Block-only, so upstream vitess merges never touch it — but it also documents the coupling to an external module's registration in one spot.

---

Verified — the pin, the TLS registry, and three attacks that dissolved

The pseudo-version pin is real and current. github.com/block/mysql v0.0.0-20260906201522-a3178f8dca69 resolves to a3178f8d, which is the squash-merge of block/mysql#3 and the current head of that repo's master — so this doesn't depend on an unmerged commit, and the merge-order constraint that would otherwise apply to this PR is already discharged.

⭐ The TLS registry is fully on one side, which is the half-migration that would have failed most confusingly. RegisterTLSConfig is called exactly once in the whole repo (server.go:189, key rds-topo), and both consumers of that key (server.go:273, notification.go:209) are in the same package. Registries in this driver are package-level globals, so registering rds-topo on one package while connecting with the other yields invalid value / unknown config name: rds-topo at DSN-parse time rather than anything about drivers. Nothing here is split.

Attack that dissolved: a *mysql.Config crossing the package boundary. connect(cfg *mysql.Config) and connectResolved take the fork's Config, whose type identity changed with the module path — but both are unexported, and no exported symbol in mysqltopo mentions a driver type. So this PR can't break a caller at compile time, and squareup/strata's companion change is its own driver switch rather than fallout from this one.

Attack that dissolved: the unrelated filippo.io/edwards25519 v1.1.1 → v1.2.0 bump. An indirect crypto-library bump inside a driver-swap PR is worth questioning; this one is MVS doing its job. block/mysql's own go.mod has require filippo.io/edwards25519 v1.2.0, one minor ahead of what upstream go-sql-driver pinned, so adding the fork raises it. No manual edit.

Attack that dissolved: an aliasing mistake around the two mysql packages. notification.go keeps mysqldriver because it also imports vitess.io/vitess/go/mysql; server.go imports the fork unaliased and only pulls vitess.io/vitess/go/mysql/sqlerror, so there's no shadowing. go build and go vet are clean on the package.

Local: go build ./go/vt/topo/mysqltopo/... and go vet clean. I could not run the package's tests here — TestMain needs vitess's built bin/mysqlctl (or MYSQL_TEST_ADDR pointed at a server with CREATE DATABASE), and neither is available in this checkout — so the driver-registration probe above is the substitute, and it was run in the real test binary. The probe file has been removed; the worktree is clean at 3cb54ef7.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Approving — the switch is complete (all five sql.Open sites, no non-test upstream import left in the repo), the pin resolves to block/mysql#3's merge commit on master, and the TLS registry is entirely on one side. Probed the package's test binary: it registers only block-mysql, so a missed call site fails loudly rather than binding upstream.

Two low findings in my review comment — both about the comments, not the code: the new import comment rests on a whole-binary premise this repo doesn't hold (eight test files still import upstream) when the local one is true and sufficient, and the comment on the *mysql.MySQLError assertion still credits go-sql-driver.

This stamp was left by Claude Code (claude-opus-5).