Skip to content

Conversation

LeftHandCold
Copy link
Contributor

@LeftHandCold LeftHandCold commented Jul 15, 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 #21981

What this PR does / why we need it:

Fix catalog gc


PR Type

Bug fix


Description

  • Fix catalog garbage collection watermark calculation

  • Replace disk cleaner scan watermark with checkpoint-based approach

  • Correct timestamp comparison logic for GC interval


Changes diagram

flowchart LR
  A["DiskCleaner ScanWaterMark"] --> B["BGCheckpointRunner MaxIncrementalCheckpoint"]
  B --> C["Checkpoint End Timestamp"]
  C --> D["GC Watermark Calculation"]
  D --> E["Catalog GCByTS"]
Loading

Changes walkthrough 📝

Relevant files
Bug fix
cronjobs.go
Fix catalog GC watermark calculation logic                             

pkg/vm/engine/tae/db/cronjobs.go

  • Replace scanWaterMark from disk cleaner with checkpoint-based wartMark
  • Fix timestamp comparison logic from LE to GE
  • Update watermark source from GetScanWaterMark() to
    MaxIncrementalCheckpoint()
  • Correct variable naming and timestamp handling
  • +10/-7   

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link

    You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    21981 - Not compliant

    Non-compliant requirements:

    • GC can be arbitrarily suspended and resumed

    Requires further human verification:

    • GC can be arbitrarily suspended and resumed - requires testing to verify if the checkpoint-based approach enables proper suspension/resumption functionality

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Logic Error

    The timestamp comparison logic was changed from LE to GE, but the variable name wartMark suggests it should be a watermark. The logic now sets wartMark to an older timestamp when the condition is true, which may be incorrect for watermark semantics.

    if wartMark.GE(&ts) {
    	wartMark = ts
    }
    Naming Issue

    Variable name wartMark appears to be a typo and should likely be waterMark for consistency and clarity.

    wartMark := ckp.GetEnd()
    if wartMark.IsEmpty() {

    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Jul 15, 2025
    @mergify mergify bot added kind/bug Something isn't working kind/feature labels Jul 15, 2025
    Copy link

    You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @mergify mergify bot merged commit 92a4526 into matrixorigin:main Jul 15, 2025
    19 checks passed
    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/feature Review effort 3/5 size/S Denotes a PR that changes [10,99] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants