Skip to content

fix(storage): protect live data during post-flashback vacuum - #20436

Draft
zhyass wants to merge 2 commits into
databendlabs:mainfrom
zhyass:fix/vacuum2-post-flashback-live-chain
Draft

fix(storage): protect live data during post-flashback vacuum#20436
zhyass wants to merge 2 commits into
databendlabs:mainfrom
zhyass:fix/vacuum2-post-flashback-live-chain

Conversation

@zhyass

@zhyass zhyass commented Sep 3, 2026

Copy link
Copy Markdown
Member

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

  • Prevent Vacuum2 from selecting a GC root from an abandoned snapshot branch after FLASHBACK.
  • Make the flashback target's least-visible-time validation part of the same metadata transaction as the table update.
  • Preserve the fast directory-based GC root selection path for tables that have not been flashed back.

After FLASHBACK, the snapshot directory can contain both the active lineage and an abandoned committed branch:

before flashback: C -> B -> A
after flashback:  E -> D -> A
listed objects:   A, B, C, D, E

For snapshot-count retention, directory-based selection could choose B from the abandoned branch as the GC root. If B is a truncate snapshot, its protection set does not contain segments still referenced by the live E -> D -> A chain. With zero time retention, Vacuum2 could consequently delete a segment referenced by the current snapshot.

This PR:

  • records an internal, monotonic vacuum2_flashback_barrier table option during flashback;
  • derives time- and snapshot-count GC roots from the current snapshot lineage while the barrier is active;
  • validates the flashback target against table LVT atomically in UpdateTableMetaReq;
  • clears an expired barrier only after a successful vacuum, using an exact table-sequence match so concurrent updates retain the conservative path;
  • keeps existing directory-based selection unchanged when no barrier exists.

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

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - Explain why

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that could cause existing functionality not to work as expected)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

AI assistance

  • AI usage: An AI coding agent assisted with root-cause analysis, implementation review, regression test drafting, validation, and the PR description.
  • Responsible human: @zhyass
  • The responsible human has read every line of this diff and can explain each change

This change is Reviewable

@github-actions github-actions Bot added the pr-bugfix this PR patches a bug in codebase label Sep 3, 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: 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".

Comment on lines +114 to +117
lvt_check: Some(TableLvtCheck {
tenant: tenant.clone(),
time: target_timestamp,
}),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge 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 👍 / 👎.

Comment on lines +65 to +67
let target_timestamp = target_snapshot
.timestamp
.ok_or_else(|| ErrorCode::Internal("flashback target snapshot has no timestamp"))?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge 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 👍 / 👎.

@zhyass
zhyass marked this pull request as draft September 3, 2026 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix this PR patches a bug in codebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant