Repository navigation
ddl: use real start ts for recover table snapshot - #68229
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
release-note
|
@expxiaoli I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
the
size/L
|
Hi @expxiaoli. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR fixes a DDL metadata correctness issue where recover operations were using stale snapshots for queued-drop scenarios. It introduces a helper to prefer the actual job start time (RealStartTS) over the queued time (StartTS), propagates this through DDL store and executor paths, and validates the fix with comprehensive regression tests. ChangesRecover Table Snapshot Timestamp Fix
Sequence DiagramsequenceDiagram
participant Session1 as Session 1: ALTER
participant Session2 as Session 2: DROP/RECOVER
participant JobQ as Job Queue
participant JobW as Job Worker
Session1->>JobQ: submit ALTER (StartTS=T1)
Session2->>JobQ: submit DROP (StartTS=T2, queued)
JobW->>JobW: execute ALTER (RealStartTS=T3)
JobW->>JobW: execute DROP (RealStartTS=T4, T4 > T2)
Session2->>JobW: RECOVER/FLASHBACK uses RealStartTS=T4 snapshot
JobW-->>Session2: restored metadata reflects T4 state
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested Labels
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/tidb-server/BUILD.bazel (1)
109-109: ⚡ Quick winRevert this unrelated shard-count churn.
tidb-server_testis untouched elsewhere in this PR, so this 5→6 bump looks like straybazel_prepareoutput rather than part of the recover-table fix. Please drop it unless you have target-specific runtime data showing this package now needs another shard.Based on learnings: In this repository, running make bazel_prepare is only required when you make structural Bazel changes (e.g., adding/moving/removing Go files, changing dependencies, or modifying go.mod/go.sum). For cosmetic changes or test-target tweaks that do not affect generated metadata (such as changing only the timeout attribute of a go_test rule), do not run or commit the make bazel_prepare output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/tidb-server/BUILD.bazel` at line 109, This change unnecessarily bumps shard_count from 5 to 6 for the tidb-server_test target; revert the unrelated bazel_prepare churn by setting shard_count back to its original value (restore "shard_count = 5" in the BUILD.bazel entry that configures tidb-server_test) and do not commit generated bazel_prepare output for non-structural changes—ensure only intentional Bazel metadata edits remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/ddl/ddl.go`:
- Around line 1334-1341: The schema-recovery path currently uses job.StartTS (in
getRecoverDBByName) and can restore stale metadata for queued DROP DATABASE
jobs; change getRecoverDBByName to use the same snapshot-selection rule as
GetRecoverTableSnapshotTS (i.e., prefer job.RealStartTS when non-zero, otherwise
fall back to job.StartTS), or factor the logic into a shared helper (e.g.,
GetRecoverSnapshotTS) and call it from both GetRecoverTableSnapshotTS and
getRecoverDBByName; update/add a regression test that simulates a queued DROP
DATABASE where earlier table DDL finishes before the drop starts to ensure
FLASHBACK/RECOVER DATABASE uses RealStartTS.
---
Nitpick comments:
In `@cmd/tidb-server/BUILD.bazel`:
- Line 109: This change unnecessarily bumps shard_count from 5 to 6 for the
tidb-server_test target; revert the unrelated bazel_prepare churn by setting
shard_count back to its original value (restore "shard_count = 5" in the
BUILD.bazel entry that configures tidb-server_test) and do not commit generated
bazel_prepare output for non-structural changes—ensure only intentional Bazel
metadata edits remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 12b9c8c8-d4be-4bc3-b4e0-828e889ccb5c
📒 Files selected for processing (6)
cmd/tidb-server/BUILD.bazelpkg/ddl/ddl.gopkg/ddl/tests/serial/BUILD.bazelpkg/ddl/tests/serial/serial_test.gopkg/executor/ddl.gopkg/util/signal/BUILD.bazel
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68229 +/- ##
================================================
+ Coverage 77.7059% 78.7740% +1.0680%
================================================
Files 1991 2003 +12
Lines 552094 561784 +9690
================================================
+ Hits 429010 442540 +13530
+ Misses 122164 117102 -5062
- Partials 920 2142 +1222
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
size/XL
|
/retest-required |
1 similar comment
|
/retest-required |
|
@expxiaoli: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest-required |
|
@expxiaoli: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest-required |
|
@expxiaoli: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
@expxiaoli: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test |
|
@expxiaoli: The The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@expxiaoli: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test check-issue-triage-complete |
|
@expxiaoli: The specified target(s) for The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@expxiaoli: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest-required |
|
@expxiaoli: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest-required |
|
@expxiaoli: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
ti-chi-bot
Bot
added
size/L
|
/retest-required |
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: djshow832, YangKeao The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
/retest-required |
7 similar comments
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
3 participants
What problem does this PR solve?
Issue Number: close #68222
Problem Summary:
RECOVER TABLEandFLASHBACK TABLEreconstruct dropped/truncated table metadata from the drop/truncate DDL history job. Before this PR, TiDB used the job'sStartTSas the snapshot timestamp.However,
StartTSis assigned when the DDL job is inserted intomysql.tidb_ddl_job, not when the job actually starts running. If aDROP TABLEjob is queued while an earlier DDL on the same table is still pending, the drop-table job'sStartTSmay point to stale table metadata.For example:
CREATE TABLE t (id INT PRIMARY KEY, col_a INT, col_b INT).ALTER TABLE t DROP COLUMN col_a.DROP TABLE tbefore the drop-column job finishes.RECOVER TABLE tmay reconstruct metadata from the drop-table job'sStartTS, wherecol_astill existed.As a result, the recovered table metadata and the recover-table history job's
BinlogInfo.TableInfomay contain stale columns that were no longer part of the table when it was actually dropped.What changed and how does it work?
This PR introduces
ddl.GetRecoverTableSnapshotTS(job)for reconstructing dropped/truncated table metadata:job.RealStartTSif it is available.job.StartTSfor compatibility with older history jobs whereRealStartTSmay be zero.RealStartTSis set when the DDL worker actually starts running the job, so it represents the schema state immediately before the drop/truncate job begins changing the table metadata. This is the correct snapshot for recovering the table definition.The new snapshot selection is used by:
RECOVER TABLE BY JOB ...RECOVER TABLE <table_name>FLASHBACK TABLE ...The GC safe point validation is also applied only after filtering to drop/truncate table jobs, so unrelated history jobs are not validated with recover-table snapshot semantics.
A regression test is added to make the stale snapshot case deterministic with DDL failpoints:
ALTER TABLE ... DROP COLUMN col_aand wait until it is queued.DROP TABLE ...and wait until it is queued.BinlogInfo.TableInfo.Columnsdo not containcol_a.make bazel_preparewas run because this PR adds a new top-level Go test function in an existing*_test.gofile.Check List
Tests
GOSUMDB=sum.golang.org GOPATH=/Users/xiaoli/go ./tools/check/failpoint-go-test.sh pkg/ddl/tests/serial -run TestRecoverTableUsesRealStartTSForQueuedDropTable -count=1GOSUMDB=sum.golang.org GOPATH=/Users/xiaoli/go make bazel_prepareGOSUMDB=sum.golang.org GOPATH=/Users/xiaoli/go make lintgit diff --checkSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Bug Fixes
Tests