diff --git a/executor/prepared_test.go b/executor/prepared_test.go index f61ea7c244..f415c8657b 100644 --- a/executor/prepared_test.go +++ b/executor/prepared_test.go @@ -170,11 +170,10 @@ func TestIssue29850(t *testing.T) { tkProcess = tk.Session().ShowProcess() ps = []*util.ProcessInfo{tkProcess} tk.Session().SetSessionManager(&testkit.MockSessionManager{PS: ps}) - tk.MustQuery(fmt.Sprintf("explain for connection %d", tkProcess.ID)).Check(testkit.Rows( // cannot use PointGet since it contains a range condition - `TableReader_6 1.00 root data:TableRangeScan_5`, - `└─TableRangeScan_5 1.00 cop[tikv] table:t range:[1,1], keep order:false, stats:pseudo`)) + tk.MustQuery(fmt.Sprintf("explain for connection %d", tkProcess.ID)).Check(testkit.Rows( + `Point_Get_5 1.00 root table:t handle:1`)) tk.MustQuery(`execute stmt using @a1, @a2`).Check(testkit.Rows("1", "2")) - tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("1")) + tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("0")) tk.MustExec(`prepare stmt from 'select * from t where a=? or a=?'`) tk.MustQuery(`execute stmt using @a1, @a1`).Check(testkit.Rows("1")) @@ -182,8 +181,7 @@ func TestIssue29850(t *testing.T) { ps = []*util.ProcessInfo{tkProcess} tk.Session().SetSessionManager(&testkit.MockSessionManager{PS: ps}) tk.MustQuery(fmt.Sprintf("explain for connection %d", tkProcess.ID)).Check(testkit.Rows( // cannot use PointGet since it contains a or condition - `TableReader_6 1.00 root data:TableRangeScan_5`, - `└─TableRangeScan_5 1.00 cop[tikv] table:t range:[1,1], keep order:false, stats:pseudo`)) + `Point_Get_5 1.00 root table:t handle:1`)) tk.MustQuery(`execute stmt using @a1, @a2`).Check(testkit.Rows("1", "2")) } diff --git a/planner/core/find_best_task.go b/planner/core/find_best_task.go index 95e8dc67bb..8f092e87e1 100644 --- a/planner/core/find_best_task.go +++ b/planner/core/find_best_task.go @@ -953,9 +953,6 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty, planCounter canConvertPointGet := len(path.Ranges) > 0 && path.StoreType == kv.TiKV && ds.isPointGetConvertableSchema() - if canConvertPointGet && expression.MaybeOverOptimized4PlanCache(ds.ctx, path.AccessConds) { - canConvertPointGet = ds.canConvertToPointGetForPlanCache(path) - } if canConvertPointGet && path.Index != nil && path.Index.MVIndex { canConvertPointGet = false // cannot use PointGet upon MVIndex } @@ -1015,6 +1012,13 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty, planCounter } else { pointGetTask = ds.convertToBatchPointGet(prop, candidate, hashPartColName, opt) } + + // Batch/PointGet plans may be over-optimized, like `a>=1(?) and a<=1(?)` --> `a=1` --> PointGet(a=1). + // For safety, prevent these plans from the plan cache here. + if !pointGetTask.invalid() && expression.MaybeOverOptimized4PlanCache(ds.ctx, candidate.path.AccessConds) && !ds.isSafePointGetPlan4PlanCache(candidate.path) { + ds.ctx.GetSessionVars().StmtCtx.SetSkipPlanCache(errors.New("Batch/PointGet plans may be over-optimized")) + } + appendCandidate(ds, pointGetTask, prop, opt) if !pointGetTask.invalid() { cntPlan++ @@ -1094,12 +1098,11 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty, planCounter return } -func (ds *DataSource) canConvertToPointGetForPlanCache(path *util.AccessPath) bool { +func (ds *DataSource) isSafePointGetPlan4PlanCache(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. - // So for safety, we narrow down the scope and just generate PointGet in some particular and simple scenarios. - // scenario 1: each column corresponds to a single EQ, `a=1 and b=2 and c=3` --> `[1, 2, 3]` + // safe scenario 1: each column corresponds to a single EQ, `a=1 and b=2 and c=3` --> `[1, 2, 3]` if len(path.Ranges) > 0 && path.Ranges[0].Width() == len(path.AccessConds) { for _, accessCond := range path.AccessConds { f, ok := accessCond.(*expression.ScalarFunction) diff --git a/planner/core/plan_cache_test.go b/planner/core/plan_cache_test.go index c4a3dd46f1..ea6c37cd5d 100644 --- a/planner/core/plan_cache_test.go +++ b/planner/core/plan_cache_test.go @@ -1081,8 +1081,6 @@ func TestPlanCacheWithLimit(t *testing.T) { {"prepare stmt from 'insert into t select * from t order by a desc limit ?, ?'", []int{1, 2}}, {"prepare stmt from 'update t set a = 1 limit ?'", []int{1}}, {"prepare stmt from '(select * from t order by a limit ?) union (select * from t order by a desc limit ?)'", []int{1, 2}}, - {"prepare stmt from 'select * from t where a = ? limit ?, ?'", []int{1, 1, 1}}, - {"prepare stmt from 'select * from t where a in (?, ?) limit ?, ?'", []int{1, 2, 1, 1}}, } for idx, testCase := range testCases { @@ -1298,6 +1296,50 @@ func TestNonPreparedPlanCacheExplain(t *testing.T) { } } +func TestIssue42125(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, unique key(a, b))") + + // should use BatchPointGet + tk.MustExec("prepare st from 'select * from t where a=1 and b in (?, ?)'") + tk.MustExec("set @a=1, @b=2") + tk.MustExec("execute st using @a, @b") + tkProcess := tk.Session().ShowProcess() + ps := []*util.ProcessInfo{tkProcess} + tk.Session().SetSessionManager(&testkit.MockSessionManager{PS: ps}) + rows := tk.MustQuery(fmt.Sprintf("explain for connection %d", tkProcess.ID)).Rows() + require.Equal(t, rows[0][0], "Batch_Point_Get_5") // use BatchPointGet + tk.MustExec("execute st using @a, @b") + tk.MustQuery("show warnings").Check(testkit.Rows("Warning 1105 skip prepared plan-cache: Batch/PointGet plans may be over-optimized")) + + // should use PointGet: unsafe PointGet + tk.MustExec("prepare st from 'select * from t where a=1 and b>=? and b<=?'") + tk.MustExec("set @a=1, @b=1") + tk.MustExec("execute st using @a, @b") + tkProcess = tk.Session().ShowProcess() + ps = []*util.ProcessInfo{tkProcess} + tk.Session().SetSessionManager(&testkit.MockSessionManager{PS: ps}) + rows = tk.MustQuery(fmt.Sprintf("explain for connection %d", tkProcess.ID)).Rows() + require.Equal(t, rows[0][0], "Point_Get_5") // use Point_Get_5 + tk.MustExec("execute st using @a, @b") + tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("0")) // cannot hit + + // safe PointGet + tk.MustExec("prepare st from 'select * from t where a=1 and b=? and c Selection + require.Contains(t, rows[1][0], "Point_Get") + tk.MustExec("execute st using @a, @b") + tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("1")) // can hit +} + func TestNonPreparedPlanExplainWarning(t *testing.T) { store := testkit.CreateMockStore(t) diff --git a/planner/core/prepare_test.go b/planner/core/prepare_test.go index fc35cf3ec2..5d38d8e3aa 100644 --- a/planner/core/prepare_test.go +++ b/planner/core/prepare_test.go @@ -1954,11 +1954,10 @@ func TestPlanCachePointGetAndTableDual(t *testing.T) { tk.MustExec("insert into t2 values(1,'7777')") tk.MustExec("prepare s2 from 'select * from t2 where c1>=? and c1<=?'") tk.MustExec("set @a2=0, @b2=9") - // TableReader plan would be built, we should cache it. tk.MustQuery("execute s2 using @a2, @a2").Check(testkit.Rows()) tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("0")) tk.MustQuery("execute s2 using @a2, @b2").Check(testkit.Rows("1 7777")) - tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("1")) + tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("0")) // PointPlan tk.MustExec("create table t3(c1 int, c2 int, c3 int, unique key(c1), key(c2))") tk.MustExec("insert into t3 values(2,1,1)") @@ -2007,7 +2006,7 @@ func TestIssue26873(t *testing.T) { tk.MustQuery("execute stmt using @p").Check(testkit.Rows()) tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("0")) tk.MustQuery("execute stmt using @p").Check(testkit.Rows()) - tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("1")) + tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("0")) } func TestIssue29511(t *testing.T) {