{{ message }}
Repository navigation
feat(parser/tidb): flip dispatcher to omni-first with pingcap fallback (Phase 1.5 §1.5.N+1 — closes Phase 1.5) - #20340
Merged
Merged
Conversation
…k (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>
…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>
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>
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>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6944f79ad
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…pSingleStatement 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>
…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>
vsai12
enabled auto-merge (squash)
May 14, 2026 04:13
h3n4l
approved these changes
May 14, 2026
2 tasks
vsai12
added a commit
that referenced
this pull request
May 14, 2026
* 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Summary
Closes Phase 1.5 advisor migration. Replaces the pingcap-only TiDB
dispatcher with the omni-first/pingcap-fallback dispatcher per plan
§1.5.0 invariant #8 (Option B). Migrated advisors now read OmniAST
directly from the dispatcher; un-migrated advisors continue to
function via the bridge (cumulative §1.5.2).
Migration count (empirically verified):
advisor_*.gofiles inbackend/plugin/advisor/tidb/.getTiDBOmniNodescallers + 3 transitive viarunNamingConventionRule).advisor_statement_dml_dry_run.go(BATCH non-transactional DML, Phase 2 deferred).advisor_builtin_prior_backup_check.gouses both omni (AST shape) and pingcap (DDL detection viaast.DDLNodeinterface).advisor_builtin_walk_through_check.go) registers the shared generic advisor — N/A for migration.Architectural shape
Option B (omni-first / pingcap-fallback) — the dispatcher tries
omni first, falls back to native pingcap on omni parse failure. The
review never hard-fails on a Tier 4 grammar gap. Trade-off accepted:
per-statement skip-with-no-advice on omni-rejected SQL (vs full-
review-failure under Option A).
Dual-path retention —
getTiDBNodesretained for two deliberateconsumers (NOT migration debt):
advisor_statement_dml_dry_run.go— Class III, blocked on omniBATCH grammar (Phase 2 deferred).
advisor_builtin_prior_backup_check.go— dual-path per cumulativeProblem syncing RDS instances because rdsadmin private database #30: pingcap for full-grammar DDL detection via
ast.DDLNodeinterface; omni for AST-shape per-table DML mixing detection.
Bridge cleanup deferred until Phase 2 BATCH grammar lands and
dml_dry_runmigrates.Per-fallback observability (invariant #8 sub-contract)
Two surfaces, both required:
tidb_dispatcher_omni_fallback_total{reason}(Prometheus). Drives the eventual Option B → A retirement
decision. Debug logs are dropped before reaching aggregation
pipelines and cannot serve this role.
individual reports.
Reason classifier (empirically derived, invariant #9): matches
case-insensitively against
(omni err msg) + " " + (input SQL).Patterns ship for
flashback,sequence,batch_dml, withunknownfallback.Empirical finding shaping the classifier signature: omni reports
unexpected token after CREATEforCREATE SEQUENCE seq;— thekeyword
SEQUENCEnever appears in the error string. Without input-side matching the SEQUENCE telemetry signal would be silently lost
for the first 30 days of production data. The two-arg
classifyOmniParseError(err, sql)signature is doing real work.Side effects from the Gatherers fold (echo_routes.go:71-73)
Folding
prometheus.Gatherers{registry, prometheus.DefaultGatherer}at the
/metricshandler exposes three new metric families:db_metrics.metadataDBQueryDurationbecomes visible at /metrics(fixes pre-existing invisibility — bonus bug-fix; the metric was
registered to DefaultRegisterer but the endpoint served only the
custom registry).
go_goroutines,go_memstats_*,process_cpu_seconds_total, etc.) auto-registered toDefaultRegisterer become visible. Operationally useful SRE /
standard observability surface; new metric-family area at the
endpoint.
tidb_dispatcher_omni_fallback_total{reason}becomes operational from day 1 — required for invariant [NOTICE] Issues are reserved for Bugs and Feature Requests ONLY #8
retirement-gate signal.
Empirical findings during implementation (invariant #9)
Plan §1.5.0 large table add cloumn #5 chosen test SQL was wrong. The draft suggested
FLASHBACK TABLE foo TO BEFORE DROP;for the Option B regressiontest. Pre-flip empirical probe found pingcap ALSO rejects that
exact syntax — it would test the both-engines-reject path, not
Option B. Switched to
BATCH ON id LIMIT N DELETEwhich omnirejects but pingcap accepts (Tier 4 grammar gap). Both paths now
have dedicated regression tests (
TestParseTiDBStatementsOmni_OptionBFallbackTestParseTiDBStatementsOmni_BothEnginesReject).SEQUENCE keyword absent from omni error. See classifier
signature note above.
Test changes worth reviewer attention
Two existing
utils_test.gotests had to change because pre-flipthey relied on
getTiDBOmniNodesdoing the parse internally:TestGetTiDBOmniNodesCachesAcrossRules: feeds*OmniASTviastmt.ASTdirectly (cache contract preserved at the new layer).TestGetTiDBOmniNodesSoftFailsOnGrammarGap→TestGetTiDBOmniNodesSkipsPingcapFallbackAST: pins the *AST-armskip behavior. End-to-end soft-fail is now exercised by
TestParseTiDBStatementsOmni_OptionBFallback.TestRunNamingConventionRuleEmitsInternalErrorAdvice: feedssynthetic *OmniAST with CreateIndexStmt.
These are necessary consequences of the contract change.
Plan doc updates made locally (not in repo)
≤ 1 file→≤ 2 filesper cumulative Problem syncing RDS instances because rdsadmin private database #30dual-path pattern.
to BATCH per empirical verification.
Verification gates (all met)
getTiDBNodes\bnon-test grep returns 2 advisor consumers (deliberate).TestParsePingCapSingleStatement_LineTrackingMatchesCanonical).Commits
feat(parser/tidb): flip dispatcher to omni-first with pingcap fallbackrefactor(advisor/tidb): simplify getTiDBOmniNodes to 3-arm switchfix(server): fold default registry into /metrics gathererBisect ordering note: Commits 1+2 are sequential (commit 2
simplifies a function that commit 1's dispatcher feeds). Bisecting
to commit 1 alone: works correctly with reduced perf — the wasteful
re-parse in
getTiDBOmniNodesis still in place but the dispatchercontract is in effect. Bisecting to commit 2 alone (without 1) would
not build cleanly because
getTiDBOmniNodesreadsstmt.AST.(*OmniAST)which the pre-flip dispatcher doesn't produce. Commit 3 is
independent of 1+2 in build terms but functionally needs commit 1 to
expose the dispatcher counter at /metrics.
Test plan
go test ./backend/plugin/parser/tidb/...greengo test ./backend/plugin/advisor/tidb/...greengolangci-lint run0 issues/metricson a running backend post-merge,confirm
tidb_dispatcher_omni_fallback_total,metadata_db_query_duration_*, andgo_*metric familiesare visible.
🤖 Generated with Claude Code