-
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
base: master
Are you sure you want to change the base?
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 72.9008% 74.7522% +1.8514%
================================================
Files 1672 1717 +45
Lines 461870 470110 +8240
================================================
+ Hits 336707 351418 +14711
+ Misses 104439 96625 -7814
- Partials 20724 22067 +1343
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 |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: D3Hunter, djshow832, xhebox The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@@ -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.
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.