-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
planner: do not push keep descending order to tiflash #13572
Changes from 7 commits
225c49b
e856a9b
ede862e
ab8f112
3bbb055
eb6e4b6
1192433
2d56cb3
4fbada5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,7 @@ import ( | |
"github.com/pingcap/tidb/expression" | ||
"github.com/pingcap/tidb/expression/aggregation" | ||
"github.com/pingcap/tidb/infoschema" | ||
"github.com/pingcap/tidb/kv" | ||
"github.com/pingcap/tidb/metrics" | ||
"github.com/pingcap/tidb/planner/property" | ||
"github.com/pingcap/tidb/privilege" | ||
|
@@ -436,10 +437,10 @@ func (ds *DataSource) setPreferredStoreType(hintInfo *tableHintInfo) { | |
} else { | ||
alias = &hintTableInfo{dbName: ds.DBName, tblName: ds.tableInfo.Name, selectOffset: ds.SelectBlockOffset()} | ||
} | ||
if hintInfo.ifPreferTiFlash(alias) { | ||
ds.preferStoreType |= preferTiFlash | ||
} | ||
if hintInfo.ifPreferTiKV(alias) { | ||
ds.preferStoreType |= preferTiKV | ||
} | ||
if hintInfo.ifPreferTiFlash(alias) { | ||
if ds.preferStoreType != 0 { | ||
errMsg := fmt.Sprintf("Storage hints are conflict, you can only specify one storage type of table %s.%s", | ||
alias.dbName.L, alias.tblName.L) | ||
|
@@ -448,7 +449,8 @@ func (ds *DataSource) setPreferredStoreType(hintInfo *tableHintInfo) { | |
ds.preferStoreType = 0 | ||
return | ||
} | ||
ds.preferStoreType |= preferTiKV | ||
ds.preferStoreType |= preferTiFlash | ||
ds.possibleAccessPaths = append(ds.possibleAccessPaths, &accessPath{isTablePath: true, storeType: kv.TiFlash}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we do it when build possibleAccessPaths? From the function name, it should not change access paths. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a temp behavior since we choose tiflash only by hint currently? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @winoros That's not a temp behavior. For now, CBO can judge whether to choose the TiFlash access path by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There would be 2 TiFlash paths then? since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but it doesn't affect the correctness. Just make the planner a bit slower. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@eurekaka Done. |
||
} | ||
} | ||
|
||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, if there is no TiFlash replica available, we still force a TiFlash plan.