sql: display retry count and time for EXPLAIN ANALYZE#142692
sql: display retry count and time for EXPLAIN ANALYZE#142692craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
There was a problem hiding this comment.
Pull Request Overview
This PR adds two new top-level fields to the output of EXPLAIN ANALYZE to display the number of transaction retries and the cumulative time spent retrying, making retries more visible during performance debugging. Key changes include:
- Adding AddRetryCount and AddRetryTime methods to the OutputBuilder.
- Updating instrumentation logic to record and pass retry count/time.
- Enhancing tests in output_test.go to verify the new fields, and updating connection executor calls to pass the retry counter.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/sql/opt/exec/explain/output_test.go | Added a new test (TestRetryFields) validating retry field output |
| pkg/sql/opt/exec/explain/output.go | Introduced AddRetryCount and AddRetryTime functions |
| pkg/sql/instrumentation.go | Updated instrumentation to capture and emit retry fields |
| pkg/sql/conn_executor_exec.go | Modified function arguments to include autoRetryCounter |
| if matches := retryCountRE.FindStringSubmatch(res); len(matches) == 0 { | ||
| foundCount = true | ||
| } | ||
| if matches := retryTimeRE.FindStringSubmatch(res); len(matches) == 0 { |
There was a problem hiding this comment.
It appears the condition is inverted; to verify that the retry count field is present, the check should verify that matches length is greater than 0 (i.e., len(matches) > 0) rather than equal to 0.
| if matches := retryCountRE.FindStringSubmatch(res); len(matches) == 0 { | |
| foundCount = true | |
| } | |
| if matches := retryTimeRE.FindStringSubmatch(res); len(matches) == 0 { | |
| if matches := retryCountRE.FindStringSubmatch(res); len(matches) > 0 { | |
| foundCount = true | |
| } | |
| if matches := retryTimeRE.FindStringSubmatch(res); len(matches) > 0 { |
There was a problem hiding this comment.
Hm, I think copilot is right 😃
| if matches := retryCountRE.FindStringSubmatch(res); len(matches) == 0 { | ||
| foundCount = true | ||
| } | ||
| if matches := retryTimeRE.FindStringSubmatch(res); len(matches) == 0 { |
There was a problem hiding this comment.
Similarly, the condition should check for matches length greater than 0 to correctly detect the presence of the retry time field.
| if matches := retryTimeRE.FindStringSubmatch(res); len(matches) == 0 { | |
| if matches := retryTimeRE.FindStringSubmatch(res); len(matches) > 0 { |
yuzefovich
left a comment
There was a problem hiding this comment.
Nice! 
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @copilot-pull-request-reviewer[bot], @DrewKimball, and @rytaft)
-- commits line 12 at r1:
nit: consider mentioning that these fields are only shown when non-zero.
pkg/sql/conn_executor_exec.go line 557 at r1 (raw file):
&stmt, os.ImplicitTxn.Get(), // This goroutine is the only one that can modify // txnState.mu.priority, so we don't need to get a mutex here.
nit: adjust the comment to mention autoRetryCounter too.
| if matches := retryCountRE.FindStringSubmatch(res); len(matches) == 0 { | ||
| foundCount = true | ||
| } | ||
| if matches := retryTimeRE.FindStringSubmatch(res); len(matches) == 0 { |
There was a problem hiding this comment.
Hm, I think copilot is right 😃
This commit adds two new top-level fields to the plan output of `EXPLAIN ANALYZE` displaying the number of transaction retries and cumulative time spent due to the retries. This will help make retries more visible when debugging poor performance via statement bundle. Fixes cockroachdb#142674 Release note (sql change): EXPLAIN ANALYZE statements now display the number of transaction retries and time spent retrying as part of the plan output, if non-zero.
5dc26f0 to
805d6dc
Compare
DrewKimball
left a comment
There was a problem hiding this comment.
Reviewable status:
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: consider mentioning that these fields are only shown when non-zero.
Done.
pkg/sql/conn_executor_exec.go line 557 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: adjust the comment to mention
autoRetryCountertoo.
Done.
| if matches := retryCountRE.FindStringSubmatch(res); len(matches) == 0 { | ||
| foundCount = true | ||
| } | ||
| if matches := retryTimeRE.FindStringSubmatch(res); len(matches) == 0 { |
|
TFTR! bors r=yuzefovich |
|
Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches. Issue #142674: branch-release-24.1, branch-release-24.3, branch-release-25.1. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 805d6dc to blathers/backport-release-24.1-142692: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 24.1.x failed. See errors above. error creating merge commit from 805d6dc to blathers/backport-release-24.3-142692: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 24.3.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
This commit adds two new top-level fields to the plan output of
EXPLAIN ANALYZEdisplaying the number of transaction retries and cumulative time spent due to the retries. This will help make retries more visible when debugging poor performance via statement bundle.Fixes #142674
Release note (sql change): EXPLAIN ANALYZE statements now display the number of transaction retries and time spent retrying as part of the plan output.