From d9affe7c4c5a453ec076ff6fefce081f888b8886 Mon Sep 17 00:00:00 2001 From: yisaer Date: Fri, 25 Nov 2022 11:44:56 +0800 Subject: [PATCH 01/18] fix Signed-off-by: yisaer --- planner/core/find_best_task.go | 46 ++++++++++++++++++++++++++- planner/property/physical_property.go | 4 +++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/planner/core/find_best_task.go b/planner/core/find_best_task.go index 6f99593514459..57af9f9ba70d7 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -392,6 +392,13 @@ func (p *baseLogicalPlan) findBestTask(prop *property.PhysicalProperty, planCoun // prop should be read only because its cached hashcode might be not consistent // when it is changed. So we clone a new one for the temporary changes. newProp := prop.CloneEssentialFields() + if _, ok := p.self.(*LogicalLock); ok && !p.ctx.GetSessionVars().InRestrictedSQL { + newProp.InLock = true + defer func() { + newProp.InLock = false + }() + } + var plansFitsProp, plansNeedEnforce []PhysicalPlan var hintWorksWithProp bool // Maybe the plan can satisfy the required property, @@ -436,7 +443,8 @@ func (p *baseLogicalPlan) findBestTask(prop *property.PhysicalProperty, planCoun var cnt int64 var curTask task - if bestTask, cnt, err = p.enumeratePhysicalPlans4Task(plansFitsProp, newProp, false, planCounter, opt); err != nil { + bestTask, cnt, err = p.enumeratePhysicalPlans4Task(plansFitsProp, newProp, false, planCounter, opt) + if err != nil { return nil, 0, err } cntPlan += cnt @@ -912,6 +920,13 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty, planCounter p: dual, }, cntPlan, nil } + + // if the path is the point get range path with for update lock, we should forbid tiflash as it's store path. + isPointGetPath := ds.isPointGetPath(path) + if isPointGetPath && path.StoreType == kv.TiFlash && prop.InLock { + continue + } + canConvertPointGet := len(path.Ranges) > 0 && path.StoreType == kv.TiKV && ds.isPointGetConvertableSchema() if canConvertPointGet && expression.MaybeOverOptimized4PlanCache(ds.ctx, path.AccessConds) { @@ -1052,6 +1067,35 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty, planCounter return } +func (ds *DataSource) isPointGetPath(path *util.AccessPath) bool { + if len(path.Ranges) < 1 { + return false + } + if !path.IsIntHandlePath { + if !path.Index.Unique || path.Index.HasPrefixIndex() { + return false + } + idxColsLen := len(path.Index.Columns) + for _, ran := range path.Ranges { + if len(ran.LowVal) != idxColsLen { + return false + } + } + } + allRangeIsPoint := true + for _, ran := range path.Ranges { + if !ran.IsPointNonNullable(ds.ctx) { + // unique indexes can have duplicated NULL rows so we cannot use PointGet if there is NULL + allRangeIsPoint = false + break + } + } + if !allRangeIsPoint { + return false + } + return true +} + func (ds *DataSource) canConvertToPointGetForPlanCache(path *util.AccessPath) bool { // PointGet might contain some over-optimized assumptions, like `a>=1 and a<=1` --> `a=1`, but // these assumptions may be broken after parameters change. diff --git a/planner/property/physical_property.go b/planner/property/physical_property.go index 60994d05b57ef..ec4bfdad83294 100644 --- a/planner/property/physical_property.go +++ b/planner/property/physical_property.go @@ -202,6 +202,9 @@ type PhysicalProperty struct { // RejectSort means rejecting the sort property from its children, but it only works for MPP tasks. // Non-MPP tasks do not care about it. RejectSort bool + + // InLock indicates the Plan is under LogicalLock + InLock bool } // NewPhysicalProperty builds property from columns. @@ -348,6 +351,7 @@ func (p *PhysicalProperty) CloneEssentialFields() *PhysicalProperty { MPPPartitionTp: p.MPPPartitionTp, MPPPartitionCols: p.MPPPartitionCols, RejectSort: p.RejectSort, + InLock: p.InLock, } return prop } From b632349507e9745f963d25f8e78cc993d8e86930 Mon Sep 17 00:00:00 2001 From: yisaer Date: Fri, 25 Nov 2022 12:19:46 +0800 Subject: [PATCH 02/18] fix Signed-off-by: yisaer --- planner/core/find_best_task.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/planner/core/find_best_task.go b/planner/core/find_best_task.go index 57af9f9ba70d7..bbf705c4d295d 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -443,8 +443,7 @@ func (p *baseLogicalPlan) findBestTask(prop *property.PhysicalProperty, planCoun var cnt int64 var curTask task - bestTask, cnt, err = p.enumeratePhysicalPlans4Task(plansFitsProp, newProp, false, planCounter, opt) - if err != nil { + if bestTask, cnt, err = p.enumeratePhysicalPlans4Task(plansFitsProp, newProp, false, planCounter, opt); err != nil { return nil, 0, err } cntPlan += cnt From d83c23c35d48585e20131539d0d227ce5b94665d Mon Sep 17 00:00:00 2001 From: yisaer Date: Fri, 25 Nov 2022 12:53:36 +0800 Subject: [PATCH 03/18] add test Signed-off-by: yisaer --- executor/executor_test.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/executor/executor_test.go b/executor/executor_test.go index 2dedfafb79170..70aa2426f4501 100644 --- a/executor/executor_test.go +++ b/executor/executor_test.go @@ -6278,3 +6278,25 @@ func TestIssue39211(t *testing.T) { tk.MustExec("set @@tidb_enable_null_aware_anti_join=true;") tk.MustQuery("select * from t where (a,b) not in (select a, b from s);").Check(testkit.Rows()) } + +func TestPointGetWithSelectLock(t *testing.T) { + require.Nil(t, failpoint.Enable("github.com/pingcap/tidb/infoschema/mockTiFlashStoreCount", `return(true)`)) + defer func() { + err := failpoint.Disable("github.com/pingcap/tidb/infoschema/mockTiFlashStoreCount") + require.NoError(t, err) + }() + store := testkit.CreateMockStore(t) + tk := testkit.NewTestKit(t, store) + tk.MustExec("use test") + tk.MustExec("create table t(a int, b int, c int, primary key(a, b));") + tk.MustExec("insert into t values(1,2,3), (4,5,6);") + tk.MustExec("alter table t set tiflash replica 1;") + tk.MustExec("set tidb_enable_tiflash_read_for_write_stmt = on;") + tk.MustExec("set tidb_isolation_read_engines='tidb,tiflash';") + tk.MustExec("begin;") + tk.MustGetErrMsg("explain select a, b from t where a = 1 and b = 2 for update;", "[planner:1815]Internal : No access path for table 't' is found with 'tidb_isolation_read_engines' = 'tidb,tiflash', valid values can be 'tikv'.") + tk.MustExec("set tidb_isolation_read_engines='tidb,tikv,tiflash';") + tk.MustQuery("explain select a, b from t where a = 1 and b = 2 for update;") + tk.MustQuery("explain select a, b from t where a = 1 for update;") + tk.MustExec("commit") +} From 238f7a114da71ec5d05b8e59eb1239476a7c4d84 Mon Sep 17 00:00:00 2001 From: yisaer Date: Fri, 25 Nov 2022 12:56:56 +0800 Subject: [PATCH 04/18] add test Signed-off-by: yisaer --- executor/executor_test.go | 2 ++ planner/core/find_best_task.go | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/executor/executor_test.go b/executor/executor_test.go index 70aa2426f4501..4d281553e423e 100644 --- a/executor/executor_test.go +++ b/executor/executor_test.go @@ -6297,6 +6297,8 @@ func TestPointGetWithSelectLock(t *testing.T) { tk.MustGetErrMsg("explain select a, b from t where a = 1 and b = 2 for update;", "[planner:1815]Internal : No access path for table 't' is found with 'tidb_isolation_read_engines' = 'tidb,tiflash', valid values can be 'tikv'.") tk.MustExec("set tidb_isolation_read_engines='tidb,tikv,tiflash';") tk.MustQuery("explain select a, b from t where a = 1 and b = 2 for update;") + + tk.MustExec("set tidb_isolation_read_engines='tidb,tiflash';") tk.MustQuery("explain select a, b from t where a = 1 for update;") tk.MustExec("commit") } diff --git a/planner/core/find_best_task.go b/planner/core/find_best_task.go index bbf705c4d295d..3897a8969e621 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -392,7 +392,7 @@ func (p *baseLogicalPlan) findBestTask(prop *property.PhysicalProperty, planCoun // prop should be read only because its cached hashcode might be not consistent // when it is changed. So we clone a new one for the temporary changes. newProp := prop.CloneEssentialFields() - if _, ok := p.self.(*LogicalLock); ok && !p.ctx.GetSessionVars().InRestrictedSQL { + if _, ok := p.self.(*LogicalLock); ok { newProp.InLock = true defer func() { newProp.InLock = false From 117a1e648c20080ba9ca17fdf55498c262a564fd Mon Sep 17 00:00:00 2001 From: yisaer Date: Fri, 25 Nov 2022 13:32:44 +0800 Subject: [PATCH 05/18] add test Signed-off-by: yisaer --- executor/executor_test.go | 24 ------------------------ planner/core/find_best_task.go | 28 +++++++++++++++------------- planner/core/integration_test.go | 17 +++++++++++++++++ 3 files changed, 32 insertions(+), 37 deletions(-) diff --git a/executor/executor_test.go b/executor/executor_test.go index 4d281553e423e..2dedfafb79170 100644 --- a/executor/executor_test.go +++ b/executor/executor_test.go @@ -6278,27 +6278,3 @@ func TestIssue39211(t *testing.T) { tk.MustExec("set @@tidb_enable_null_aware_anti_join=true;") tk.MustQuery("select * from t where (a,b) not in (select a, b from s);").Check(testkit.Rows()) } - -func TestPointGetWithSelectLock(t *testing.T) { - require.Nil(t, failpoint.Enable("github.com/pingcap/tidb/infoschema/mockTiFlashStoreCount", `return(true)`)) - defer func() { - err := failpoint.Disable("github.com/pingcap/tidb/infoschema/mockTiFlashStoreCount") - require.NoError(t, err) - }() - store := testkit.CreateMockStore(t) - tk := testkit.NewTestKit(t, store) - tk.MustExec("use test") - tk.MustExec("create table t(a int, b int, c int, primary key(a, b));") - tk.MustExec("insert into t values(1,2,3), (4,5,6);") - tk.MustExec("alter table t set tiflash replica 1;") - tk.MustExec("set tidb_enable_tiflash_read_for_write_stmt = on;") - tk.MustExec("set tidb_isolation_read_engines='tidb,tiflash';") - tk.MustExec("begin;") - tk.MustGetErrMsg("explain select a, b from t where a = 1 and b = 2 for update;", "[planner:1815]Internal : No access path for table 't' is found with 'tidb_isolation_read_engines' = 'tidb,tiflash', valid values can be 'tikv'.") - tk.MustExec("set tidb_isolation_read_engines='tidb,tikv,tiflash';") - tk.MustQuery("explain select a, b from t where a = 1 and b = 2 for update;") - - tk.MustExec("set tidb_isolation_read_engines='tidb,tiflash';") - tk.MustQuery("explain select a, b from t where a = 1 for update;") - tk.MustExec("commit") -} diff --git a/planner/core/find_best_task.go b/planner/core/find_best_task.go index 3897a8969e621..8703bc79dd9f3 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -920,11 +920,12 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty, planCounter }, cntPlan, nil } - // if the path is the point get range path with for update lock, we should forbid tiflash as it's store path. - isPointGetPath := ds.isPointGetPath(path) - if isPointGetPath && path.StoreType == kv.TiFlash && prop.InLock { - continue - } + //// if the path is the point get range path with for update lock, we should forbid tiflash as it's store path. + //if path.StoreType == kv.TiFlash { + // if ds.isPointGetPath(path) && prop.InLock { + // continue + // } + //} canConvertPointGet := len(path.Ranges) > 0 && path.StoreType == kv.TiKV && ds.isPointGetConvertableSchema() @@ -1070,16 +1071,17 @@ func (ds *DataSource) isPointGetPath(path *util.AccessPath) bool { if len(path.Ranges) < 1 { return false } - if !path.IsIntHandlePath { - if !path.Index.Unique || path.Index.HasPrefixIndex() { + if path.Index.HasPrefixIndex() { + return false + } + if !path.IsIntHandlePath && !path.Index.Unique { + return false + } + idxColsLen := len(path.Index.Columns) + for _, ran := range path.Ranges { + if len(ran.LowVal) != idxColsLen { return false } - idxColsLen := len(path.Index.Columns) - for _, ran := range path.Ranges { - if len(ran.LowVal) != idxColsLen { - return false - } - } } allRangeIsPoint := true for _, ran := range path.Ranges { diff --git a/planner/core/integration_test.go b/planner/core/integration_test.go index db60c99fdf4c5..c011930700d00 100644 --- a/planner/core/integration_test.go +++ b/planner/core/integration_test.go @@ -7554,6 +7554,23 @@ func TestEnableTiFlashReadForWriteStmt(t *testing.T) { checkMpp(rs) } +func TestPointGetWithSelectLock(t *testing.T) { + store := testkit.CreateMockStore(t) + tk := testkit.NewTestKit(t, store) + tk.MustExec("use test") + tk.MustExec("create table t(a int, b int, c int, primary key(a, b));") + tk.MustExec("insert into t values(1,2,3), (4,5,6);") + tk.MustExec("alter table t set tiflash replica 1;") + tk.MustExec("set @@tidb_enable_tiflash_read_for_write_stmt = on;") + tk.MustExec("set @@tidb_isolation_read_engines='tidb,tiflash';") + tk.MustExec("begin;") + tk.MustGetErrMsg("explain select a, b from t where a = 1 and b = 2 for update;", "ERROR 1815 (HY000): Internal : Can't find a proper physical plan for this query") + tk.MustQuery("explain select a, b from t where a = 1 for update;") + tk.MustExec("set tidb_isolation_read_engines='tidb,tikv,tiflash';") + tk.MustQuery("explain select a, b from t where a = 1 and b = 2 for update;") + tk.MustExec("commit") +} + func TestTableRangeFallback(t *testing.T) { store := testkit.CreateMockStore(t) tk := testkit.NewTestKit(t, store) From f1e48da6f558e748d13f530407a14fd653a2b0fb Mon Sep 17 00:00:00 2001 From: yisaer Date: Fri, 25 Nov 2022 13:39:05 +0800 Subject: [PATCH 06/18] add test Signed-off-by: yisaer --- planner/core/find_best_task.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/planner/core/find_best_task.go b/planner/core/find_best_task.go index 8703bc79dd9f3..9cf2a5cf82ace 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -920,12 +920,12 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty, planCounter }, cntPlan, nil } - //// if the path is the point get range path with for update lock, we should forbid tiflash as it's store path. - //if path.StoreType == kv.TiFlash { - // if ds.isPointGetPath(path) && prop.InLock { - // continue - // } - //} + // if the path is the point get range path with for update lock, we should forbid tiflash as it's store path. + if path.StoreType == kv.TiFlash { + if ds.isPointGetPath(path) && prop.InLock { + continue + } + } canConvertPointGet := len(path.Ranges) > 0 && path.StoreType == kv.TiKV && ds.isPointGetConvertableSchema() From c7b6250c4723b566990110909a637282a9f74418 Mon Sep 17 00:00:00 2001 From: yisaer Date: Fri, 25 Nov 2022 14:58:25 +0800 Subject: [PATCH 07/18] fix Signed-off-by: yisaer --- planner/core/find_best_task.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/planner/core/find_best_task.go b/planner/core/find_best_task.go index 9cf2a5cf82ace..39b94594fb906 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -1091,10 +1091,7 @@ func (ds *DataSource) isPointGetPath(path *util.AccessPath) bool { break } } - if !allRangeIsPoint { - return false - } - return true + return allRangeIsPoint } func (ds *DataSource) canConvertToPointGetForPlanCache(path *util.AccessPath) bool { From b8e571253d43365500d49da611b46a9f2e51fe66 Mon Sep 17 00:00:00 2001 From: yisaer Date: Mon, 28 Nov 2022 11:35:29 +0800 Subject: [PATCH 08/18] address the comment Signed-off-by: yisaer address the comment Signed-off-by: yisaer --- planner/core/find_best_task.go | 11 ++--------- planner/property/physical_property.go | 4 ---- 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/planner/core/find_best_task.go b/planner/core/find_best_task.go index 39b94594fb906..9b938cc743845 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -392,13 +392,6 @@ func (p *baseLogicalPlan) findBestTask(prop *property.PhysicalProperty, planCoun // prop should be read only because its cached hashcode might be not consistent // when it is changed. So we clone a new one for the temporary changes. newProp := prop.CloneEssentialFields() - if _, ok := p.self.(*LogicalLock); ok { - newProp.InLock = true - defer func() { - newProp.InLock = false - }() - } - var plansFitsProp, plansNeedEnforce []PhysicalPlan var hintWorksWithProp bool // Maybe the plan can satisfy the required property, @@ -921,8 +914,8 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty, planCounter } // if the path is the point get range path with for update lock, we should forbid tiflash as it's store path. - if path.StoreType == kv.TiFlash { - if ds.isPointGetPath(path) && prop.InLock { + if path.StoreType == kv.TiFlash && ds.isPointGetPath(path) { + if _, ok := ds.ctx.GetSessionVars().StmtCtx.LockTableIDs[ds.table.Meta().ID]; ok { continue } } diff --git a/planner/property/physical_property.go b/planner/property/physical_property.go index ec4bfdad83294..60994d05b57ef 100644 --- a/planner/property/physical_property.go +++ b/planner/property/physical_property.go @@ -202,9 +202,6 @@ type PhysicalProperty struct { // RejectSort means rejecting the sort property from its children, but it only works for MPP tasks. // Non-MPP tasks do not care about it. RejectSort bool - - // InLock indicates the Plan is under LogicalLock - InLock bool } // NewPhysicalProperty builds property from columns. @@ -351,7 +348,6 @@ func (p *PhysicalProperty) CloneEssentialFields() *PhysicalProperty { MPPPartitionTp: p.MPPPartitionTp, MPPPartitionCols: p.MPPPartitionCols, RejectSort: p.RejectSort, - InLock: p.InLock, } return prop } From 89f9eaa5387bdedbef284c6d610bc32a01c75a78 Mon Sep 17 00:00:00 2001 From: yisaer Date: Mon, 28 Nov 2022 13:03:24 +0800 Subject: [PATCH 09/18] fix test Signed-off-by: yisaer --- planner/core/find_best_task.go | 6 ++---- planner/core/integration_test.go | 9 ++++++--- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/planner/core/find_best_task.go b/planner/core/find_best_task.go index 9b938cc743845..04d986719bd60 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -914,10 +914,8 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty, planCounter } // if the path is the point get range path with for update lock, we should forbid tiflash as it's store path. - if path.StoreType == kv.TiFlash && ds.isPointGetPath(path) { - if _, ok := ds.ctx.GetSessionVars().StmtCtx.LockTableIDs[ds.table.Meta().ID]; ok { - continue - } + if path.StoreType == kv.TiFlash && ds.isPointGetPath(path) && ds.isForUpdateRead { + continue } canConvertPointGet := len(path.Ranges) > 0 && path.StoreType == kv.TiKV && ds.isPointGetConvertableSchema() diff --git a/planner/core/integration_test.go b/planner/core/integration_test.go index c011930700d00..7f54f4ed549c8 100644 --- a/planner/core/integration_test.go +++ b/planner/core/integration_test.go @@ -7555,16 +7555,19 @@ func TestEnableTiFlashReadForWriteStmt(t *testing.T) { } func TestPointGetWithSelectLock(t *testing.T) { - store := testkit.CreateMockStore(t) + store, dom := testkit.CreateMockStoreAndDomain(t) tk := testkit.NewTestKit(t, store) tk.MustExec("use test") tk.MustExec("create table t(a int, b int, c int, primary key(a, b));") tk.MustExec("insert into t values(1,2,3), (4,5,6);") - tk.MustExec("alter table t set tiflash replica 1;") + tbl, err := dom.InfoSchema().TableByName(model.CIStr{O: "test", L: "test"}, model.CIStr{O: "t", L: "t"}) + require.NoError(t, err) + // Set the hacked TiFlash replica for explain tests. + tbl.Meta().TiFlashReplica = &model.TiFlashReplicaInfo{Count: 1, Available: true} tk.MustExec("set @@tidb_enable_tiflash_read_for_write_stmt = on;") tk.MustExec("set @@tidb_isolation_read_engines='tidb,tiflash';") tk.MustExec("begin;") - tk.MustGetErrMsg("explain select a, b from t where a = 1 and b = 2 for update;", "ERROR 1815 (HY000): Internal : Can't find a proper physical plan for this query") + tk.MustGetErrMsg("explain select a, b from t where a = 1 and b = 2 for update;", "[planner:1815]Internal : Can't find a proper physical plan for this query") tk.MustQuery("explain select a, b from t where a = 1 for update;") tk.MustExec("set tidb_isolation_read_engines='tidb,tikv,tiflash';") tk.MustQuery("explain select a, b from t where a = 1 and b = 2 for update;") From 4af70e7af7812ad31a1f2a925e78a3b8f5116cbf Mon Sep 17 00:00:00 2001 From: yisaer Date: Mon, 28 Nov 2022 13:33:41 +0800 Subject: [PATCH 10/18] fix test Signed-off-by: yisaer --- planner/core/find_best_task.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/planner/core/find_best_task.go b/planner/core/find_best_task.go index 04d986719bd60..ed21618fbdd94 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -1062,11 +1062,14 @@ func (ds *DataSource) isPointGetPath(path *util.AccessPath) bool { if len(path.Ranges) < 1 { return false } - if path.Index.HasPrefixIndex() { - return false - } - if !path.IsIntHandlePath && !path.Index.Unique { - return false + if path.Index != nil { + if path.Index.HasPrefixIndex() || !path.Index.Unique { + return false + } + } else { + if !path.IsIntHandlePath { + return false + } } idxColsLen := len(path.Index.Columns) for _, ran := range path.Ranges { @@ -1077,7 +1080,6 @@ func (ds *DataSource) isPointGetPath(path *util.AccessPath) bool { allRangeIsPoint := true for _, ran := range path.Ranges { if !ran.IsPointNonNullable(ds.ctx) { - // unique indexes can have duplicated NULL rows so we cannot use PointGet if there is NULL allRangeIsPoint = false break } From d5aaf6fb5629d550eb3c83b92ac6968ac7725d9b Mon Sep 17 00:00:00 2001 From: yisaer Date: Mon, 28 Nov 2022 14:34:03 +0800 Subject: [PATCH 11/18] fix Signed-off-by: yisaer --- planner/core/find_best_task.go | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/planner/core/find_best_task.go b/planner/core/find_best_task.go index ed21618fbdd94..83fcf79c53699 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -1062,29 +1062,27 @@ func (ds *DataSource) isPointGetPath(path *util.AccessPath) bool { if len(path.Ranges) < 1 { return false } - if path.Index != nil { - if path.Index.HasPrefixIndex() || !path.Index.Unique { - return false - } - } else { - if !path.IsIntHandlePath { + for _, ran := range path.Ranges { + if !ran.IsPointNonNullable(ds.ctx) { return false } } + if path.IsIntHandlePath { + return true + } + if path.Index == nil { + return false + } + if path.Index.HasPrefixIndex() || !path.Index.Unique { + return false + } idxColsLen := len(path.Index.Columns) for _, ran := range path.Ranges { if len(ran.LowVal) != idxColsLen { return false } } - allRangeIsPoint := true - for _, ran := range path.Ranges { - if !ran.IsPointNonNullable(ds.ctx) { - allRangeIsPoint = false - break - } - } - return allRangeIsPoint + return true } func (ds *DataSource) canConvertToPointGetForPlanCache(path *util.AccessPath) bool { From fcc9b0cff2c5a61cb5f2dc2199aec053c7ec371d Mon Sep 17 00:00:00 2001 From: yisaer Date: Tue, 29 Nov 2022 13:19:49 +0800 Subject: [PATCH 12/18] add unique testcase --- planner/core/find_best_task.go | 89 +++++++++++++++++++++----------- planner/core/integration_test.go | 12 ++++- 2 files changed, 70 insertions(+), 31 deletions(-) diff --git a/planner/core/find_best_task.go b/planner/core/find_best_task.go index 7046532b9c0dd..896077af2445a 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -914,8 +914,10 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty, planCounter } // if the path is the point get range path with for update lock, we should forbid tiflash as it's store path. - if path.StoreType == kv.TiFlash && ds.isPointGetPath(path) && ds.isForUpdateRead { - continue + if path.StoreType == kv.TiFlash && ds.isForUpdateRead { + if ds.isPointGetConditions() { + continue + } } canConvertPointGet := len(path.Ranges) > 0 && path.StoreType == kv.TiKV && ds.isPointGetConvertableSchema() @@ -1058,33 +1060,6 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty, planCounter return } -func (ds *DataSource) isPointGetPath(path *util.AccessPath) bool { - if len(path.Ranges) < 1 { - return false - } - for _, ran := range path.Ranges { - if !ran.IsPointNonNullable(ds.ctx) { - return false - } - } - if path.IsIntHandlePath { - return true - } - if path.Index == nil { - return false - } - if path.Index.HasPrefixIndex() || !path.Index.Unique { - return false - } - idxColsLen := len(path.Index.Columns) - for _, ran := range path.Ranges { - if len(ran.LowVal) != idxColsLen { - return false - } - } - return true -} - func (ds *DataSource) canConvertToPointGetForPlanCache(path *util.AccessPath) bool { // PointGet might contain some over-optimized assumptions, like `a>=1 and a<=1` --> `a=1`, but // these assumptions may be broken after parameters change. @@ -1936,6 +1911,62 @@ func (s *LogicalIndexScan) GetPhysicalIndexScan(_ *expression.Schema, stats *pro return is } +func (ds *DataSource) isPointGetConditions() bool { + t, _ := ds.is.TableByID(ds.physicalTableID) + columns := map[string]struct{}{} + for _, cond := range ds.allConds { + s, ok := cond.(*expression.ScalarFunction) + if !ok { + return false + } + if s.FuncName.L != ast.EQ || (s.FuncName.L == ast.In && len(s.GetArgs()) != 2) { + return false + } + arg0 := s.GetArgs()[0] + arg1 := s.GetArgs()[1] + _, ok1 := arg0.(*expression.Constant) + col, ok2 := arg1.(*expression.Column) + if ok1 && ok2 { + columns[t.Meta().FindColumnNameByID(col.ID)] = struct{}{} + continue + } + col, ok1 = arg0.(*expression.Column) + _, ok2 = arg1.(*expression.Constant) + if ok1 && ok2 { + columns[t.Meta().FindColumnNameByID(col.ID)] = struct{}{} + continue + } + } + return ds.findPKOrUniqueIndexMatchColumns(columns) +} + +func (ds *DataSource) findPKOrUniqueIndexMatchColumns(columns map[string]struct{}) bool { + for _, idx := range ds.tableInfo.Indices { + if !idx.Unique && !idx.Primary { + continue + } + if idx.HasPrefixIndex() { + continue + } + if len(idx.Columns) != len(columns) { + continue + } + flag := true + for _, idxCol := range idx.Columns { + _, ok := columns[idxCol.Name.String()] + if !ok { + flag = false + break + } + } + if !flag { + continue + } + return true + } + return false +} + // convertToTableScan converts the DataSource to table scan. func (ds *DataSource) convertToTableScan(prop *property.PhysicalProperty, candidate *candidatePath, _ *physicalOptimizeOp) (task task, err error) { // It will be handled in convertToIndexScan. diff --git a/planner/core/integration_test.go b/planner/core/integration_test.go index 7f54f4ed549c8..46b12cd37a1a3 100644 --- a/planner/core/integration_test.go +++ b/planner/core/integration_test.go @@ -7558,19 +7558,27 @@ func TestPointGetWithSelectLock(t *testing.T) { store, dom := testkit.CreateMockStoreAndDomain(t) tk := testkit.NewTestKit(t, store) tk.MustExec("use test") - tk.MustExec("create table t(a int, b int, c int, primary key(a, b));") - tk.MustExec("insert into t values(1,2,3), (4,5,6);") + tk.MustExec("create table t(a int, b int, primary key(a, b));") + tk.MustExec("create table t1(c int unique, d int);") tbl, err := dom.InfoSchema().TableByName(model.CIStr{O: "test", L: "test"}, model.CIStr{O: "t", L: "t"}) require.NoError(t, err) // Set the hacked TiFlash replica for explain tests. tbl.Meta().TiFlashReplica = &model.TiFlashReplicaInfo{Count: 1, Available: true} + tbl1, err := dom.InfoSchema().TableByName(model.CIStr{O: "test", L: "test"}, model.CIStr{O: "t1", L: "t1"}) + require.NoError(t, err) + // Set the hacked TiFlash replica for explain tests. + tbl1.Meta().TiFlashReplica = &model.TiFlashReplicaInfo{Count: 1, Available: true} + tk.MustExec("set @@tidb_enable_tiflash_read_for_write_stmt = on;") tk.MustExec("set @@tidb_isolation_read_engines='tidb,tiflash';") tk.MustExec("begin;") tk.MustGetErrMsg("explain select a, b from t where a = 1 and b = 2 for update;", "[planner:1815]Internal : Can't find a proper physical plan for this query") + tk.MustGetErrMsg("explain select c, d from t1 where c = 1 for update;", "[planner:1815]Internal : Can't find a proper physical plan for this query") tk.MustQuery("explain select a, b from t where a = 1 for update;") + tk.MustQuery("explain select c, d from t1 where c = 1 and d = 1 for update;") tk.MustExec("set tidb_isolation_read_engines='tidb,tikv,tiflash';") tk.MustQuery("explain select a, b from t where a = 1 and b = 2 for update;") + tk.MustQuery("explain select c, d from t1 where c = 1 for update;") tk.MustExec("commit") } From 285950b769e8cb46fab7eed81029a26fbaf400aa Mon Sep 17 00:00:00 2001 From: yisaer Date: Tue, 29 Nov 2022 13:32:12 +0800 Subject: [PATCH 13/18] add comment --- planner/core/find_best_task.go | 6 +++++- planner/core/integration_test.go | 3 ++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/planner/core/find_best_task.go b/planner/core/find_best_task.go index 896077af2445a..a6d8b4b68aa8a 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -1911,6 +1911,10 @@ func (s *LogicalIndexScan) GetPhysicalIndexScan(_ *expression.Schema, stats *pro return is } +// isPointGetConditions indicates whether the conditions are point-get-able. +// eg: create table t(a int, b int,c int unique, primary (a,b)) +// select * from t where a = 1 and b = 1 and c =1; +// the datasource can access by primary key(a,b) or unique key (c) which are both point-get-able func (ds *DataSource) isPointGetConditions() bool { t, _ := ds.is.TableByID(ds.physicalTableID) columns := map[string]struct{}{} @@ -1948,7 +1952,7 @@ func (ds *DataSource) findPKOrUniqueIndexMatchColumns(columns map[string]struct{ if idx.HasPrefixIndex() { continue } - if len(idx.Columns) != len(columns) { + if len(idx.Columns) > len(columns) { continue } flag := true diff --git a/planner/core/integration_test.go b/planner/core/integration_test.go index 46b12cd37a1a3..755e2c5b3ca92 100644 --- a/planner/core/integration_test.go +++ b/planner/core/integration_test.go @@ -7574,8 +7574,9 @@ func TestPointGetWithSelectLock(t *testing.T) { tk.MustExec("begin;") tk.MustGetErrMsg("explain select a, b from t where a = 1 and b = 2 for update;", "[planner:1815]Internal : Can't find a proper physical plan for this query") tk.MustGetErrMsg("explain select c, d from t1 where c = 1 for update;", "[planner:1815]Internal : Can't find a proper physical plan for this query") + tk.MustGetErrMsg("explain select c, d from t1 where c = 1 and d = 1 for update;", "[planner:1815]Internal : Can't find a proper physical plan for this query") tk.MustQuery("explain select a, b from t where a = 1 for update;") - tk.MustQuery("explain select c, d from t1 where c = 1 and d = 1 for update;") + tk.MustQuery("explain select c, d from t1 where c > 1 for update;") tk.MustExec("set tidb_isolation_read_engines='tidb,tikv,tiflash';") tk.MustQuery("explain select a, b from t where a = 1 and b = 2 for update;") tk.MustQuery("explain select c, d from t1 where c = 1 for update;") From 941cbe33f87ec92c6edfe285ecb97886834c9ecf Mon Sep 17 00:00:00 2001 From: yisaer Date: Tue, 29 Nov 2022 15:58:47 +0800 Subject: [PATCH 14/18] address the comment --- planner/core/find_best_task.go | 2 +- planner/core/integration_test.go | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/planner/core/find_best_task.go b/planner/core/find_best_task.go index a6d8b4b68aa8a..2710c9df29b43 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -914,7 +914,7 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty, planCounter } // if the path is the point get range path with for update lock, we should forbid tiflash as it's store path. - if path.StoreType == kv.TiFlash && ds.isForUpdateRead { + if path.StoreType == kv.TiFlash && ds.isForUpdateRead && ds.ctx.GetSessionVars().InTxn() { if ds.isPointGetConditions() { continue } diff --git a/planner/core/integration_test.go b/planner/core/integration_test.go index 755e2c5b3ca92..ff2ec6b6fd631 100644 --- a/planner/core/integration_test.go +++ b/planner/core/integration_test.go @@ -7572,6 +7572,7 @@ func TestPointGetWithSelectLock(t *testing.T) { tk.MustExec("set @@tidb_enable_tiflash_read_for_write_stmt = on;") tk.MustExec("set @@tidb_isolation_read_engines='tidb,tiflash';") tk.MustExec("begin;") + // assert point get condition with txn commit and tiflash store tk.MustGetErrMsg("explain select a, b from t where a = 1 and b = 2 for update;", "[planner:1815]Internal : Can't find a proper physical plan for this query") tk.MustGetErrMsg("explain select c, d from t1 where c = 1 for update;", "[planner:1815]Internal : Can't find a proper physical plan for this query") tk.MustGetErrMsg("explain select c, d from t1 where c = 1 and d = 1 for update;", "[planner:1815]Internal : Can't find a proper physical plan for this query") @@ -7581,6 +7582,10 @@ func TestPointGetWithSelectLock(t *testing.T) { tk.MustQuery("explain select a, b from t where a = 1 and b = 2 for update;") tk.MustQuery("explain select c, d from t1 where c = 1 for update;") tk.MustExec("commit") + tk.MustExec("set tidb_isolation_read_engines='tidb,tiflash';") + // assert point get condition with auto commit and tiflash store + tk.MustQuery("explain select a, b from t where a = 1 and b = 2 for update;") + tk.MustQuery("explain select c, d from t1 where c = 1 for update;") } func TestTableRangeFallback(t *testing.T) { From 13ef191f579a12ba437053d3a10653f8b6179895 Mon Sep 17 00:00:00 2001 From: yisaer Date: Tue, 29 Nov 2022 16:07:18 +0800 Subject: [PATCH 15/18] address the comment --- planner/core/find_best_task.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/planner/core/find_best_task.go b/planner/core/find_best_task.go index 2710c9df29b43..218f84c39713b 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -914,7 +914,7 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty, planCounter } // if the path is the point get range path with for update lock, we should forbid tiflash as it's store path. - if path.StoreType == kv.TiFlash && ds.isForUpdateRead && ds.ctx.GetSessionVars().InTxn() { + if path.StoreType == kv.TiFlash && ds.isForUpdateRead && ds.ctx.GetSessionVars().InTxn() && !ds.ctx.GetSessionVars().IsAutocommit() { if ds.isPointGetConditions() { continue } From 05e5d8231ad34a1d61b22abb03f60e27d3335216 Mon Sep 17 00:00:00 2001 From: yisaer Date: Tue, 29 Nov 2022 16:58:06 +0800 Subject: [PATCH 16/18] address the comment --- planner/core/find_best_task.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/planner/core/find_best_task.go b/planner/core/find_best_task.go index 218f84c39713b..5d90ff851be76 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -914,7 +914,7 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty, planCounter } // if the path is the point get range path with for update lock, we should forbid tiflash as it's store path. - if path.StoreType == kv.TiFlash && ds.isForUpdateRead && ds.ctx.GetSessionVars().InTxn() && !ds.ctx.GetSessionVars().IsAutocommit() { + if path.StoreType == kv.TiFlash && ds.isForUpdateRead && ds.ctx.GetSessionVars().TxnCtx.IsExplicit { if ds.isPointGetConditions() { continue } From 38e1593c203a7d14e062efd20e7fc35ecab42f07 Mon Sep 17 00:00:00 2001 From: yisaer Date: Tue, 29 Nov 2022 18:13:41 +0800 Subject: [PATCH 17/18] address the comment --- planner/core/find_best_task.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/planner/core/find_best_task.go b/planner/core/find_best_task.go index 5d90ff851be76..903e2f1a0b66c 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -914,7 +914,7 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty, planCounter } // if the path is the point get range path with for update lock, we should forbid tiflash as it's store path. - if path.StoreType == kv.TiFlash && ds.isForUpdateRead && ds.ctx.GetSessionVars().TxnCtx.IsExplicit { + if path.StoreType == kv.TiFlash && ds.isForUpdateRead && !ds.ctx.GetSessionVars().IsAutocommit() { if ds.isPointGetConditions() { continue } From 2cc606b8a589c4fc4e92a2b316fcafb2f73b5913 Mon Sep 17 00:00:00 2001 From: yisaer Date: Tue, 29 Nov 2022 22:19:03 +0800 Subject: [PATCH 18/18] address the comment --- planner/core/find_best_task.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/planner/core/find_best_task.go b/planner/core/find_best_task.go index 903e2f1a0b66c..5d90ff851be76 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -914,7 +914,7 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty, planCounter } // if the path is the point get range path with for update lock, we should forbid tiflash as it's store path. - if path.StoreType == kv.TiFlash && ds.isForUpdateRead && !ds.ctx.GetSessionVars().IsAutocommit() { + if path.StoreType == kv.TiFlash && ds.isForUpdateRead && ds.ctx.GetSessionVars().TxnCtx.IsExplicit { if ds.isPointGetConditions() { continue }