Skip to content

Commit

Permalink
planner: fix the issue that cached IndexMerge plans can return wrong …
Browse files Browse the repository at this point in the history
…results in some cases (#41870)

close #41828
  • Loading branch information
qw4990 authored Mar 2, 2023
1 parent caa1a7c commit a4aa274
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 5 deletions.
2 changes: 1 addition & 1 deletion executor/explainfor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -768,7 +768,7 @@ func TestIndexMerge4PlanCache(t *testing.T) {
ps := []*util.ProcessInfo{tkProcess}
tk.Session().SetSessionManager(&testkit.MockSessionManager{PS: ps})
res := tk.MustQuery("explain for connection " + strconv.FormatUint(tkProcess.ID, 10))
require.Len(t, res.Rows(), 6)
require.Len(t, res.Rows(), 7)
require.Regexp(t, ".*IndexMerge.*", res.Rows()[1][0])
require.Regexp(t, ".*IndexRangeScan.*", res.Rows()[3][0])
require.Equal(t, "range:(NULL,\"mm\"), (\"mm\",+inf], keep order:false, stats:pseudo", res.Rows()[3][4])
Expand Down
10 changes: 10 additions & 0 deletions planner/core/indexmerge_path.go
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,11 @@ func (ds *DataSource) buildIndexMergeOrPath(
path.TableFilters = nil
}
}

// Keep this filter as a part of table filters for safety if it has any parameter.
if expression.MaybeOverOptimized4PlanCache(ds.ctx, filters[current:current+1]) {
shouldKeepCurrentFilter = true
}
if shouldKeepCurrentFilter {
indexMergePath.TableFilters = append(indexMergePath.TableFilters, filters[current])
}
Expand Down Expand Up @@ -437,6 +442,11 @@ func (ds *DataSource) generateIndexMergeAndPaths(normalPathCnt int) *util.Access
}
}

// Keep these partial filters as a part of table filters for safety if there is any parameter.
if expression.MaybeOverOptimized4PlanCache(ds.ctx, partialFilters) {
dedupedFinalFilters = append(dedupedFinalFilters, partialFilters...)
}

// 3. Estimate the row count after partial paths.
sel, _, err := ds.tableStats.HistColl.Selectivity(ds.ctx, partialFilters, nil)
if err != nil {
Expand Down
20 changes: 20 additions & 0 deletions planner/core/plan_cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1100,6 +1100,26 @@ func TestPlanCacheWithSubquery(t *testing.T) {
}
}

func TestIssue41828(t *testing.T) {
store := testkit.CreateMockStore(t)
tk := testkit.NewTestKit(t, store)
tk.MustExec("use test")
tk.MustExec(`CREATE TABLE IDT_MULTI15840STROBJSTROBJ (
COL1 enum('aa', 'zzz') DEFAULT NULL,
COL2 smallint(6) DEFAULT NULL,
COL3 date DEFAULT NULL,
KEY U_M_COL4 (COL1,COL2),
KEY U_M_COL5 (COL3,COL2))`)

tk.MustExec(`INSERT INTO IDT_MULTI15840STROBJSTROBJ VALUES ('zzz',1047,'6115-06-05'),('zzz',-23221,'4250-09-03'),('zzz',27138,'1568-07-30'),('zzz',-30903,'6753-08-21'),('zzz',-26875,'6117-10-10')`)
tk.MustExec(`prepare stmt from 'select * from IDT_MULTI15840STROBJSTROBJ where col3 <=> ? or col1 in (?, ?, ?) and col2 not between ? and ?'`)
tk.MustExec(`set @a="0051-12-23", @b="none", @c="none", @d="none", @e=-32757, @f=-32757`)
tk.MustQuery(`execute stmt using @a,@b,@c,@d,@e,@f`).Check(testkit.Rows())
tk.MustQuery(`show warnings`).Check(testkit.Rows(`Warning 1105 skip plan-cache: IndexMerge plan with full-scan is un-cacheable`))
tk.MustExec(`set @a="9795-01-10", @b="aa", @c="aa", @d="aa", @e=31928, @f=31928`)
tk.MustQuery(`execute stmt using @a,@b,@c,@d,@e,@f`).Check(testkit.Rows())
}

func TestPlanCacheSubquerySPMEffective(t *testing.T) {
store := testkit.CreateMockStore(t)
tk := testkit.NewTestKit(t, store)
Expand Down
20 changes: 16 additions & 4 deletions planner/core/plan_cacheable_checker.go
Original file line number Diff line number Diff line change
Expand Up @@ -376,11 +376,12 @@ func isPlanCacheable(sctx sessionctx.Context, p Plan, paramNum, limitParamNum in
if limitParamNum != 0 && !sctx.GetSessionVars().EnablePlanCacheForParamLimit {
return false, "skip plan-cache: the switch 'tidb_enable_plan_cache_for_param_limit' is off"
}
return isPhysicalPlanCacheable(sctx, pp, paramNum, limitParamNum)
return isPhysicalPlanCacheable(sctx, pp, paramNum, limitParamNum, false)
}

// isPhysicalPlanCacheable returns whether this physical plan is cacheable and return the reason if not.
func isPhysicalPlanCacheable(sctx sessionctx.Context, p PhysicalPlan, paramNum, limitParamNum int) (cacheable bool, reason string) {
func isPhysicalPlanCacheable(sctx sessionctx.Context, p PhysicalPlan, paramNum, limitParamNum int, underIndexMerge bool) (cacheable bool, reason string) {
var subPlans []PhysicalPlan
switch x := p.(type) {
case *PhysicalTableDual:
if paramNum > 0 {
Expand All @@ -398,12 +399,23 @@ func isPhysicalPlanCacheable(sctx sessionctx.Context, p PhysicalPlan, paramNum,
if x.AccessMVIndex {
return false, "skip plan-cache: the plan with IndexMerge accessing Multi-Valued Index is un-cacheable"
}
underIndexMerge = true
subPlans = append(subPlans, x.partialPlans...)
case *PhysicalIndexScan:
if underIndexMerge && x.isFullScan() {
return false, "skip plan-cache: IndexMerge plan with full-scan is un-cacheable"
}
case *PhysicalTableScan:
if underIndexMerge && x.isFullScan() {
return false, "skip plan-cache: IndexMerge plan with full-scan is un-cacheable"
}
case *PhysicalApply:
return false, "skip plan-cache: PhysicalApply plan is un-cacheable"
}

for _, c := range p.Children() {
if cacheable, reason = isPhysicalPlanCacheable(sctx, c, paramNum, limitParamNum); !cacheable {
subPlans = append(subPlans, p.Children()...)
for _, c := range subPlans {
if cacheable, reason = isPhysicalPlanCacheable(sctx, c, paramNum, limitParamNum, underIndexMerge); !cacheable {
return cacheable, reason
}
}
Expand Down

0 comments on commit a4aa274

Please sign in to comment.