-
Notifications
You must be signed in to change notification settings - Fork 6k
planner: calculate plan_digest in place when fetching plan exec info from stmt_stats in SHOW PLAN FOR <SQL>
#60273
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
Conversation
Hi @qw4990. 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 #60273 +/- ##
================================================
+ Coverage 73.1221% 75.0402% +1.9180%
================================================
Files 1710 1756 +46
Lines 472557 482549 +9992
================================================
+ Hits 345544 362106 +16562
+ Misses 105730 97765 -7965
- Partials 21283 22678 +1395
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
/test unit-test |
@qw4990: 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. |
/test all |
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.
Seems fine to me.
pkg/bindinfo/binding_auto.go
Outdated
if planDigest == "" { | ||
_ = callWithSCtx(ba.sPool, false, func(sctx sessionctx.Context) error { | ||
planDigest = getBindingPlanDigest(sctx, binding.Db, binding.BindSQL) | ||
return nil | ||
}) | ||
} |
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.
Since generating a new plan digest seems like an expensive operation (invoking the planner and optimizing the plan), maybe we should log an info message when we do this? Just a suggestion, though: I'm fine with not logging if you think that makes more sense.
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.
Got it, but no logging seems OK since the generation operation is a in-memory operation, which shouldn't take a long time, and SHOW PLAN FOR <SQL>
should be a low-frequently operation, so it should be OK here without any logging.
@henrybw: adding LGTM is restricted to approvers and reviewers in OWNERS files. 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. |
[LGTM Timeline notifier]Timeline:
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hawkingrei, henrybw, tangenta, time-and-fate 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 |
/retest |
/test unit-test |
@qw4990: 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. |
/test unit-test |
@qw4990: 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. |
/test mysql-test |
@qw4990: 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. |
/test check-dev2 |
@qw4990: 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. |
/retest |
@qw4990: 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. |
/ok-to-test |
/retest |
@qw4990: 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. |
@qw4990: 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. |
/test check_dev_2 |
@qw4990: 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. |
/ok-to-test |
@terry1purcell: 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. |
/ok-to-test |
/retest |
1 similar comment
/retest |
@qw4990: 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. |
/test check-dev2 |
@qw4990: 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. |
/test unit-test |
@qw4990: 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. |
…from stmt_stats in `SHOW PLAN FOR <SQL>` (pingcap#60273) ref pingcap#60148
What problem does this PR solve?
Issue Number: ref #60148
Problem Summary: planner: calculate plan_digest in place when fetching plan exec info from stmt_stats in
SHOW PLAN FOR <SQL>
What changed and how does it work?
Due to some historical reason,
binding.plan_digest
might be empty, which prevents us from joiningmysql.bind_info
andstmt_stats
to get bindings' corresponding plan execution info.For example, if a binding is created manually like
create global binding using select ...
, then itsplan_digest
is empty by default.This PR calculates plan_digest for bindings in place when trying to fetch exec info for a binding. All logic is behind
getBindingPlanDigest
.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.