From 3c216c7194c6728092ad5b38ab7f4440607eb130 Mon Sep 17 00:00:00 2001 From: yisaer Date: Thu, 1 Dec 2022 19:08:50 +0800 Subject: [PATCH 1/6] fix --- planner/core/find_best_task.go | 66 +++++++++----------------------- planner/core/integration_test.go | 13 ++----- 2 files changed, 21 insertions(+), 58 deletions(-) diff --git a/planner/core/find_best_task.go b/planner/core/find_best_task.go index 12f298e6cd2ba..95f83f7994f4e 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -916,7 +916,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 ds.isPointGetConditions() { + if ds.isPointGetPath(path) { continue } } @@ -1915,61 +1915,31 @@ func (s *LogicalIndexScan) GetPhysicalIndexScan(_ *expression.Schema, stats *pro // 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{}{} - for _, cond := range ds.allConds { - s, ok := cond.(*expression.ScalarFunction) - if !ok { +// the datasource can access by primary key(a,b) or unique key c which are both point-get-able +func (ds *DataSource) isPointGetPath(path *util.AccessPath) bool { + if len(path.Ranges) < 1 { + return false + } + if !path.IsIntHandlePath { + if path.Index == nil { return false } - if s.FuncName.L != ast.EQ || (s.FuncName.L == ast.In && len(s.GetArgs()) != 2) { + if !path.Index.Unique || path.Index.HasPrefixIndex() { 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 + idxColsLen := len(path.Index.Columns) + for _, ran := range path.Ranges { + if len(ran.LowVal) != idxColsLen { + return false } } - if !flag { - continue + } + for _, ran := range path.Ranges { + if !ran.IsPointNonNullable(ds.ctx) { + return false } - return true } - return false + return true } // convertToTableScan converts the DataSource to table scan. diff --git a/planner/core/integration_test.go b/planner/core/integration_test.go index ce4b299cdf328..9b1c8b7934a43 100644 --- a/planner/core/integration_test.go +++ b/planner/core/integration_test.go @@ -7628,33 +7628,26 @@ func TestPointGetWithSelectLock(t *testing.T) { tk := testkit.NewTestKit(t, store) tk.MustExec("use test") 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;") // assert point get condition with txn commit and tiflash store + tk.MustGetErrMsg("explain select a, b from t where (a = 1 and b = 2) or (a =2 and b = 1) for update;", "[planner:1815]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.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.MustGetErrMsg("explain select a,b from t where a = 1 and (b = 1 or 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.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;") 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;") + tk.MustQuery("explain select a, b from t where (a = 1 and b = 2) or (a =2 and b = 1) for update;") } func TestTableRangeFallback(t *testing.T) { From bf2eaf0a7068ea0d3e72895d826f3d20ef65d2e9 Mon Sep 17 00:00:00 2001 From: yisaer Date: Thu, 1 Dec 2022 19:59:50 +0800 Subject: [PATCH 2/6] fix --- planner/core/find_best_task.go | 32 +++++++++++++++++------ planner/core/integration_test.go | 39 ++++++++++++++++++++-------- planner/core/logical_plan_builder.go | 10 ++++--- 3 files changed, 58 insertions(+), 23 deletions(-) diff --git a/planner/core/find_best_task.go b/planner/core/find_best_task.go index 95f83f7994f4e..35247700ce32d 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -806,7 +806,30 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty, planCounter planCounter.Dec(1) return nil, 1, nil } - + if ds.isForUpdateRead && ds.ctx.GetSessionVars().TxnCtx.IsExplicit { + hasPointGetPath := false + for _, path := range ds.possibleAccessPaths { + if ds.isPointGetPath(path) { + hasPointGetPath = true + break + } + } + tblName := ds.tableInfo.Name + ds.possibleAccessPaths, err = filterPathByIsolationRead(ds.ctx, ds.possibleAccessPaths, tblName, ds.DBName) + if err != nil { + return nil, 1, err + } + if hasPointGetPath { + newPaths := make([]*util.AccessPath, 0) + for _, path := range ds.possibleAccessPaths { + // 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 { + newPaths = append(newPaths, path) + } + } + ds.possibleAccessPaths = newPaths + } + } t = ds.getTask(prop) if t != nil { cntPlan = 1 @@ -914,13 +937,6 @@ 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 && ds.isForUpdateRead && ds.ctx.GetSessionVars().TxnCtx.IsExplicit { - if ds.isPointGetPath(path) { - continue - } - } - canConvertPointGet := len(path.Ranges) > 0 && path.StoreType == kv.TiKV && ds.isPointGetConvertableSchema() if canConvertPointGet && expression.MaybeOverOptimized4PlanCache(ds.ctx, path.AccessConds) { diff --git a/planner/core/integration_test.go b/planner/core/integration_test.go index 9b1c8b7934a43..ab2cbc49310c3 100644 --- a/planner/core/integration_test.go +++ b/planner/core/integration_test.go @@ -7628,26 +7628,43 @@ func TestPointGetWithSelectLock(t *testing.T) { tk := testkit.NewTestKit(t, store) tk.MustExec("use test") 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} + sqls := []string{ + "explain select a, b from t where (a = 1 and b = 2) or (a =2 and b = 1) for update;", + "explain select a, b from t where a = 1 and b = 2 for update;", + "explain select c, d from t1 where c = 1 for update;", + "explain select c, d from t1 where c = 1 and d = 1 for update;", + "explain select c, d from t1 where c = 1 and d = 1 for update;", + "explain select c, d from t1 where c in (1,2,3,4) for update;", + } 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) or (a =2 and b = 1) for update;", "[planner:1815]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.MustGetErrMsg("explain select a,b from t where a = 1 and (b = 1 or 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;") + // // assert point get / batch point get can't work with tiflash in interaction txn + for _, sql := range sqls { + err = tk.ExecToErr(sql) + require.Error(t, err) + } + // assert point get / batch point get can work with tikv in interaction txn + tk.MustExec("set @@tidb_isolation_read_engines='tidb,tikv,tiflash';") + for _, sql := range sqls { + tk.MustQuery(sql) + } 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 a, b from t where (a = 1 and b = 2) or (a =2 and b = 1) for update;") + // assert point get / batch point get can work with tiflash in auto commit + tk.MustExec("set @@tidb_isolation_read_engines='tidb,tiflash';") + for _, sql := range sqls { + tk.MustQuery(sql) + } } func TestTableRangeFallback(t *testing.T) { diff --git a/planner/core/logical_plan_builder.go b/planner/core/logical_plan_builder.go index ad28e951ccdba..c4312f8851f57 100644 --- a/planner/core/logical_plan_builder.go +++ b/planner/core/logical_plan_builder.go @@ -4495,10 +4495,12 @@ func (b *PlanBuilder) buildDataSource(ctx context.Context, tn *ast.TableName, as } // Skip storage engine check for CreateView. - if b.capFlag&canExpandAST == 0 { - possiblePaths, err = filterPathByIsolationRead(b.ctx, possiblePaths, tblName, dbName) - if err != nil { - return nil, err + if !(b.isForUpdateRead && b.ctx.GetSessionVars().TxnCtx.IsExplicit) { + if b.capFlag&canExpandAST == 0 { + possiblePaths, err = filterPathByIsolationRead(b.ctx, possiblePaths, tblName, dbName) + if err != nil { + return nil, err + } } } From 86acbec20ce4d7ea5b7ca603be4f62e2c354c491 Mon Sep 17 00:00:00 2001 From: yisaer Date: Thu, 1 Dec 2022 20:01:07 +0800 Subject: [PATCH 3/6] fix --- planner/core/integration_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/planner/core/integration_test.go b/planner/core/integration_test.go index ab2cbc49310c3..11bfc3e9deb94 100644 --- a/planner/core/integration_test.go +++ b/planner/core/integration_test.go @@ -7649,7 +7649,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 / batch point get can't work with tiflash in interaction txn + // assert point get / batch point get can't work with tiflash in interaction txn for _, sql := range sqls { err = tk.ExecToErr(sql) require.Error(t, err) From 58808f3b7d45c2afc4b7909cecbf0471b31a88f8 Mon Sep 17 00:00:00 2001 From: yisaer Date: Thu, 1 Dec 2022 20:02:56 +0800 Subject: [PATCH 4/6] fix --- planner/core/integration_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/planner/core/integration_test.go b/planner/core/integration_test.go index 11bfc3e9deb94..743c6b87dc6d0 100644 --- a/planner/core/integration_test.go +++ b/planner/core/integration_test.go @@ -7643,7 +7643,7 @@ func TestPointGetWithSelectLock(t *testing.T) { "explain select a, b from t where a = 1 and b = 2 for update;", "explain select c, d from t1 where c = 1 for update;", "explain select c, d from t1 where c = 1 and d = 1 for update;", - "explain select c, d from t1 where c = 1 and d = 1 for update;", + "explain select c, d from t1 where (c = 1 or c = 2 )and d = 1 for update;", "explain select c, d from t1 where c in (1,2,3,4) for update;", } tk.MustExec("set @@tidb_enable_tiflash_read_for_write_stmt = on;") From e4b076220d9b4a29eded82562579f3ad672fb640 Mon Sep 17 00:00:00 2001 From: yisaer Date: Mon, 5 Dec 2022 21:14:29 +0800 Subject: [PATCH 5/6] address the comment --- planner/core/find_best_task.go | 2 +- planner/core/logical_plan_builder.go | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/planner/core/find_best_task.go b/planner/core/find_best_task.go index 35247700ce32d..2b1f098ff9b36 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -1928,7 +1928,7 @@ func (s *LogicalIndexScan) GetPhysicalIndexScan(_ *expression.Schema, stats *pro return is } -// isPointGetConditions indicates whether the conditions are point-get-able. +// isPointGetPath 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 diff --git a/planner/core/logical_plan_builder.go b/planner/core/logical_plan_builder.go index c4312f8851f57..5067bcbfde551 100644 --- a/planner/core/logical_plan_builder.go +++ b/planner/core/logical_plan_builder.go @@ -4494,8 +4494,10 @@ func (b *PlanBuilder) buildDataSource(ctx context.Context, tn *ast.TableName, as return nil, ErrPartitionClauseOnNonpartitioned } - // Skip storage engine check for CreateView. + // remain tikv access path to generate point get acceess path if existed + // see detail in issue: https://github.com/pingcap/tidb/issues/39543 if !(b.isForUpdateRead && b.ctx.GetSessionVars().TxnCtx.IsExplicit) { + // Skip storage engine check for CreateView. if b.capFlag&canExpandAST == 0 { possiblePaths, err = filterPathByIsolationRead(b.ctx, possiblePaths, tblName, dbName) if err != nil { From 73fe3a89df9344d9a01d06e674201c6aaaab0bb5 Mon Sep 17 00:00:00 2001 From: Yuanjia Zhang Date: Mon, 5 Dec 2022 23:11:18 +0800 Subject: [PATCH 6/6] Update planner/core/find_best_task.go --- 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 2b1f098ff9b36..b75942f3a2639 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -822,7 +822,7 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty, planCounter if hasPointGetPath { newPaths := make([]*util.AccessPath, 0) for _, path := range ds.possibleAccessPaths { - // if the path is the point get range path with for update lock, we should forbid tiflash as it's store path. + // if the path is the point get range path with for update lock, we should forbid tiflash as it's store path (#39543) if path.StoreType != kv.TiFlash { newPaths = append(newPaths, path) }