fix(parser/tidb): add DML rollback support (closes BYT-7201) by vsai12 · Pull Request #20345 · bytebase/bytebase · GitHub
Skip to content

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

Merged
vsai12 merged 15 commits into
mainfrom
feat/tidb-restore-rollback
May 14, 2026
Merged

vsai12 merged 15 commits into
mainfrom
feat/tidb-restore-rollback

Conversation

@vsai12

@vsai12 vsai12 commented May 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes BYT-7201 — longstanding
customer ticket. Pre-this-PR, TiDB rollback preview returns hard error
engine TIDB is not supported. Mysql, Postgres, MSSQL, Oracle all
register GenerateRestoreSQL; TiDB was the only miss.

This PR ports backend/plugin/parser/mysql/restore.go to TiDB. The
port is mechanical — same algorithm, swapped omni package and parser
function name.

What changed

  • backend/plugin/parser/tidb/restore.go (new) — port of mysql analog.
  • backend/plugin/parser/tidb/restore_test.go (new) — yaml-driven
    golden test, standalone registration test, and a negative-path
    unit test (TestGenerateRestoreSQLNoDisjointUniqueKey) covering
    the "UPDATE without disjoint unique key → error" path that the
    yaml golden format cannot express (success-only by design).
  • backend/plugin/parser/tidb/test-data/test_restore.yaml (new) —
    fixtures covering UPDATE with generated columns, DELETE, multi-table
    DELETE with JOIN, UPDATE with alias, UPDATE with subset columns,
    multi-statement filtering by source table.
  • backend/plugin/parser/tidb/backup_test.go — added 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 (new) — pingcap/tidb:v8.5.5 compose for local
    manual verification.

Verification

Unit tests

  • TestRestore (yaml-driven): all 8 fixtures pass.
  • TestTiDBGenerateRestoreSQLRegistration: confirms Engine_TIDB
    registration produces the expected DELETE rollback SQL via
    base.GenerateRestoreSQL.
  • TestGenerateRestoreSQLNoDisjointUniqueKey: confirms the no-
    disjoint-key error path returns the expected error.

Container round-trip (against real TiDB v8.5.5)

  • UPDATE with generated column: mutated a=2 → 99, ran generated
    rollback, restored to a=2 with c_generated recalculated.
  • DELETE: deleted all rows, ran generated rollback, 3 rows restored.
  • UPDATE with alias: executes cleanly against real TiDB.

Independence from PR #20340

This PR bypasses the dispatcher — calls ParseTiDBOmni directly per
mysql precedent (mysql/restore.go:48,313). It does not depend on
PR #20340's dispatcher flip and ships independently.

Known follow-up

The mysql analog (mysql/restore_test.go) lacks symmetric coverage
for the no-disjoint-unique-key error path. Worth a small follow-up
to mirror TestGenerateRestoreSQLNoDisjointUniqueKey there for
parity — out of scope here to preserve mysql/tidb test-shape parity
in this PR's diff.

Test plan

  • CI green (`go test ./backend/plugin/parser/tidb/...`,
    `golangci-lint run`)
  • Manual: BYT-7201 reproducer (rollback preview against a real
    TiDB instance) returns SQL, no error

🤖 Generated with Claude Code

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>
@vsai12
vsai12 requested a review from a team as a code owner May 14, 2026 06:17
@cla-bot cla-bot Bot added the cla-signed label May 14, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 13ddb095d7

ℹ️ 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".

Comment thread backend/plugin/parser/tidb/restore.go Outdated
Comment thread backend/plugin/parser/tidb/restore.go Outdated
vsai12 and others added 2 commits May 14, 2026 15:30
…xprReferences

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>
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>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: be45bf2568

ℹ️ 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".

Comment thread backend/plugin/parser/tidb/restore.go
…ion 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>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 656b7327b1

ℹ️ 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".

Comment thread backend/plugin/parser/tidb/restore.go Outdated
vsai12 and others added 2 commits May 14, 2026 16:00
… 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>
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>
@d-bytebase
d-bytebase requested a review from rebelice May 14, 2026 07:12

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a33c1d169d

ℹ️ 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".

Comment thread backend/plugin/parser/tidb/restore.go
vsai12 and others added 2 commits May 14, 2026 16:16
…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>
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>
@vsai12
vsai12 enabled auto-merge (squash) May 14, 2026 07:30
@rebelice

Copy link
Copy Markdown
Contributor

@vsai12
vsai12 disabled auto-merge May 14, 2026 07:37
vsai12 and others added 2 commits May 14, 2026 16:45
…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>
…ion 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>
@vsai12
vsai12 enabled auto-merge (squash) May 14, 2026 07:51

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4eb570f20f

ℹ️ 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".

Comment thread backend/plugin/parser/tidb/restore.go Outdated
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>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5a806bc5f6

ℹ️ 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".

Comment thread backend/plugin/parser/tidb/restore.go Outdated
@vsai12
vsai12 disabled auto-merge May 14, 2026 08:08
…tual 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>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6501126c75

ℹ️ 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".

Comment thread backend/plugin/parser/tidb/restore.go Outdated
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>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 878a8d5151

ℹ️ 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".

Comment thread backend/plugin/parser/tidb/restore.go
@vsai12
vsai12 enabled auto-merge (squash) May 14, 2026 08:34
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>
@vsai12
vsai12 disabled auto-merge May 14, 2026 08:43

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 151e680902

ℹ️ 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".

Comment thread backend/plugin/parser/tidb/restore.go
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>
@sonarqubecloud

Copy link
Copy Markdown

@vsai12
vsai12 enabled auto-merge (squash) May 14, 2026 09:07
@vsai12
vsai12 merged commit 0351d3d into main May 14, 2026
17 of 18 checks passed
@vsai12
vsai12 deleted the feat/tidb-restore-rollback branch May 14, 2026 09:10
vsai12 added a commit that referenced this pull request Jun 4, 2026
Prior backup keys each backup item by the physical table name, but for a
multi-table DELETE (DELETE t1, t2 FROM test t1, test2 t2 ...) the delete
targets in n.Tables are ALIASES. containsTable compared those aliases to the
physical table name and never matched, so GenerateRestoreSQL returned "no DML
statement found in extracted SQL" for the whole class of alias-target
multi-table DELETEs -- the backup was generated but could never be restored
(silent data loss on rollback). The DELETE t1, t2 FROM ... form is even in the
backup golden.

Resolve each n.Tables target alias through the FROM/USING ref map
(extractSingleTablesFromTableExprs, lowercase-keyed) before comparing the
resolved physical Database+Table. n.Using is used only to resolve aliases,
never matched as a target -- preserving the no-re-insert property (#20345).

Verified end-to-end on pingcap/tidb:v8.5.0: backup -> DELETE -> restore
recovers the deleted rows. Tests cover all three forms (comma-FROM, JOIN,
USING) plus the negative (a JOIN-only USING table must not restore).

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants