Skip to content

Use the block-mysql driver - #24

Merged
morgo merged 3 commits into
mainfrom
driver/block-mysql
Sep 6, 2026
Merged

Use the block-mysql driver#24
morgo merged 3 commits into
mainfrom
driver/block-mysql

Conversation

@morgo

@morgo morgo commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

github.com/block/mysql is Block's fork of go-sql-driver/mysql. It is moving to
its own module path in block/mysql#3, because a replace directive is not
inherited across module boundaries — a consumer building against a library that
depends on the fork would silently link upstream instead.

polt reaches the driver through spirit's dbconn, and spirit is moving with it
(block/spirit#1219), so polt follows. Keeping one driver in the binary also keeps
one *mysql.MySQLError type: with both linked, errors.As across the boundary
returns false silently.

The change

Two mechanical substitutions across 12 files:

  • import path github.com/go-sql-driver/mysqlgithub.com/block/mysql
  • sql.Open("mysql", ...)sql.Open("block-mysql", ...)

There is only one import form in the tree (no aliases), and only one non-test
file: pkg/test/test.go.

No API had to change. pkg/test's SetupDB(cfg *mysql.Config, ...) reads
User/Passwd/Addr/DBName off the config to build a DSN string rather than
handing the struct to spirit, so no driver type crosses a module boundary. (strata
does hand a *mysql.Config to applier.Target, which is why strata does not
compile until spirit moves; polt has no such site.) There are no MySQLError
assertions anywhere in polt.

go-sql-driver leaves go.mod entirely — not even as an indirect dependency —
so polt is single-driver after this.

Pins

Both are on merged code — nothing outstanding:

Module Pin Merged as
github.com/block/mysql v0.0.0-20260906201522-a3178f8dca69 block/mysql#3, head of master
github.com/block/spirit v0.17.1-0.20260906214441-dc3d4c9f4c3b block/spirit#1219, ancestor of main

Verification

go build ./..., go vet ./... and gofmt clean, with no residual references to
the old import path or driver name. README's Dependencies section now names the fork. Full suite against MySQL 8.0.44 — all 8
packages pass (archive, audit, boot, parquet, query, runner, stage,
upload).

block/mysql is Block's fork of go-sql-driver/mysql, moving to its own module
path (block/mysql#3) because a `replace` directive is not inherited across
module boundaries. polt reaches the driver through spirit's dbconn, and spirit
is moving with it (block/spirit#1219), so polt follows to keep one driver — and
one set of driver types — in the binary.

Mechanical: the import path, and `sql.Open("mysql", ...)` -> `"block-mysql"`.
Nothing crosses a type boundary here; pkg/test's SetupDB reads fields off
*mysql.Config to build a DSN string rather than handing the struct to spirit,
so no API had to change. go-sql-driver leaves go.mod entirely, including as an
indirect dependency.

The block/mysql and block/spirit pins point at unmerged PR branches and must be
re-pointed at merged commits before this merges.

Verified: build, vet and gofmt clean; all 8 packages pass against MySQL 8.0.44.
Copilot AI lite review requested due to automatic review settings September 6, 2026 19:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The go.mod pins github.com/block/mysql and github.com/block/spirit to pseudo-versions that appear to track unmerged PR branches and should be repointed to merged commits/tags before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR migrates polt from github.com/go-sql-driver/mysql to Block’s fork (github.com/block/mysql) and updates MySQL driver usage to ensure the intended driver is linked across module boundaries (including consistent MySQLError typing).

Changes:

  • Replaces imports of github.com/go-sql-driver/mysql with github.com/block/mysql across tests and pkg/test.
  • Updates sql.Open("mysql", ...) calls to sql.Open("block-mysql", ...) across the test suite.
  • Updates module dependencies to include github.com/block/mysql and newer github.com/block/spirit (plus associated go.sum churn).
File summaries
File Description
pkg/test/test.go Switches driver import and updates RunSQL to open the block-mysql driver.
pkg/stage/stager_test.go Updates MySQL driver import and sql.Open driver name.
pkg/runner/stage_runner_test.go Updates MySQL driver import and sql.Open driver name.
pkg/runner/runner_test.go Updates sql.Open driver name to block-mysql.
pkg/runner/archive_runner_test.go Updates MySQL driver import and sql.Open driver name.
pkg/query/validator_test.go Updates MySQL driver import and sql.Open driver name.
pkg/query/expression_folder_test.go Updates MySQL driver import and sql.Open driver name (including DSN query params case).
pkg/query/explain_test.go Updates sql.Open driver name to block-mysql.
pkg/parquet/write_buffer_test.go Updates sql.Open driver name to block-mysql.
pkg/parquet/arrow_schema_test.go Updates sql.Open driver name to block-mysql.
pkg/boot/stage_booter_test.go Updates MySQL driver import and sql.Open driver name (incl. replica/open-invalid-DSN cases).
pkg/boot/booter_test.go Updates MySQL driver import for DSN parsing compatibility.
pkg/boot/archive_booter_test.go Updates MySQL driver import and sql.Open driver name.
pkg/audit/db_test.go Updates sql.Open driver name to block-mysql.
pkg/archive/table_test.go Updates MySQL driver import and sql.Open driver name.
pkg/archive/file_test.go Updates MySQL driver import and sql.Open driver name.
pkg/archive/buffer_stager_test.go Updates MySQL driver import and sql.Open driver name.
go.mod Replaces go-sql-driver with block/mysql; updates spirit pin; also bumps testify.
go.sum Updates checksums to reflect dependency migration and version changes.
Review details

Suppressed comments (1)

go.mod:20

  • go.mod pins github.com/block/spirit to a pseudo-version that appears to reference an unmerged PR branch; this should be updated to a merged commit/tag before merging to avoid depending on a mutable branch reference.
	github.com/block/spirit v0.17.1-0.20260906193437-6f267e49110f
  • Files reviewed: 18/19 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread go.mod Outdated
github.com/aws/aws-sdk-go-v2/config v1.29.1
github.com/aws/aws-sdk-go-v2/service/s3 v1.73.2
github.com/go-sql-driver/mysql v1.10.0
github.com/block/mysql v0.0.0-20260906185715-6346a1ec80d2
Comment thread go.mod
Comment on lines 12 to 15
github.com/siddontang/loggers v1.0.3
github.com/sirupsen/logrus v1.9.3
github.com/stretchr/testify v1.12.0
github.com/stretchr/testify v1.12.1
golang.org/x/sync v0.22.0
@morgo
morgo marked this pull request as ready for review September 6, 2026 20:26
@aparajon

aparajon commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

🤖 Adversarial correctness review784441b1 (+73/-90, 19 files)

Unlike the sibling change in block/vitess, this one actually reaches the single-driver state the body claims, and that is worth establishing first because it is what makes a 31-site mechanical rename safe. go list -m all has no go-sql-driver entry at all — not direct, not indirect — and I probed the built test binary rather than inferring from go.mod:

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

So a site you missed cannot silently bind to upstream; it fails at the sql.Open. I checked that this is real rather than theoretical by mutating one back — pkg/audit/db_test.go to sql.Open("mysql", …), image rebuilt — and all three tests in the package fail with exactly that error. Every go.mod line in the diff is MVS doing its job rather than hand-editing, and every claim in the body about polt's own surface checks out: no MySQLError, no errors.As, no Register* call anywhere in the repo, and no driver type crossing into spirit.

What I would change before merge is a pin and a README line.

# Sev Where What
1 low-med go.mod:20, PR body The spirit pin is still on the open block/spirit#1219 branch, and the body's "Before merge" list is stale — it names both pins at their pre-784441b SHAs and gates on leaving draft, which has already happened
2 low README.md:58 The Dependencies section still tells readers polt relies on go-sql-driver, which after this PR is not in the module graph at all

1 — one of the two pins is discharged and the body no longer says which (low-med)

784441b did the block/mysql half properly: a3178f8d is the squash-merge of block/mysql#3 and the current head of that repo's master, so that pin is on merged code. The spirit half can't be discharged yet — v0.17.1-0.20260906201921-2d412cd16671 is the head of the still-open block/spirit#1219, on branch driver/block-mysql, 2 ahead and 0 behind spirit main.

The body hasn't caught up. Its "Before merge" section still lists both pins, at their pre-784441b SHAs:

Both must be re-pointed at merged commits before this leaves draft.

Neither SHA is in go.mod any more, one of the two items is already done, and the gate it states has been passed — the PR is out of draft and up for review. A reviewer reading top-down can't tell what's actually outstanding, which is the whole job of that section.

I went looking for what breaks if this merges ahead of #1219, and the obvious fear turns out not to be the risk, so it's worth writing down which one is:

  • Not a build break. proxy.golang.org already has the pseudo-version cached (.info returns 200) and proxy entries are immutable, so resolution is permanent. delete_branch_on_merge is false on block/spirit, so driver/block-mysql survives the merge regardless.
  • The cost is silent divergence. spirit squash-merges — I checked the last ten merged PRs and merge_commit_sha != head.sha in every one — so once #1219 lands, the same change is on main under a different SHA and polt's main is pinned to a commit that is not an ancestor of spirit main. polt then stops tracking spirit from a point nobody can git log to, and every later spirit commit is invisible until someone re-pins by hand.
  • And it leaks through MVS. v0.17.1-0.2026… sorts above v0.17.0 and below a future v0.17.1 tag, so any module that composes polt with an older spirit silently gets the PR-branch build.

Re-pin after #1219 merges, and rewrite the list to name only the outstanding pin.

2 — the Dependencies section still points at the old driver (low)

## Dependencies
* Relies on [mysql-client-driver](https://github.com/go-sql-driver/mysql) for connecting to MySQL databases.
* Relies on [Spirit](https://github.com/block/spirit) for chunking the data ...

This is the only remaining reference to go-sql-driver anywhere in the repo — one grep hit, no others in Go files, workflows, compose or scripts. It is also the one place a reader is now told something false: after this PR upstream is absent from go.mod, go.sum and go list -m all. Pointing it at github.com/block/mysql and saying in a clause that it's Block's fork of go-sql-driver is the whole fix, and it sits two lines above the Spirit bullet that the same PR effectively updates.

Same category, not its own row: "block-mysql" is now a literal in 31 places across 16 test files, and 26 of them are the identical pair —

db, err := sql.Open("block-mysql", test.DSN())
require.NoError(t, err)

Every one of those files already imports pkg/test, which already owns DSN(). A test.OpenDB(t) *sql.DB there would put the driver name and the DSN in the one place that should hold both, and would make the next driver move a one-line change rather than a 31-line one. Cheap to do while all the sites are open anyway; a package-level const driverName is the smaller version if the helper feels like scope creep.


Verified — the local runs, the mutation, and four attacks that dissolved

Local: go build ./..., go vet ./... and gofmt -l all clean. go mod tidy is a no-op on this tree, so the module files are exactly what the toolchain produces. The suite ran the way CI runs it (compose up mysql test, MySQL 8.0.34): all 8 packages green at HEAD from a cold database and a fresh image, and green again warm. I also ran the same cold suite at origin/main for a baseline — also green.

Mutation. pkg/audit/db_test.go sql.Open("block-mysql", …)"mysql", image rebuilt: killed, all three tests failing with sql: unknown driver "mysql" (forgotten import?). So the rename is self-verifying here — this is the property block/vitess has and block/mysql itself does not, and it is what makes reviewing 31 call sites by grep rather than by eye defensible.

⚠️ One flake worth knowing about, not caused by this PR: on the very first cold run I saw TestStageBooter_Setup fail at stage_booter_test.go:42 (TableExists("polt", "runs") false), and it did not reproduce in three subsequent cold and warm runs, at HEAD or at base. The mechanism is pre-existing and independent of the driver: test.TableExists does _ = db.QueryRowContext(...).Scan(&count) and discards the error, so any transient query failure under concurrent package load reads as "table absent" rather than as an error. Worth a require.NoError there sometime — a swallowed error in a boolean helper is exactly the shape that turns a resource problem into a mystery assertion failure.

Attack that dissolved: the go.mod churn beyond the driver swap. A testify bump and six golang.org/x/* bumps inside a driver PR are worth questioning; all of it is MVS. spirit@2d412cd1 requires testify v1.12.1 (raising polt's direct v1.12.0) and golang.org/x/text v0.41.0; testify v1.12.1 in turn swapped gopkg.in/yaml.v3 for go.yaml.in/yaml/v3 v3.0.5, which is why yaml.v3 leaves and kr/pretty and rogpeppe/go-internal — its gopkg.in/check.v1 test chain — go with it. That accounts for the whole 55-line go.sum movement, including the net shrink.

Attack that dissolved: an exported signature whose type identity changed. pkg/test.SetupDB(cfg *mysql.Config, …) is exported from a public module, and *mysql.Config is a different type after the module path change — a source break for any external importer. Public code search for github.com/block/polt/pkg returns 5 hits, all inside block/polt itself, so there is nothing to break. (Incidentally SetupDB rebuilds the DSN with fmt.Sprintf("%s:%s@tcp(%s)/%s", …) where cfg.FormatDSN() would do it without dropping params or mangling a password containing @ or / — pre-existing and test-only, but it is the reason this file needs the driver import at all.)

Attack that dissolved: polt's production driver isn't actually set by this diff. Only one non-test file changed, and no production path calls sql.Open — everything goes through spirit's dbconn.New (pkg/runner/db_helper.go:41). So polt's real driver is spirit's choice, and dropping go-sql-driver from go.mod would be a runtime failure if the pinned spirit still opened "mysql". It doesn't: all 14 non-test sql.Open sites in spirit@2d412cd1 say "block-mysql" and none say "mysql", and its four remaining go-sql-driver strings are all comments (dbconn/conn.go:324,342, copier/buffered.go:195, migration/check/version.go:19). The pin and the driver drop are consistent.

Leak check: clean. Nothing internal in the diff. The body names strata and its applier.Target call, but applier.Target with its Config *mysql.Config field is a public symbol in block/spirit/pkg/applier, and strata is already named in public Block source, so the only thing stated is that a Block consumer calls a public spirit API — no new disclosure.

The probe file was moved out of the tree and the mutation restored from backup; the worktree is clean at 784441b1.

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

@aparajon aparajon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Approving. Two low-severity items in the review above: the spirit pin needs re-pointing once block/spirit#1219 merges, and README.md:58 still names go-sql-driver. Neither is a correctness problem — the substitution is complete and self-verifying (mutation killed, full suite green cold and warm).

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

… fork

block/spirit#1219 squash-merged as dc3d4c9f, so the pin moves off the PR
branch and onto a commit that is an ancestor of spirit main.

The Dependencies section still named go-sql-driver, which after this change
is not in polt's module graph at all.
@morgo
morgo enabled auto-merge (squash) September 6, 2026 21:47
@morgo
morgo merged commit 9c18be0 into main Sep 6, 2026
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants