-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
stmtsummary: implement tidb_statements_stats, a cumulative version of statements_summary #57155
Conversation
Hi @henrybw. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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 kubernetes-sigs/prow repository. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #57155 +/- ##
================================================
+ Coverage 73.1839% 75.3833% +2.1994%
================================================
Files 1671 1719 +48
Lines 460731 474058 +13327
================================================
+ Hits 337181 357361 +20180
+ Misses 102827 94612 -8215
- Partials 20723 22085 +1362
Flags with carried forward coverage won't be shown. Click here to find out more.
|
aa3f776
to
b6e59d5
Compare
/retest |
@xhebox: Cannot trigger testing until a trusted user reviews the PR and leaves an In 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 kubernetes-sigs/prow repository. |
/hold |
/approve infoschema part lgtm |
@@ -64,6 +64,27 @@ func NewStmtSummaryReader(user *auth.UserIdentity, hasProcessPriv bool, cols []* | |||
return reader | |||
} | |||
|
|||
// GetStmtSummaryCumulativeRows gets statement summary rows with cumulative metrics. | |||
func (ssr *stmtSummaryReader) GetStmtSummaryCumulativeRows() [][]types.Datum { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some input parameters to only needed stats back?like only return records after some ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that is a good idea, but I'm not sure if it makes sense in this context. For in-memory tables like this, I think the kind of filtering you're referring to would be done at a higher level, by the executor.
Perhaps we could implement a "pushdown" mechanism (like predicate pushdown to TiKV) for in-memory tables like this. While that could be a useful feature, I think building something like that would be a pretty large project, and is thus out of scope for this PR.
b6e59d5
to
8900c16
Compare
/rebuild |
/retest |
@xhebox: Cannot trigger testing until a trusted user reviews the PR and leaves an In 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 kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, djshow832, Rustin170506, xhebox 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 |
/unhold |
What problem does this PR solve?
Issue Number: ref #57147
Problem Summary: The statement summary tables are the current mechanism we have for collecting cumulative metrics about the execution and performance of SQL queries. However, these metrics aren't truly cumulative: they are refreshed at a regular interval, meaning the cumulative metrics collected for queries are reset, and their previous values are made accessible via a separate history table. For the workload repository, we want to be able to snapshot cumulative metrics aggregated over the lifetime of a TiDB instance, which the statement summary tables do not easily provide.
What is changed and how it works?
stmtSummaryStats
, that each statement summary interval (stmtSummaryByDigestElement
) now uses instead. The struct is embedded so existing field accesses continue to work as-is.stmtSummaryByDigest
), add a new "cumulative" set of statistics (i.e. anotherstmtSummaryStats
struct).information_schema.tidb_statements_stats
, that uses the samestmtSummaryRetriever
as the statement summary tables.stmtSummaryStats
parameter. For the existing statement summary tables, this parameter is thestmtSummaryStats
struct embedded in thestmtSummaryByDigestElement
(i.e. the same values as before). For the new cumulative statement stats table, this parameter is the cumulativestmtSummaryStats
struct in thestmtSummaryByDigest
. The column factories then read from thisstmtSummaryStats
parameter, rather than thestmtSummaryByDigestElement
struct directly, thus making them polymorphic and able to work for both the cumulative and statements summary tables.cluster_tidb_statements_stats
.(The diff for this PR is pretty large, so I'd recommend reading each individual commit one at a time, since they build on each other incrementally.)
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.