-
Notifications
You must be signed in to change notification settings - Fork 824
feat: implement irreversible vacuum drop table protection #18809
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
dantengsky
wants to merge
22
commits into
databendlabs:main
Choose a base branch
from
dantengsky:feat/irreversible-vacuum-drop-table
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
feat: implement irreversible vacuum drop table protection #18809
dantengsky
wants to merge
22
commits into
databendlabs:main
from
dantengsky:feat/irreversible-vacuum-drop-table
Conversation
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
Add retention guard mechanism to prevent undrop operations after vacuum has started, ensuring data consistency by blocking restoration of tables whose data may have been partially or fully cleaned up. Key changes: - Add VacuumRetention metadata with monotonic timestamp semantics - Implement fetch_set_vacuum_timestamp API with correct CAS behavior - Integrate retention checks in vacuum drop table workflow - Add retention guard validation in undrop table operations - Include comprehensive error handling and user-friendly messages - Add protobuf serialization support with v151 compatibility - Provide full integration test coverage Fixes data integrity issue where undrop could succeed on tables with incomplete S3 data after vacuum cleanup has begun. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Make error message more user-friendly by: - Using clear language about why undrop is blocked - Including vacuum start timestamp for better context - Removing technical jargon like 'retention guard' and 'precedes' 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Rename VacuumRetention to VacuumWatermark for clarity - Remove unnecessary fields: updated_by, updated_at, version - Keep only essential 'time' field for monotonic timestamp tracking - Update protobuf conversion and tests accordingly - Maintain API compatibility and retention guard functionality 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Move vacuum timestamp setting from gc_drop_tables to VacuumDropTablesInterpreter::execute2 - Use actual retention settings instead of hardcoded 7 days - Set timestamp before vacuum operation starts for better timing - Simplify gc_drop_tables to focus only on metadata cleanup - Improve separation of concerns between business logic and cleanup operations 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Only set vacuum timestamp when NOT in dry run mode - Maintains consistency with existing dry run behavior for metadata cleanup - Dry run should not modify any state including vacuum watermark - Preserves read-only nature of dry run operations 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Returns Option<VacuumWatermark> instead
…alues - Replace unwrap_or_default() with explicit Option handling in fetch_set_vacuum_timestamp - Use clear match semantics: None = never set, Some = previous value - Update get_vacuum_timestamp to return Option<VacuumWatermark> instead of using epoch default - Fix schema_api retention guard to only check when vacuum timestamp is actually set - Update tests to handle proper Option semantics for first-time vs subsequent calls - Remove dependency on artificial epoch time as "unset" indicator - Improve type safety by letting Option express the "possibly unset" state This eliminates confusion around artificial default values and makes the vacuum watermark semantics clearer through the type system. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Adds vacuum timestamp seq checking to undrop transaction conditions to prevent race conditions where vacuum and undrop operations could execute concurrently, leading to data inconsistency. - Read vacuum timestamp with seq before undrop transaction - Add vacuum timestamp seq check to transaction conditions - Ensures undrop fails atomically if vacuum timestamp changes during operation - Existing test coverage in vacuum_retention_timestamp validates concurrent scenario 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
The vacuum_retention_timestamp test was failing because it tried to create a table without first creating the database. Added util.create_db() call to fix the test execution. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Fixed timing assertion failure by: - Capturing drop_time after the actual drop operation completes - Using relative time comparison instead of exact equality - Ensuring vacuum_time is always after drop_time as expected The test now properly validates that undrop is blocked when vacuum timestamp is set after the drop time, without timing precision issues. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Ran `cargo fmt --all` to ensure consistent code formatting across vacuum retention implementation files. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Updated test_decode_v152_vacuum_retention to match the standard proto-conv test format: - Added complete copyright header - Added serialized bytes array for backward compatibility testing - Added test_load_old call to test deserialization from v152 format - Added proper documentation comments about byte array immutability - Used correct serialized bytes generated by test framework Follows the same pattern as other version tests like v150_role_comment.rs to ensure proper backward compatibility testing. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
The get_vacuum_timestamp method was only used in tests and had no other consumers. Removed it to follow YAGNI principle and simplify the API: - Removed get_vacuum_timestamp from GarbageCollectionApi trait - Updated test to use direct KV API call (kv_api.get_pb) - Added VacuumRetentionIdent import to test file This aligns with the existing pattern in undrop logic which also uses direct KV API access for reading vacuum timestamps. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Make vacuum timestamp setting a blocking operation that must succeed before any data cleanup begins. This prevents a critical race condition where data could be cleaned up without proper undrop protection. Changes: - Convert timestamp setting from best-effort to critical operation - Vacuum operation now fails fast if timestamp cannot be set - Added detailed error message explaining the safety abort - Ensures vacuum watermark always precedes any data cleanup This maintains the core safety guarantee: tables that may have incomplete data after vacuum can never be restored via undrop. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Improve consistency with existing codebase naming conventions: 1. Rename protobuf message from VacuumRetention to VacuumWatermark - Aligns with Rust struct name (VacuumWatermark) - Follows existing pattern: Rust struct name = Protobuf message name - Examples: DatabaseMeta, CatalogMeta, IndexMeta all use same names 2. Remove unused protobuf fields - Removed: updated_by, updated_at, version (all set to empty/0) - Kept: ver, min_reader_ver (required for versioning), time (core data) - Maintains same serialization bytes (backward compatible) 3. Simplify proto-conv implementation - Removed unused field mappings in to_pb() - Cleaner, more maintainable conversion code The protobuf now only contains the essential fields actually used, following the principle of minimal necessary data representation. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Rename protobuf definition file to match the message name inside it. This improves consistency and makes the codebase easier to navigate: - File: vacuum_retention.proto → vacuum_watermark.proto - Message: VacuumWatermark (unchanged) - Rust struct: VacuumWatermark (unchanged) Following the common pattern where protobuf files are named after their primary message type. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Replace extreme epoch timestamp (1970-01-01) with realistic test value: - Old: timestamp(0, 0) → 1970-01-01 00:00:00 UTC (too extreme) - New: timestamp(1702603569, 0) → 2023-12-15 01:26:09 UTC (realistic) This aligns with other protobuf tests in the codebase that use similar recent timestamps for better test readability and maintainability. Updated corresponding serialized bytes array to match new timestamp. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
🤖 Smart Auto-retry Analysis
📊 Summary
❌ NO RETRY NEEDEDAll failures appear to be code/test issues requiring manual fixes. 🔍 Job Details
🤖 AboutAutomated analysis using job annotations to distinguish infrastructure issues (auto-retried) from code/test issues (manual fixes needed). |
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.
I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/
Summary
🤖 Generated with Claude Code
This PR implements a critical data integrity feature that prevents undrop operations on tables whose data may have been partially or fully cleaned up by vacuum processes. The core principle is: once vacuum has started for a retention period, tables dropped before that period can never be restored, ensuring users cannot accidentally restore tables with incomplete or inconsistent data.
Problem Statement
Previously, there was a dangerous race condition where:
This could lead to silent data corruption and inconsistent database state.
Design Philosophy
Tenant-Level Global Watermark Design
We chose a tenant-level global vacuum watermark approach instead of per-table or per-database granularity for several key reasons:
VacuumRetentionIdent::new_table(tenant, db_id, table_id)
VacuumRetentionIdent::new_global(tenant)
Implementation Approach
1. Monotonic Timestamp Mechanism
drop_time <= vacuum_timestamp
→ undrop FORBIDDEN2. Atomic Compare-and-Swap (CAS) Operations
crud_upsert_with
for race-safe timestamp updates3. Multi-Layer Concurrent Safety
4. Fail-Safe Error Handling
Key Implementation Details
VacuumWatermark Structure (Rust)
Protobuf Serialization Format
Integration Points
VacuumDropTablesInterpreter::execute2()
handle_undrop_table()
with early validationVacuumRetentionIdent
for tenant-global coordinationMetaStore Storage Details
The vacuum watermark is stored as a KV pair in the MetaStore:
Design Rationale: The tenant-scoped key format enables:
Backwards Compatibility
Option<VacuumWatermark>
handles "never set" vs "set to value" correctlySafety Guarantees
Critical Safety Flow
Undrop Behavior Matrix (Sample Scenarios)
Key Insight: When no vacuum watermark exists, undrop operations are both safe and correct - it means no vacuum cleanup has ever been performed on this tenant, so all dropped table data remains intact. This also ensures smooth migration from older versions without breaking existing workflows.
State Flow Diagram
Implementation Logic Flow
Concurrent Safety Example
Test Coverage
Files Modified
garbage_collection_api.rs
,schema_api.rs
interpreter_vacuum_drop_tables.rs
vacuum_watermark.rs
,vacuum_retention_ident.rs
vacuum_retention_from_to_protobuf_impl.rs
vacuum_watermark.proto
app_error.rs
,exception_code.rs
schema_api_test_suite.rs
,v152_vacuum_retention.rs
This implementation provides rock-solid data integrity guarantees while maintaining performance and operational simplicity. The tenant-level design offers the right balance of safety, simplicity, and future extensibility.
Tests
Type of change
This change is