Fix EXPLAIN on MySQL 9 and MariaDB, add live-database integration tests - #18
Merged
Conversation
The explain package failed on every query against MySQL 9 and MariaDB. Three independent defects, all in analyzeMySQL, all invisible to unit tests because they concern what a real server actually returns: - MySQL 9 defaults @@explain_format to TREE, so a plain EXPLAIN returns one free-text column instead of the twelve-column plan the code scanned. Request EXPLAIN FORMAT=TRADITIONAL, which MySQL 5.7/8/9 and MariaDB all accept, so no server version detection is needed. - MariaDB emits ten plan columns where MySQL emits twelve (no partitions, no filtered), breaking the positional Scan. Address columns by name. - MySQL and MariaDB reject every statement inside a READ ONLY transaction with error 1792, including an EXPLAIN that only plans it, so WithAllowDML never worked there. Use a regular transaction for DML; safety still rests on validate(), on plain EXPLAIN never executing the statement, and on the unconditional rollback. Postgres allows it and keeps its read-only tx. Also drops a false positive: a UNION RESULT row names a temporary table (<union1,2>) with type=ALL and no key, and was reported as an unindexed full table scan. validate() now returns the statement kind so the MySQL path can choose its transaction mode, and the per-row rules move into mysqlRowIssues to keep analyzeMySQL under the gocyclo limit. test/integration is a new unpublished module that runs explain against live PostgreSQL 18.3, MySQL 9.7.1 and MariaDB 12.3.2 via docker compose, behind an `integration` build tag so `go test ./...` stays Docker-free. Verified these are real guards: with the fix reverted, every MySQL and MariaDB case fails and every Postgres case passes. Separately, the satellite modules were compiling against the *published* core rather than the working tree, because `make release-prep` had stripped their replace directives and nothing restored them. A breaking change to analyzer/ or middleware/ would have passed CI green. A committed go.work now handles local resolution for all ten modules; no go.mod carries a replace, and release-prep is removed in favour of the manual steps in CONTRIBUTING. Consumers are unaffected (GOWORK=off reproduces their build).
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 59 minutes. |
Contributor
WalkthroughThis PR reworks MySQL/MariaDB EXPLAIN handling in ChangesEXPLAIN fixes, workspace wiring, integration test suite, and docs
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@explain/explain_test.go`:
- Line 34: The test around p.validate(c.query) is discarding the returned
statement kind, so update the table-driven cases to assert kind alongside safe
and err; add wantKind expectations for existing statements and CTE-wrapped DML
where supported, using the validate function’s returned kind to catch fallback
classification regressions. Also include a case that exercises the failure path
for the new behavior where practical so the test proves both accepted and
rejected classification outcomes.
In `@explain/explain.go`:
- Line 247: The BeginTx call in explain.go currently flips transaction mode with
ReadOnly: !dml, which breaks the EXPLAIN safety invariant. Update the EXPLAIN
flow in the relevant transaction setup so it always uses a read-only transaction
in the EXPLAIN path, and keep the always-Rollback behavior intact in the
surrounding EXPLAIN logic. If DML EXPLAIN is not supported under a read-only
transaction, have the EXPLAIN handling in the same code path return an error
instead of relaxing the transaction to read-write.
- Around line 261-268: The tabular EXPLAIN path in explain.go should fail closed
when the expected columns are missing. After rows.Columns() in the EXPLAIN
parsing logic, validate that the required tabular fields such as table are
present in the index map before continuing; if FORMAT=TRADITIONAL returns an
incompatible shape, return an error instead of proceeding with an empty
col("table"). Update the column-validation logic near the rows.Columns()
handling and keep the existing EXPLAIN row parsing flow guarded by this check.
In `@test/integration/go.mod`:
- Around line 13-14: Upgrade the dependency entry for github.com/jackc/pgx/v5 in
the test/integration go.mod module to v5.9.2 or later to address the reported
vulnerability, and keep the github.com/go-sql-driver/mysql requirement as-is
unless you also choose to bump it to the available v1.10.0. Make sure the
version change is applied in the go.mod dependency list so any integration tests
importing pgx/v5 resolve the patched release.
In `@test/integration/integration_test.go`:
- Around line 65-72: The helper exec currently calls db.ExecContext with
context.Background(), so hanging setup statements can block until the test suite
timeout. Update exec to use testCtx(t) or a short per-statement context with a
deadline when executing each statement, so failures surface as context deadline
errors; keep the fix localized in exec and preserve the existing t.Helper and
t.Fatalf behavior.
In `@test/integration/mysql_test.go`:
- Around line 179-180: The cleanup in the MySQL integration test uses exec,
which calls t.Fatalf and can panic when invoked from t.Cleanup in Go 1.21+.
Update the restore path in the test around the SET GLOBAL explain_format setup
to avoid fataling inside the cleanup closure; instead, perform the restore with
inline error handling and log failures with t.Logf (or equivalent) so a failed
cleanup does not obscure the test outcome.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ba412e75-59e1-45fb-8522-3ca3ffd35ff5
⛔ Files ignored due to path filters (6)
go.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumintegrations/gormguard/go.sumis excluded by!**/*.sumintegrations/pgxguard/go.sumis excluded by!**/*.sumparsers/pgparser/go.sumis excluded by!**/*.sumtest/integration/go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
.coderabbit.yaml.github/workflows/ci.yml.gitignoreAGENTS.mdCHANGELOG.mdCONTRIBUTING.mdMakefileREADME.mdexplain/explain.goexplain/explain_test.gointegrations/gormguard/go.modintegrations/pgxguard/go.modparsers/pgparser/go.modtest/integration/docker-compose.ymltest/integration/go.modtest/integration/integration_test.gotest/integration/mysql_test.gotest/integration/postgres_test.go
| // (error 1792), including an EXPLAIN that only plans it. Fall back to a | ||
| // regular transaction for DML: plain EXPLAIN never executes the statement, | ||
| // and the rollback below is what guarantees nothing is committed. | ||
| tx, err := p.db.BeginTx(ctx, &sql.TxOptions{ReadOnly: !dml}) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ���️ Heavy lift
Keep EXPLAIN transactions read-only.
ReadOnly: !dml makes opt-in DML use a read-write transaction, weakening the documented EXPLAIN safety invariant. If MySQL/MariaDB require this exception, update the repository policy explicitly; otherwise keep the transaction read-only and handle unsupported DML EXPLAIN as an error.
As per path instructions, explain/** must “keep the read-only BeginTx + always-Rollback” and “EXPLAIN must never execute the statement.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@explain/explain.go` at line 247, The BeginTx call in explain.go currently
flips transaction mode with ReadOnly: !dml, which breaks the EXPLAIN safety
invariant. Update the EXPLAIN flow in the relevant transaction setup so it
always uses a read-only transaction in the EXPLAIN path, and keep the
always-Rollback behavior intact in the surrounding EXPLAIN logic. If DML EXPLAIN
is not supported under a read-only transaction, have the EXPLAIN handling in the
same code path return an error instead of relaxing the transaction to
read-write.
Source: Path instructions
Comment on lines
+179
to
+180
| exec(t, db, `SET GLOBAL explain_format = 'TREE'`) | ||
| t.Cleanup(func() { exec(t, db, `SET GLOBAL explain_format = '`+original+`'`) }) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
t.Fatalf inside t.Cleanup will panic.
exec calls t.Fatalf on error, but in Go 1.21+ calling Fatal/Fatalf from a Cleanup function panics with "test executed Fatal(...) method in Cleanup function". If the SET GLOBAL restore fails (e.g., container stopped), the panic obscures the real test result. Handle the error inline with t.Logf instead.
🐛 Proposed fix
exec(t, db, `SET GLOBAL explain_format = 'TREE'`)
- t.Cleanup(func() { exec(t, db, `SET GLOBAL explain_format = '`+original+`'`) })
+ t.Cleanup(func() {
+ _, err := db.ExecContext(context.Background(), `SET GLOBAL explain_format = '`+original+`'`)
+ if err != nil {
+ t.Logf("warning: failed to restore @@explain_format to %q: %v", original, err)
+ }
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| exec(t, db, `SET GLOBAL explain_format = 'TREE'`) | |
| t.Cleanup(func() { exec(t, db, `SET GLOBAL explain_format = '`+original+`'`) }) | |
| exec(t, db, `SET GLOBAL explain_format = 'TREE'`) | |
| t.Cleanup(func() { | |
| _, err := db.ExecContext(context.Background(), `SET GLOBAL explain_format = '`+original+`'`) | |
| if err != nil { | |
| t.Logf("warning: failed to restore @@explain_format to %q: %v", original, err) | |
| } | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/integration/mysql_test.go` around lines 179 - 180, The cleanup in the
MySQL integration test uses exec, which calls t.Fatalf and can panic when
invoked from t.Cleanup in Go 1.21+. Update the restore path in the test around
the SET GLOBAL explain_format setup to avoid fataling inside the cleanup
closure; instead, perform the restore with inline error handling and log
failures with t.Logf (or equivalent) so a failed cleanup does not obscure the
test outcome.
Applies four of six review findings; the other two rest on premises that do not hold against the current code. Applied: - explain: fail closed when the MySQL EXPLAIN plan lacks an expected column. Previously a plan of the wrong shape made every col() lookup return "" and Analyze reported zero issues. Verified by pointing the analyzer at a plain EXPLAIN on MySQL 9, which now errors with `unrecognized MySQL EXPLAIN plan (missing "table" column; got [EXPLAIN])` rather than silently passing. - explain: assert the statement kind in TestValidate, including CTE-wrapped DML. The kind drives isDML, which picks the transaction mode, so a classification regression would quietly reintroduce MySQL error 1792. - test/integration: give each statement in exec its own deadline, so a wedged server surfaces as a context error instead of hanging to the suite timeout. Uses a standalone context rather than testCtx so exec stays safe in cleanups. - test/integration: pgx v5.7.6 -> v5.10.0, mysql v1.9.3 -> v1.10.0. Not a security fix — govulncheck reports no advisory against v5.7.6, called or otherwise. pgxguard already requires pgx v5.10.0, so the workspace resolved it there while go.mod said v5.7.6; the module now resolves identically with and without GOWORK. Not applied: - "BeginTx ReadOnly: !dml breaks the EXPLAIN safety invariant; error out instead." Doing so removes WithAllowDML on MySQL and MariaDB entirely: both reject every statement in a READ ONLY transaction with error 1792, including a planning-only EXPLAIN. That was the bug this branch fixes. The invariant the code actually guarantees — the statement is never executed — is upheld by validate(), by never using EXPLAIN ANALYZE, and by the unconditional rollback, and is covered by TestMySQL_ExplainDMLDoesNotMutate. - "t.Fatalf in t.Cleanup can panic on Go 1.21+." It does not. Verified on Go 1.26: a Fatalf inside a cleanup fails the test normally and does not skip the other registered cleanups. Failing loudly is also right here, since a failed restore leaves @@explain_format mutated on a shared server.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The explain package failed on every query against MySQL 9 and MariaDB. Three independent defects, all in analyzeMySQL, all invisible to unit tests because they concern what a real server actually returns:
Also drops a false positive: a UNION RESULT row names a temporary table (<union1,2>) with type=ALL and no key, and was reported as an unindexed full table scan.
validate() now returns the statement kind so the MySQL path can choose its transaction mode, and the per-row rules move into mysqlRowIssues to keep analyzeMySQL under the gocyclo limit.
test/integration is a new unpublished module that runs explain against live PostgreSQL 18.3, MySQL 9.7.1 and MariaDB 12.3.2 via docker compose, behind an
integrationbuild tag sogo test ./...stays Docker-free. Verified these are real guards: with the fix reverted, every MySQL and MariaDB case fails and every Postgres case passes.Separately, the satellite modules were compiling against the published core rather than the working tree, because
make release-prephad stripped their replace directives and nothing restored them. A breaking change to analyzer/ or middleware/ would have passed CI green. A committed go.work now handles local resolution for all ten modules; no go.mod carries a replace, and release-prep is removed in favour of the manual steps in CONTRIBUTING. Consumers are unaffected (GOWORK=off reproduces their build).Summary
What does this PR change, and why?
Closes #
Type of change
Checklist
make cipasses (fmt-check, vet, lint, test-race) across all modulesREADME.md,AGENTS.md,.sqlguard.example.yml)## [Unreleased]inCHANGELOG.mdanalyzer/middleware/reporterResult)Notes for reviewers
Anything reviewers should focus on — tricky areas, trade-offs, follow-ups.
Summary by CodeRabbit
New Features
EXPLAINhandling with optional DML analysis support and clearer plan output behavior.Bug Fixes
UNION-related temporary tables and better handled read-only transaction behavior.Documentation