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.

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

planner: fix index join range display when executing the cached plan #38284

Merged
merged 6 commits into from
Oct 10, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions planner/core/exhaust_physical_plans.go
Original file line number Diff line number Diff line change
Expand Up @@ -1598,7 +1598,10 @@ func (ijHelper *indexJoinBuildHelper) analyzeLookUpFilters(path *util.AccessPath
func (ijHelper *indexJoinBuildHelper) updateBestChoice(ranges ranger.MutableRanges, path *util.AccessPath, accesses,
remained []expression.Expression, lastColManager *ColWithCmpFuncManager, usedColsLen int, rebuildMode bool) {
if rebuildMode { // rebuild the range for plan-cache, update the chosenRanges anyway
ijHelper.chosenPath = path
ijHelper.chosenRanges = ranges
ijHelper.chosenAccess = accesses
ijHelper.idxOff2KeyOff = ijHelper.curIdxOff2KeyOff
return
}

Expand Down
3 changes: 1 addition & 2 deletions planner/core/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7652,8 +7652,7 @@ func TestPlanCacheForIndexJoinRangeFallback(t *testing.T) {
tk.Session().SetSessionManager(&testkit.MockSessionManager{PS: ps})
rows = tk.MustQuery(fmt.Sprintf("explain for connection %d", tkProcess.ID)).Rows()
// We don't limit range mem usage when rebuilding index join ranges for the cached plan. So [? aaaaaa,? aaaaaa], [? bbbbbb,? bbbbbb], [? cccccc,? cccccc] can be built.
// TODO: use the right range `range: decided by [eq(test.t1.a, test.t2.d) in(test.t1.b, aaaaaa, bbbbbb, cccccc)]` after https://github.com/pingcap/tidb/issues/38269 is fixed.
require.True(t, strings.Contains(rows[6][4].(string), "range: decided by [eq(test.t1.a, test.t2.d) in(test.t1.b, a, b, c)]"))
require.True(t, strings.Contains(rows[6][4].(string), "range: decided by [eq(test.t1.a, test.t2.d) in(test.t1.b, aaaaaa, bbbbbb, cccccc)]"))

// Test the plan with range fallback would not be put into cache.
tk.MustExec("prepare stmt2 from 'select /*+ inl_join(t1) */ * from t1 join t2 on t1.a = t2.d where t1.b in (?, ?, ?, ?, ?)'")
Expand Down
29 changes: 29 additions & 0 deletions planner/core/plan_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,26 @@ func RebuildPlan4CachedPlan(p Plan) error {
return rebuildRange(p)
}

func updateRange(p PhysicalPlan, ranges ranger.Ranges, rangeInfo string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this function return an error? If not, should we remove the returned error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

switch x := p.(type) {
case *PhysicalTableScan:
x.Ranges = ranges
x.rangeInfo = rangeInfo
return nil
case *PhysicalIndexScan:
x.Ranges = ranges
x.rangeInfo = rangeInfo
return nil
case *PhysicalTableReader:
return updateRange(x.TablePlans[0], ranges, rangeInfo)
case *PhysicalIndexReader:
return updateRange(x.IndexPlans[0], ranges, rangeInfo)
case *PhysicalIndexLookUpReader:
return updateRange(x.IndexPlans[0], ranges, rangeInfo)
}
return nil
}

// rebuildRange doesn't set mem limit for building ranges. There are two reasons why we don't restrict range mem usage here.
// 1. The cached plan must be able to build complete ranges under mem limit when it is generated. Hence we can just build
// ranges from x.AccessConditions. The only difference between the last ranges and new ranges is the change of parameter
Expand All @@ -327,6 +347,15 @@ func rebuildRange(p Plan) error {
if err := x.Ranges.Rebuild(); err != nil {
return err
}
if mutableRange, ok := x.Ranges.(*mutableIndexJoinRange); ok {
helper := mutableRange.buildHelper
rangeInfo := helper.buildRangeDecidedByInformation(helper.chosenPath.IdxCols, mutableRange.outerJoinKeys)
innerPlan := x.Children()[x.InnerChildIdx]
err = updateRange(innerPlan, x.Ranges.Range(), rangeInfo)
if err != nil {
return err
}
}
for _, child := range x.Children() {
err = rebuildRange(child)
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 @@ -27,6 +27,7 @@ import (
plannercore "github.com/pingcap/tidb/planner/core"
"github.com/pingcap/tidb/testkit"
"github.com/pingcap/tidb/types"
"github.com/pingcap/tidb/util"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -110,3 +111,22 @@ func TestGeneralPlanCacheBasically(t *testing.T) {
tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("1")) // this plan is from plan-cache
}
}

func TestIssue38269(t *testing.T) {
store := testkit.CreateMockStore(t)
tk := testkit.NewTestKit(t, store)
tk.MustExec(`set @@tidb_enable_prepared_plan_cache=1`)
tk.MustExec("set @@tidb_enable_collect_execution_info=0")
tk.MustExec("use test")
tk.MustExec("create table t1(a int)")
tk.MustExec("create table t2(a int, b int, c int, index idx(a, b))")
tk.MustExec("prepare stmt1 from 'select /*+ inl_join(t2) */ * from t1 join t2 on t1.a = t2.a where t2.b in (?, ?, ?)'")
tk.MustExec("set @a = 10, @b = 20, @c = 30, @d = 40, @e = 50, @f = 60")
tk.MustExec("execute stmt1 using @a, @b, @c")
tk.MustExec("execute stmt1 using @d, @e, @f")
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.Contains(t, rows[6][4], "range: decided by [eq(test.t2.a, test.t1.a) in(test.t2.b, 40, 50, 60)]")
}