*: add last ru consumption for tidb_last_query_info #49769
Conversation
Signed-off-by: nolouch <nolouch@gmail.com>
ti-chi-bot
bot
added
release-note-none
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #49769 +/- ##
================================================
+ Coverage 70.9391% 71.0126% +0.0735%
================================================
Files 1368 1433 +65
Lines 396705 426472 +29767
================================================
+ Hits 281419 302849 +21430
- Misses 95593 104616 +9023
+ Partials 19693 19007 -686
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
| @@ -2351,23 +2351,6 @@ func runStmt(ctx context.Context, se *session, s sqlexec.Statement) (rs sqlexec. | |||
|
|
|||
| sessVars := se.sessionVars | |||
|
|
|||
| // Record diagnostic information for DML statements | |||
There was a problem hiding this comment.
This seems to be a behavior change. Before this PR, LastQueryInfo is record even if this function returns error, but after this change, if runStmt returns error and the returned RecordSet is nil, FinishExecuteStmt won't be called so LastQueryInfo won't be recorded either.
There was a problem hiding this comment.
Most errors occur during the execution stage, before it records the error of the compile+optimize stage. I think it's reasonable move to here. and you can see the manual test on the issue text. I kill the pd, it can record pd timeout error during execution.
There was a problem hiding this comment.
cc @djshow832 is it ok to move to FinishExecuteStmt?
| lastRUConsumption = float64(len(sessVars.StmtCtx.OriginalSQL)) | ||
| }) | ||
| // Keep the previous queryInfo for `show session_states` because the statement needs to encode it. | ||
| sessVars.LastQueryInfo = sessionstates.QueryInfo{ |
There was a problem hiding this comment.
Another question: is it better that when the statement type is not exec or dml, the lastQueryInfo/LastRUConsumption should be reset instead of just keep unchanged?
There was a problem hiding this comment.
I think we can keep unchanged.
ti-chi-bot
bot
added
approved
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CabinfeverB, glorv The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
ti-chi-bot
added
the
needs-cherry-pick-release-7.5
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
needs-cherry-pick-release-7.1
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
What problem does this PR solve?
Issue Number: ref #49318
Problem Summary:
What changed and how does it work?
Check List
Tests
with debug version (last query)
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.