Skip to content
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.

Sign up for GitHub

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

Jump to bottom

planner: do not push keep descending order to tiflash #13572

Merged
merged 9 commits into from
Dec 2, 2019
Merged

planner: do not push keep descending order to tiflash #13572

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
225c49b
do not push keep descending order to tiflash
lzmhhh123 Nov 19, 2019
e856a9b
add test
lzmhhh123 Nov 19, 2019
ede862e
Merge remote-tracking branch 'origin/master' into dev/ban_descent_tif…
lzmhhh123 Nov 20, 2019
ab8f112
address comments
lzmhhh123 Nov 27, 2019
3bbb055
fix explain test
lzmhhh123 Nov 27, 2019
eb6e4b6
Merge remote-tracking branch 'origin/master' into dev/ban_descent_tif…
lzmhhh123 Nov 27, 2019
1192433
fix explain test
lzmhhh123 Nov 27, 2019
2d56cb3
address comments
lzmhhh123 Dec 2, 2019
4fbada5
Merge branch 'master' into dev/ban_descent_tiflash
sre-bot Dec 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 27 additions & 12 deletions cmd/explaintest/r/access_tiflash.result
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,28 +8,28 @@ StreamAgg_20 1.00 root funcs:avg(Column#8, Column#9)
└─IndexScan_19 10000.00 cop[tikv] table:t, index:a, range:[NULL,+inf], keep order:false, stats:pseudo
desc select /*+ read_from_storage(tiflash[t]) */ avg(a) from t;
id count task operator info
StreamAgg_16 1.00 root funcs:avg(Column#8, Column#9)
└─TableReader_17 1.00 root data:StreamAgg_8
StreamAgg_20 1.00 root funcs:avg(Column#8, Column#9)
└─TableReader_21 1.00 root data:StreamAgg_8
└─StreamAgg_8 1.00 cop[tiflash] funcs:count(Column#1), sum(Column#1)
└─TableScan_15 10000.00 cop[tiflash] table:t, range:[-inf,+inf], keep order:false, stats:pseudo
└─TableScan_19 10000.00 cop[tiflash] table:t, range:[-inf,+inf], keep order:false, stats:pseudo
desc select /*+ read_from_storage(tiflash[t]) */ sum(a) from t;
id count task operator info
StreamAgg_16 1.00 root funcs:sum(Column#7)
└─TableReader_17 1.00 root data:StreamAgg_8
StreamAgg_20 1.00 root funcs:sum(Column#7)
└─TableReader_21 1.00 root data:StreamAgg_8
└─StreamAgg_8 1.00 cop[tiflash] funcs:sum(Column#1)
└─TableScan_15 10000.00 cop[tiflash] table:t, range:[-inf,+inf], keep order:false, stats:pseudo
└─TableScan_19 10000.00 cop[tiflash] table:t, range:[-inf,+inf], keep order:false, stats:pseudo
desc select /*+ read_from_storage(tiflash[t]) */ sum(a+1) from t;
id count task operator info
StreamAgg_16 1.00 root funcs:sum(Column#7)
└─TableReader_17 1.00 root data:StreamAgg_8
StreamAgg_20 1.00 root funcs:sum(Column#7)
└─TableReader_21 1.00 root data:StreamAgg_8
└─StreamAgg_8 1.00 cop[tiflash] funcs:sum(plus(Column#1, 1))
└─TableScan_15 10000.00 cop[tiflash] table:t, range:[-inf,+inf], keep order:false, stats:pseudo
└─TableScan_19 10000.00 cop[tiflash] table:t, range:[-inf,+inf], keep order:false, stats:pseudo
desc select /*+ read_from_storage(tiflash[t]) */ sum(isnull(a)) from t;
id count task operator info
StreamAgg_16 1.00 root funcs:sum(Column#7)
└─TableReader_17 1.00 root data:StreamAgg_8
StreamAgg_20 1.00 root funcs:sum(Column#7)
└─TableReader_21 1.00 root data:StreamAgg_8
└─StreamAgg_8 1.00 cop[tiflash] funcs:sum(isnull(Column#1))
└─TableScan_15 10000.00 cop[tiflash] table:t, range:[-inf,+inf], keep order:false, stats:pseudo
└─TableScan_19 10000.00 cop[tiflash] table:t, range:[-inf,+inf], keep order:false, stats:pseudo
create table tt(a int, b int, primary key(a));
desc select * from tt where (tt.a > 1 and tt.a < 20) or (tt.a >= 30 and tt.a < 55);
id count task operator info
Expand All @@ -40,3 +40,18 @@ id count task operator info
TableReader_7 44.00 root data:Selection_6
└─Selection_6 44.00 cop[tiflash] or(and(gt(Column#1, 1), lt(Column#1, 20)), and(ge(Column#1, 30), lt(Column#1, 55)))
└─TableScan_5 44.00 cop[tiflash] table:tt, range:[-inf,+inf], keep order:false, stats:pseudo
drop table if exists ttt;
create table ttt (a int, primary key (a desc));
desc select * from ttt order by ttt.a desc;
id count task operator info
TableReader_11 10000.00 root data:TableScan_10
└─TableScan_10 10000.00 cop[tikv] table:ttt, range:[-inf,+inf], keep order:true, desc, stats:pseudo
desc select /*+ read_from_storage(tiflash[ttt]) */ * from ttt order by ttt.a desc;
id count task operator info
Sort_4 10000.00 root Column#2:desc
└─TableReader_8 10000.00 root data:TableScan_7
└─TableScan_7 10000.00 cop[tiflash] table:ttt, range:[-inf,+inf], keep order:false, stats:pseudo
desc select /*+ read_from_storage(tiflash[ttt]) */ * from ttt order by ttt.a;
id count task operator info
TableReader_11 10000.00 root data:TableScan_10
└─TableScan_10 10000.00 cop[tiflash] table:ttt, range:[-inf,+inf], keep order:true, stats:pseudo
9 changes: 8 additions & 1 deletion cmd/explaintest/t/access_tiflash.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,11 @@ desc select /*+ read_from_storage(tiflash[t]) */ sum(isnull(a)) from t;
create table tt(a int, b int, primary key(a));

desc select * from tt where (tt.a > 1 and tt.a < 20) or (tt.a >= 30 and tt.a < 55);
desc select /*+ read_from_storage(tiflash[tt]) */ * from tt where (tt.a > 1 and tt.a < 20) or (tt.a >= 30 and tt.a < 55);
desc select /*+ read_from_storage(tiflash[tt]) */ * from tt where (tt.a > 1 and tt.a < 20) or (tt.a >= 30 and tt.a < 55);

drop table if exists ttt;
create table ttt (a int, primary key (a desc));

desc select * from ttt order by ttt.a desc;
desc select /*+ read_from_storage(tiflash[ttt]) */ * from ttt order by ttt.a desc;
desc select /*+ read_from_storage(tiflash[ttt]) */ * from ttt order by ttt.a;
23 changes: 12 additions & 11 deletions planner/core/find_best_task.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,12 @@ func compareCandidates(lhs, rhs *candidatePath) int {
func (ds *DataSource) getTableCandidate(path *accessPath, prop *property.PhysicalProperty) *candidatePath {
candidate := &candidatePath{path: path}
pkCol := ds.getPKIsHandleCol()
candidate.isMatchProp = len(prop.Items) == 1 && pkCol != nil && prop.Items[0].Col.Equal(nil, pkCol)
if len(prop.Items) == 1 && pkCol != nil {
candidate.isMatchProp = prop.Items[0].Col.Equal(nil, pkCol)
if path.storeType == kv.TiFlash {
candidate.isMatchProp = candidate.isMatchProp && !prop.Items[0].Desc
}
}
candidate.columnSet = expression.ExtractColumnSet(path.accessConds)
candidate.isSingleScan = true
return candidate
Expand Down Expand Up @@ -448,6 +453,12 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty) (t task, err
}, nil
}
if path.isTablePath {
if ds.preferStoreType&preferTiFlash != 0 && path.storeType == kv.TiKV {
continue
}
if ds.preferStoreType&preferTiKV != 0 && path.storeType == kv.TiFlash {
continue
}
Copy link
Contributor

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?

Copy link
Contributor Author

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.

tblTask, err := ds.convertToTableScan(prop, candidate)
if err != nil {
return nil, err
Expand All @@ -457,10 +468,6 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty) (t task, err
}
continue
}
// TiFlash storage do not support index scan.
lzmhhh123 marked this conversation as resolved.
Show resolved Hide resolved
if ds.preferStoreType&preferTiFlash != 0 {
continue
}
idxTask, err := ds.convertToIndexScan(prop, candidate)
if err != nil {
return nil, err
Expand Down Expand Up @@ -1046,12 +1053,6 @@ func (ds *DataSource) getOriginalPhysicalTableScan(prop *property.PhysicalProper
filterCondition: path.tableFilters,
StoreType: path.storeType,
}.Init(ds.ctx, ds.blockOffset)
if ds.preferStoreType&preferTiFlash != 0 {
ts.StoreType = kv.TiFlash
}
if ds.preferStoreType&preferTiKV != 0 {
ts.StoreType = kv.TiKV
}
if ts.StoreType == kv.TiFlash {
// Append the AccessCondition to filterCondition because TiFlash only support full range scan for each
// region, do not reset ts.Ranges as it will help prune regions during `buildCopTasks`
Expand Down
2 changes: 2 additions & 0 deletions planner/core/logical_plan_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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})
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The 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?
After we can choose tiflash by default, we don't need to add it here?
You can add a TODO here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 tblInfo.TiFlashReplica.Available. I hope when users set the hint, the planner enforce a TiFlash path without check the TiFlash replica status.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There would be 2 TiFlash paths then? since getPossibleAccessPaths may has appended one already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

@eurekaka Done.

lzmhhh123 marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down