statistics, executor, session: integrate analyze resource control#69452
Merged
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note-none
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesStats processing source type update
Sequence Diagram(s)sequenceDiagram
participant RunAnalyzeStmt
participant GetAutoAnalyzeParameters
participant autoAnalyzeRequestContext
participant statsutilExec as statsutil.ExecWithCtxAndOpts
participant ExecRestrictedSQL
RunAnalyzeStmt->>GetAutoAnalyzeParameters: read auto-analyze parameters
GetAutoAnalyzeParameters-->>RunAnalyzeStmt: analyzeParameters
RunAnalyzeStmt->>autoAnalyzeRequestContext: build request context
autoAnalyzeRequestContext-->>RunAnalyzeStmt: ctx with internal source type
RunAnalyzeStmt->>statsutilExec: execute stats SQL with ctx
statsutilExec->>ExecRestrictedSQL: run restricted SQL with provided ctx
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
🤖 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/statistics/handle/util/util.go`:
- Around line 59-60: The capture guard in the plan replayer currently only
excludes requests marked with kv.InternalTxnStats, so the new StatsCtx source
type kv.InternalTxnStatsProcessing can still be captured. Update the condition
in pkg/executor/adapter.go where the capture skip is decided to treat both
internal stats source types as excluded, using the existing context/source check
around the adapter logic so StatsCtx remains uncaptured.
🪄 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: f71aa3c7-8506-4a82-8108-53fe436714be
📒 Files selected for processing (4)
pkg/kv/option.gopkg/statistics/handle/lockstats/query_lock_test.gopkg/statistics/handle/util/test/ctx_matcher.gopkg/statistics/handle/util/util.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69452 +/- ##
================================================
- Coverage 76.3227% 75.9371% -0.3857%
================================================
Files 2041 2078 +37
Lines 560399 580342 +19943
================================================
+ Hits 427712 440695 +12983
- Misses 131786 137508 +5722
- Partials 901 2139 +1238
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/M
0xPoe
force-pushed
the
poe-patch-stats-processing
branch
from
6d2329a to
873577b
Compare
ti-chi-bot
Bot
added
size/XL
0xPoe
changed the title
ti-chi-bot
Bot
added
release-note
0xPoe
force-pushed
the
poe-patch-stats-processing
branch
from
873577b to
9ba3af4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/session/bootstrap.go (1)
421-422: 🩺 Stability & Availability | 🔵 TrivialFresh-bootstrap-only configuration creates fresh-vs-upgraded divergence.
This
ALTER RESOURCE GROUPruns indoDDLWorks, which only executes on fresh bootstrap. The accompanying upgrade test asserts that upgraded clusters end up withBackground == nil. Net effect: newly bootstrapped clusters get thestatsbackground throttling (UTILIZATION_LIMIT=30) on thedefaultgroup, while existing clusters upgraded to this version do not. If that asymmetry is intentional (preserve operator-configured settings on existing clusters), no action is needed; otherwise consider whether upgraded clusters should receive an equivalent default.🤖 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/session/bootstrap.go` around lines 421 - 422, The ALTER RESOURCE GROUP in doDDLWorks only affects fresh bootstrap, creating a mismatch with upgraded clusters that keep Background nil. Check the bootstrap/upgrade path around doDDLWorks and the default ResourceGroup setup for resourcegroup.DefaultResourceGroupName, and decide whether upgraded clusters should also receive the same stats throttling (kv.InternalTxnStats with defaultAnalyzeBackgroundUtilizationLimit) or whether the asymmetry is intentional. If the default should be consistent, add the equivalent initialization in the upgrade path/testable bootstrap logic rather than only in fresh bootstrap.pkg/statistics/handle/autoanalyze/exec/exec.go (1)
112-139: 🎯 Functional Correctness | 🔵 TrivialRunAnalyzeStmt is exported but hardcoded to auto-analyze context.
Verification found that
RunAnalyzeStmtis currently called only byAutoAnalyzewithin the same package, confirming no immediate non-auto-analyze callers exist. However, the function is exported and unconditionally derives its request context from auto-analyze window parameters (GetAutoAnalyzeParameters). This design couples the function strictly to the auto-analyze flow, creating a risk if exported usage expands to manual or ad-hoc analysis paths in the future. Consider renaming toRunAutoAnalyzeStmtor factoring the context derivation to allow explicit caller control.🤖 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/statistics/handle/autoanalyze/exec/exec.go` around lines 112 - 139, RunAnalyzeStmt is exported but its behavior is hardwired to the auto-analyze flow by always building the request context with auto-analyze window parameters. Update the API in exec.go so the naming and intent match the behavior: either rename RunAnalyzeStmt to RunAutoAnalyzeStmt, or refactor it to accept an explicit request context/parameter source instead of always calling autoAnalyzeRequestContext(GetAutoAnalyzeParameters(sctx)). Keep the AutoAnalyze caller aligned with the chosen symbol.
🤖 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/distsql/distsql.go`:
- Around line 209-214: The analyze execution path is using a context-based
source type in distsql.Analyze while the stats-processing session var is already
set to InternalTxnStatsProcessing, so the two paths diverge. Update the analyze
call sites in the analyze execution flow to pass a context wrapped with
contextWithAnalyzeRequestSource (or an equivalent kv.WithInternalSourceType
setup) before invoking distsql.Analyze, so analyzeRequestSourceType in
pkg/distsql/distsql.go sees InternalTxnStatsProcessing just like the sibling
helper in pkg/executor/analyze.go.
In `@pkg/session/bootstrap.go`:
- Around line 421-422: The bootstrap flow in doDDLWorks() alters the default
resource group without guaranteeing it exists in storage first. Add a CREATE
RESOURCE GROUP IF NOT EXISTS for resourcegroup.DefaultResourceGroupName before
the existing ALTER RESOURCE GROUP call, so the default group is present for both
classic and next-gen kernels. Keep the fix localized around the mustExecute DDL
sequence in pkg/session/bootstrap.go and preserve the existing ALTER RESOURCE
GROUP settings afterward.
---
Nitpick comments:
In `@pkg/session/bootstrap.go`:
- Around line 421-422: The ALTER RESOURCE GROUP in doDDLWorks only affects fresh
bootstrap, creating a mismatch with upgraded clusters that keep Background nil.
Check the bootstrap/upgrade path around doDDLWorks and the default ResourceGroup
setup for resourcegroup.DefaultResourceGroupName, and decide whether upgraded
clusters should also receive the same stats throttling (kv.InternalTxnStats with
defaultAnalyzeBackgroundUtilizationLimit) or whether the asymmetry is
intentional. If the default should be consistent, add the equivalent
initialization in the upgrade path/testable bootstrap logic rather than only in
fresh bootstrap.
In `@pkg/statistics/handle/autoanalyze/exec/exec.go`:
- Around line 112-139: RunAnalyzeStmt is exported but its behavior is hardwired
to the auto-analyze flow by always building the request context with
auto-analyze window parameters. Update the API in exec.go so the naming and
intent match the behavior: either rename RunAnalyzeStmt to RunAutoAnalyzeStmt,
or refactor it to accept an explicit request context/parameter source instead of
always calling autoAnalyzeRequestContext(GetAutoAnalyzeParameters(sctx)). Keep
the AutoAnalyze caller aligned with the chosen symbol.
🪄 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: c6ef31fe-eb21-4694-831d-1ee5caf4a0b9
📒 Files selected for processing (31)
pkg/ddl/index.gopkg/ddl/options_test.gopkg/distsql/BUILD.bazelpkg/distsql/distsql.gopkg/distsql/distsql_test.gopkg/domain/domain.gopkg/domain/extract.gopkg/domain/plan_replayer.gopkg/executor/adapter.gopkg/executor/analyze.gopkg/executor/builder.gopkg/executor/infoschema_reader.gopkg/executor/internal/pdhelper/pd.gopkg/executor/plan_replayer.gopkg/executor/show.gopkg/executor/test/planreplayer/BUILD.bazelpkg/executor/test/planreplayer/plan_replayer_test.gopkg/kv/option.gopkg/planner/indexadvisor/utils.gopkg/session/bootstrap.gopkg/session/session.gopkg/session/test/bootstraptest/BUILD.bazelpkg/session/test/bootstraptest/bootstrap_upgrade_test.gopkg/session/tidb_test.gopkg/statistics/handle/autoanalyze/exec/BUILD.bazelpkg/statistics/handle/autoanalyze/exec/exec.gopkg/statistics/handle/autoanalyze/exec/request_source_test.gopkg/statistics/handle/bootstrap.gopkg/statistics/handle/lockstats/query_lock_test.gopkg/statistics/handle/util/test/ctx_matcher.gopkg/statistics/handle/util/util.go
✅ Files skipped from review due to trivial changes (5)
- pkg/executor/test/planreplayer/BUILD.bazel
- pkg/distsql/BUILD.bazel
- pkg/session/test/bootstraptest/BUILD.bazel
- pkg/executor/builder.go
- pkg/kv/option.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/statistics/handle/util/test/ctx_matcher.go
- pkg/executor/adapter.go
- pkg/statistics/handle/lockstats/query_lock_test.go
- pkg/executor/test/planreplayer/plan_replayer_test.go
0xPoe
force-pushed
the
poe-patch-stats-processing
branch
from
9ba3af4 to
166063b
Compare
ti-chi-bot
Bot
added
size/L
Member
Author
|
/retest |
0xPoe
force-pushed
the
poe-patch-stats-processing
branch
from
166063b to
2d5b22f
Compare
0xPoe
commented
Jun 26, 2026
0xPoe
force-pushed
the
poe-patch-stats-processing
branch
3 times, most recently
from
8e1b291 to
5bdef14
Compare
ti-chi-bot
Bot
added
the
do-not-merge/hold
Signed-off-by: 0xPoe <techregister@pm.me>
Signed-off-by: 0xPoe <techregister@pm.me>
Signed-off-by: 0xPoe <poe.liu@pm.me>
Signed-off-by: 0xPoe <poe.liu@pm.me>
0xPoe
force-pushed
the
poe-patch-stats-processing
branch
from
69fbf08 to
54de99a
Compare
Signed-off-by: 0xPoe <poe.liu@pm.me>
0xPoe
force-pushed
the
poe-patch-stats-processing
branch
from
4e5a683 to
8a7eb6c
Compare
Member
Author
|
/retest |
Member
Author
|
/test pull-unit-test-next-gen |
Member
Author
|
/test pull-unit-test-next-gen |
Member
Author
|
/retest |
1 similar comment
Member
Author
|
/retest |
time-and-fate
approved these changes
Jul 7, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: time-and-fate, winoros, YangKeao 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 |
Member
Author
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
D3Hunter
pushed a commit
to D3Hunter/tidb
that referenced
this pull request
Jul 9, 2026
This was referenced Jul 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: ref #69472
Problem Summary:
ANALYZE Resource Control needs to throttle ANALYZE without throttling unrelated internal statistics work.
What changed and how does it work?
StatsForegroundPriorityrequest source for non-ANALYZE statistics work.The test result summary: #69452 (comment) I will do more tests once the whole project is finished. Then we can connect all the dots to see what the real performance impact is.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Bug Fixes