executor: expose read pool task details in diagnostics (#69971) - #70067
Conversation
ti-chi-bot
added
do-not-merge/hold
|
@lcwangchao This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. 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 ti-community-infra/tichi repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
🚧 Files skipped from review as they are similar to previous changes (15)
📝 WalkthroughWalkthroughThe change adds read-pool task details to execution statistics, propagates them through coprocessor and point-get paths, and exposes them in slow-query and ChangesRead-pool diagnostics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant UnistoreRPC
participant Coprocessor
participant RuntimeStats
participant StmtCtx
participant SlowQuery
UnistoreRPC->>Coprocessor: Return read-pool execution details
Coprocessor->>RuntimeStats: Record read-pool task details
RuntimeStats->>StmtCtx: Merge execution details
StmtCtx->>SlowQuery: Emit read_pool_task_details
Possibly related PRs
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
1-1: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winUnresolved git merge-conflict markers across six files — must be resolved before merge.
Literal
<<<<<<< HEAD/=======/>>>>>>>markers remain in the diff. This breaksgo.modparsing (blocking every Go build), Bazel dependency resolution, and Go compilation of the affected source/test files. This matches the PR's own hold status ("merge conflicts... resolve conflicts before/unhold"), but each site still needs explicit resolution:
go.mod#L103-107: resolve thekvprotoversion conflict, keeping the incoming (newer) pseudo-version to matchDEPS.bzl.go.mod#L130-136: resolve theclient-go/pd/clientconflict; confirm thepd/clientpin isn't an unintended downgrade (see separate verification comment).DEPS.bzl#L3522-3529: resolve thekvprotosum/versionconflict to match the resolvedgo.modvalue.DEPS.bzl#L4214-4221: resolve theclient-go/v2sum/versionconflict to match the resolvedgo.modvalue.pkg/infoschema/tables.go#L937-943: resolve by keeping the incoming side's 4 new column definitions (IA remote read +ReadPoolTaskDetailsStr).pkg/infoschema/test/clustertablestest/tables_test.go#L448-454: keep the incoming side's 4 added placeholder values to match the resolved column list intables.go.pkg/infoschema/test/clustertablestest/tables_test.go#L551-557: same as above, second expected row.pkg/executor/slow_query_test.go#L193-197: keep the incoming side's expanded CSV fragment (4,4096,0.015,,,,) matching the new columns.pkg/executor/slow_query_test.go#L220-224: same as above, second occurrence.pkg/executor/slow_query_test.go#L292-300: keep the incoming side's shifted indices ([45]/[46]) consistent with the 4 added columns.🤖 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 `@go.mod` at line 1, Resolve all remaining merge-conflict markers across the listed dependency, schema, and test files. In go.mod and DEPS.bzl, retain the incoming newer kvproto and matching client-go/v2 dependency values, verifying the pd/client pin is not downgraded; in tables.go retain the four incoming column definitions, and update the corresponding clustertablestest expected rows and slow_query_test CSV fragments and indices to match them. Remove every conflict marker and ensure all dependency and test values remain consistent.
🤖 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 `@go.mod`:
- Around line 103-107: Resolve the merge conflicts in go.mod by removing all
conflict markers and retaining the intended single github.com/pingcap/kvproto
dependency version at both conflicted sections. Verify the resulting go.mod is
valid and contains no remaining <<<<<<<, =======, or >>>>>>> markers.
In `@pkg/distsql/select_result.go`:
- Around line 405-422: Resolve the merge conflict in the error-handling block of
the select-result flow by removing all literal conflict markers while preserving
the intended resultSubset-based CopExecDetails and ReadPoolTaskDetails merging
before return errors.Trace(err). Keep the existing guard and CopRuntimeStats
handling intact, including skipping updateCopRuntimeStats on this path.
---
Outside diff comments:
In `@go.mod`:
- Line 1: Resolve all remaining merge-conflict markers across the listed
dependency, schema, and test files. In go.mod and DEPS.bzl, retain the incoming
newer kvproto and matching client-go/v2 dependency values, verifying the
pd/client pin is not downgraded; in tables.go retain the four incoming column
definitions, and update the corresponding clustertablestest expected rows and
slow_query_test CSV fragments and indices to match them. Remove every conflict
marker and ensure all dependency and test values remain consistent.
🪄 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 Plus
Run ID: 6e3da196-92f1-498a-a985-8d159c82f87a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
DEPS.bzlgo.modpkg/distsql/select_result.gopkg/executor/batch_point_get.gopkg/executor/point_get.gopkg/executor/slow_query.gopkg/executor/slow_query_sql_test.gopkg/executor/slow_query_test.gopkg/infoschema/tables.gopkg/infoschema/test/clustertablestest/tables_test.gopkg/store/copr/coprocessor.gopkg/store/mockstore/unistore/rpc.gopkg/util/execdetails/execdetails.gopkg/util/execdetails/execdetails_test.gopkg/util/execdetails/runtime_stats.go
|
Cherry-pick conflicts appear resolved; removing the |
ti-chi-bot
removed
the
do-not-merge/hold
|
/unhold |
|
/retest |
7 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
ti-chi-bot
Bot
added
approved
lgtm
and removed
needs-1-more-lgtm
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfzjywxk, D3Hunter, lcwangchao 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 |
|
/retest |
1 similar comment
|
/retest |
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
|
/test pull-unit-test-next-gen |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-nextgen-202603 #70067 +/- ##
===========================================================
Coverage ? 76.1963%
===========================================================
Files ? 1937
Lines ? 541273
Branches ? 0
===========================================================
Hits ? 412430
Misses ? 128843
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
c85ef3a to
3ad4b69
Compare
|
/retest |
fb769a3
into
pingcap:release-nextgen-202603
5 participants
This is an automated cherry-pick of #69971
What problem does this PR solve?
Issue Number: close #69970
Problem Summary:
TiDB does not expose read-pool task scheduling and execution details, making it difficult to diagnose fairness scheduling latency from SQL diagnostics.
What changed and how does it work?
READ_POOL_TASK_DETAILStoINFORMATION_SCHEMA.SLOW_QUERY.read_pool:{...}inEXPLAIN ANALYZEruntime statistics.Example outputs
The following examples were captured from actual SQL execution against embedded unistore.
EXPLAIN ANALYZE
Slow log
INFORMATION_SCHEMA.SLOW_QUERY
Check List
Tests
Side effects
Documentation
Release note
Tests
GOWORK=off ./tools/check/failpoint-go-test.sh pkg/executor -run '^TestReadPoolTaskDetailsInDiagnostics$' -count=1GOWORK=off go test -tags=intest,deadlock ./pkg/util/execdetails -run '^(TestString|TestCopRuntimeStats)$' -count=1GOWORK=off go test -tags=intest,deadlock ./pkg/infoschema/test/clustertablestest -run '^TestSlowQuery$' -count=1Summary by CodeRabbit
information_schemaslow-query records andEXPLAIN ANALYZE, with stats merged across coprocessor and point/batch lookups.read_pool_task_detailsand updated slow-query expectation data.