util/stmtsummary: add tidb_stmt_summary_persist_evicted#68513
Conversation
Introduce a new global system variable tidb_stmt_summary_log_evicted: when ON, the v2 (persistent) backend writes a JSON entry (with "evicted":true) to the stmt log for each LRU eviction, allowing downstream consumers to track which records were dropped from memory. Uses a buffered async channel so the Add() hot path never blocks; when the channel is full, evictions are dropped and a periodic warn log reports the dropped count. Off by default because it adds log volume proportional to eviction rate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note
|
@nolouch I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
the
size/L
📝 WalkthroughWalkthroughThis PR adds optional per-record LRU eviction logging to TiDB's v2 persistent statement summary. When the ChangesStatement Summary Eviction Logging
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/util/stmtsummary/v2/stmtsummary_test.go (1)
99-105: ⚡ Quick winReplace
time.Sleepwithrequire.Neverfor deterministic negative assertion.Using
time.Sleepto verify that no further evictions are logged is not fully deterministic. Consider replacing it withrequire.Never, which polls periodically and provides a clearer intent that the condition should never become true.♻️ Proposed refactor using require.Never
// Disable and verify no further log writes. require.NoError(t, ss.SetLogEvicted(false)) ss.Add(GenerateStmtExecInfo4Test("digest5")) // evicts digest3 - time.Sleep(50 * time.Millisecond) - storage.Lock() - require.Equal(t, 2, len(storage.evicted)) - storage.Unlock() + require.Never(t, func() bool { + storage.Lock() + defer storage.Unlock() + return len(storage.evicted) != 2 + }, 100*time.Millisecond, 10*time.Millisecond, "evicted count should remain 2 after disabling") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/util/stmtsummary/v2/stmtsummary_test.go` around lines 99 - 105, Replace the non-deterministic time.Sleep assertion with a polling-based negative assertion using require.Never: after calling ss.SetLogEvicted(false) and ss.Add(...), assert that storage.evicted never changes from the expected length by polling the condition (use a closure that locks storage, checks len(storage.evicted) == 2, and unlocks) for a short timeout and interval; target the existing symbols ss.SetLogEvicted, ss.Add, and storage.evicted and use require.Never(t, ...) from testify to reliably assert the condition instead of sleeping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/util/stmtsummary/v2/stmtsummary.go`:
- Around line 425-460: evictedLogLoop currently returns on s.ctx.Done() and
drains evictedCh immediately, which can lose records if Add() races during
shutdown; modify evictedLogLoop so that on ctx.Done() it first waits for the
producer shutdown gate (call the same wait used by Close(), e.g. closeWg.Wait()
or wait until s.closed is set) before doing the final drain and return, then
drain evictedCh completely calling s.storage.logEvicted(r), call report(), and
return; reference the symbols evictedLogLoop, s.ctx.Done(), evictedCh, Add(),
Close(), s.closed, closeWg.Wait(), and s.storage.logEvicted when making the
change.
---
Nitpick comments:
In `@pkg/util/stmtsummary/v2/stmtsummary_test.go`:
- Around line 99-105: Replace the non-deterministic time.Sleep assertion with a
polling-based negative assertion using require.Never: after calling
ss.SetLogEvicted(false) and ss.Add(...), assert that storage.evicted never
changes from the expected length by polling the condition (use a closure that
locks storage, checks len(storage.evicted) == 2, and unlocks) for a short
timeout and interval; target the existing symbols ss.SetLogEvicted, ss.Add, and
storage.evicted and use require.Never(t, ...) from testify to reliably assert
the condition instead of sleeping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 95ec43e1-52e2-4ce6-bea3-fd35936e0f07
📒 Files selected for processing (6)
pkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/sysvar.gopkg/util/stmtsummary/v2/BUILD.bazelpkg/util/stmtsummary/v2/logger.gopkg/util/stmtsummary/v2/stmtsummary.gopkg/util/stmtsummary/v2/stmtsummary_test.go
…icted # Conflicts: # pkg/util/stmtsummary/v2/BUILD.bazel # pkg/util/stmtsummary/v2/stmtsummary_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68513 +/- ##
================================================
- Coverage 76.3149% 75.1507% -1.1642%
================================================
Files 2041 2026 -15
Lines 562966 568648 +5682
================================================
- Hits 429627 427343 -2284
- Misses 132425 141252 +8827
+ Partials 914 53 -861
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
size/XL
nolouch
changed the title
|
minor: cloneRecordForLog currently relies on the invariant that r is never nil (from the current construction path). Could we add a defensive nil guard (e.g. early return) to avoid panic if future refactors accidentally break that invariant? |
|
|
||
| // evictedCh carries per-record evictions to the async logger. | ||
| // A nil channel means evicted-logging is disabled. Sends are non-blocking. | ||
| evictedCh chan *StmtRecord |
There was a problem hiding this comment.
Actually，even if evicted-logging is disabled, the channel seems non-nil, the comment is a little miss-leading.
There was a problem hiding this comment.
Addressed in the current version: the comment now says eviction persistence is controlled by optPersistEvicted, and evictedCh only carries records to the async logger with non-blocking sends.
|
/retest |
…icted # Conflicts: # pkg/util/stmtsummary/v2/logger.go
ti-chi-bot
Bot
added
size/XXL
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yibin87, yudongusa, zimulala The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…icted # Conflicts: # pkg/sessionctx/vardef/tidb_vars.go # pkg/sessionctx/variable/sysvar.go # pkg/util/stmtsummary/v2/BUILD.bazel # pkg/util/stmtsummary/v2/stmtsummary.go # pkg/util/stmtsummary/v2/stmtsummary_test.go
4 participants
What problem does this PR solve?
Issue Number: ref #67924
Problem Summary:
The v2 (persistent) statement summary maintains an LRU per window; when an entry is evicted before the window is persisted, downstream consumers of the stmt log have no record that the digest ever existed. Today the only signal is the aggregate "other" bucket appended at rotation time, which loses per-record identity.
What changed and how does it work?
Introduce a new global system variable
tidb_stmt_summary_persist_evicted(default OFF)."evicted":truemarker, embedding the fullStmtRecordsnapshot, so downstream consumers can track which records were dropped from memory between rotations.Add()hot path never blocks on log I/O.The window callback was extended to optionally fire a per-eviction hook (
onEvictFn). When the flag is OFF, the hook short-circuits immediately and no record cloning is performed.Check List
Tests
New test:
pkg/util/stmtsummary/v2/stmtsummary_test.go::TestStmtSummaryPersistEvictedVerified locally:
GOCACHE=/private/tmp/tidb-go-build go test -run 'TestStmtSummaryPersistEvicted|TestWindowEvictedCountResetOnRotate|TestDefaultConfig' -tags=intest,deadlock -count=1 ./pkg/util/stmtsummary/v2GOCACHE=/private/tmp/tidb-go-build go test -tags=intest,deadlock -count=1 ./pkg/util/stmtsummary/v2make bazel_prepareGOCACHE=/private/tmp/tidb-go-build make -o tools/bin/revive lintSide effects
When ON, stmt summary log volume grows in proportion to the eviction rate. The feature is default OFF, uses bounded buffering, performs non-blocking enqueue on the hot path, batches writes in the background, and may drop evicted records when the queue is full.
Documentation
Release note
Summary by CodeRabbit
Release Notes