Skip to content

Stop using the *sql.DB pool; use a single *sql.Conn#6

Merged
winebarrel merged 1 commit into
mainfrom
no-connection-pool
May 2, 2026
Merged

Stop using the *sql.DB pool; use a single *sql.Conn#6
winebarrel merged 1 commit into
mainfrom
no-connection-pool

Conversation

@winebarrel

Copy link
Copy Markdown
Owner

Summary

myschema is a one-shot CLI — each invocation runs one plan/apply/dump against one MySQL connection — so the default *sql.DB pool buys nothing and silently breaks session-scoped statements like USE <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.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 runs DROP/CREATE/USE on the same conn so USE actually persists into the test body.

Test plan

  • make test (parser + diff + catalog round-trip)
  • make test-scenario (smoke)
  • golangci-lint clean
  • CI green on this PR

🤖 Generated with Claude Code

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

codecov Bot commented May 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 18.51852% with 44 lines in your changes missing coverage. Please review.
✅ Project coverage is 32.14%. Comparing base (cce3078) to head (b8a3bd7).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
internal/testutil/testutil.go 0.00% 17 Missing ⚠️
client.go 0.00% 14 Missing ⚠️
apply.go 0.00% 4 Missing ⚠️
dump.go 0.00% 3 Missing ⚠️
plan.go 0.00% 3 Missing ⚠️
diff_all.go 0.00% 2 Missing ⚠️
catalog/catalog.go 83.33% 0 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@winebarrel winebarrel merged commit a8d676c into main May 2, 2026
2 of 4 checks passed
@winebarrel winebarrel deleted the no-connection-pool branch May 2, 2026 02:28
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>
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.

1 participant