planner: do not push keep descending order to tiflash #13572
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13572 +/- ##
===========================================
Coverage 80.4764% 80.4764%
===========================================
Files 474 474
Lines 118457 118457
===========================================
Hits 95330 95330
Misses 15746 15746
Partials 7381 7381 |
planner/core/logical_plan_builder.go
Outdated
| @@ -428,6 +429,7 @@ func (ds *DataSource) setPreferredStoreType(hintInfo *tableHintInfo) { | |||
| } | |||
| if hintInfo.ifPreferTiFlash(alias) { | |||
| ds.preferStoreType |= preferTiFlash | |||
| ds.possibleAccessPaths = append(ds.possibleAccessPaths, &accessPath{isTablePath: true, storeType: kv.TiFlash}) | |||
There was a problem hiding this comment.
Can we do it when build possibleAccessPaths? From the function name, it should not change access paths.
There was a problem hiding this comment.
If the TiFlash replica is not available. We can't generate a flash access path. But when the hint is set. we should enforce a flash access path.
There was a problem hiding this comment.
This is a temp behavior since we choose tiflash only by hint currently?
After we can choose tiflash by default, we don't need to add it here?
You can add a TODO here.
There was a problem hiding this comment.
@winoros That's not a temp behavior. For now, CBO can judge whether to choose the TiFlash access path by tblInfo.TiFlashReplica.Available. I hope when users set the hint, the planner enforce a TiFlash path without check the TiFlash replica status.
There was a problem hiding this comment.
Hint should not enforce something that tidb cannot do, it is only a suggestion for optimizer. Therefore, if the tiflash access path is not available, optimizer should not use it even if there are hints.
There was a problem hiding this comment.
Yes, it's a normal performance. But I still want to leave a function to force accessing the TiFlash. It's easy for both PD and TiFlash to check the bug when the replica is unavailable.
There was a problem hiding this comment.
There would be 2 TiFlash paths then? since getPossibleAccessPaths may has appended one already.
There was a problem hiding this comment.
Yes, but it doesn't affect the correctness. Just make the planner a bit slower.
There was a problem hiding this comment.
I prefer to check if TiFlash path exists before adding a new one. Also, please add comment to explain that this change is for testing purpose, and add a TODO to remind reverting it finally.
There was a problem hiding this comment.
I prefer to check if TiFlash path exists before adding a new one. Also, please add comment to explain that this change is for testing purpose, and add a TODO to remind reverting it finally.
@eurekaka Done.
| } | ||
| if ds.preferStoreType&preferTiKV != 0 && path.storeType == kv.TiFlash { | ||
| continue | ||
| } |
There was a problem hiding this comment.
We may end up with no plan generated if the hint specifies TiFlash while there is no TiFlash replica available?
There was a problem hiding this comment.
For now, if there is no TiFlash replica available, we still force a TiFlash plan.
planner/core/logical_plan_builder.go
Outdated
| @@ -428,6 +429,7 @@ func (ds *DataSource) setPreferredStoreType(hintInfo *tableHintInfo) { | |||
| } | |||
| if hintInfo.ifPreferTiFlash(alias) { | |||
| ds.preferStoreType |= preferTiFlash | |||
| ds.possibleAccessPaths = append(ds.possibleAccessPaths, &accessPath{isTablePath: true, storeType: kv.TiFlash}) | |||
There was a problem hiding this comment.
There would be 2 TiFlash paths then? since getPossibleAccessPaths may has appended one already.
|
@lamxTyler, @winoros, @eurekaka, @francis0407, PTAL. |
eurekaka
added
the
status/can-merge
|
/run-all-tests |
What problem does this PR solve?
As the title says.
What is changed and how it works?
When do skyline pruning, check the properties for TiFlash access path.
Check List
Tests
Side effects