Snapshot support for databases and tables

[LeftHandCold]

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

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

• Replace map-based snapshot storage with unified SnapshotInfo structure

• Eliminate manual snapshot transformation and cleanup operations

• Extend snapshot types to include database and table levels

---

Diagram Walkthrough

flowchart LR
  A["Old: map[uint32]containers.Vector"] -->|"Refactor"| B["New: SnapshotInfo struct"]
  B -->|"Supports"| C["Cluster snapshots"]
  B -->|"Supports"| D["Account snapshots"]
  B -->|"Supports"| E["Database snapshots"]
  B -->|"Supports"| F["Table snapshots"]
  G["GC Operations"] -->|"Use"| B
  H["PITR Operations"] -->|"Use"| B

Loading

File Walkthrough

	Relevant files
Enhancement	8 files snapshot.go Unified snapshot structure supporting multiple hierarchy levels +306/-133 checkpoint.go Update GC checkpoint to use new SnapshotInfo type                +17/-24  exec_v1.go Refactor GC job to use SnapshotInfo instead of map              +22/-22  window.go Update GC window to accept SnapshotInfo parameter                +2/-2      types.go Update Cleaner interface snapshot return type                        +1/-2      util.go Remove snapshot transformation utility function                    +0/-12    mock_cleaner.go Update mock cleaner snapshot method signature                        +1/-2      storage_usage.go Update storage usage calculation to use SnapshotInfo          +2/-2
Tests	1 files db_test.go Fix snapshot test to use UnixNano and new API                        +8/-16
Additional files	1 files window_test.go +3/-3

---

[qodo-merge-pro]

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 🟢 Ensure snapshots support databases and tables in addition to cluster/account levels. Prevent GC from deleting data referenced by database-level or table-level snapshots. Integrate snapshot handling into GC, PITR, and usage collection flows using a unified structure. Update tests to validate snapshot behavior for new levels.
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.
Update

Compliance status legend

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

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Unify snapshot and PITR data structures The current design uses an alias for PitrInfo and SnapshotInfo, which is awkward because PITR needs a single timestamp while snapshots need a list. A new unified struct with explicit fields for both PITR and snapshots for each level would improve clarity and robustness. Examples: pkg/vm/engine/tae/logtail/snapshot.go [185] type PitrInfo = SnapshotInfo pkg/vm/engine/tae/logtail/snapshot.go [1061-1130] pitrInfo := &PitrInfo{ cluster: make([]types.TS, 1), account: make(map[uint32][]types.TS), database: make(map[uint64][]types.TS), tables: make(map[uint64][]types.TS), } for _, object := range sm.pitr.objects { select { case <-ctx.Done(): return nil, context.Cause(ctx) ... (clipped 60 lines) Solution Walkthrough: Before: // pkg/vm/engine/tae/logtail/snapshot.go // SnapshotInfo holds lists of timestamps for snapshots type SnapshotInfo struct { cluster []types.TS account map[uint32][]types.TS database map[uint64][]types.TS tables map[uint64][]types.TS } // PitrInfo is an alias, but logically needs only one TS per level type PitrInfo = SnapshotInfo // GetPITR implementation creates slices of size 1 for PITR func (sm *SnapshotMeta) GetPITR(...) (*PitrInfo, error) { pitrInfo := &PitrInfo{ cluster: make([]types.TS, 1), // ... } // ... logic to populate pitrInfo.cluster[0], etc. return pitrInfo, nil } After: // A new proposed structure // ProtectionLevelInfo holds both PITR and Snapshot data explicitly type ProtectionLevelInfo struct { PITRTimestamp types.TS Snapshots []types.TS } // ProtectionInfo uses the new explicit structure for each level type ProtectionInfo struct { Cluster ProtectionLevelInfo Account map[uint32]ProtectionLevelInfo Database map[uint64]ProtectionLevelInfo Table map[uint64]ProtectionLevelInfo } // GetPITR would now populate the specific PITRTimestamp field func (sm *SnapshotMeta) GetPITR(...) (*ProtectionInfo, error) { // ... logic to populate protectionInfo.Cluster.PITRTimestamp, etc. } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a significant design flaw where PitrInfo is an alias for SnapshotInfo, forcing PITR logic to awkwardly handle lists of timestamps when it only needs one, making the code less clear and potentially brittle.	Medium
General	Use a dedicated struct for PITR Refactor GetPITR to use a dedicated internal struct that holds a single timestamp per level, instead of misusing the SnapshotInfo struct which is designed for multiple timestamps. This will improve code clarity and maintainability. pkg/vm/engine/tae/logtail/snapshot.go [1047-1176] +type PitrInfoSingle struct { + cluster types.TS + account map[uint32]types.TS + database map[uint64]types.TS + tables map[uint64]types.TS +} + func (sm *SnapshotMeta) GetPITR( ctx context.Context, sid string, gcTime time.Time, fs fileservice.FileService, mp *mpool.MPool, -) (*PitrInfo, error) { -... (clipped 16 lines) - pitrInfo := &PitrInfo{ - cluster: make([]types.TS, 1), - account: make(map[uint32][]types.TS), - database: make(map[uint64][]types.TS), - tables: make(map[uint64][]types.TS), +) (*PitrInfo, error) { // Keep returning *PitrInfo for compatibility if needed, but build it from PitrInfoSingle + idxes := []uint16{ColPitrLevel, ColPitrObjId, ColPitrLength, ColPitrUnit} + tombstonesStats := make([]objectio.ObjectStats, 0) + for _, obj := range sm.pitr.tombstones { + tombstonesStats = append(tombstonesStats, obj.stats) + } + checkpointTS := types.BuildTS(time.Now().UTC().UnixNano(), 0) + ds := NewSnapshotDataSource(ctx, fs, checkpointTS, tombstonesStats) + pitrInfoSingle := &PitrInfoSingle{ + cluster: types.TS{}, + account: make(map[uint32]types.TS), + database: make(map[uint64]types.TS), + tables: make(map[uint64]types.TS), } for _, object := range sm.pitr.objects { -... (clipped 21 lines) + select { + case <-ctx.Done(): + return nil, ctx.Err() + default: + } + reader, err := blockio.NewFileReader(sid, fs, object.stats.Name().String()) + if err != nil { + return nil, err + } + bat, err := reader.ReadOneBlock(ctx, idxes, mp) + if err != nil { + return nil, err + } + if bat != nil && bat.Vecs[0] != nil { + defer bat.Clean(mp) + objIDList := vector.MustFixedColNoTypeCheck[uint64](bat.Vecs[1]) + for r := 0; r < bat.Vecs[0].Length(); r++ { + val := bat.Vecs[2].GetGeneric(r).(int64) + unit := bat.Vecs[3].GetStringAt(r) + var ts time.Time + switch unit { + case "y": + ts = AddDate(gcTime, -int(val), 0, 0) + case "m": + ts = AddDate(gcTime, 0, -int(val), 0) + case "d": + ts = AddDate(gcTime, 0, 0, -int(val)) + case "h": + ts = gcTime.Add(-time.Duration(val) * time.Hour) + case "s": + ts = gcTime.Add(-time.Duration(val) * time.Second) + default: + ts = gcTime.Add(-time.Duration(val) * time.Minute) + } + pitrTS := types.BuildTS(ts.UnixNano(), 0) + account := objIDList[r] level := bat.Vecs[0].GetStringAt(r) if level == PitrLevelCluster { - if !pitrInfo.cluster[0].IsEmpty() { -... (clipped 6 lines) - if pitrInfo.cluster[0].LT(&pitrTS) { - continue - } + if !pitrInfoSingle.cluster.IsEmpty() && pitrInfoSingle.cluster.LT(&pitrTS) { + continue } - pitrInfo.cluster[0] = pitrTS - + pitrInfoSingle.cluster = pitrTS } else if level == PitrLevelAccount { id := uint32(account) - if len(pitrInfo.account[id]) == 0 { - pitrInfo.account[id] = make([]types.TS, 1) - } - p := pitrInfo.account[id][0] - if !p.IsEmpty() && p.LT(&pitrTS) { + if p, ok := pitrInfoSingle.account[id]; ok && !p.IsEmpty() && p.LT(&pitrTS) { continue } - pitrInfo.account[id][0] = pitrTS + pitrInfoSingle.account[id] = pitrTS } else if level == PitrLevelDatabase { -... (clipped 33 lines) + id := uint64(account) + if p, ok := pitrInfoSingle.database[id]; ok && !p.IsEmpty() && p.LT(&pitrTS) { + continue + } + pitrInfoSingle.database[id] = pitrTS + } else if level == PitrLevelTable { + id := uint64(account) + if p, ok := pitrInfoSingle.tables[id]; ok && !p.IsEmpty() && p.LT(&pitrTS) { + continue + } + pitrInfoSingle.tables[id] = pitrTS } + logutil.Info( + "GC-GetPITR", + zap.String("level", level), + zap.Uint64("id", account), + zap.String("ts", pitrTS.ToString()), + ) } } } + + // Convert to *PitrInfo (*SnapshotInfo) to maintain API compatibility + pitrInfo := NewSnapshotInfo() + if !pitrInfoSingle.cluster.IsEmpty() { + pitrInfo.cluster = []types.TS{pitrInfoSingle.cluster} + } + for id, ts := range pitrInfoSingle.account { + pitrInfo.account[id] = []types.TS{ts} + } + for id, ts := range pitrInfoSingle.database { + pitrInfo.database[id] = []types.TS{ts} + } + for id, ts := range pitrInfoSingle.tables { + pitrInfo.tables[id] = []types.TS{ts} + } + return pitrInfo, nil } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that GetPITR misuses the SnapshotInfo struct by only ever using the first element of its timestamp slices, which is confusing and error-prone. Introducing a dedicated internal struct for PITR logic would make the code cleaner and the intent clearer.	Medium
	Improve test assertions for snapshot verification Improve the TestSnapshotMeta test by adding assertions to verify the actual timestamp values of reloaded snapshots, not just their count. This involves sorting and comparing the original and reloaded snapshot lists for equality. pkg/vm/engine/tae/db/test/db_test.go [7336-7362] assert.NotNil(t, minMerged) snaps, err := db.DiskCleaner.GetCleaner().GetSnapshots() assert.Nil(t, err) - assert.Equal(t, len(snapshots), len(snaps.ToTsList())) + reloadedSnapshots := snaps.ToTsList() + sort.Slice(snapshots, func(i, j int) bool { return snapshots[i] < snapshots[j] }) + sort.Slice(reloadedSnapshots, func(i, j int) bool { return reloadedSnapshots[i].Less(&reloadedSnapshots[j]) }) + require.Equal(t, len(snapshots), len(reloadedSnapshots)) + for i := range snapshots { + assert.Equal(t, types.BuildTS(snapshots[i], 0), reloadedSnapshots[i]) + } + err = db.DiskCleaner.GetCleaner().DoCheck(ctx) assert.Nil(t, err) tae.RestartDisableGC(ctx) ... (clipped 13 lines) minEnd = db.DiskCleaner.GetCleaner().GetMinMerged().GetEnd() assert.True(t, end.GE(&minEnd)) snaps, err = db.DiskCleaner.GetCleaner().GetSnapshots() assert.Nil(t, err) - assert.Equal(t, len(snapshots), len(snaps.ToTsList())) + reloadedSnapshots = snaps.ToTsList() + sort.Slice(reloadedSnapshots, func(i, j int) bool { return reloadedSnapshots[i].Less(&reloadedSnapshots[j]) }) + require.Equal(t, len(snapshots), len(reloadedSnapshots)) + for i := range snapshots { + assert.Equal(t, types.BuildTS(snapshots[i], 0), reloadedSnapshots[i]) + } err = db.DiskCleaner.GetCleaner().DoCheck(ctx) assert.Nil(t, err) [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6 __ Why: The suggestion correctly points out that the test only verifies the count of snapshots, not their values, which is a weakness. Adding assertions to compare the actual timestamp values would make the test significantly more robust and capable of catching more subtle bugs.	Low
Update