fix: handle logtail compaction during dirty scan by aptend · Pull Request #23664 · matrixorigin/matrixone

aptend · 2026-02-03T11:14:02Z

User description

What type of PR is this?

Which issue(s) this PR fixes:

What this PR does / why we need it:

Avoid missing dirty tables when logtail blocks are compacted during
dirty scan. The reader now falls back to block summaries and waits
for compaction summary to be published before merging.

PR Type

Bug fix

Description

Handle logtail block compaction during dirty table scan
Add fallback to block summaries when memo becomes nil
Wait for compaction summary publication before merging
Simplify IsDirtyOnTable by reusing GetDirty logic

Diagram Walkthrough

flowchart LR
  A["GetDirty scan"] --> B{"Block memo nil?"}
  B -->|No| C["Merge memo dirty data"]
  B -->|Yes| D["Set compact flag"]
  D --> E["Wait for summary"]
  E --> F["Merge summary data"]
  F --> G["Return dirty tree"]
  C --> G

File Walkthrough

Relevant files

Bug fix

reader.go `Handle block compaction during dirty scan` pkg/vm/engine/tae/logtail/reader.go Added `runtime` import for `Gosched()` call Introduced `mergeSummary()` helper function to consolidate table merging logic Enhanced `GetDirty()` to detect and handle block compaction during iteration Added `postBlkOp` callback to wait for compaction summary and merge data Simplified `IsDirtyOnTable()` to reuse `GetDirty()` instead of custom logic Updated `GetMaxLSN()` call signature to include new `postBlkOp` parameter	+41/-27

Enhancement

table.go `Add post-block operation callback` pkg/vm/engine/tae/logtail/table.go Added `postBlkOp` parameter to `ForeachRowInBetween()` function signature Added callback invocation after processing each block to handle post-block operations	+5/-0

qodo-code-review · 2026-02-03T11:14:33Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Unbounded busy-wait Description: The new `postBlkOp` introduces an unbounded busy-wait loop (`for summary == nil {` `runtime.Gosched(); ... }`) that can spin indefinitely if `blk.summary` is never published, enabling a potential denial-of-service via goroutine starvation/CPU consumption and stuck dirty scans. reader.go [58-71] Referred Code postBlkOp := func(blk BlockT) { if !iterateOnCompact { return } summary := blk.summary.Load() // wait TryCompact to finish for summary == nil { runtime.Gosched() summary = blk.summary.Load() } mergeSummary(tree, summary) count += len(blk.rows) iterateOnCompact = false }
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Infinite spin wait: The new `postBlkOp` busy-waits in a `for summary == nil` loop with `runtime.Gosched()` and no timeout/cancel path, which can hang indefinitely if the compaction summary is never published. Referred Code postBlkOp := func(blk BlockT) { if !iterateOnCompact { return } summary := blk.summary.Load() // wait TryCompact to finish for summary == nil { runtime.Gosched() summary = blk.summary.Load() } mergeSummary(tree, summary) count += len(blk.rows) iterateOnCompact = false } Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

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

qodo-code-review · 2026-02-03T11:15:44Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix function call with incorrect arguments Correct the function call in `IsDirtyOnTable` from `r.GetDirty(false)` to `r.GetDirty()` to match the updated function signature and prevent a compilation error. pkg/vm/engine/tae/logtail/reader.go [86-90] func (r Reader) IsDirtyOnTable(DbID, id uint64) bool { // TODO: optimize if needed - tree, _ := r.GetDirty(false) + tree, _ := r.GetDirty() return tree.HasTable(id) } Apply / Chat* Suggestion importance[1-10]: 10 __ Why: The suggestion correctly identifies a call to `r.GetDirty(false)` with an argument, while the function signature was changed in this PR to take no arguments, which would cause a compilation error.	High
Possible issue	Avoid busy-waiting and fix race condition In the `postBlkOp` function, remove the busy-wait loop that uses `runtime.Gosched()` to avoid high CPU usage while waiting for compaction to finish. pkg/vm/engine/tae/logtail/reader.go [58-71] postBlkOp := func(blk BlockT) { if !iterateOnCompact { return } summary := blk.summary.Load() - // wait TryCompact to finish - for summary == nil { - runtime.Gosched() - summary = blk.summary.Load() + if summary == nil { + // TryCompact is not finished yet, wait for next block's postBlkOp to check again. + // runtime.Gosched() is an option here for proactive waiting, but it's better to avoid busy-waiting. + return } mergeSummary(tree, summary) count += len(blk.rows) iterateOnCompact = false } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a busy-wait loop using `runtime.Gosched()`, which is a valid performance concern, and proposes removing it.	Medium
General	✅ ~~Fix inconsistent row counting logic~~ Suggestion Impact: The commit moved count++ out of the memo.HasAnyTableDataChanges() conditional so it runs for every row, matching the suggested consistent counting behavior. It also removed a separate count += len(blk.rows) increment in the compacted-block path, further adjusting row counting logic. code diff: @@ -50,8 +50,8 @@ } else if memo.HasAnyTableDataChanges() { row.GetTxnState(true) tree.Merge(memo.GetDirty()) - count++ } + count++ return true } @@ -66,7 +66,6 @@ summary = blk.summary.Load() } mergeSummary(tree, summary) - count += len(blk.rows) iterateOnCompact = false } In the `op` function, move the `count++` increment outside the conditional block to ensure it is executed for every row, maintaining consistent counting logic. pkg/vm/engine/tae/logtail/reader.go [44-56] op := func(row RowT) (moveOn bool) { memo := row.GetMemo() if memo == nil { // current block is compacted during iterating // retrieve the memo from blk summary, only once. iterateOnCompact = true } else if memo.HasAnyTableDataChanges() { row.GetTxnState(true) tree.Merge(memo.GetDirty()) - count++ } + count++ return true } `[Suggestion processed]` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the PR changed the logic for incrementing `count`, making it conditional, which contradicts the logic in other parts of the function and the original implementation.	Medium
Update

mergify · 2026-02-04T08:33:08Z

aptend requested a review from XuPeng-SH as a code owner February 3, 2026 11:14

aptend had a problem deploying to ci February 3, 2026 11:14 — with GitHub Actions Failure

aptend temporarily deployed to ci February 3, 2026 11:14 — with GitHub Actions Inactive

aptend had a problem deploying to ci February 3, 2026 11:14 — with GitHub Actions Failure

aptend temporarily deployed to ci February 3, 2026 11:14 — with GitHub Actions Inactive

aptend had a problem deploying to ci February 3, 2026 11:14 — with GitHub Actions Failure

aptend temporarily deployed to ci February 3, 2026 11:14 — with GitHub Actions Inactive

matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Feb 3, 2026

qodo-code-review Bot added the Review effort 3/5 label Feb 3, 2026

mergify Bot added the kind/bug Something isn't working label Feb 3, 2026

aptend force-pushed the fix-dirty branch from 790363d to 9d452b8 Compare February 4, 2026 02:29

aptend had a problem deploying to ci February 4, 2026 02:30 — with GitHub Actions Error

aptend temporarily deployed to ci February 4, 2026 02:30 — with GitHub Actions Inactive

aptend had a problem deploying to ci February 4, 2026 02:30 — with GitHub Actions Error

aptend had a problem deploying to ci February 4, 2026 02:30 — with GitHub Actions Failure

aptend temporarily deployed to ci February 4, 2026 02:30 — with GitHub Actions Inactive

fix dirty table scan

e2e9ed5

aptend temporarily deployed to ci February 4, 2026 06:48 — with GitHub Actions Inactive

aptend had a problem deploying to ci February 4, 2026 06:48 — with GitHub Actions Failure

aptend had a problem deploying to ci February 4, 2026 06:49 — with GitHub Actions Failure

aptend temporarily deployed to ci February 4, 2026 06:49 — with GitHub Actions Inactive

matrix-meow added size/M Denotes a PR that changes [100,499] lines and removed size/S Denotes a PR that changes [10,99] lines labels Feb 4, 2026

aptend added the do-not-merge/wip label Feb 4, 2026

fix ut

c934412

aptend temporarily deployed to ci February 4, 2026 07:37 — with GitHub Actions Inactive

aptend removed the do-not-merge/wip label Feb 4, 2026

aptend temporarily deployed to ci February 4, 2026 07:37 — with GitHub Actions Inactive

mergify Bot added the queued label Feb 4, 2026

Merge branch 'main' into fix-dirty

1c65cdf

mergify Bot temporarily deployed to ci February 4, 2026 08:33 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle logtail compaction during dirty scan#23664

fix: handle logtail compaction during dirty scan#23664
mergify[bot] merged 4 commits intomatrixorigin:mainfrom
aptend:fix-dirty

aptend commented Feb 3, 2026 •

edited by qodo-code-review Bot

Loading

Uh oh!

qodo-code-review Bot commented Feb 3, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented Feb 3, 2026 •

edited

Loading

Uh oh!

mergify Bot commented Feb 4, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

aptend commented Feb 3, 2026 • edited by qodo-code-review Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

What type of PR is this?

Which issue(s) this PR fixes:

What this PR does / why we need it:

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

qodo-code-review Bot commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Compliance Guide 🔍

Uh oh!

qodo-code-review Bot commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Uh oh!

mergify Bot commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aptend commented Feb 3, 2026 •

edited by qodo-code-review Bot

Loading

qodo-code-review Bot commented Feb 3, 2026 •

edited

Loading

qodo-code-review Bot commented Feb 3, 2026 •

edited

Loading

mergify Bot commented Feb 4, 2026 •

edited

Loading