planner: fix the issue of reusing wrong point-plan for "select ... for update" #54661
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-triage-completed
release-note-none
|
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. |
There was a problem hiding this comment.
LGTM, some refactor and improvements mentioned by @MyonKeminta could be done in later PRs.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #54661 +/- ##
=================================================
- Coverage 72.8656% 56.0969% -16.7687%
=================================================
Files 1551 1672 +121
Lines 436701 611860 +175159
=================================================
+ Hits 318205 343235 +25030
- Misses 98995 245315 +146320
- Partials 19501 23310 +3809
Flags with carried forward coverage won't be shown. Click here to find out more.
|
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
|
@qw4990: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
| @@ -935,7 +935,7 @@ func TestMDLPreparePlanCacheExecute(t *testing.T) { | |||
| tk.MustQuery("select * from t2") | |||
| tk.MustExec(`set @a = 2, @b=4;`) | |||
| tk.MustExec(`execute stmt_test_1 using @a, @b;`) | |||
| tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("1")) | |||
| tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("0")) | |||
There was a problem hiding this comment.
Should we also have a separated test case for the case about preparing select ... for update statement outside transaction? This test targets on testing MDL, and I'm afraid that this test might being modified / removed without being known by us and making the case lose coverage...
There was a problem hiding this comment.
I enhancemented this test, PTAL
|
/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. |
| tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("0")) | ||
| tk.MustExec(`execute stmt_test_1 using @a, @b;`) // can't reuse the prior plan since this table becomes dirty. | ||
| tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("0")) | ||
| tk.MustExec(`execute stmt_test_1 using @a, @b;`) // can't reuse the prior plan now. |
There was a problem hiding this comment.
| tk.MustExec(`execute stmt_test_1 using @a, @b;`) // can't reuse the prior plan now. | |
| tk.MustExec(`execute stmt_test_1 using @a, @b;`) // can reuse the prior plan now. |
Did you mean this?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AilinKid, cfzjywxk, MyonKeminta, wjhuang2016 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
added
the
needs-cherry-pick-release-7.5
Signed-off-by: ti-chi-bot <[email protected]>
|
In response to a cherrypick label: new pull request created to branch |
ti-chi-bot
added
needs-cherry-pick-release-8.1
Signed-off-by: ti-chi-bot <[email protected]>
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <[email protected]>
|
In response to a cherrypick label: new pull request created to branch |
ti-chi-bot
added
the
needs-cherry-pick-release-6.5
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <[email protected]>
What problem does this PR solve?
Issue Number: close #54652
Problem Summary: planner: fix the issue of reusing wrong point-plan for "select ... for update"
What changed and how does it work?
Encode more txn state into the plan cache key, and check whether the key has changed before reusing point-get plans.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.