planner: fix plan cache not working caused by type difference #23175
Conversation
ti-chi-bot
added
do-not-merge/work-in-progress
ti-chi-bot
added
size/M
time-and-fate
requested review from
wshwsh12 and
eurekaka
and removed request for
a team
ti-chi-bot
removed
the
do-not-merge/work-in-progress
|
/sig planner |
|
/uncc wshwsh12 |
|
/run-all-tests |
|
/run-check_dev_2 |
|
/run-tics-test |
|
/run-unit-test |
|
/run-check_dev |
|
/run-check_dev_2 |
|
/run-all-tests |
|
/run-tics-test |
| func (s FieldSlice) Equal(tps []*types.FieldType) bool { | ||
| if len(s) != len(tps) { | ||
| return false | ||
| } | ||
| for i := range tps { | ||
| if !s[i].Equal(tps[i]) { | ||
| // We only use part of logic of `func (ft *FieldType) Equal(other *FieldType)` here because (1) only numeric and | ||
| // string types will show up here, and (2) we don't need flen and decimal to be matched exactly to use plan cache |
There was a problem hiding this comment.
IMHO, this assumption is pretty fragile. Consider this case:
MySQL [test]> create table t(a varchar(20), b int, index ia(a(4)));
Query OK, 0 rows affected (0.12 sec)
MySQL [test]> explain select * from t use index(ia) where a = 'xx';
+-------------------------------+---------+-----------+----------------------+---------------------------------------------------+
| id | estRows | task | access object | operator info |
+-------------------------------+---------+-----------+----------------------+---------------------------------------------------+
| IndexLookUp_8 | 10.00 | root | | |
| ├─IndexRangeScan_5(Build) | 10.00 | cop[tikv] | table:t, index:ia(a) | range:["xx","xx"], keep order:false, stats:pseudo |
| └─Selection_7(Probe) | 10.00 | cop[tikv] | | eq(test.t.a, "xx") |
| └─TableRowIDScan_6 | 10.00 | cop[tikv] | table:t | keep order:false, stats:pseudo |
+-------------------------------+---------+-----------+----------------------+---------------------------------------------------+
4 rows in set (0.00 sec)
If we apply an optimization in the future to check the length of the constant string and decide if we need to keep the Selection_7, then this cached plan may return wrong results if we reuse it.
For this case, we can check curr_flen <= cached_flen to avoid potential bugs, but I am not sure if there are other possibilities that our plan may be dependent on the flen / decimal of the constants.
There was a problem hiding this comment.
it's better to use another function to perform this special equal validation for plan cache.
There was a problem hiding this comment.
After discussion, we decide to leave behind the prefix index case mentioned above for now.
Because in getPhysicalPlan, we don't know exactly how will a user variable be used. It might be inside a filter expression or part of a range.
If now we want to prevent the case, we can expect that we'll need to implement something like "find if this user variable is used on some indexes in this plan", which is complicated.
It will be easier just not to cache that plan when we implemented this optimization someday.
Also, I added some comments to clarify this.
|
/run-all-tests |
|
/run-unit-test |
1 similar comment
|
/run-unit-test |
|
/run-all-tests |
|
/run-check_dev_2 |
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by writing |
ti-chi-bot
added
status/LGT2
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: 95d0556
|
ti-chi-bot
added
the
status/can-merge
|
@time-and-fate: Your PR was out of date, I have automatically updated it for you. At the same time I will also trigger all tests for you: /run-all-tests If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes. 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 ti-community-infra/tichi repository. |
What problem does this PR solve?
Issue Number: close #22952
Note: Changes in this PR assume that different Flen and Decimal of user variables won't cause a different plan.
Problem Summary:
If you set
@m = 20, @n = 200.The FieldType for m and n will be different. The flen of m will be 2 and the flen of n will be 3. Like this:
Because of the mechanism introduced in #18973, this will unnecessarily make this plan cache unusable.
Another problem is that the
SimpleLRUCache'sPut(key, value)method is only effective when it's a newkey, which means you can't usePut()to update an existing key-value.What is changed and how it works?
Only require tp, charset, and collate to be matched when verifying if a plan cache is valid.
Change (*SimpleLRUCache).Put to an override style.
Check List
Tests
Release note