executor: expose read pool task details in diagnostics#69971
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-tests-checked
release-note
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
the
size/XL
|
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:
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 selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRead-pool task details are collected from TiKV responses, propagated through runtime statistics and statement contexts, and exposed in slow-query diagnostics and ChangesRead-pool diagnostics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TiKV
participant Coprocessor
participant RuntimeStats
participant SlowQuery
TiKV->>Coprocessor: Return ExecDetailsV2
Coprocessor->>RuntimeStats: Record ReadPoolTaskDetails
RuntimeStats->>SlowQuery: Serialize read-pool details
Possibly related PRs
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 |
0268288 to
13ad727
Compare
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/util/execdetails/runtime_stats.go (1)
599-607: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment doesn't capture the new parameter's non-obvious clone/merge contract.
The comment just restates the function name. Given the new
readPoolTaskDetailsparameter is cloned on first record but merged in-place on subsequent records (lines 619-631), documenting that asymmetry would help future readers avoid re-introducing aliasing bugs.As per coding guidelines: "Comments SHOULD explain non-obvious intent, constraints, invariants... SHOULD NOT restate what the code already makes clear."
🤖 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/execdetails/runtime_stats.go` around lines 599 - 607, Update the doc comment for RuntimeStatsColl.RecordCopStats to document the readPoolTaskDetails ownership contract: the first record clones the provided details, while subsequent records merge into the existing details in place. Preserve the existing behavior and focus the comment on this aliasing constraint rather than restating the method name.Source: Coding guidelines
pkg/util/execdetails/execdetails_test.go (1)
393-393: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test exercises
RecordCopStats's actual clone/merge branches forreadPoolTaskDetails.All three
RecordCopStatscall sites here passnil; the only assertion on read-pool formatting (416-426) setscop.readPoolTaskDetailsdirectly, bypassingRecordCopStats. The clone-on-first-call and merge-on-repeat-call logic added inruntime_stats.go(619-631) — the path actually used when merging real coprocessor responses for the same plan node — has no direct test coverage.Also applies to: 416-426, 747-747, 1100-1102
🤖 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/execdetails/execdetails_test.go` at line 393, Add tests that invoke RecordCopStats with non-nil readPoolTaskDetails to cover both cloning on the first call and merging on subsequent calls for the same plan node. Assert the stored values after each call, and update the existing formatting test to populate readPoolTaskDetails through RecordCopStats rather than assigning cop.readPoolTaskDetails directly; preserve the current nil-input coverage.
🤖 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/util/execdetails/execdetails_test.go`:
- Line 393: Add tests that invoke RecordCopStats with non-nil
readPoolTaskDetails to cover both cloning on the first call and merging on
subsequent calls for the same plan node. Assert the stored values after each
call, and update the existing formatting test to populate readPoolTaskDetails
through RecordCopStats rather than assigning cop.readPoolTaskDetails directly;
preserve the current nil-input coverage.
In `@pkg/util/execdetails/runtime_stats.go`:
- Around line 599-607: Update the doc comment for
RuntimeStatsColl.RecordCopStats to document the readPoolTaskDetails ownership
contract: the first record clones the provided details, while subsequent records
merge into the existing details in place. Preserve the existing behavior and
focus the comment on this aliasing constraint rather than restating the method
name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 61693938-3f8e-41da-b417-71a3302f37fa
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
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/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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69971 +/- ##
================================================
- Coverage 76.3266% 75.5501% -0.7766%
================================================
Files 2041 2086 +45
Lines 559579 583026 +23447
================================================
+ Hits 427108 440477 +13369
- Misses 131570 140379 +8809
- Partials 901 2170 +1269
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
13ad727 to
b01d9df
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/util/execdetails/execdetails_test.go (2)
269-273: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover aggregation, not only initial assignment.
This single merge would also pass if the implementation overwrote instead of accumulated details. Merge two deterministic payloads and assert summed fields while confirming
RequestCountremains unchanged.As per coding guidelines, keep unit-test changes minimal and deterministic.
🤖 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/execdetails/execdetails_test.go` around lines 269 - 273, Update the test around SyncExecDetails.MergeReadPoolTaskDetails to merge two deterministic poolTaskDetails payloads, then assert merged ReadPoolTaskDetails fields equal their sums while confirming RequestCount remains zero. Keep the existing initial assignment coverage and make only the minimal test changes needed to distinguish accumulation from overwriting.Source: Coding guidelines
393-393: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExercise
RecordCopStatspropagation instead of mutating runtime stats directly.The test passes
nilat Line 393 and then assignscop.readPoolTaskDetailsdirectly at Lines 416-424, so a brokenRecordCopStats(..., readPoolTaskDetails, ...)propagation would remain undetected. Supply the details throughRecordCopStatsand assert the resulting output.This is needed to cover the runtime-statistics contract described in the PR objectives.
Also applies to: 416-426
🤖 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/execdetails/execdetails_test.go` at line 393, Update the test around RecordCopStats to pass readPoolTaskDetails through its corresponding argument instead of supplying nil, remove the direct cop.readPoolTaskDetails assignment, and assert that RecordCopStats propagates those details into the resulting output.
🤖 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/util/execdetails/execdetails_test.go`:
- Around line 269-273: Update the test around
SyncExecDetails.MergeReadPoolTaskDetails to merge two deterministic
poolTaskDetails payloads, then assert merged ReadPoolTaskDetails fields equal
their sums while confirming RequestCount remains zero. Keep the existing initial
assignment coverage and make only the minimal test changes needed to distinguish
accumulation from overwriting.
- Line 393: Update the test around RecordCopStats to pass readPoolTaskDetails
through its corresponding argument instead of supplying nil, remove the direct
cop.readPoolTaskDetails assignment, and assert that RecordCopStats propagates
those details into the resulting output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c8b353c-ef1b-4752-bd1f-86083ef98c18
⛔ 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
🚧 Files skipped from review as they are similar to previous changes (13)
- pkg/infoschema/tables.go
- DEPS.bzl
- pkg/executor/slow_query.go
- go.mod
- pkg/store/mockstore/unistore/rpc.go
- pkg/executor/point_get.go
- pkg/store/copr/coprocessor.go
- pkg/executor/batch_point_get.go
- pkg/executor/slow_query_sql_test.go
- pkg/distsql/select_result.go
- pkg/util/execdetails/runtime_stats.go
- pkg/infoschema/test/clustertablestest/tables_test.go
- pkg/util/execdetails/execdetails.go
|
/retest |
ti-chi-bot
Bot
added
approved
lgtm
and removed
needs-1-more-lgtm
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
3465a72 to
3cda80b
Compare
Signed-off-by: Chao Wang <cclcwangchao@hotmail.com>
036e0fd to
23d59dd
Compare
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
23d59dd to
228452d
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfzjywxk, D3Hunter, wjhuang2016 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 |
|
/cherry-pick release-nextgen-202603 |
|
@lcwangchao: new pull request created to branch DetailsIn response to this:
Instructions 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. |
5 participants
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
EXPLAIN ANALYZE, slow-query logs, andinformation_schema.slow_query, including runtime-statistics propagation.information_schema.slow_querywith a newread_pool_task_detailscolumn.