Skip to content

Conversation

@LeftHandCold
Copy link
Contributor

@LeftHandCold LeftHandCold commented Oct 20, 2025

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #22634

What this PR does / why we need it:

Snapshot support for databases and tables


PR Type

Enhancement, Bug fix


Description

  • Refactored snapshot handling to support database and table-level snapshots

    • Changed from map[uint32]containers.Vector to *logtail.SnapshotInfo structure
    • Added support for cluster, account, database, and table-level snapshot types
  • Unified PITR and Snapshot functionality through shared SnapshotInfo structure

    • Made PitrInfo an alias for SnapshotInfo for backward compatibility
    • Updated all snapshot-related methods to use the new structure
  • Enhanced snapshot distribution logic to apply table snapshots across database

    • Table-level snapshots now protect all tables in the same database
    • Implemented proper snapshot hierarchy and deduplication
  • Removed manual snapshot cleanup and vector transformation code

    • Eliminated TransformToTSList() and CloseSnapshotList() functions
    • Simplified snapshot lifecycle management

Diagram Walkthrough

flowchart LR
  A["Old Structure<br/>map[uint32]containers.Vector"] -->|Refactor| B["SnapshotInfo<br/>cluster/account/database/tables"]
  B -->|Unified| C["PitrInfo<br/>Alias for SnapshotInfo"]
  B -->|Enhanced| D["Snapshot Distribution<br/>Table→Database→Account→Cluster"]
  D -->|Protects| E["Multi-level<br/>Table Protection"]
  F["GetSnapshot()"] -->|Returns| B
  G["AccountToTableSnapshots()"] -->|Uses| B
  H["MergeTableInfo()"] -->|Uses| B
Loading

File Walkthrough

Relevant files
Refactoring
7 files
checkpoint.go
Updated snapshot parameter types throughout GC logic         
+17/-24 
exec_v1.go
Changed snapshot field from map to SnapshotInfo pointer   
+26/-26 
mock_cleaner.go
Updated mock GetSnapshots return type signature                   
+1/-2     
types.go
Updated Cleaner interface GetSnapshots method signature   
+1/-2     
util.go
Removed TransformToTSList transformation function               
+0/-12   
window.go
Updated ExecuteGlobalCheckpointBasedGC snapshot parameter
+2/-2     
storage_usage.go
Updated FillUsageBatOfCompacted to use SnapshotInfo parameter
+2/-2     
Tests
3 files
window_test.go
Updated mock AccountToTableSnapshots return type                 
+3/-3     
db_test.go
Updated TestSnapshotMeta to use new SnapshotInfo API         
+8/-16   
snapshot_test.go
Added comprehensive tests for SnapshotInfo functionality 
+418/-0 
Enhancement
1 files
snapshot.go
Refactored SnapshotInfo structure with multi-level support
+409/-144

Snapshot support for databases and tables

Approved by: @XuPeng-SH
@qodo-merge-pro
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡
🎫 #22634
🟢 Snapshots must support databases and tables in addition to cluster/account.
Refactor snapshot handling so snapshot lifecycles are respected during GC.
Created database or table snapshots should not be garbage collected (GC) prematurely.
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@mergify mergify bot added kind/bug Something isn't working kind/feature kind/enhancement labels Oct 20, 2025
@qodo-merge-pro
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Table snapshots protect the entire database

The suggestion questions the design choice of making a table-level snapshot
protect all tables within the same database. It recommends considering a
stricter scope for snapshots to prevent unexpected garbage collection behavior.

Examples:

pkg/vm/engine/tae/logtail/snapshot.go [1919-1945]
	// First, collect all table snapshots that should be applied to all tables in the same database
	dbTableSnapshots := make(map[uint64][]types.TS) // dbID -> []types.TS
	for tableID, tableTSList := range snapshots.tables {
		if len(tableTSList) > 0 {
			if info := sm.tableIDIndex[tableID]; info != nil {
				dbID := info.dbID
				if dbTableSnapshots[dbID] == nil {
					dbTableSnapshots[dbID] = make([]types.TS, 0)
				}
				dbTableSnapshots[dbID] = append(dbTableSnapshots[dbID], tableTSList...)

 ... (clipped 17 lines)
pkg/vm/engine/tae/logtail/snapshot.go [2027-2053]
	// First, collect all table snapshots that should be applied to all tables in the same database
	dbTableSnapshots := make(map[uint64][]types.TS) // dbID -> []types.TS
	for tableID, tableTSList := range snapshots.tables {
		if len(tableTSList) > 0 {
			if info := sm.tableIDIndex[tableID]; info != nil {
				dbID := info.dbID
				if dbTableSnapshots[dbID] == nil {
					dbTableSnapshots[dbID] = make([]types.TS, 0)
				}
				dbTableSnapshots[dbID] = append(dbTableSnapshots[dbID], tableTSList...)

 ... (clipped 17 lines)

Solution Walkthrough:

Before:

// In AccountToTableSnapshots function

// 1. Collect all table-level snapshots and group them by database ID.
dbTableSnapshots = map[dbID][]snapshotTS
for tableID, tableTSList in all_table_snapshots:
    dbID = get_db_id_for_table(tableID)
    dbTableSnapshots[dbID].append(tableTSList)

// 2. For each table, determine its applicable snapshots.
for tableID, tableInfo in all_tables:
    allApplicableSnapshots = []
    
    // Add snapshots from other tables in the same database.
    db_snapshots = dbTableSnapshots[tableInfo.dbID]
    allApplicableSnapshots.append(db_snapshots)

    // Add database, account, and cluster snapshots...
    allApplicableSnapshots.append(...)

    tableSnapshots[tableID] = deduplicate(allApplicableSnapshots)

After:

// In AccountToTableSnapshots function

// For each table, determine its applicable snapshots.
for tableID, tableInfo in all_tables:
    allApplicableSnapshots = []
    
    // 1. Add table-specific snapshots ONLY.
    if tableTSList := all_table_snapshots[tableID]; tableTSList is not empty:
        allApplicableSnapshots.append(tableTSList)

    // 2. Add database-specific snapshots.
    if dbTSList := all_database_snapshots[tableInfo.dbID]; dbTSList is not empty:
        allApplicableSnapshots.append(dbTSList)

    // 3. Add account and cluster snapshots...
    allApplicableSnapshots.append(...)

    tableSnapshots[tableID] = deduplicate(allApplicableSnapshots)
Suggestion importance[1-10]: 8

__

Why: This is a significant design critique that correctly identifies an intended but potentially surprising behavior introduced in the PR, where a table-level snapshot affects garbage collection for the entire database, which could lead to unintended storage consumption.

Medium
  • More

@mergify mergify bot added the queued label Oct 20, 2025
@mergify mergify bot merged commit 7abfd01 into matrixorigin:main Oct 20, 2025
31 of 34 checks passed
@mergify mergify bot removed the queued label Oct 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working kind/enhancement kind/feature Review effort 4/5 size/XL Denotes a PR that changes [1000, 1999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants