PR review caught three issues. All addressed before merge.

#3 (ship-blocker) — Line-tracking regression. The original bridge called
bare ParseTiDB and skipped the SetOriginTextPosition + per-column line
work that the canonical pre-flip path (ParseTiDBForSyntaxCheck) applies
at tidb.go:64-75. 49 of 51 tidb advisors call node.OriginTextPosition()
for advice line numbers; without the fix, post-flip un-migrated
advisors silently regress to line 1 for every statement.

Fix: extract applyTiDBLineTracking(node, baseLine, originalText) helper
from ParseTiDBForSyntaxCheck. Both the canonical path and the bridge
call it, so consumers reading OriginTextPosition() see consistent
values across both paths. Helper returns the 1-based actualStartLine
for *AST.StartPosition use.

Regression test: TestAsPingCapASTLineTrackingMatchesCanonical parses a
two-statement input via both paths and asserts the second statement's
node.OriginTextPosition() matches (both must report 2, not 1).

#1 — Godoc overpromise. Original wording claimed "review continuity
from both directions" but the bridge only handles one direction —
omni-success → pingcap-fallback. The opposite (omni rejects at
dispatcher level so no *OmniAST exists) is the dispatcher-fallback
contract, not the bridge's job. Godoc now narrows the claim and
forward-references plan §1.5.0 invariant #5 (the dispatcher-fallback
invariant being added in a follow-up plan amendment).

#2 — Logging asymmetry. Pilot side soft-fails with slog.Debug + log.
Bridge silently returned (nil, false). Now logs symmetrically on each
failure mode (parse error, zero nodes, line-tracking error). Negative
cache means a single Debug per OmniAST instance, not per call.

#4 — Strict ParseTiDB note. Godoc now explains why the bridge uses
strict ParseTiDB (no error-recovery) instead of mysql's lenient ANTLR
parse: pingcap has no equivalent error-recovery mode, and the bridge
operates on omni-validated SQL where strict parsing is correct.

#5 — Multi-statement assumption made explicit. nodes[0] used; nodes[1:]
logged as a contract violation instead of silently dropped.

Validation:
  go test -count=1 ./backend/plugin/parser/tidb/...   # green incl. new regression test
  go test -count=1 ./backend/plugin/advisor/tidb/...  # green
  go test -count=1 ./backend/plugin/advisor/mysql/... # green (cross-engine regression)
  go build ...                                        # green
  golangci-lint run ...                               # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…0179)

* feat(tidb): add AsPingCapAST bridge on OmniAST (Phase 1.5 §1.5.2)

Mirror of mysql/OmniAST.AsANTLRAST for the Phase 1.5 advisor migration.
Required prerequisite for the eventual dispatcher flip (§1.5.N+1):
once the registered ParseStatementsFunc is flipped to omni and
*OmniAST values reach the ~50 un-migrated tidb advisors at any point
in the migration, those advisors keep working because GetTiDBAST
falls through to AsPingCapAST() automatically.

Implementation mirrors mysql/omni.go:39-55:
  - Lazy parse: native pingcap parse runs once on first AsPingCapAST
    call per OmniAST instance.
  - Cached result: pingcapParsed bool flag distinguishes "not yet
    parsed" from "parsed and got nil"; cache hits skip the work.
  - Negative caching: pingcap parse failure also short-circuits future
    calls so unparseable-by-pingcap SQL doesn't re-parse per advisor.

backend/plugin/parser/tidb/ast.go:
  - Adds PingCapASTProvider interface (analog of base.AntlrASTProvider).
  - GetTiDBAST extended to check for the provider, mirroring
    base.GetANTLRAST's pattern.

backend/plugin/parser/tidb/omni.go:
  - OmniAST gains pingcapAST + pingcapParsed unexported fields.
  - AsPingCapAST method implements the provider contract.
  - Godoc updated: removes the "neither fallback" rationale (correct
    for PR #20107's scope but inverted by the migration plan), points
    at plan §1.5.0 invariant #4 + bridge persistence rule for context.

Tests:
  - TestAsPingCapASTReturnsNativeAST: bridge contract for valid SQL.
  - TestAsPingCapASTCachesAcrossCalls: a.Same() pointer equality on
    repeat calls proves cache hit, not fresh re-parse.
  - TestAsPingCapASTCachesNegativeResult: parse failure cached too.
  - TestGetTiDBASTFallsBackToProvider: end-to-end bridge through
    GetTiDBAST — the path un-migrated advisors will use.

Bridge persistence: the bridge stays in the codebase indefinitely
post-Phase-1.5. The one Class III advisor (dml_dry_run, blocked on
omni's NonTransactionalDMLStmt grammar — Phase 2 §Tier 4g) requires
this fallback. Bridge cleanup is gated on dml_dry_run unblocking, not
on Phase 1.5 completion.

Validation:
  go test -count=1 ./backend/plugin/parser/tidb/...    # green (incl. 4 new bridge tests)
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green (no cross-engine regression)
  go build ...                                         # green
  golangci-lint run ...                                # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(tidb): bridge line-tracking + symmetric logging + scoped godoc

PR review caught three issues. All addressed before merge.

#3 (ship-blocker) — Line-tracking regression. The original bridge called
bare ParseTiDB and skipped the SetOriginTextPosition + per-column line
work that the canonical pre-flip path (ParseTiDBForSyntaxCheck) applies
at tidb.go:64-75. 49 of 51 tidb advisors call node.OriginTextPosition()
for advice line numbers; without the fix, post-flip un-migrated
advisors silently regress to line 1 for every statement.

Fix: extract applyTiDBLineTracking(node, baseLine, originalText) helper
from ParseTiDBForSyntaxCheck. Both the canonical path and the bridge
call it, so consumers reading OriginTextPosition() see consistent
values across both paths. Helper returns the 1-based actualStartLine
for *AST.StartPosition use.

Regression test: TestAsPingCapASTLineTrackingMatchesCanonical parses a
two-statement input via both paths and asserts the second statement's
node.OriginTextPosition() matches (both must report 2, not 1).

#1 — Godoc overpromise. Original wording claimed "review continuity
from both directions" but the bridge only handles one direction —
omni-success → pingcap-fallback. The opposite (omni rejects at
dispatcher level so no *OmniAST exists) is the dispatcher-fallback
contract, not the bridge's job. Godoc now narrows the claim and
forward-references plan §1.5.0 invariant #5 (the dispatcher-fallback
invariant being added in a follow-up plan amendment).

#2 — Logging asymmetry. Pilot side soft-fails with slog.Debug + log.
Bridge silently returned (nil, false). Now logs symmetrically on each
failure mode (parse error, zero nodes, line-tracking error). Negative
cache means a single Debug per OmniAST instance, not per call.

#4 — Strict ParseTiDB note. Godoc now explains why the bridge uses
strict ParseTiDB (no error-recovery) instead of mysql's lenient ANTLR
parse: pingcap has no equivalent error-recovery mode, and the bridge
operates on omni-validated SQL where strict parsing is correct.

#5 — Multi-statement assumption made explicit. nodes[0] used; nodes[1:]
logged as a contract violation instead of silently dropped.

Validation:
  go test -count=1 ./backend/plugin/parser/tidb/...   # green incl. new regression test
  go test -count=1 ./backend/plugin/advisor/tidb/...  # green
  go test -count=1 ./backend/plugin/advisor/mysql/... # green (cross-engine regression)
  go build ...                                        # green
  golangci-lint run ...                               # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(tidb): correct godoc reference to plan invariant #8

Numbering drift caught in review: the godoc cited "§1.5.0 invariant #5"
but the landed plan invariant is #8 (the dispatcher-fallback contract).
The §5 numbering came from an earlier draft of the plan amendment;
final landed numbering is #8 because three additional invariants (#5
no-mutation, #6 recipe-default, #7 shape-audit) had already accreted.

One-line fix.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): soft-fail getTiDBNodes on bridge miss

Codex P1 catch on PR #20179.

Without this fix, post-§1.5.N+1 dispatcher flip:
  ParsedStatement.AST = *OmniAST → un-migrated advisor calls
  getTiDBNodes → tidbparser.GetTiDBAST(stmt.AST) →
  AsPingCapAST fails (pingcap rejects what omni accepted) →
  GetTiDBAST returns (nil, false) →
  getTiDBNodes returns 'AST type mismatch: expected TiDB parser result' →
  advisor.Check propagates as 'Rule check failed' →
  every un-migrated advisor breaks for that review

That breaks the migration continuity the bridge is meant to preserve.
Phase 1.5 invariant #2 says soft-fail (skip statement, review
continues), not hard-fail.

Fix: when GetTiDBAST returns false AND stmt.AST is a
PingCapASTProvider (i.e. an *OmniAST that just routed through the
bridge and missed), skip the statement instead of returning an error.
The bridge has already logged the parse failure at debug level.

True engine-mismatch (stmt.AST is some unrelated type, neither
*tidb.AST nor a PingCapASTProvider) is still surfaced as an error —
that's a programmer error in registration, not a soft-fail case.

Regression test:
  TestGetTiDBNodesSoftFailsOnBridgeMiss feeds a context with two
  ParsedStatements: one *OmniAST whose AsPingCapAST will fail
  ('SELECT FROM WHERE;') bracketed by one that succeeds ('SELECT 1').
  Expects no error and len(got) == 1 (the successful one).

Sanity-checked: GetTiDBAST returns false on a pingcap-invalid OmniAST,
confirming the pre-fix path would have errored.

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green incl. new test
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  go test -count=1 ./backend/plugin/parser/tidb/...    # green
  golangci-lint run ...                                # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Per code review on PR #20198, six issues addressed:

#1 Acknowledge silent bug fix in migration_compatibility.go.
   The original pingcap-typed code at lines 104-106 of the pre-
   migration file checked ConstraintUniq + ConstraintUniqKey but
   missed ConstraintUniqIndex, so `ALTER TABLE t ADD UNIQUE INDEX
   idx (col)` previously emitted no compatibility advice. omni
   unifies all three under ConstrUnique, so the migration silently
   started flagging the missed case.
   Side-effect: ADD UNIQUE INDEX now correctly flags as
   compatibility-affecting (was a missed case in original code).
   Documented inline at the ConstrUnique branch.

#2 Remove dead cmd.NewName fallback in advisor_table_require_pk.go's
   ATChangeColumn case. Per omni contract (parsenodes.go:224),
   NewName is for RENAME ops, NOT ATChangeColumn — for ChangeColumn
   the new name is in cmd.Column.Name. The fallback either never
   fired (dead) or fired on malformed input (wrong). Now reads
   cmd.Column.Name only, with comment documenting the contract.

#3 Move addColumnTargets to utils.go. Was defined in
   advisor_table_require_pk.go but consumed by advisor_index_pk_type.go
   — same cross-file coupling pattern that the pre-batch checklist is
   designed to catch (and that we caught for tableNewColumn). Now
   shared from utils.go alongside other omni-typed helpers.

#4 Acknowledge graceful-skip behavior change in constraintPKColumns.
   The original pingcap-typed convertConstraintToKeySlice did
   `key.Column.Name.String()` without nil-checking key.Column. For
   `PRIMARY KEY ((expr))` syntax pingcap left key.Column nil and
   populated key.Expr instead, so the original would panic with a
   nil-pointer dereference on functional PKs. The migrated version
   returns an empty slice (graceful skip) for expression PKs.
   Documented inline.

#5 Narrow formatColumnType godoc. The previous comment claimed full
   pingcap tp.String() reproduction but the helper only handles
   types with Length/Scale; ENUM/SET (value lists), DECIMAL with
   implicit Scale, BIT, etc. would diverge. Updated godoc to state
   the contract is "fixture stability for INDEX_PK_TYPE_LIMIT" not
   full pingcap parity, with a note to extend when new fixture
   cases appear.

#6 Consolidate locStart into checkStmt arms. Previous structure
   had two switches covering the same node types — adding a node
   type to one without the other would silently degrade line
   numbers to default (0 → line 1). Per peer recommendation, line
   capture moves into the same arm as the code classification, so
   the trap is structurally impossible. Slight verbosity for
   guaranteed sync.

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  go test -count=1 ./backend/plugin/parser/tidb/...    # green
  go build ...                                         # green
  golangci-lint run ...                                # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…1.5.3 batch 2) (#20198)

* refactor(advisor/tidb): migrate 5 more rules to omni AST (Phase 1.5 §1.5.3 batch 2)

Class I batch following the recipe established in PR #20140 (pilot) and
extended in PR #20195 (batch 1).

Migrated:
  - advisor_database_drop_empty_db.go (DATABASE_DROP_EMPTY_DATABASE)
  - advisor_table_no_fk.go            (TABLE_NO_FOREIGN_KEY)
  - advisor_migration_compatibility.go (SCHEMA_BACKWARD_COMPATIBILITY)
  - advisor_table_require_pk.go       (TABLE_REQUIRE_PK)
  - advisor_index_pk_type.go          (INDEX_PK_TYPE_LIMIT)

Pre-batch checklist:
  1. Coupling scan: caught tableNewColumn / columnNameToColumnDef shared
     across 3 files (advisor_index_pk_type.go defines, +2 consumers).
     Moved pingcap-typed versions to utils.go for un-migrated consumers
     (advisor_index_primary_key_type_allowlist.go,
      advisor_index_type_no_blob.go); migrated advisor uses omni-typed
     siblings (tableNewOmniColumn / omniColumnNameToColumnDef).
     Cleanup tracked: BYT-9395.
  2. Shape audit: compared each against analogous mysql/rule_*.go omni
     rule. migration_compatibility surfaced one divergence — pingcap had
     three separate ConstraintUniq/UniqKey/UniqIndex types that all
     produce the same advice; omni unifies them under ConstrUnique.
     Code consolidates the three branches into one. PRIMARY KEY drop is
     also unified — pingcap had AlterTableDropPrimaryKey; omni routes
     through ATDropConstraint with cmd.Name == "PRIMARY".
  3. Yaml fixture coverage: TestTiDBRules green for all 5 rules.
  4. Pingcap-import quarantine: clean for the 5 migrated files.

Notable mappings used:
  ast.AlterTableDropPrimaryKey → ATDropConstraint with cmd.Name == "PRIMARY"
  ast.AlterTableDropIndex      → ATDropIndex (cmd.Name)
  ast.AlterTableAlterCheck     → ATAlterCheckEnforced
  ast.ConstraintUniq/UniqKey   → ConstrUnique (single)
  spec.OldColumnName.Name      → cmd.Name (in ATChangeColumn / ATDropColumn)
  node.ReferTable              → n.Like
  node.KeyType == IndexKeyTypeUnique → n.Unique (CreateIndexStmt)
  column.Tp (FieldType)        → column.TypeName (DataType: Name+Length+Scale)

advisor_index_pk_type's formatColumnType helper reproduces pingcap's
mixed casing convention (INT/BIGINT uppercase; everything else lowercase
with optional length/scale) so YAML fixture content like "varchar(5)"
remains stable.

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green (TestTiDBRules covers all 5 rules)
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green (cross-engine)
  go test -count=1 ./backend/plugin/parser/tidb/...    # green
  go build ...                                         # green
  golangci-lint run ...                                # 0 issues

Migration progress: 10 of 51 advisors on omni AST.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(advisor/tidb): address review feedback round 1

Per code review on PR #20198, six issues addressed:

#1 Acknowledge silent bug fix in migration_compatibility.go.
   The original pingcap-typed code at lines 104-106 of the pre-
   migration file checked ConstraintUniq + ConstraintUniqKey but
   missed ConstraintUniqIndex, so `ALTER TABLE t ADD UNIQUE INDEX
   idx (col)` previously emitted no compatibility advice. omni
   unifies all three under ConstrUnique, so the migration silently
   started flagging the missed case.
   Side-effect: ADD UNIQUE INDEX now correctly flags as
   compatibility-affecting (was a missed case in original code).
   Documented inline at the ConstrUnique branch.

#2 Remove dead cmd.NewName fallback in advisor_table_require_pk.go's
   ATChangeColumn case. Per omni contract (parsenodes.go:224),
   NewName is for RENAME ops, NOT ATChangeColumn — for ChangeColumn
   the new name is in cmd.Column.Name. The fallback either never
   fired (dead) or fired on malformed input (wrong). Now reads
   cmd.Column.Name only, with comment documenting the contract.

#3 Move addColumnTargets to utils.go. Was defined in
   advisor_table_require_pk.go but consumed by advisor_index_pk_type.go
   — same cross-file coupling pattern that the pre-batch checklist is
   designed to catch (and that we caught for tableNewColumn). Now
   shared from utils.go alongside other omni-typed helpers.

#4 Acknowledge graceful-skip behavior change in constraintPKColumns.
   The original pingcap-typed convertConstraintToKeySlice did
   `key.Column.Name.String()` without nil-checking key.Column. For
   `PRIMARY KEY ((expr))` syntax pingcap left key.Column nil and
   populated key.Expr instead, so the original would panic with a
   nil-pointer dereference on functional PKs. The migrated version
   returns an empty slice (graceful skip) for expression PKs.
   Documented inline.

#5 Narrow formatColumnType godoc. The previous comment claimed full
   pingcap tp.String() reproduction but the helper only handles
   types with Length/Scale; ENUM/SET (value lists), DECIMAL with
   implicit Scale, BIT, etc. would diverge. Updated godoc to state
   the contract is "fixture stability for INDEX_PK_TYPE_LIMIT" not
   full pingcap parity, with a note to extend when new fixture
   cases appear.

#6 Consolidate locStart into checkStmt arms. Previous structure
   had two switches covering the same node types — adding a node
   type to one without the other would silently degrade line
   numbers to default (0 → line 1). Per peer recommendation, line
   capture moves into the same arm as the code classification, so
   the trap is structurally impossible. Slight verbosity for
   guaranteed sync.

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  go test -count=1 ./backend/plugin/parser/tidb/...    # green
  go build ...                                         # green
  golangci-lint run ...                                # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(advisor/tidb): trim comments to code-comment scope

Round-1 review feedback added narrative comments documenting silent
behavior changes (UniqIndex bug fix, expression-PK panic-fix), the
fixture-stability contract on formatColumnType, and the locStart
synchronization rationale. That content belongs in the PR description
and commit messages, not in code comments — future readers don't need
the historical "what pingcap used to do" framing, just what the code
does and why.

Trimmed scope to what a future maintainer needs at read time:
contracts (what fields mean, mutual exclusivity, what each branch is
checking), gotchas (why both ATDropConstraint and ATDropIndex are
handled for PRIMARY), and TODO trackers (Linear tickets for the
moved-but-still-pingcap helpers).

Removed:
  - Bug-fix narratives ("the original pingcap code at lines X-Y missed Z")
  - Migration-history framing ("Moved here from <file>...", "post-flip...")
  - Plan cross-references and "see plans/..." pointers
  - Multi-paragraph contract docs that just restate the code

Kept:
  - Field-shape contracts (cmd.Name OLD vs cmd.Column.Name NEW; omni's
    Columns/Column mutual exclusivity)
  - Non-obvious branch semantics (DROP INDEX `PRIMARY` aliases DROP
    PRIMARY KEY)
  - Linear cleanup tickets on un-migrated-helper types

Net: -95 lines of comments across 4 files. No behavior change.

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  golangci-lint run ...                                # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): ATAlterCheckEnforced reads cmd.NewName, not cmd.Constraint

Codex P1 catch on PR #20198. The migrated migration_compatibility
advisor stopped flagging ALTER TABLE ... ALTER CHECK chk ENFORCED as
a backward-compat issue.

omni populates ATAlterCheckEnforced with cmd.NewName ("ENFORCED" /
"NOT ENFORCED") and leaves cmd.Constraint nil — verified against
omni/tidb/parser/alter_table.go. The previous check
`cmd.Constraint != nil && !cmd.Constraint.NotEnforced` always
evaluated false, so the case never emitted advice. Pre-migration
pingcap path emitted code 110 when spec.Constraint.Enforced was true.

Fix: check cmd.NewName == "ENFORCED" instead. Pingcap parity:
ENFORCED transitions get flagged, NOT ENFORCED transitions don't.

Fixture additions pin the regression:
  - ALTER TABLE t ALTER CHECK chk ENFORCED → code 110 advice
  - ALTER TABLE t ALTER CHECK chk NOT ENFORCED → no advice

Sanity-checked by reverting the fix: TestTiDBRules fails on the
ENFORCED case with the expected diff (got nil advice, want code 110)
exactly as Codex described.

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  golangci-lint run ...                                # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): defensive empty-name guard for ATDropConstraint + INTEGER fixture

Codex round-2 review on PR #20198. Two findings:

#1 INTEGER alias (NOT VALID).
Codex claimed CREATE TABLE t(id INTEGER PRIMARY KEY) would render the
type as "integer" (lowercase) and false-positive the rule. Empirically
omni's parser already normalizes INTEGER → DataType.Name = "INT" at
parse time, so formatColumnType correctly returns "INT" and the
column passes the INT/BIGINT check. Verified by adding two fixture
cases (INTEGER inline and table-level PK); both produce no advice.
No code change for this finding — the fixture pins the contract so
any future omni grammar change that breaks the alias surfaces.

#2 Empty cmd.Name on DROP PRIMARY KEY (DEFENSIVE FIX).
Codex claimed omni AST can represent DROP PRIMARY KEY with cmd.Name
== "" and that the mysql omni rule already guards this. Empirically
current omni grammar (both mysql and tidb) produces cmd.Name ==
"PRIMARY" for ALTER TABLE t DROP PRIMARY KEY — the regression Codex
described doesn't reproduce.

However, mysql/rule_table_require_pk.go has a defensive guard
strings.EqualFold(cmd.Name, primaryKeyName) || cmd.Name == "" with
comment "cmd.Name == '' when it's DROP PRIMARY KEY (no explicit
constraint name)". Either an older omni version produced empty name
or the guard is pure future-proofing. Adopting the same defensive
posture for tidb maintains cross-engine parity at no real cost.
Existing fixture covers DROP PRIMARY KEY (cmd.Name == "PRIMARY"
case) so non-empty path is already pinned; the empty-name path is
unreachable today but the guard is in place if grammar ever changes.

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  golangci-lint run ...                                # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): pin ENFORCED case-normalization + SonarCloud cleanup

Codex round-3 review on PR #20198 flagged `cmd.NewName == "ENFORCED"` as
case-sensitive. Empirically false: omni's tidb lexer lowercases identifiers
before keyword lookup (omni/tidb/parser/lexer.go:2267), and the ALTER CHECK
arms assign canonical literal strings to cmd.NewName ("ENFORCED" /
"NOT ENFORCED"; alter_table.go:762-765, 785-788). The advisor matches
across all user-typed cases.

Added 4 fixture cases to schema_backward_compatibility.yaml pinning the
contract:
- "ALTER CHECK chk enforced" (lowercase) -> flagged
- "ALTER CHECK chk Enforced" (mixed) -> flagged
- "ALTER CHECK chk not enforced" (lowercase) -> not flagged
- "ALTER CHECK chk Not Enforced" (mixed) -> not flagged

If omni ever stops normalizing, the lowercase/mixed cases will start
regressing -- empirical pin per Phase 1.5 invariant #9.

SonarCloud cleanup (8 findings):
- S1192 critical: extracted "cannot find the type of \`%s\`.\`%s\`" to
  errCannotFindColumnTypeFmt const in advisor_index_pk_type.go (was
  duplicated 3x).
- S8209 minor (x7): grouped consecutive same-type Go params in
  advisor_index_pk_type.go (set/get/delete/getPKColumnType),
  advisor_table_require_pk.go (dropColumn/changeColumn), and utils.go
  (set/get/delete on tableNewColumn).

CodeQL Analyze (go) FAILURE on the previous commit was an infra-side
runner shutdown ("The runner has received a shutdown signal"), not a
real code finding. This push triggers a fresh run.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…arial review

Spec (section 4): tighten ResetPlanWebhookDelivery to only delete
PIPELINE_FAILED rows, addressing adversarial #4 (a recovery endpoint
must never wipe a terminal PIPELINE_COMPLETED row).
Spec (section 6): document the in-flight skip race as pre-existing,
unrelated to this fix; corrects the misleading comment about
"duplicate at worst, never missing".
Spec (section 7): list the in-flight cancel and the reset-after-validation
moves as out-of-scope follow-ups.

Plan: rewrite to fix all critical findings from the four review passes:
- Replace task.LatestTaskRunStatus (nonexistent) with task.Status against
  v1pb.Task_FAILED/_DONE/_SKIPPED throughout helpers.
- Drop the leaky outer ctl.project webhook setup; subtests own webhooks.
- Add project-name filtering to webhook count helpers so async late
  deliveries from a previous subtest cannot pollute the next one.
- installWorkspaceApprovalRule snapshots prior setting and registers
  t.Cleanup to restore it; appends instead of replacing rules.
- SB2 routes through RequestIssue (creator-only) before re-approval,
  matching how the issue service actually clears rejected approvers.
- Add waitForAllTasksTerminal before recovery actions in C3/C6/C7/F2
  to remove still-running-task races.
- Reword Task 1 reset comment so it doesn't claim a missing row only
  causes "duplicate at worst" (a missing reset can suppress notifications).
- Reframe F2 as "PK dedup of second-task failure" rather than
  "simultaneous", which serial scheduling cannot guarantee.
- Use path.Base instead of a custom lastSegment helper.
- Drop createDatabaseInProject and retryTaskByDB thin wrappers; use
  ctl.createDatabase + dbTargetName + runTaskByDB directly.
- I1 now asserts issue title and project resource path in the payload.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

… (Codex round-4 on batch 11)

Codex P2 round-4 correctly flagged: omniBuildColumnTypeString dropped
EnumValues entirely. Empirical reproduction:

  Pingcap ENUM('x','y','z')  → Tp.String() = "enum('x','y','z')"
  Pingcap SET('a','b','c')   → Tp.String() = "set('a','b','c')"
  Omni                       → DataType{Name="ENUM", EnumValues=[x y z]}
  Omni-built (pre-fix)       → "enum"  (values dropped entirely)

Catalog stores the literal value list via MySQL info_schema's
COLUMN_TYPE column (verified per backend/plugin/db/tidb/sync.go).

Pre-fix false-positive:
  catalog: "enum('a','b')"
  user:    ALTER TABLE t MODIFY status ENUM('a','b')   (no-op)
  omni-built: "enum"
  comparison: "enum('a','b')" ≠ "enum" → flagged as type change

Fix:
- isEnumOrSetTypeName(lower) covers "enum" and "set".
- formatEnumValueList(values []string) renders the canonical
  `'v1','v2',…` form with single-quote escaping (' → '') per SQL
  convention. Matches pingcap's Tp.String() output AND info_schema's
  COLUMN_TYPE rendering.
- ENUM/SET case takes precedence over length/scale rendering in the
  switch (the value list is the canonical body for these types).

Honest pattern recognition: this is the FOURTH round of Codex catches
on omniBuildColumnTypeString in a single batch:
  #1 (cumulative #21):  BLOB/TINYBLOB/VARBINARY " BINARY" suffix
  #2 (round-2 commit):  DECIMAL(N) zero-scale + ZEROFILL drop
  #3 (round-3 commit):  FLOAT/DOUBLE/REAL precision-hint drop
  #4 (this commit):     ENUM/SET value literal drop

The common failure mode: my builder enumerates the DataType fields I
already knew about (Name + Length + Scale + Unsigned + Zerofill) but
silently drops fields I didn't anticipate (Charset annotations,
EnumValues). The discipline I should have started with:

  enumerate ALL DataType fields (Name, Length, Scale, Unsigned,
  Zerofill, Charset, Collate, EnumValues, ArrayDim, SRID) and ask
  per-field: does pingcap's Tp.String() render it? Does the catalog
  store it? If yes to either, my builder must render it.

That's the field-by-field audit that would have caught all four
rounds in one pass. Adding to the pre-batch protocol for any future
advisor that uses Tp.String() comparison against a catalog string.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…mnType (Codex round-5 on batch 11)

Codex P2 round-5 correctly flagged: omniBuildColumnTypeString renders
DataType.Name verbatim ("boolean"), but TiDB/MySQL stores BOOLEAN
columns as tinyint(1) (verified via backend/plugin/schema/tidb/get_database_metadata.go:437-438
and backend/plugin/schema/mysql/get_database_metadata.go:259-261).

Empirical reproduction:

  Pingcap BOOL/BOOLEAN     → Tp.String() = "tinyint(1)"  (canonicalized)
  Pingcap TINYINT(1)       → Tp.String() = "tinyint(1)"  (same)
  Pingcap TINYINT (bare)   → Tp.String() = "tinyint(4)"  (default width)
  Omni BOOL/BOOLEAN        → DataType{Name="BOOLEAN", Length=0}
  Omni TINYINT(1)          → DataType{Name="TINYINT", Length=1}

Catalog stores "tinyint(1)" for boolean columns.

Pre-fix false-positive:
  catalog:    "tinyint(1)"
  user:       ALTER TABLE t MODIFY x BOOLEAN   (no-op against a boolean column)
  omni-built: "boolean"
  normalize:  default branch → identity → "boolean"
  comparison: "boolean" ≠ "tinyint(1)" → flagged as type change

Fix: add `"boolean"` and `"bool"` cases to normalizeColumnType, both
mapping to `"tinyint(1)"`. Placed in normalizeColumnType rather than
omniBuildColumnTypeString because the normalize layer is where the
existing canonicalization conventions live (integer default widths
already follow this pattern: `int` ↔ `int(11)`, `tinyint` ↔ `tinyint(4)`).

Both sides of the comparison go through normalize, so the canonicalization
is symmetric — works regardless of which side originally rendered which
form.

Codex round-5 of 5 on omniBuildColumnTypeString in this PR:
  #1 BLOB/TINYBLOB/VARBINARY " BINARY" suffix     → cumulative #21
  #2 DECIMAL zero-scale + ZEROFILL drop           → round-2 commit
  #3 FLOAT/DOUBLE/REAL precision-hint drop        → round-3 commit
  #4 ENUM/SET value-literal drop                  → round-4 commit
  #5 BOOLEAN ↔ tinyint(1) alias normalization     → this commit

The field-by-field DataType audit (Name, Length, Scale, Unsigned,
Zerofill, Charset, Collate, EnumValues, ArrayDim, SRID) that I
identified in round-4 as the missing pre-batch discipline would NOT
have caught round-5 — this is a TYPE-NAME canonicalization concern,
not a field-rendering concern. The pre-batch discipline addition for
this class: when porting a rule that compares type-strings against
the catalog, audit normalizeColumnType for alias coverage by
cross-referencing MySQL's documented type aliases (BOOL/BOOLEAN ↔
TINYINT(1), DEC ↔ DECIMAL, REAL ↔ DOUBLE, INTEGER ↔ INT, etc.) and
backend/plugin/schema/*/get_database_metadata.go's explicit
normalization map.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…(10,0) (Codex round-6 on batch 11)

Codex P2 round-6 correctly flagged: omniBuildColumnTypeString renders
bare DECIMAL (no parens) as "decimal" (Length=0 → default case),
while pingcap Tp.String() and MySQL INFORMATION_SCHEMA both
canonicalize to "decimal(10,0)" (MySQL defaults M=10, D=0).

Pre-fix false-positive:
  catalog:    "decimal(10,0)"
  user:       ALTER TABLE t MODIFY amount DECIMAL   (no-op against decimal(10,0))
  omni:       Name="DECIMAL" Length=0 Scale=0
  omni-built: "decimal"
  normalize:  default branch → identity → "decimal"
  comparison: "decimal" ≠ "decimal(10,0)" → flagged as type change

Empirical (from prior verification): pingcap renders bare DECIMAL,
DECIMAL(10), and DECIMAL(10,0) all as "decimal(10,0)" — MySQL applies
the default precision regardless of which syntactic form the user
typed.

Fix: add `"decimal"`, `"numeric"`, `"fixed"` cases to
normalizeColumnType, all mapping to `"decimal(10,0)"`. NUMERIC and
FIXED are MySQL aliases for DECIMAL — included defensively for the
same canonicalization story. Same layer as the round-5 BOOLEAN
canonicalization and the integer default-width entries — symmetric
across both sides of the comparison.

Codex round-6 of 6 (so far) on the type-string comparison path:
  #1 BLOB/TINYBLOB/VARBINARY " BINARY" suffix     → cumulative #21
  #2 DECIMAL zero-scale + ZEROFILL drop           → round-2 commit
  #3 FLOAT/DOUBLE/REAL precision-hint drop        → round-3 commit
  #4 ENUM/SET value-literal drop                  → round-4 commit
  #5 BOOLEAN ↔ tinyint(1) alias                   → round-5 commit
  #6 bare DECIMAL ↔ decimal(10,0) default         → this commit

Refined pre-batch protocol going forward (third axis added):

  For any rule that compares type-strings against catalog, audit:
    (a) omniBuildColumnTypeString — every DataType field that
        pingcap renders or catalog stores. Field-by-field check
        across Name, Length, Scale, Unsigned, Zerofill, Charset,
        Collate, EnumValues, ArrayDim, SRID.
    (b) normalizeColumnType — every MySQL type alias and default-
        application. Cross-reference backend/plugin/schema/*/
        get_database_metadata.go's normalization map.
    (c) Bare-form defaults — for every type with implicit precision
        defaults (DECIMAL → (10,0), INT → (11), TINYINT → (4), etc.),
        verify the bare-form is canonicalized at the normalize layer.

Axes (a) caught rounds 2-4. (b) caught round 5. (c) caught round 6.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

… AST (Phase 1.5 §1.5.3 batch 11) (#20302)

* refactor(advisor/tidb): migrate column_disallow_changing trio to omni AST (Phase 1.5 §1.5.3 batch 11)

Three theme-coupled advisors migrated as one unit. No helper coupling
between them (verified via pre-batch grep) — ship-as-unit is theme-
and reviewer-cadence-driven, not load-bearing.

- advisor_column_disallow_changing.go
- advisor_column_disallow_changing_order.go
- advisor_column_disallow_changing_type.go

All Recipe A. ALTER TABLE only (no CreateTable arm in pingcap-tidb
predecessors; preserved).

35 of 51 advisors merged once this lands (32 → 35). 16 remaining +
1 Class III blocked.

## Single-advice-per-statement cardinality contract preserved

All three pingcap-typed predecessors used a Visitor with `break` after
the first match, emitting at most ONE advice per top-level ALTER TABLE
statement. The omni port replicates this with explicit `break`. **Mysql
analog diverges** — emits per-cmd without breaking, producing N advices
for N matching specs. Cardinality divergence preserved on the tidb side
per invariant #7's "preserve pingcap behavior" rule.

Pinned each advisor with multi-CHANGE/MODIFY fixtures (expected: 1
advice for 2-spec statement).

## changing_type: cumulative #21 — latent pingcap false-positive fixed

Empirical finding from pre-batch protocol step 2a:

Pingcap's `FieldType.String()` rendering appends a ` BINARY` charset
annotation on types stored with binary charset:
  BLOB        → "blob BINARY"
  TINYBLOB    → "tinyblob BINARY"
  VARBINARY(N) → "varbinary(N) BINARY"

The pingcap-typed `disallow_changing_type` rule compared this rendered
string against `column.GetProto().Type` from the catalog (which stores
the bare type name from INFORMATION_SCHEMA — no charset suffix). The
`normalizeColumnType` helper has no "blob" / "blob binary" case, so its
default branch is identity-lowercase. Result:

  "blob" (catalog) ≠ "blob binary" (pingcap render)
  → rule flagged `ALTER TABLE t MODIFY a BLOB` on a BLOB column
    (no-op type change) as a type change.

LATENT FALSE-POSITIVE on the pingcap-typed advisor.

Omni's `DataType.Name` is the bare type name with no charset suffix.
The omni port builds the type string from `Name + Length + Scale +
Unsigned` (omniBuildColumnTypeString helper). Catalog "blob" matches
omni-built "blob" → false-positive silently fixed by this migration.

**Not a regression; a UX improvement.** Same shape as cumulative #18's
"is text" advice content fix.

**Why no fixture pin:** mock test catalog (MockMySQLDatabase.tech_book)
doesn't carry a BLOB column; adding one would have cross-engine impact
(mysql + tidb + mariadb + oceanbase all share it). The fix is locked
conceptually via plan-doc cumulative #21, empirically verified during
pre-batch protocol via parse-through-both-engines test.

## FIRST/AFTER detection for changing_order

Pingcap: `spec.Position.Tp != ast.ColumnPositionNone`.
Omni: `cmd.First || cmd.After != ""` (separate fields; both empty when
no position specified). Verified empirically.

## Helpers

In advisor_column_disallow_changing_type.go (local, not extracted):
- omniBuildColumnTypeString(dt *DataType) string — renders the
  "name[(length[,scale])] [unsigned]" form. Mirrors the mysql analog's
  inline rendering. Local because the rendering specifically targets
  the normalizeColumnType contract used only by this advisor.
- normalizeColumnType — kept structurally identical to the pingcap-
  typed predecessor + mysql analog for fixture parity.

## Cumulative #21 added to plan doc

Documents the BLOB/BINARY charset-suffix latent false-positive fix.
Mysql parallel verified free of the same issue (mysql analog rebuilt
type string from DataType during omni migration, same as my tidb port).

## Mysql cardinality divergence noted but not filed

The three mysql analogs emit per-cmd (no break), which differs from
both pre-omni mysql (likely had break) and pingcap-tidb (had break).
This is a potential mysql regression introduced by the omni migration,
but verifying pre-omni behavior requires git-show + parse test that's
out of scope for this PR. Flagged here for future investigation under
plan invariant #10 if/when a customer reports duplicate advices on
multi-CHANGE statements.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(advisor/tidb): extract single-shot alter-table helper + fix DECIMAL(N)/ZEROFILL type rendering (Codex round-2 on batch 11)

Two Codex P2 findings + one Sonar duplication signal addressed:

## Codex P2 #1: zero-scale DECIMAL rendering

Empirical reproduction:
  pingcap: DECIMAL(10) → Tp.String() = "decimal(10,0)"   (explicit 0 scale)
  pingcap: DECIMAL(10,0) → Tp.String() = "decimal(10,0)" (canonicalized)
  omni (current): both → DataType{Length=10, Scale=0}
  omni-built (pre-fix): "decimal(10)"   (Scale=0 → no comma form)

Catalog stores "decimal(10,0)" from INFORMATION_SCHEMA — false-positive
on no-op MODIFY when user writes DECIMAL(10) or DECIMAL(10,0).

Fix: omniBuildColumnTypeString now renders explicit scale when
Length > 0 AND the type is decimal-family (decimal/numeric/fixed/
float/double/real). New helper isDecimalFamilyTypeName mirrors the
MySQL canonicalization convention.

## Codex P2 #2: ZEROFILL rendering

Empirical reproduction:
  pingcap: INT ZEROFILL → Tp.String() = "int(11) UNSIGNED ZEROFILL"
  pingcap: INT UNSIGNED ZEROFILL → same
  omni (current): DataType{Unsigned: false-or-true, Zerofill: true}
  omni-built (pre-fix): "int" / "int unsigned"   (zerofill dropped)

Fix: append " zerofill" when Zerofill is set; treat Zerofill as
implying Unsigned in the rendering (pingcap convention; MySQL storage
requires UNSIGNED whenever ZEROFILL is set).

Caveat documented in code: MySQL canonicalizes ZEROFILL display widths
to maximums (int 11 → 10 etc.); the normalizeColumnType map doesn't
cover zerofill cases, so display-width canonicalization differences
may still surface as false-positives on rare ZEROFILL no-op modifies.
Out of scope to fix (ZEROFILL is deprecated in MySQL 8.0.17+).

## Sonar duplication: extract firstAlterCommandMatching

PR #20302 was flagged at 18.8% duplicated lines on new code (59.1% on
advisor_column_disallow_changing.go, 54.5% on advisor_column_disallow_changing_order.go).
The flagged duplication was the same shape across both single-shot
advisors:

  for _, ostmt := range stmts {
      alter, ok := ostmt.Node.(*ast.AlterTableStmt)
      if !ok { continue }
      for _, cmd := range alter.Commands {
          if cmd == nil { continue }
          if <matcher> { append advice; break }
      }
  }

Extracted firstAlterCommandMatching(n, matcher) int in utils.go —
single matcher callback, returns first matching index or -1. Each
single-shot advisor's body collapses to one matcher closure + one
conditional advice emit. Same shape as batch 8's collectIndexFamily*
and batch 10's collectColumnViolations helpers.

advisor_column_disallow_changing_type is structurally different
(catalog lookup, no break) — does NOT use this helper.

## No fixture pins for the rendering fixes

Same constraint as cumulative #21's original framing: the mock test
catalog (MockMySQLDatabase.tech_book + orders) has no decimal(N,0) or
ZEROFILL columns; adding them would have cross-engine impact. The
fixes are locked conceptually via the cumulative #21 plan-doc entry +
the empirical verification recorded in this commit message.

Net: net-zero LOC (helper offsets extraction); duplication eliminated;
two latent false-positive cases fixed; cumulative #21 strengthened.
TestTiDBRules + cross-engine + lint + build all green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): restrict zero-scale rendering to exact-decimal types (Codex round-3 on batch 11)

Codex P2 correctly flagged: my omniBuildColumnTypeString from the
previous round treated float/double/real like DECIMAL — always
rendering (length, scale) when Length > 0. Empirical verification
shows MySQL/pingcap behavior differs between exact and approximate
numeric types:

  Exact-precision (DECIMAL / NUMERIC / FIXED):
    DECIMAL       → pingcap "decimal(10,0)"  (defaults applied)
    DECIMAL(10)   → pingcap "decimal(10,0)"  (canonicalized)
    DECIMAL(10,0) → pingcap "decimal(10,0)"
    DECIMAL(10,2) → pingcap "decimal(10,2)"

  Approximate (FLOAT / DOUBLE / REAL):
    FLOAT         → pingcap "float"
    FLOAT(10)     → pingcap "float"          (precision hint DROPPED)
    FLOAT(10,0)   → pingcap "float"          (precision hint DROPPED)
    FLOAT(10,2)   → pingcap "float(10,2)"    (only when scale > 0)
    DOUBLE(10,0)  → pingcap "double"         (same shape)
    REAL          → pingcap "double"         (REAL is aliased to DOUBLE)
    REAL(10,0)    → pingcap "double"
    REAL(10,2)    → pingcap "double(10,2)"

Catalog stores what MySQL info_schema returns — same canonicalization.

Pre-fix behavior with float/double/real in the decimal-family list:
  MODIFY price FLOAT(10) → my port renders "float(10,0)"
  Catalog (real DB):     stores "float"
  Comparison:            DIFFERENT → false-positive

Post-fix (this commit):
- isExactDecimalTypeName covers decimal / numeric / fixed only.
- isApproximateFloatTypeName covers float / double / real — these
  drop both Length and Scale unless Scale > 0 (rendering bare type
  name instead).
- All other types: render (Length) when Length > 0, no scale.

Honest reflection: third rendering shape divergence Codex has caught
on this single advisor (cumulative #21 BLOB BINARY → batch-11 commit-1
DECIMAL(10,0) + ZEROFILL → this commit FLOAT/DOUBLE/REAL). The common
pattern is that pingcap's `Tp.String()` has many type-specific
canonicalization rules that my builder needs to mirror per-family,
not generalize across. The "approximate vs exact" split is the
canonical MySQL distinction and should be the structural framing
for future migration of any rule that renders type-strings for
comparison.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): preserve ENUM/SET value literals in type rendering (Codex round-4 on batch 11)

Codex P2 round-4 correctly flagged: omniBuildColumnTypeString dropped
EnumValues entirely. Empirical reproduction:

  Pingcap ENUM('x','y','z')  → Tp.String() = "enum('x','y','z')"
  Pingcap SET('a','b','c')   → Tp.String() = "set('a','b','c')"
  Omni                       → DataType{Name="ENUM", EnumValues=[x y z]}
  Omni-built (pre-fix)       → "enum"  (values dropped entirely)

Catalog stores the literal value list via MySQL info_schema's
COLUMN_TYPE column (verified per backend/plugin/db/tidb/sync.go).

Pre-fix false-positive:
  catalog: "enum('a','b')"
  user:    ALTER TABLE t MODIFY status ENUM('a','b')   (no-op)
  omni-built: "enum"
  comparison: "enum('a','b')" ≠ "enum" → flagged as type change

Fix:
- isEnumOrSetTypeName(lower) covers "enum" and "set".
- formatEnumValueList(values []string) renders the canonical
  `'v1','v2',…` form with single-quote escaping (' → '') per SQL
  convention. Matches pingcap's Tp.String() output AND info_schema's
  COLUMN_TYPE rendering.
- ENUM/SET case takes precedence over length/scale rendering in the
  switch (the value list is the canonical body for these types).

Honest pattern recognition: this is the FOURTH round of Codex catches
on omniBuildColumnTypeString in a single batch:
  #1 (cumulative #21):  BLOB/TINYBLOB/VARBINARY " BINARY" suffix
  #2 (round-2 commit):  DECIMAL(N) zero-scale + ZEROFILL drop
  #3 (round-3 commit):  FLOAT/DOUBLE/REAL precision-hint drop
  #4 (this commit):     ENUM/SET value literal drop

The common failure mode: my builder enumerates the DataType fields I
already knew about (Name + Length + Scale + Unsigned + Zerofill) but
silently drops fields I didn't anticipate (Charset annotations,
EnumValues). The discipline I should have started with:

  enumerate ALL DataType fields (Name, Length, Scale, Unsigned,
  Zerofill, Charset, Collate, EnumValues, ArrayDim, SRID) and ask
  per-field: does pingcap's Tp.String() render it? Does the catalog
  store it? If yes to either, my builder must render it.

That's the field-by-field audit that would have caught all four
rounds in one pass. Adding to the pre-batch protocol for any future
advisor that uses Tp.String() comparison against a catalog string.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): canonicalize BOOLEAN ↔ tinyint(1) in normalizeColumnType (Codex round-5 on batch 11)

Codex P2 round-5 correctly flagged: omniBuildColumnTypeString renders
DataType.Name verbatim ("boolean"), but TiDB/MySQL stores BOOLEAN
columns as tinyint(1) (verified via backend/plugin/schema/tidb/get_database_metadata.go:437-438
and backend/plugin/schema/mysql/get_database_metadata.go:259-261).

Empirical reproduction:

  Pingcap BOOL/BOOLEAN     → Tp.String() = "tinyint(1)"  (canonicalized)
  Pingcap TINYINT(1)       → Tp.String() = "tinyint(1)"  (same)
  Pingcap TINYINT (bare)   → Tp.String() = "tinyint(4)"  (default width)
  Omni BOOL/BOOLEAN        → DataType{Name="BOOLEAN", Length=0}
  Omni TINYINT(1)          → DataType{Name="TINYINT", Length=1}

Catalog stores "tinyint(1)" for boolean columns.

Pre-fix false-positive:
  catalog:    "tinyint(1)"
  user:       ALTER TABLE t MODIFY x BOOLEAN   (no-op against a boolean column)
  omni-built: "boolean"
  normalize:  default branch → identity → "boolean"
  comparison: "boolean" ≠ "tinyint(1)" → flagged as type change

Fix: add `"boolean"` and `"bool"` cases to normalizeColumnType, both
mapping to `"tinyint(1)"`. Placed in normalizeColumnType rather than
omniBuildColumnTypeString because the normalize layer is where the
existing canonicalization conventions live (integer default widths
already follow this pattern: `int` ↔ `int(11)`, `tinyint` ↔ `tinyint(4)`).

Both sides of the comparison go through normalize, so the canonicalization
is symmetric — works regardless of which side originally rendered which
form.

Codex round-5 of 5 on omniBuildColumnTypeString in this PR:
  #1 BLOB/TINYBLOB/VARBINARY " BINARY" suffix     → cumulative #21
  #2 DECIMAL zero-scale + ZEROFILL drop           → round-2 commit
  #3 FLOAT/DOUBLE/REAL precision-hint drop        → round-3 commit
  #4 ENUM/SET value-literal drop                  → round-4 commit
  #5 BOOLEAN ↔ tinyint(1) alias normalization     → this commit

The field-by-field DataType audit (Name, Length, Scale, Unsigned,
Zerofill, Charset, Collate, EnumValues, ArrayDim, SRID) that I
identified in round-4 as the missing pre-batch discipline would NOT
have caught round-5 — this is a TYPE-NAME canonicalization concern,
not a field-rendering concern. The pre-batch discipline addition for
this class: when porting a rule that compares type-strings against
the catalog, audit normalizeColumnType for alias coverage by
cross-referencing MySQL's documented type aliases (BOOL/BOOLEAN ↔
TINYINT(1), DEC ↔ DECIMAL, REAL ↔ DOUBLE, INTEGER ↔ INT, etc.) and
backend/plugin/schema/*/get_database_metadata.go's explicit
normalization map.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): canonicalize bare DECIMAL/NUMERIC/FIXED to decimal(10,0) (Codex round-6 on batch 11)

Codex P2 round-6 correctly flagged: omniBuildColumnTypeString renders
bare DECIMAL (no parens) as "decimal" (Length=0 → default case),
while pingcap Tp.String() and MySQL INFORMATION_SCHEMA both
canonicalize to "decimal(10,0)" (MySQL defaults M=10, D=0).

Pre-fix false-positive:
  catalog:    "decimal(10,0)"
  user:       ALTER TABLE t MODIFY amount DECIMAL   (no-op against decimal(10,0))
  omni:       Name="DECIMAL" Length=0 Scale=0
  omni-built: "decimal"
  normalize:  default branch → identity → "decimal"
  comparison: "decimal" ≠ "decimal(10,0)" → flagged as type change

Empirical (from prior verification): pingcap renders bare DECIMAL,
DECIMAL(10), and DECIMAL(10,0) all as "decimal(10,0)" — MySQL applies
the default precision regardless of which syntactic form the user
typed.

Fix: add `"decimal"`, `"numeric"`, `"fixed"` cases to
normalizeColumnType, all mapping to `"decimal(10,0)"`. NUMERIC and
FIXED are MySQL aliases for DECIMAL — included defensively for the
same canonicalization story. Same layer as the round-5 BOOLEAN
canonicalization and the integer default-width entries — symmetric
across both sides of the comparison.

Codex round-6 of 6 (so far) on the type-string comparison path:
  #1 BLOB/TINYBLOB/VARBINARY " BINARY" suffix     → cumulative #21
  #2 DECIMAL zero-scale + ZEROFILL drop           → round-2 commit
  #3 FLOAT/DOUBLE/REAL precision-hint drop        → round-3 commit
  #4 ENUM/SET value-literal drop                  → round-4 commit
  #5 BOOLEAN ↔ tinyint(1) alias                   → round-5 commit
  #6 bare DECIMAL ↔ decimal(10,0) default         → this commit

Refined pre-batch protocol going forward (third axis added):

  For any rule that compares type-strings against catalog, audit:
    (a) omniBuildColumnTypeString — every DataType field that
        pingcap renders or catalog stores. Field-by-field check
        across Name, Length, Scale, Unsigned, Zerofill, Charset,
        Collate, EnumValues, ArrayDim, SRID.
    (b) normalizeColumnType — every MySQL type alias and default-
        application. Cross-reference backend/plugin/schema/*/
        get_database_metadata.go's normalization map.
    (c) Bare-form defaults — for every type with implicit precision
        defaults (DECIMAL → (10,0), INT → (11), TINYINT → (4), etc.),
        verify the bare-form is canonicalized at the normalize layer.

Axes (a) caught rounds 2-4. (b) caught round 5. (c) caught round 6.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(advisor/tidb): pin type-rendering rounds 1-7 via pure-function unit tests; close round-7 risk with BIT/BINARY/YEAR canonicalization

Peer review on PR #20302 flagged three concerns. Addressing all three.

## #1: Pure-function unit tests (peer recommendation #1)

Six rounds of Codex catches across the PR all carried the same
"no fixture pin due to shared mock catalog cross-engine impact"
deferral. Peer correctly identified this as a test-infrastructure
limitation, not a fundamental constraint — the fixes are pure
(DataType, string) → string transforms and can be tested package-
locally without the advisor/catalog framework.

Added advisor_column_disallow_changing_type_test.go with three
suites:

  - TestOmniBuildColumnTypeString: 31 cases covering rounds 1-7
    (BLOB family, exact-decimal, ZEROFILL, FLOAT/DOUBLE/REAL,
    ENUM/SET escape, bare-form defaults, regulars).

  - TestNormalizeColumnType: 18 cases covering all canonicalization
    rules (BOOLEAN ↔ tinyint(1), bare DECIMAL ↔ decimal(10,0),
    bare BIT/BINARY/YEAR ↔ defaults, integer default widths,
    identity branch).

  - TestTypeStringRoundTripAgainstNormalize: 24 cases asserting the
    load-bearing invariant for columnTypeChanged — that
    normalize(omni-built) == normalize(catalog) across every
    no-op-modify scenario we've fixed. Plus negative cases (actual
    type changes) confirming the invariant correctly rejects them.

~120 LOC of tests covering every Codex round's empirical receipt.
Locks the contract in CI; future refactors of the type-rendering
path that re-introduce any class of bug will fail these tests.

## #2: Round-7 risk check (peer recommendation #2)

Ran the harness across edge type families (BIT, JSON, GEOMETRY,
spatial, BINARY/VARBINARY, COLLATE columns, DATETIME with fractional
seconds, YEAR, integer display widths). Surfaced THREE real round-7
candidates pre-merge:

  - bare BIT     → pingcap "bit(1)",     omni-built "bit"     (mismatch)
  - bare BINARY  → pingcap "binary(1)",  omni-built "binary"  (mismatch)
  - bare YEAR    → pingcap "year(-1)",   omni-built "year"    (mismatch, plus pingcap render is itself a `-1` sentinel artifact; catalog stores year(4))

Plus two expected-divergence cases (cumulative #21 shape — pingcap-
side ` BINARY` / ` CHARACTER SET` annotation artifacts that omni
correctly omits, matching catalog).

Fixed all three at the normalize layer (same as BOOLEAN round-5 +
DECIMAL round-6 pattern):

  case "bit":    return "bit(1)"
  case "binary": return "binary(1)"
  case "year":   return "year(4)"

Symmetric across both comparison sides; bounded by the empirical
verification harness; locked by the unit tests above.

The round-7 risk check finished with all remaining cases either
matching or known-fix-direction. Strong confidence the type-string
comparison path is settled.

## #3: Mysql cardinality claim verified (peer recommendation #3)

Previous PR description's batch-11 commit body claimed:
  "potential mysql regression introduced by the mysql omni
   migration ... verifying pre-omni behavior requires git-show
   + parse test that's out of scope for this PR"

This was an unverified-regression assertion — exactly the class
plan invariant #10 was designed to prevent.

Verified empirically (~3 minutes via git-show 699ff60^):
pre-omni mysql also emitted per-cmd, no break. The "for each item
in AlterListItem ... AddAdvice(...) ..." loop is structurally
identical to the post-omni "for each cmd ... AddAdvice(...)"
loop. Per-cmd cardinality is LONG-STANDING pre-omni behavior
preserved by the omni migration, NOT a regression.

Plan-doc cumulative table entry not needed (no novel pattern;
matches existing #19 framing). PR description correction will
follow in the merge body.

## Net for PR #20302

  - 6 Codex rounds of type-rendering fixes (rounds 1-6)
  - Round-7 risk check found 3 more pre-merge fixes (BIT/BINARY/YEAR)
  - 49 unit-test cases pinning every round's contract
  - mysql cardinality claim corrected per empirical verification

The type-string comparison path on this advisor is now bounded by
unit tests in CI. Future regressions of the rounds-1-7 classes
have a test guard.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): move bare-form canonicalization into builder for composability with UNSIGNED/ZEROFILL modifiers (Codex round-8 on batch 11)

Codex P2 (two comments, one underlying issue) + peer review correctly
identified the same gap: bare-form types with modifiers (UNSIGNED,
ZEROFILL) bypassed both the builder's family-specific bare-form
handling AND the normalize layer's canonicalization map.

Empirical reproduction (pingcap renders the canonical default
precision EVEN for bare forms with modifiers):

  DECIMAL UNSIGNED         → "decimal(10,0) UNSIGNED"
  DECIMAL ZEROFILL         → "decimal(10,0) UNSIGNED ZEROFILL"
  DECIMAL UNSIGNED ZEROFILL→ "decimal(10,0) UNSIGNED ZEROFILL"
  INT ZEROFILL             → "int(11) UNSIGNED ZEROFILL"
  TINYINT ZEROFILL         → "tinyint(4) UNSIGNED ZEROFILL"

Catalog stores the same canonical form. My pre-fix builder emitted
"decimal unsigned" / "int unsigned zerofill" — false-positive on
no-op `MODIFY x DECIMAL UNSIGNED` against a `decimal(10,0) unsigned`
column. normalizeColumnType had entries for bare names ("decimal",
"int") but not for bare + modifier ("decimal unsigned",
"int unsigned zerofill") — the Cartesian product is unbounded as a
normalize map.

## Structural fix: canonicalize at builder, not normalize

Per peer recommendation, the fix moves canonicalization to the
builder. Round-3's FLOAT/DOUBLE/REAL path was already builder-based;
making the bare-form canonicalization builder-based too means the
canonical-base + modifier suffix composes naturally:

  buildBaseTypeForm: emits canonical body (decimal(10,0), int(11),
                     tinyint(4), bit(1), binary(1), year(4),
                     tinyint(1) for BOOLEAN — via the new
                     canonicalBareTypeForm map). FLOAT/DOUBLE/REAL
                     keep their bare-form (no MySQL default).
  Modifier suffixes:  appended once at the end (unsigned + zerofill).

Now any bare-form × {UNSIGNED, ZEROFILL, UNSIGNED ZEROFILL}
combination composes correctly without combinatorial normalize
entries. New normalize entries from rounds 5/6/7 (BOOLEAN ↔ tinyint(1),
bare DECIMAL ↔ decimal(10,0), bare BIT/BINARY/YEAR ↔ defaults) stay
as belt-and-suspenders — they handle the case where catalog ever
stores a bare form, even though catalog typically canonicalizes.

## Discipline refinement: axis (c) was incomplete

Post-round-6 the discipline had three axes:
  (a) Field-by-field DataType audit         (caught rounds 2, 3, 4)
  (b) Type-name alias canonicalization      (caught round 5)
  (c) Bare-form defaults                    (caught round 6)

Round 8 reveals (c) was only enumerating bare-form, not bare-form ×
{applicable modifiers}. Revised:

  (c-revised) For each canonical-bare-form type family, enumerate
              the Cartesian product with applicable modifiers
              (UNSIGNED, ZEROFILL, UNSIGNED ZEROFILL for integer +
              exact-decimal families; verify no modifiers for
              BIT/BINARY/YEAR/BOOLEAN; verify FLOAT-family does
              NOT apply default precision under modifiers).

## Tests

Updated TestOmniBuildColumnTypeString:
  - Replaced old round-2 "INT ZEROFILL → int unsigned zerofill"
    case (which was pinning the broken bare-form behavior) with
    the canonical "int(11) unsigned zerofill" output.
  - Added 18 new cases covering the Cartesian product:
    DECIMAL/NUMERIC × {UNSIGNED, ZEROFILL, UNSIGNED+ZEROFILL},
    TINYINT/SMALLINT/MEDIUMINT/INT/BIGINT × {ZEROFILL, UNSIGNED+ZEROFILL,
    UNSIGNED}, FLOAT/DOUBLE × {UNSIGNED} (verifies float-family
    keeps bare form even under modifier).

Updated TestTypeStringRoundTripAgainstNormalize:
  - 6 new round-8 no-op-modify scenarios with bare-form × modifier.
    Catalog stores canonical (decimal(10,0) unsigned etc.); builder
    now produces matching form. normalize(both) yields equal strings.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): canonicalize NUMERIC alias to decimal in builder (Codex round-9 on batch 11)

Codex P2 round-9 correctly flagged: buildBaseTypeForm renders the
exact-decimal family verbatim from DataType.Name. NUMERIC(8,2)
emits "numeric(8,2)"; catalog stores "decimal(8,2)" (per
backend/plugin/schema/tidb/get_database_metadata.go line ~440
which does `strings.Replace(fullType, "numeric", "decimal", 1)`).

Empirical reproduction:

  Pingcap NUMERIC(8,2)   → Tp.String() = "decimal(8,2)"  (canonicalized)
  Pingcap FIXED(8,2)     → Tp.String() = "decimal(8,2)"  (canonicalized)
  Pingcap DEC(8,2)       → Tp.String() = "decimal(8,2)"  (canonicalized)
  Omni NUMERIC(8,2)      → Name="NUMERIC" Length=8 Scale=2   (NOT canonicalized)
  Omni FIXED(8,2)        → Name="DECIMAL" Length=8 Scale=2   (pre-canonicalized)
  Omni DEC(8,2)          → Name="DECIMAL" Length=8 Scale=2   (pre-canonicalized)
  Omni INTEGER(8)        → Name="INT" Length=8               (pre-canonicalized)

So omni pre-normalizes FIXED / DEC / INTEGER but NOT NUMERIC. My
builder previously emitted whatever Name carried — NUMERIC(8,2)
produced "numeric(8,2)" verbatim.

Fix: render the canonical name "decimal" for the exact-decimal
family in buildBaseTypeForm. Same builder-layer pattern as round-8
(canonicalize at the source rather than relying on normalize map
entries). Defensively keeps FIXED in the isExactDecimalTypeName
list even though omni currently pre-normalizes it — future grammar
changes won't introduce a regression.

## Tests

Updated TestOmniBuildColumnTypeString:
  - Replaced "NUMERIC(8) → numeric(8,0)" case (which was pinning
    the broken non-canonicalized output) with "NUMERIC(8) → decimal(8,0)".
  - Added new cases: NUMERIC(8,2), FIXED(8,2) (defensive), and
    NUMERIC(10,2) UNSIGNED.

Updated TestTypeStringRoundTripAgainstNormalize:
  - 2 new round-9 no-op-modify scenarios: NUMERIC(8,2) against
    catalog "decimal(8,2)" and bare NUMERIC against catalog
    "decimal(10,0)".

## Discipline refinement (axis-b extension)

Round 9 reveals axis (b) — type-name alias canonicalization — also
applies to TYPED-form aliases, not just bare-form aliases. Round 5
covered BOOLEAN ↔ tinyint(1) (bare alias). Round 9 covers
NUMERIC(M,D) ↔ DECIMAL(M,D) (typed alias preserving M,D).

Revised (b): For each type alias MySQL canonicalizes, verify the
canonicalization in BOTH the bare form AND every typed form
(typed length/scale/precision combinations). Bare form lives in
canonicalBareTypeForm; typed form lives in buildBaseTypeForm.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…k (Phase 1.5 §1.5.N+1 — closes Phase 1.5) (#20340)

* feat(parser/tidb): flip dispatcher to omni-first with pingcap fallback (Phase 1.5 §1.5.N+1)

Closes Phase 1.5 advisor migration. Replaces the pingcap-only
parseTiDBStatements registration with parseTiDBStatementsOmni
implementing Option B per plan §1.5.0 invariant #8: omni first,
pingcap fallback per statement on omni parse failure. The review
never hard-fails at the dispatcher level on a Tier 4 grammar gap.

Per-fallback observability, both surfaces required (sub-contract):
- Counter tidb_dispatcher_omni_fallback_total{reason} drives the
  eventual Option B → A retirement decision. Debug logs are dropped
  before reaching aggregation pipelines and cannot serve this role.
- slog.Debug per fallback for per-report developer signal.

Reason classifier matches against (omni err msg) + " " + (input SQL),
case-insensitive. Empirical finding (invariant #9): omni reports
"unexpected token after CREATE" for CREATE SEQUENCE — the keyword
SEQUENCE never appears in the error string. Without input-side
matching the SEQUENCE telemetry signal would be silently lost.

Empirical correction to plan §1.5.0 #5 (separate plan-doc edit):
the suggested FLASHBACK TABLE foo TO BEFORE DROP regression-test
input is rejected by BOTH pingcap AND omni — would test the
both-engines-reject path, not Option B. BATCH ON id LIMIT N DELETE
is the empirically-correct Option B input (omni rejects, pingcap
accepts). Both paths now have dedicated regression tests.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(advisor/tidb): simplify getTiDBOmniNodes to 3-arm switch on dispatcher AST (Phase 1.5 §1.5.N+1)

Post-flip the dispatcher already parsed each statement at split time
and populated stmt.AST. Removes the per-statement re-parse from
getTiDBOmniNodes; replaces with a 3-arm type switch per plan §1.5.0
invariant #8:

  - *tidbparser.OmniAST: omni accepted — collect the node.
  - *tidbparser.AST: dispatcher fell back to pingcap (omni rejected).
    Skip — migrated advisors emit no advice for omni-rejected SQL;
    un-migrated advisors using getTiDBNodes still see the pingcap AST.
  - default: warn (mandatory — a future engine introducing a third AST
    type for tidb shouldn't silently drop statements).

Soft-fail responsibility moves from this helper to the dispatcher;
the end-to-end soft-fail invariant is preserved at the new layer.

Cache contract (single-parse-per-review) preserved — the result slice
is still memoized on checkCtx.Memo. The cache now amortizes type-
switch + slice construction rather than the parse itself.

Test updates:
- TestGetTiDBOmniNodesCachesAcrossRules: feeds *OmniAST directly
  (was raw text expecting internal parse).
- TestGetTiDBOmniNodesSoftFailsOnGrammarGap renamed to
  TestGetTiDBOmniNodesSkipsPingcapFallbackAST: pins the *AST-arm
  skip behavior (was pinning internal parse soft-fail). End-to-end
  soft-fail is now exercised by TestParseTiDBStatementsOmni_OptionBFallback
  in the parser package.
- TestRunNamingConventionRuleEmitsInternalErrorAdvice: feeds
  synthetic *OmniAST.

Bridge cost note added to advisor_builtin_prior_backup_check.go:
post-flip, getTiDBNodes triggers per-statement bridge re-parse via
OmniAST.AsPingCapAST. Bounded to the dual-path advisor + dml_dry_run
(2 deliberate consumers, not migration debt). Lazy + cached per
OmniAST instance per invariant #4. No fix needed; bridge cleanup
happens in Phase 2 when dml_dry_run migrates.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(server): fold default registry into /metrics gatherer

The /metrics endpoint at backend/server/echo_routes.go served only
the local custom registry used to insulate echo HTTP middleware
metrics from duplicate-registration in tests. Metrics registered
via promauto (against prometheus.DefaultRegisterer) — including
backend/store/db_metrics.go's metadataDBQueryDuration — were never
exposed at /metrics.

Fold prometheus.Gatherers{registry, prometheus.DefaultGatherer} at
scrape time so both registries reach the endpoint while preserving
the registration-time test isolation contract.

Side effects (visible at /metrics post-merge):
- db_metrics.metadataDBQueryDuration: pre-existing metric becomes
  visible (bonus bug-fix; prior to this fold it was registered but
  never scraped).
- Go runtime metrics (go_goroutines, go_memstats_*,
  process_cpu_seconds_total, etc.) auto-registered to
  DefaultRegisterer by client_golang's init become visible.
  Operationally useful SRE surface; not previously exposed.
- tidb_dispatcher_omni_fallback_total{reason} added by the Phase 1.5
  §1.5.N+1 dispatcher flip becomes operational from day 1.

Required for invariant #8's retirement-gate sub-contract: the
counter MUST be visible at /metrics for production telemetry to
drive the eventual Option B → A retirement decision. A counter
registered but never scraped fails the same load-bearing test as
debug logs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(parser/tidb): escalate "unknown" fallback reason to slog.Warn

Per peer review on PR #20340. Production telemetry pipelines
typically drop slog.Debug before aggregation, so when the
tidb_dispatcher_omni_fallback_total{reason="unknown"} counter
increments (an unrecognized grammar gap surfaced) ops sees the
number but has no signal — no input excerpt, no error string —
to triage what new classifier pattern to add.

Without that signal the Option B → A retirement gate cannot drive
"unknown" to zero, because ops can't act on a metric they can't
explain.

Known reasons (flashback / sequence / batch_dml) stay at Debug:
high-frequency, expected, counter-tracked. Only "unknown" escalates
— bounded volume by definition (it should be rare; if it isn't,
that's a signal the classifier needs new patterns).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(parser/tidb): dedupe ParseTiDBForSyntaxCheck via parsePingCapSingleStatement

Per SonarCloud finding on PR #20340 — dispatcher.go's
parsePingCapSingleStatement duplicated the per-statement loop body
of ParseTiDBForSyntaxCheck (~13 LOC of parser invocation, error
conversion, single-node check, line-tracking, and *AST construction).

Refactor ParseTiDBForSyntaxCheck to call parsePingCapSingleStatement,
collapsing the loop body to a 5-line per-statement pattern. The
helper's (*AST, nil) | (nil, nil) | (nil, err) signature handles
the three cases the original inline body covered:
  - successful single-node parse → append
  - non-1 node count → continue (skip)
  - hard parser error → return error

Single source of truth for "parse one TiDB statement and produce
*AST" now lives in parsePingCapSingleStatement. Both the dispatcher
fallback path and ParseTiDBForSyntaxCheck callers (omni_test.go
bridge parity test, tidb_test.go syntax error tests) get identical
behavior — line-tracking parity verified by existing
TestAsPingCapASTLineTrackingMatchesCanonical and new
TestParsePingCapSingleStatement_LineTrackingMatchesCanonical.

Behavioral change: ParseTiDBForSyntaxCheck previously instantiated
ONE parser outside the loop; now creates one per statement via
the helper. Pingcap parser is stateless after configuration
(newTiDBParser just calls New() + EnableWindowFunc(true)), so
per-call instantiation is safe. Extra allocation overhead is
negligible at advisor-review statement counts.

Net: -32 / +12 LOC in tidb.go.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(parser/tidb,server): preserve scrape self-instrumentation; count fallback only on pingcap success

Two Codex P2 catches on PR #20340.

(1) /metrics handler self-instrumentation lost.
echoprometheus.NewHandlerWithConfig only applies
promhttp.InstrumentMetricHandler when its Gatherer also implements
prometheus.Registerer (echoprometheus/prometheus.go:129). The
prior commit's prometheus.Gatherers{registry, DefaultGatherer}
fold satisfied the Gatherer interface but NOT Registerer, so the
type assertion silently failed and the handler-self-instrumentation
wrap was dropped. Result: scrape-health metrics
(promhttp_metric_handler_requests_total, _in_flight, _errors_total)
were no longer exposed — operationally important for detecting
failed/slow scrapes.

Fix: bypass echoprometheus's helper for the handler. Use promhttp
directly with the local registry as Registerer (for self-
instrumentation) and the Gatherers fold as the gather source.
Both observability surfaces now coexist as designed. The middleware
registration above (echoprometheus.NewMiddlewareWithConfig) is
unchanged — it correctly receives the local registry as Registerer.

(2) Fallback counter inflated by both-engines-reject.
Original ordering incremented tidb_dispatcher_omni_fallback_total
BEFORE attempting parsePingCapSingleStatement. Statements rejected
by BOTH engines (genuine bad SQL) thus inflated the counter — most
acutely the "unknown" bucket, since malformed SQL won't match
Tier-4 keywords. This skewed the Option B → A retirement-gate
signal: even after omni grammar reaches completeness, customer-
side bad SQL would keep the counter non-zero indefinitely and
the gate would never fire.

Fix: move the counter increment + slog.Debug/Warn to AFTER
parsePingCapSingleStatement returns success. The counter now
measures "omni rejected AND pingcap accepted" — the cases that
genuinely justify Option B. Both-engines-reject still surfaces
omni's error to the customer (Q2 design choice preserved); only
the counter changes.

TestParseTiDBStatementsOmni_BothEnginesReject extended with a
no-counter-movement assertion to pin the new contract.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…olumns

Per Codex P1 follow-on catch on PR #20345 (review of Fix #4 self-
join determinism patch). The alias map I introduced in commit
137d077 stored map keys verbatim from omni AST, then did an
exact-string lookup in extractUpdateColumns. TiDB ACCEPTS
statements like

    UPDATE test T1 JOIN test T2 ON T1.c = T2.c SET t1.a = 1 ...

(alias declared as `T1` but referenced as `t1` in the SET clause —
TiDB identifier comparison is case-insensitive in practice).
Pre-fix, the map lookup `singleTables["t1"]` against entries keyed
"T1"/"T2" missed; fallback `EqualFold("t1", "test")` also missed;
result was empty updateColumns → invalid rollback SQL:

    INSERT INTO `db`.`test` SELECT * FROM `bbarchive`.`prefix_1_test` ON DUPLICATE KEY UPDATE ;

(empty ODKU clause = invalid TiDB SQL)

Codex validated TiDB acceptance externally by executing the input
against pingcap/tidb:v8.5.5. Reproduced empirically in the new
TestGenerateRestoreSQLSelfJoinMixedCaseAlias regression test.

Fix:

(1) collectTableRefs: lowercase the map key on insert
    (key = strings.ToLower(alias) or strings.ToLower(name)).

(2) extractUpdateColumns: lowercase the lookup key
    (singleTables[strings.ToLower(col.Table)]).

This matches the case-insensitive semantics already used elsewhere
in the same code path (Schema check uses EqualFold; table-name
fallback uses EqualFold; hasDisjointUniqueKey uses strings.ToLower
for column dedup).

Honest framing: this is a follow-on flaw from Fix #4 (commit
137d077). The original mysql code had a case-sensitive
matchedTable.Alias exact-match too, but the iteration
nondeterminism (Bug 4) was the more visible failure mode that
caught attention first. When refactoring to per-SET-clause map
lookup, I preserved the case-sensitive-exact-match without
thinking about it. Codex caught what Fix #4 should have addressed
simultaneously.

Mysql analog (mysql/restore.go) has the same case-sensitive
matchedTable.Alias exact-match — symmetric fix needed (different
code shape pre-refactor, but same case-sensitivity flaw); tracked
in BYT-9526.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(parser/tidb): add DML rollback support (closes BYT-7201)

Mirrors backend/plugin/parser/mysql/restore.go for TiDB. Closes
longstanding customer ticket BYT-7201 — pre-this-PR, TiDB rollback
preview returns hard error "engine TIDB is not supported"; mysql/
postgres/mssql/oracle all register GenerateRestoreSQL but TiDB
was the conspicuous miss.

Mechanical port of the mysql analog:
- restore.go: imports omni/tidb/ast (vs omni/mysql/ast),
  ParseTiDBOmni (vs ParseMySQLOmni), registers for Engine_TIDB
  only (mysql analog also registers for MARIADB; not applicable
  here).
- backup_test.go: adds Indexes to t_generated and test mock
  fixtures so hasDisjointUniqueKey can locate a unique key during
  UPDATE rollback tests. Mirrors the mysql mock shape exactly.
- docker/tidb.yaml: pingcap/tidb:v8.5.5 compose for local manual
  verification.

Negative-path coverage: TestGenerateRestoreSQLNoDisjointUniqueKey
pins the "UPDATE without disjoint unique key → error" path that
yaml golden tests can't cover (success-only format). Mysql analog
has the same gap and would benefit from a symmetric follow-up.

Container verification (against real TiDB v8.5.5):
- UPDATE rollback with generated column: round-trip restored
- DELETE rollback: round-trip restored
- UPDATE rollback with alias: executes cleanly

Bypasses the dispatcher (calls ParseTiDBOmni directly per mysql
precedent at mysql/restore.go:48,313) — independent of the in-
flight Phase 1.5 dispatcher flip (PR #20340).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(parser/tidb): case-insensitive database-qualifier match in tableExprReferences

Per Codex P2 catch on PR #20345. tableExprReferences used `db ==
database` (case-sensitive) for database comparison while the
table-name comparison on the same line used strings.EqualFold
(case-insensitive). The asymmetry silently dropped schema-
qualified references whose qualifier case differed from the
backup item's stored case (e.g., SQL has `UPDATE DB.test` while
backupItem stores `db`).

TiDB/MySQL identifier comparisons are typically case-insensitive
in practice, so the table-name side's EqualFold is the correct
shape; the database side should match. One-line fix:

    return strings.EqualFold(db, database) && strings.EqualFold(n.Name, table)

Pinned by TestGenerateRestoreSQLCaseInsensitiveDatabaseQualifier:
input uses uppercase `DB.test`, backupItem stores lowercase `db`,
asserts the rollback succeeds (pre-fix would surface "no DML
statement found").

Mysql analog (mysql/restore.go) has the same case-sensitivity
asymmetry; symmetric follow-up filed for parity.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(parser/tidb): union SET columns across multi-DML rollback bundle

Per Codex P1 catch on PR #20345. backup.go's single-table path
(generateSQLForSingleTable, triggered when count > maxMixedDMLCount)
bundles multiple same-table DMLs into one PriorBackupDetail_Item
spanning Start[first] → End[last]. restore.go's findFirstDML then
returned only the FIRST DML node from the bundle, so the generated
ON DUPLICATE KEY UPDATE list included only that first stmt's
columns — columns mutated by LATER UPDATEs in the bundle were left
mutated, producing incomplete rollback for valid batched backups.

Fix:

(1) findFirstDML → findMatchingDMLs returns ALL UPDATE/DELETE nodes
    referencing the target table (not just first).

(2) doGenerate accepts []ast.Node; partitions into UPDATE vs
    DELETE. If any UPDATE exists in the bundle, generates ODKU
    (pure INSERT SELECT would FAIL on the surviving UPDATE-modified
    rows due to duplicate key); pure-DELETE bundles fall back to
    INSERT SELECT (no surviving rows means no conflicts).

(3) generateUpdateRestore accepts []*ast.UpdateStmt; unions SET
    columns across the bundle (case-insensitive dedup via lowercase
    map; first-seen iteration order for deterministic ODKU output).
    The disjoint-unique-key check operates on the union — if any
    UPDATE in the bundle touched a column that overlaps every
    unique key, the rollback is unsafe (same semantic as before,
    just applied at the bundle level).

Existing fixtures still pass (their multi-stmt cases all touch
the same column → union = single column → identical output to the
pre-fix single-stmt path). New fixture pins the bug scenario:
6 UPDATEs on `test` interleaving SET a / SET b → expected ODKU
clause covers BOTH `a = VALUES(a), b = VALUES(b)`.

Mysql analog (mysql/restore.go) has the same multi-DML bug;
symmetric follow-up filed for parity.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(parser/tidb): exclude boundary stmt in extractStatement (EndPosition is exclusive)

Per Codex P1 catch on PR #20345. base/statement.go:16-18 documents
Statement.End as EXCLUSIVE — "points to the position AFTER the last
character of the statement." The end-boundary check in
extractStatement used `equalOrGreater(list[i].Start,
backupItem.EndPosition)` and set `end = i`, which INCLUDED stmts
whose Start equals EndPosition. Those stmts belong to the NEXT
backup item, not the current one.

In mixed-DML mode (each backup item maps to ONE stmt), this caused
extractStatement to bleed into the next backup item's stmt. The
generated rollback SQL then referenced columns from the wrong
backup scope.

The bug existed pre-Fix-1 (multi-DML union, commit be45bf2)
but findFirstDML masked it — only the first DML node from the
extracted text reached the rollback SQL, so the leaked stmt was
silently dropped. Post-Fix-1, findMatchingDMLs returns ALL matching
DMLs from the extraction, so the boundary bleed now contributes
columns from the next backup item to the union — producing extra
ODKU columns or false "no disjoint unique key found" errors.

Fix: change `end = i` to `end = i - 1` in the end-boundary loop.
The semantic is now correctly "end = largest i where Start <
EndPosition" matching the exclusive convention. If list[0].Start
>= EndPosition, end = -1 and the for-loop iterates zero times
(empty extraction — correct for the "no stmts in range" case).

TestGenerateRestoreSQLEndPositionExclusive pins the contract:
splits a 2-stmt input via SplitSQL to get the actual stmt[0].End
boundary, sets backupItem.EndPosition = stmt[0].End, asserts the
rollback ODKU contains stmt[0]'s column but NOT stmt[1]'s. Pre-
fix the test fails (both columns in ODKU); post-fix it passes.

Mysql analog (mysql/restore.go) has identical extractStatement
code — symmetric fix needed; tracked in BYT-9526.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(parser/tidb): deterministic alias resolution for self-join UPDATE rollback

Per Codex P1 catch on PR #20345. extractSingleTablesFromTableExprs
returns map[alias]*TableReference. Pre-fix, generateUpdateRestore
ranged over that map to pick a single matchedTable, then
extractUpdateColumns matched each qualified SET column against
matchedTable's alias.

For self-joins (`UPDATE test t1 JOIN test t2 ON ... SET t1.a = ...`),
both t1 and t2 alias the same physical table. Either could be picked
nondeterministically per Go's randomized map iteration. If t2 was
picked but the SET clause referenced t1, extractUpdateColumns
returned empty, producing invalid rollback SQL:

    INSERT INTO `db`.`test` SELECT * FROM `bbarchive`.`prefix_1_test` ON DUPLICATE KEY UPDATE ;

Reproduced empirically: TestGenerateRestoreSQLSelfJoinUpdate looped
50× pre-fix flakes around iteration ~18 with the empty-ODKU output.

Fix: change extractUpdateColumns signature to accept the full
singleTables map. For each qualified SET column, look up the
qualifier directly in the map; include the column if the entry's
Table equals originalTable. This is deterministic AND correct for
self-joins (each qualifier independently resolves to its own alias
entry, not a single pre-picked one).

Pre-fix signature:
    extractUpdateColumns(setList, db, _, *TableReference, normalCols)
Post-fix signature:
    extractUpdateColumns(setList, db, originalTable, map[string]*TableReference, normalCols)

Function is unexported, single caller — safe to change shape.

Mysql analog (mysql/restore.go) has identical map-iteration code —
symmetric fix needed; tracked in BYT-9526.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore(parser/tidb): extract errMsgFailedToGetSourceDB constant

Per SonarCloud Critical code-smell finding on PR #20345
(restore.go:33). The literal "failed to get source database ID for
%s" appeared 3× across GenerateRestoreSQL, doGenerate, and
extractStatement. Centralized as a named const at the top of the
file to prevent string drift.

Mysql analog (mysql/restore.go) has the same triplicate pattern
pre-existing — symmetric cleanup tracked in BYT-9526.

Sonar's other finding on this PR (generateUpdateRestore has 9
parameters > authorized 7) is left as-is: the function's 9 inputs
are conceptually distinct, the threshold is heuristic, and the
mysql analog has the same shape. Bundling into a struct adds
indirection without cognitive benefit; if pursued, do it
symmetrically across mysql + tidb in a separate refactor PR.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(parser/tidb): case-insensitive alias-map lookup in extractUpdateColumns

Per Codex P1 follow-on catch on PR #20345 (review of Fix #4 self-
join determinism patch). The alias map I introduced in commit
137d077 stored map keys verbatim from omni AST, then did an
exact-string lookup in extractUpdateColumns. TiDB ACCEPTS
statements like

    UPDATE test T1 JOIN test T2 ON T1.c = T2.c SET t1.a = 1 ...

(alias declared as `T1` but referenced as `t1` in the SET clause —
TiDB identifier comparison is case-insensitive in practice).
Pre-fix, the map lookup `singleTables["t1"]` against entries keyed
"T1"/"T2" missed; fallback `EqualFold("t1", "test")` also missed;
result was empty updateColumns → invalid rollback SQL:

    INSERT INTO `db`.`test` SELECT * FROM `bbarchive`.`prefix_1_test` ON DUPLICATE KEY UPDATE ;

(empty ODKU clause = invalid TiDB SQL)

Codex validated TiDB acceptance externally by executing the input
against pingcap/tidb:v8.5.5. Reproduced empirically in the new
TestGenerateRestoreSQLSelfJoinMixedCaseAlias regression test.

Fix:

(1) collectTableRefs: lowercase the map key on insert
    (key = strings.ToLower(alias) or strings.ToLower(name)).

(2) extractUpdateColumns: lowercase the lookup key
    (singleTables[strings.ToLower(col.Table)]).

This matches the case-insensitive semantics already used elsewhere
in the same code path (Schema check uses EqualFold; table-name
fallback uses EqualFold; hasDisjointUniqueKey uses strings.ToLower
for column dedup).

Honest framing: this is a follow-on flaw from Fix #4 (commit
137d077). The original mysql code had a case-sensitive
matchedTable.Alias exact-match too, but the iteration
nondeterminism (Bug 4) was the more visible failure mode that
caught attention first. When refactoring to per-SET-clause map
lookup, I preserved the case-sensitive-exact-match without
thinking about it. Codex caught what Fix #4 should have addressed
simultaneously.

Mysql analog (mysql/restore.go) has the same case-sensitive
matchedTable.Alias exact-match — symmetric fix needed (different
code shape pre-refactor, but same case-sensitivity flaw); tracked
in BYT-9526.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore(parser/tidb): remove unreachable nil-list defenses (YAGNI cleanup)

Per Codex P1 finding on PR #20345 + empirical verification.

Codex flagged that extractStatement could panic if ParseTiDBOmni
returned a nil List, citing the existing defense in findMatchingDMLs
as evidence that nil was reachable.

Empirically verified across 9 representative edge cases (empty
string, whitespace-only, lone semicolon, multiple semicolons, line
comment, block comment, mixed comment+semicolon variants) — omni's
underlying parser ALWAYS returns a non-nil *List with len(Items)
== 0 for these inputs. Nil is not reachable.

Resolution:

1. Did NOT add the defensive nil check to extractStatement.
   Adding code for an unreachable path is dead code — YAGNI.

2. REMOVED the symmetric defensive nil check from findMatchingDMLs
   (added in commit be45bf2 / Fix #1 without empirical
   justification). Same evidence, same conclusion: not reachable,
   not load-bearing.

3. REMOVED the test (TestGenerateRestoreSQLCommentOnlySegment)
   that pinned the comment-only-segment path. The test passed both
   pre- and post-fix because the bug doesn't reproduce; it pinned
   nothing useful.

If a future omni change introduces nil returns, both call sites
will need the defense added together — but adding it speculatively
today obscures the actual contract (omni returns non-nil).

Mysql analog has shipped the same shape (no nil check) for ~6
weeks without reported panics — independent corroboration that the
defense is unnecessary.

Net: -48 LOC of unused code paths.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(parser/tidb): UPDATE rollback must check SET-clause targets, not joined tables

Per peer review on PR #20345 (Finding 2). containsTable's UpdateStmt
arm previously matched the target table if it appeared anywhere in
n.Tables — including JOIN-only references. For

    UPDATE test2 JOIN test ON test2.c = test.c SET test2.a = 1 WHERE test2.c = 1;

with backupItem targeting `test`, the match incorrectly fired (test
is in n.Tables via the JOIN). extractUpdateColumns then correctly
returned no `test`-table columns (SET only mentions test2.a). Fix #1
(multi-DML union) saw the matched UPDATE in the bundle and emitted
ODKU with empty column list:

    INSERT INTO `db`.`test` SELECT * FROM `bbarchive`.`prefix_1_test` ON DUPLICATE KEY UPDATE ;

— invalid SQL.

Fix: introduce updateMutatesTable helper that checks whether the
UPDATE's SET clauses actually reference the target table. Resolution
mirrors extractUpdateColumns:

  - For each SET assignment, resolve the qualifier through the
    singleTables alias map (lowercased per Bug 5).
  - Unqualified columns in multi-table UPDATE are ambiguous;
    conservatively count as mutation when target is in scope.
  - Bare-table-name fallback for unaliased qualifiers.

containsTable's UpdateStmt arm now delegates to updateMutatesTable.
DELETE behavior unchanged (peer's specific finding was about UPDATE;
DELETE n.Using-vs-n.Tables semantics deferred to a follow-up
cleanup — existing fixtures don't exercise the bleed path there).

TestGenerateRestoreSQLJoinedNotMutated regression pin: backup item
for joined-only-not-mutated table must surface "no DML statement
found" (correct outcome — there's genuinely no DML to roll back),
NOT generate invalid SQL with empty ODKU.

Mysql analog (mysql/restore.go) has the same containsTable shape;
symmetric fix needed; tracked in BYT-9526.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(parser/tidb): hasDisjointUniqueKey skips UKs on generated/expression columns

Per peer review on PR #20345 (Finding 4). Pre-fix, disjoint() did
naive string comparison between index.Expressions and the SET-cols
map. For a UK on a generated column (e.g., c_generated = a + b),
updating columns {a, b} appeared "disjoint" from the UK's
Expressions ["c_generated"] — the string "c_generated" is not in
{a, b}.

But updating a or b CHANGES c_generated's value. The UK is NOT
safe for ON DUPLICATE KEY UPDATE matching: the rollback would
silently fail to match rows whose c_generated changed.

Same false-positive shape applies to functional indexes (e.g.,
Expressions = ["(LOWER(email))"] won't string-match {"email": true}).

Conservative fix: hasDisjointUniqueKey filters out UKs whose
Expressions don't all map to regular (non-generated) columns in
the table metadata. Such UKs are skipped entirely (treated as
overlapping). Never returns false-positive disjoint.

Trade-off: tables whose only "would-be-disjoint" UK is generated/
functional now correctly reject the rollback with "no disjoint
unique key found" instead of generating unsafe SQL. Strictly
safer; over-rejection rate bounded to tables with this specific
shape.

A more principled fix would walk Generation.Expression to extract
column refs and add them to the unsafe set — out of scope here;
conservative skip handles the common case correctly.

Mock fixture (backup_test.go): added a UK on c_generated to
t_generated for the regression test. Existing fixtures unchanged
(they UPDATE single columns where the regular UKs are still
disjoint, so the c_generated UK skip doesn't change their
outcome).

TestGenerateRestoreSQLGeneratedColumnUKSkipped regression pin:
UPDATE t_generated SET a = 1, b = 2 — overlaps PK on b AND UK on
a; pre-fix the c_generated UK would false-positive as disjoint
and produce unsafe rollback; post-fix all three UKs are correctly
classified (regular UKs not disjoint, generated UK skipped) and
the function returns the no-disjoint-key error.

Mysql analog (mysql/restore.go) has the same string-comparison
hasDisjointUniqueKey; symmetric fix needed; tracked in BYT-9526.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(parser/tidb): DELETE rollback must ignore USING-only tables

Per Codex P1 follow-on catch on PR #20345 — symmetric to the UPDATE-
side joined-not-mutated fix in commit c4042da.

Pre-fix containsTable's DeleteStmt arm matched both n.Tables AND
n.Using. For multi-table DELETE syntax `DELETE test FROM test, test2 ...`,
test is the explicit delete-target (in n.Tables) and test2 is JOIN-
only (in n.Using). With backupItem targeting test2, the match
incorrectly fired; doGenerate emitted

    INSERT INTO `db`.`test2` SELECT * FROM `bbarchive`.`prefix_1_test2`;

— which would re-introduce stale data into test2 if applied as
rollback (test2 was never actually deleted).

Per omni AST contract: DeleteStmt.Tables = explicit delete targets
(mutation set); DeleteStmt.Using = JOIN-only refs (filter set).
Only n.Tables counts as mutation.

Fix: drop the n.Using loop in containsTable's DeleteStmt arm.

Honest framing: I deferred this in commit c4042da (Finding 2 fix)
with the rationale "existing fixtures don't exercise the bleed
path." Codex correctly called out that deferral — the bug is the
same shape as the UPDATE-side bug I fixed in c4042da; deferring
was wrong. Should have addressed both arms together.

TestGenerateRestoreSQLDeleteUsingOnlyTable regression pin: backup
item for USING-only-not-deleted table must surface "no DML
statement found" (correct outcome — no DML mutates test2), NOT
generate rollback SQL that re-introduces stale data.

Existing fixture `DELETE test FROM test, test2 as t2 ...` with
originaltable: test still passes — test is in n.Tables (the
delete target). Single-table DELETE fixtures unaffected.

Mysql analog (mysql/restore.go) has the same containsTable shape
with the n.Using bleed; symmetric fix needed; tracked in BYT-9526.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(parser/tidb): resolve unqualified SET columns against target's actual columns

Per Codex P1 follow-on catch on PR #20345 — gap in the Bug 6 fix
(commit c4042da). updateMutatesTable's unqualified-column branch
returned true for any unqualified SET when the target was in scope,
regardless of whether the column actually existed on the target.
For

    UPDATE test JOIN t1 ON test.c = t1.c SET name = 1 WHERE test.c = 1;

(`name` exists on the joined table but not on `test`),
updateMutatesTable misclassified the UPDATE as mutating `test`;
extractUpdateColumns correctly returned no `test`-table columns;
doGenerate emitted invalid `... ON DUPLICATE KEY UPDATE ;` for `test`.

Fix: align updateMutatesTable's unqualified-column resolution with
extractUpdateColumns's (which already correctly checks normalColumns
for the target). Refactor:

  1. New getNormalColumnsLower helper fetches the target table's
     regular column set as a lowercased map[string]bool.
  2. GenerateRestoreSQL fetches once at the top (after sourceDatabase
     is resolved) and plumbs through extractStatement, findMatchingDMLs,
     containsTable, and updateMutatesTable.
  3. updateMutatesTable's unqualified branch checks the column against
     the target column set. Non-target columns no longer trigger a
     match.

Single-table UPDATEs with unqualified SETs (the common case, e.g.
`UPDATE test SET a = 1`) continue to work — `a` is in target's
normal columns.

Trade-off: one additional metadata fetch per GenerateRestoreSQL call
(classifyColumns and hasDisjointUniqueKey already each fetch
metadata). Could be optimized later by hoisting all three behind a
single fetch.

Two regression tests:

  - TestGenerateRestoreSQLUnqualifiedSetNonTargetColumn: pins the
    bug — unqualified SET on a column not on the target produces
    "no DML statement found" error (correct), not invalid empty
    ODKU SQL.
  - TestGenerateRestoreSQLUnqualifiedSetTargetColumn: pins the
    positive path — single-table UPDATE with unqualified SET on a
    target column still generates the correct ODKU.

Mysql analog (mysql/restore.go) has the same updateMutatesTable
shape via its containsTable fix; symmetric fix needed; tracked in
BYT-9526.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(parser/tidb): include database in alias-map qualifier resolution

Per Codex P1 catch on PR #20345. The alias-map qualifier resolution
in both updateMutatesTable AND extractUpdateColumns matched only on
entry.Table — never verified entry.Database. For cross-database
joins with homonymous tables —

    UPDATE db1.test t1 JOIN db2.test t2 ON t1.c = t2.c SET t2.a = 1 ...

— alias `t2` resolves to {Database: "db2", Table: "test"}. Pre-fix
condition `entry.Table == "test"` matched (because the table NAME
is "test"), so a backup item targeting db1.test mis-classified the
UPDATE as mutating db1.test even though the SET only touched
db2.test.

Two consequences:
  - updateMutatesTable returned true → the UPDATE was added to the
    matching DMLs for db1.test → rollback generation kicked off.
  - extractUpdateColumns then matched the SET column under the
    wrong database, treating it as a column on db1.test → could
    restore or overwrite unrelated rows when applied as rollback.

Fix: both branches now require `entry.Database == database` AND
`entry.Table == table` (case-insensitive).

The bare-table fallback (qualifier IS the bare table name, not an
alias) is unchanged — col.Schema check at the top of the loop
already filters out explicit-schema mismatches before the fallback
sees them.

TestGenerateRestoreSQLCrossDatabaseAliasResolution regression pin:
cross-DB alias (t2 → otherdb.test) must not match a backup item
targeting db.test. Pre-fix produced rollback for the wrong table;
post-fix surfaces "no DML statement found" (correct — no DML
mutates db.test).

Mysql analog (mysql/restore.go) has the same alias-map shape
without the Database check; symmetric fix needed; tracked in
BYT-9526.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(parser/tidb): restore nil-backupItem guard in GenerateRestoreSQL

Per Codex P2 catch on PR #20345 — Bug 9 regression recovery.
Pre-Bug-9 the nil-backupItem guard lived in extractStatement (the
first function GenerateRestoreSQL called). The Bug 9 plumbing
refactor (commit 6501126) moved metadata-fetching ahead of
extractStatement, so the guard was bypassed and any caller passing
nil backupItem (or nil SourceTable / TargetTable) triggered

    panic: invalid memory address or nil pointer dereference

Codex empirically verified the panic by direct call.

Per Vincent's reachability principle: defensive checks for paths
that ARE reachable (not hypothetical) are not YAGNI — they're
contract recovery / regression prevention. The previous behavior
(graceful error return) was a real contract; the refactor broke
it; Codex found it.

Fix: nil guards at top of GenerateRestoreSQL for backupItem,
SourceTable, and TargetTable. Each is independently dereferenced
later in the function (and in helpers it calls). Restoring all
three matches the strict reachability principle — any caller can
construct a malformed PriorBackupDetail_Item with nil sub-fields,
and we don't want to panic.

Removed the now-dead `if backupItem == nil` check in
extractStatement (unreachable since GenerateRestoreSQL guards
first; extractStatement is unexported with no other callers).
Documented the invariant via comment.

TestGenerateRestoreSQLNilBackupItemGuards regression pin: three
sub-tests cover nil backupItem, nil SourceTable, nil TargetTable;
each must return error (not panic).

Mysql analog (mysql/restore.go) does NOT currently have the
nil-backupItem regression because it never had the Bug 9 plumbing
refactor — its extractStatement still holds the original nil
guard. When BYT-9526 mirrors the Bug 9 plumbing refactor, it must
also restore the top-level guard.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(parser/tidb): skip empty-Expressions UKs in hasDisjointUniqueKey

Per Codex P1 catch on PR #20345 — gap in the Bug 7 fix (commit
4eb570f). `disjoint([])` returns vacuously true (the for-loop
iterates zero times). The Bug 7 `safe` filter likewise iterates
zero times for empty Expressions, leaving `safe=true`. So a UK
with empty Expressions passed through both checks and
short-circuited hasDisjointUniqueKey to true — even though the UK
has no actual columns to match against.

Reachable in production: TiDB schema metadata produces empty
Expressions for some expression-based index parts (per
backend/plugin/schema/tidb/get_database_metadata.go's
getIndexColumnsInfo, parts without key.Column aren't appended to
the Expressions slice).

Consequence: rollback SQL generated for UPDATEs that should be
rejected; ON DUPLICATE KEY UPDATE matching against a UK with no
real columns is unsafe.

Fix: explicit empty-Expressions check at the top of each UK
iteration (`if len(index.Expressions) == 0 { continue }`); such
UKs are skipped (treated as overlapping). Conservative —
preserves the never-false-positive-disjoint contract from Bug 7.

Mock fixture (backup_test.go): added a UK with nil Expressions
to t_generated for the regression test. Existing tests
unaffected — the empty-Expressions UK is skipped post-fix and the
other UKs remain disjoint/non-disjoint based on the SET columns
as before.

TestGenerateRestoreSQLEmptyExpressionsUKSkipped regression pin:
UPDATE t_generated SET a=1, b=2 — overlaps PK on b AND UK on a;
c_generated UK skipped (Bug 7); empty-Expressions UK now also
skipped (Bug 12). No disjoint UK remains, function returns the
no-disjoint-key error.

Mysql analog (mysql/restore.go) has the same string-comparison
hasDisjointUniqueKey shape; symmetric fix needed; tracked in
BYT-9526.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>