fix(storage): protect live data during post-flashback vacuum - #20436
fix(storage): protect live data during post-flashback vacuum#20436zhyass wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 353c784419
ℹ️ 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".
| lvt_check: Some(TableLvtCheck { | ||
| tenant: tenant.clone(), | ||
| time: target_timestamp, | ||
| }), |
There was a problem hiding this comment.
Revalidate table metadata before vacuum deletion
When Vacuum2 has loaded the pre-flashback table but pauses before publishing its LVT, this check can still succeed and commit the flashback plus barrier; the stale vacuum can then advance LVT and delete using the abandoned branch without observing the barrier or checking the new table sequence. For a chain where the stale GC root omits blocks referenced by the flashback target, this recreates the live-data deletion the change is meant to prevent. Couple the vacuum's LVT/selection to the observed table sequence, or re-read/CAS table metadata before deletion, and add a regression covering this interleaving.
AGENTS.md reference: AGENTS.md:L10-L11
Useful? React with 👍 / 👎.
| let target_timestamp = target_snapshot | ||
| .timestamp | ||
| .ok_or_else(|| ErrorCode::Internal("flashback target snapshot has no timestamp"))?; |
There was a problem hiding this comment.
Preserve flashback for snapshots without timestamps
For mixed-format tables whose requested target is a legacy v0 snapshot, timestamp is legitimately None because the field is optional for backward compatibility and v0 conversion supplies no timestamp. Snapshot-ID flashback previously accepted such an existing target, but this new mandatory extraction now returns an internal error before the metadata update; the analogous current-snapshot check also prevents flashback while the table head is still v0. Handle the legacy absence explicitly rather than rejecting these otherwise navigable snapshots.
Useful? React with 👍 / 👎.
I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/
Summary
FLASHBACK.After
FLASHBACK, the snapshot directory can contain both the active lineage and an abandoned committed branch:For snapshot-count retention, directory-based selection could choose
Bfrom the abandoned branch as the GC root. IfBis a truncate snapshot, its protection set does not contain segments still referenced by the liveE -> D -> Achain. With zero time retention, Vacuum2 could consequently delete a segment referenced by the current snapshot.This PR:
vacuum2_flashback_barriertable option during flashback;UpdateTableMetaReq;The internal barrier may make post-flashback vacuum more conservative until a live-chain GC root advances beyond the transaction safety window. It requires no user migration or backfill.
Tests
Type of change
AI assistance
This change is