statistics: fix targeted stats cache refresh after analyze#69883
statistics: fix targeted stats cache refresh after analyze#69883wlwilliamx wants to merge 2 commits into
Conversation
Targeted stats refreshes reused the cache-wide version lower bound. An unrelated table could therefore move the watermark past the analyzed table and leave pseudo stats in memory after ANALYZE. Reload explicitly requested IDs without that lower bound and only refresh physical or global IDs whose analyze results were persisted. Preserve the first global stats write error while tracking partial writes.
ti-chi-bot
Bot
added
the
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
size/XL
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change fixes targeted statistics refreshes by using physical statistics IDs, separating targeted and full cache queries, and tracking which global statistics writes persisted. New tests cover partial persistence failures, pseudo-stat replacement, version handling, and partition analyze behavior. ChangesStatistics refresh correctness
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: context deadline exceeded" 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.
🧹 Nitpick comments (1)
pkg/executor/analyze_global_stats.go (1)
46-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEliminate the redundant
tableIDsmap.The
tableIDsmap is an exact copy ofglobalStatsTableIDsand can be safely removed to avoid unnecessary allocations. You can iterate overglobalStatsTableIDsdirectly when dumping stats to historical storage later in the function.♻️ Proposed refactor
Apply the following change here:
- tableIDs := make(map[int64]struct{}, len(globalStatsTableIDs)) updatedTableIDs := make([]int64, 0, len(globalStatsTableIDs)) for tableID := range globalStatsTableIDs { - tableIDs[tableID] = struct{}{} tableUpdated := falseAnd update the historical storage loop below (line 92):
for tableID := range globalStatsTableIDs { // Dump stats to historical storage. if err := recordHistoricalStats(e.Ctx(), tableID); err != nil { logutil.BgLogger().Error("record historical stats failed", zap.Error(err)) } }🤖 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/executor/analyze_global_stats.go` around lines 46 - 50, Remove the redundant tableIDs map from the analyzeGlobalStats flow, including its population inside the loop. Update the historical stats dumping loop to iterate directly over globalStatsTableIDs while preserving the existing recordHistoricalStats and error-logging behavior.
🤖 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.
Nitpick comments:
In `@pkg/executor/analyze_global_stats.go`:
- Around line 46-50: Remove the redundant tableIDs map from the
analyzeGlobalStats flow, including its population inside the loop. Update the
historical stats dumping loop to iterate directly over globalStatsTableIDs while
preserving the existing recordHistoricalStats and error-logging behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3fa63576-7f55-4c2a-8695-5b2e821540c8
📒 Files selected for processing (12)
.agents/skills/tidb-test-guidelines/references/executor-case-map.md.agents/skills/tidb-test-guidelines/references/statistics-case-map.mdpkg/executor/analyze.gopkg/executor/analyze_global_stats.gopkg/executor/test/analyzetest/BUILD.bazelpkg/executor/test/analyzetest/analyze_test.gopkg/statistics/handle/cache/statscache.gopkg/statistics/handle/globalstats/BUILD.bazelpkg/statistics/handle/globalstats/global_stats.gopkg/statistics/handle/globalstats/global_stats_test.gopkg/statistics/handle/handletest/statstest/stats_test.gopkg/statistics/handle/types/interfaces.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69883 +/- ##
================================================
- Coverage 76.3207% 74.2762% -2.0445%
================================================
Files 2041 2068 +27
Lines 559975 585444 +25469
================================================
+ Hits 427377 434846 +7469
- Misses 131697 149282 +17585
- Partials 901 1316 +415
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@wlwilliamx: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
What problem does this PR solve?
Issue Number: close #69882
Problem Summary:
ANALYZE TABLEsynchronously refreshes analyzed statistics through a targeted stats cache update. That path reused the cache-wide version lower bound, so an unrelated stats update could move the watermark past the analyzed table's persisted version and leave its cached entry pseudo.What changed and how does it work?
Check List
Tests
Unit tests:
./tools/check/failpoint-go-test.sh pkg/statistics/handle/handletest/statstest -run '^TestStatsCacheProcess$' -count=1./tools/check/failpoint-go-test.sh pkg/executor/test/analyzetest -run '^TestAnalyzeRefreshesOnlyPersistedStats$' -count=1./tools/check/failpoint-go-test.sh pkg/statistics/handle/globalstats -run '^TestWriteGlobalStatsToStoragePreservesFirstError$' -count=1Manual checks:
make bazel_preparemake check-bazel-preparemake fmtmake lintgit diff --checkSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Bug Fixes
ANALYZEto refresh global stats metadata based on what was actually persisted, and avoid incorrectly refreshing from non-updated sources.Tests
Documentation