Stop using the *sql.DB pool; use a single *sql.Conn#6
Merged
Conversation
myschema is a one-shot CLI: each invocation runs one plan/apply/dump against one connection. The default *sql.DB pool buys nothing here and silently breaks session-scoped statements (e.g. `USE <db>`) when the driver hands the next query to a fresh connection. Switch every layer to *sql.Conn: - client.connect(ctx) opens a *sql.DB, caps it to one connection (SetMaxOpen/IdleConns(1)), then immediately reserves a *sql.Conn. A small wrapper closes both on caller's defer. - catalog.Catalog stores *sql.Conn instead of *sql.DB. - apply/plan/dump/diff_all pass conn.Conn into catalog.NewCatalog and use conn.ExecContext for DDL. - testutil.ConnectDB returns *sql.Conn (bound to a 1-conn pool) and registers cleanups for both the conn and its parent DB. - testutil.SetupDB now executes DROP/CREATE/USE on the same conn, so USE actually carries over to the test body. All existing tests + the smoke scenario pass against docker compose MySQL 8.0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6 +/- ##
==========================================
- Coverage 32.58% 32.14% -0.45%
==========================================
Files 20 20
Lines 1295 1313 +18
==========================================
Hits 422 422
- Misses 787 806 +19
+ Partials 86 85 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
winebarrel
added a commit
that referenced
this pull request
May 2, 2026
Seven inline comments, all valid. Fixes:
parser/directive.go
- Whitespace tolerance: classifyInlineLine() now goes through
strings.Fields/tokenize() so tabs and runs of multiple spaces
between KEY/INDEX/UNIQUE/etc. and the name don't defeat the
kind classifier. (Copilot #1, two comments on the same issue.)
- Kind-aware extraction: ExtractInlineRenames() returns
*InlineRenames with separate Columns / Indexes maps plus an
Unsupported list, so a column and an index that share a name
(which happens when a KEY auto-names after its first column)
no longer compete for the same map key. (Copilot #2.)
- Directives that resolve to constraints / FKs / PRIMARY KEY /
anonymous FOREIGN KEY / unrecognised line shapes are now
recorded in Unsupported instead of silently dropped — the
parser turns them into errors. (Copilot #3.)
- renameDirectivePattern no longer accepts `.` in the old name;
the surrounding comment now matches reality (qualified names
were never plumbed through and are intentionally rejected).
(Copilot #6.)
- ExtractInlineRenames skips the leading-comment block (those
are statement-level, owned by ExtractStmtRenameFrom) so the
statement-level directive doesn't get re-attached to the
`CREATE TABLE …` opener line.
parser/parser.go
- Uses InlineRenames.Columns / .Indexes to attach RenameFrom by
kind, not by flat lookup. Errors out on Unsupported entries
(constraint / FK / dangling directive) so a typo or
mis-positioned directive fails the parse instead of silently
degrading into a destructive DROP+CREATE.
diff/rename.go
- Removes the misleading trailing comment in applyColumnRenames
(the "propagate to indexes" work happens in the caller, not
here). (Copilot #4.)
- Adds rewriteFKColumnRefs(), the FK counterpart to
rewriteIndexColumnRefs: rewrites FK Columns entries from
old → new after a column rename so fkEqual stays quiet. Without
this, a plain column rename emitted DROP FOREIGN KEY +
ADD CONSTRAINT for any FK on the renamed column. (Copilot #5.)
diff/tables.go
- Calls rewriteFKColumnRefs after applyColumnRenames.
Tests
- parser/directive_test.go: rewritten for the kind-aware API,
plus new cases for whitespace tolerance (tab, multi-space),
column/index name collision, qualified-name rejection,
constraint-target = unsupported, dangling-directive = error,
full ParseSQL error wrapping.
- diff/rename_test.go: new TestDiffRenameColumnAlsoRewritesFKColumns
covers the FK-rewrite path end-to-end.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel
added a commit
that referenced
this pull request
May 2, 2026
Seven inline comments, all valid; two are real bugs.
parser/directive.go
- End-of-loop guard in ExtractInlineRenames: a `pending` directive
whose target line never arrived (statement body ran out) is now
appended to Unsupported instead of being silently dropped.
(Copilot #1.)
- leadingBacktickedIdent comment rewritten: previous wording had
curly quotes ("…" instead of `…`) and didn't explain MySQL's
backtick-doubling escape. New comment is precise: doubled
backticks remain unsupported; the regex correctly rejects them
so the validator surfaces "malformed directive". (Copilot #2, #3.)
diff/rename.go
- applyTableRenames / applyColumnRenames / applyIndexRenames now
short-circuit when RenameFrom equals the desired name. This
avoids generating `ALTER TABLE x RENAME TO x` (rejected on some
MySQL versions, no-op on others) for what is almost always a
user typo. (Copilot #5, #6, #7.)
- rewriteConstraintColumnRefs: PRIMARY KEY column lists in
current.Constraints are rewritten old → new alongside the index
and FK rewrites, so a renamed-PK column doesn't surface as
DROP+ADD PRIMARY KEY in diffConstraints. CHECK constraints are
deliberately skipped — their Definition is a free-form
expression and rewriting it requires a full SQL parser.
(Copilot #4.)
diff/tables.go
- Calls rewriteConstraintColumnRefs after applyColumnRenames,
alongside the existing index / FK rewrites.
Tests
- parser/directive_test.go: TestExtractInlineRenamesTrailingPendingUnsupported
covers the end-of-loop pending path.
- diff/rename_test.go: TestDiffRenameSelfRenameIsNoOp covers the
table / column / index self-rename guards in one shot.
TestDiffRenameColumnAlsoRewritesPKConstraint covers the new PK
rewrite end-to-end (renames `users.old_id` → `users.id` and
asserts no DROP/ADD PRIMARY KEY appears).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel
added a commit
that referenced
this pull request
May 2, 2026
Copilot review #6: `k` is already used by the GetOk check above the rewrite call; the trailing `_ = k` was a leftover from an earlier draft. Drop it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel
added a commit
that referenced
this pull request
May 3, 2026
#1 Pin the RANGE COLUMNS REORGANIZE end-to-end. The plan-only fixture (partition_value_change_reorganize_range_columns.yml) covered the generator side, but nothing confirmed MySQL accepts `REORGANIZE PARTITION ... INTO (...)` with tuple boundaries or that the rewritten boundaries round-trip cleanly. Add the matching apply / verify_no_drift fixture. #2 The case-only partition-name no-op already has a regression test (`partition_name_case_only_diff_no_op.yml`, commit 4c6998f). #3 Pin the per-partition *option*-only diff (here: COMMENT-only) end-to-end. partitionDefEqual delegates to `parser.FormatPartitionDefinition`, which preserves COMMENT / MAX_ROWS / TABLESPACE / etc., so any byte-different definition already routed through the REORGANIZE branch — but no fixture pinned that MySQL accepts the generated statement or that the rewritten options round-trip. New apply fixture `partition_per_partition_option_change_reorganize.yml` covers both. LIST avoids the RANGE upper-bound cascade so the REORGANIZE only touches the one slot whose option actually changed. #4 / #5 / #6 Widen the Coverage / Not-yet-implemented sections of AGENTS.md (in scope + v1 cuts) and the TODO.md "Already shipped" block from "pure value-change (same names, only `VALUES` differs)" to "per-partition definition rewrite ... when both sides have the same partition names in the same order — covers `VALUES …` boundary tweaks plus COMMENT / MAX_ROWS / TABLESPACE / other per-partition option changes that round-trip through vitess's PartitionDefinition formatter". Brings the docs in line with what diffPartitions actually does (and with what CAVEATS.md "Per-partition definition change" already describes). #7 Fix the misleading comment in partitionNameListEqual. `parser.NormalizePartitionOption` only lower-cases function / column-reference identifiers inside the partition expression — it does NOT lower-case partition names. The case-insensitive `EqualFold` here is needed precisely because the round-trip preserves whatever case the user wrote on the partition name.
winebarrel
added a commit
that referenced
this pull request
May 5, 2026
Round 1 review on PR #75 caught eight issues; this commit addresses all of them. 1. (review #1) loadPreSQL treated any non-empty PreSQL string as "set", including whitespace-only values. Whitespace-only env var (`MYSCHEMA_PRE_SQL=`) would trigger the mutually-exclusive error against a legitimate --pre-sql-file, and skip the no-op short-circuit. Fix: TrimSpace both fields before deciding "set". 2. (review #2) loadPreSQL read --pre-sql-file with os.ReadFile, which doesn't support the repo's existing `-` for stdin convention used by parser.ReadSQLFile (the desired-SQL file args already accept it). Fix: route through parser.ReadSQLFile so `--pre-sql-file=-` works. 3. (review #3 + #4) runPreSQL was invoked AFTER connect(), so flag-validation errors (both flags set, missing file) could be masked by a downstream connection failure, and the client opened a DB connection it then threw away. Split into: - loadPreSQL: validate / read, no DB contact (called BEFORE connect) - execPreSQL: run on conn (called AFTER connect) apply.go and plan.go updated accordingly. 4. CLI-level mutual exclusion: tag both PreSQL and PreSQLFile with kong's `xor:"pre-sql"` so kong rejects the both-set case at parse time before our code sees it. The runtime check in loadPreSQL stays in place for programmatic API callers (Apply / Plan invoked from Go without going through kong). 5. (review #5 / #6 / #7 / #8) Test reshuffle. The original suite over-relied on success-only smoke tests that would have silently passed even if pre-SQL were skipped: - TestApply_PreSQLString and TestApply_PreSQLMultiStatement only checked NoError. - TestApply_PreSQLAppliesToSession claimed to be the "strongest behavioural pin" but probed nothing — the connection it would have probed is closed by Apply's defer. - TestPlan_PreSQLString same issue. Fix: load-bearing pins now feed INVALID pre-SQL and assert the apply / plan aborts with a wrapped "pre-sql" error containing the exact failing piece. The multi-statement split test now uses a payload with an invalid SECOND piece and asserts the error references that piece exactly (proves the split happened — without splitting, the driver would reject the whole concatenated string with a different error). 6. YAML migration: the new harness field `pre_sql` lets inline-payload tests live as testdata/apply/pre_sql_*.yml and testdata/plan/pre_sql_*.yml fixtures (3 + 1 = 4 new fixtures). File-related and programmatic-API tests stay in Go because the YAML harness has no `pre_sql_file` field (would need fixture-relative path handling, out of scope). Coverage is unchanged at the package level; loadPreSQL stays at 100% and execPreSQL is exercised by all the real-execution paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
Summary
myschema is a one-shot CLI — each invocation runs one plan/apply/dump against one MySQL connection — so the default
*sql.DBpool buys nothing and silently breaks session-scoped statements likeUSE <db>when the driver routes the next query to a fresh connection.Refactor every layer to operate on a single dedicated
*sql.Conn:client.connect(ctx)opens a*sql.DB, caps it to one connection (SetMax{Open,Idle}Conns(1)), and immediately reserves a*sql.Conn. A small wrapper closes both on caller's defer.catalog.Catalogstores*sql.Conninstead of*sql.DB.apply/plan/dump/diff_allpassconn.Connintocatalog.NewCatalogand useconn.ExecContextfor DDL.testutil.ConnectDBreturns*sql.Conn(bound to a 1-conn pool) and registers cleanups for both the conn and its parent DB.testutil.SetupDBruns DROP/CREATE/USE on the same conn soUSEactually persists into the test body.Test plan
make test(parser + diff + catalog round-trip)make test-scenario(smoke)golangci-lintclean🤖 Generated with Claude Code