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.

Sign up for GitHub

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

Jump to bottom

planner: fix the issue that some PointGet plans generated in physical-stage cannot be cached #28185

Closed
wants to merge 12 commits into from
Closed

planner: fix the issue that some PointGet plans generated in physical-stage cannot be cached #28185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1e8999a
fixup
qw4990 Sep 17, 2021
9a0ec34
fixup
qw4990 Sep 17, 2021
acaa143
fixup
qw4990 Sep 17, 2021
754c51b
add test cases
qw4990 Sep 17, 2021
0f9ae01
update comment
qw4990 Sep 17, 2021
ba280fd
update the comment
qw4990 Sep 17, 2021
2c56f59
fixup
qw4990 Sep 18, 2021
1e722bf
fixup
qw4990 Sep 18, 2021
bba86b6
fixup
qw4990 Sep 18, 2021
ebb766f
fixup
qw4990 Sep 18, 2021
fa21297
fixup
qw4990 Sep 18, 2021
fd9e1ac
fixup
qw4990 Sep 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions executor/seqtest/prepared_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,14 +503,14 @@ func (s *seqTestSuite) TestPreparedInsert(c *C) {
err = counter.Write(pb)
c.Assert(err, IsNil)
hit := pb.GetCounter().GetValue()
c.Check(hit, Equals, float64(2))
c.Check(hit, Equals, float64(3))
}
tk.MustExec(`set @a=3; execute stmt_insert_select using @a;`)
if flag {
err = counter.Write(pb)
c.Assert(err, IsNil)
hit := pb.GetCounter().GetValue()
c.Check(hit, Equals, float64(2))
c.Check(hit, Equals, float64(4))
}

result = tk.MustQuery("select id, c1 from prepare_test where id = ?", 101)
Expand Down
4 changes: 2 additions & 2 deletions expression/expression.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -789,8 +789,8 @@ func SplitDNFItems(onExpr Expression) []Expression {
// EvaluateExprWithNull sets columns in schema as null and calculate the final result of the scalar function.
// If the Expression is a non-constant value, it means the result is unknown.
func EvaluateExprWithNull(ctx sessionctx.Context, schema *Schema, expr Expression) Expression {
if ContainMutableConst(ctx, []Expression{expr}) {
ctx.GetSessionVars().StmtCtx.OptimDependOnMutableConst = true
if ctx.GetSessionVars().StmtCtx.UseCache && ContainMutableConst(ctx, []Expression{expr}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The function ContainMutableConst contains the check for ctx.GetSessionVars().StmtCtx.UseCache.

return expr
}
return evaluateExprWithNull(ctx, schema, expr)
}
Expand Down
5 changes: 4 additions & 1 deletion planner/core/find_best_task.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,7 +811,10 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty, planCounter
p: dual,
}, cntPlan, nil
}
canConvertPointGet := len(path.Ranges) > 0 && path.StoreType == kv.TiKV && ds.isPointGetConvertableSchema()
canConvertPointGet := len(path.Ranges) > 0 && path.StoreType == kv.TiKV && ds.isPointGetConvertableSchema() &&
// to avoid the over-optimized risk, do not generate PointGet for plan cache, for example,
// `pk>=$a and pk<=$b` can be optimized to a PointGet when `$a==$b`, but it can cause wrong results when `$a!=$b`.
!ds.ctx.GetSessionVars().StmtCtx.UseCache
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we cannot generate PointGet if prepared-cache-plan is enabled. Will that be a problem in OLTP workload? For example, as for prepare stmt1 from "insert into t2 select * from t1 where a=?", if the sql will be executed frequently, we may hope to not only generate PointGet plan but also cache the plan.

if canConvertPointGet && !path.IsIntHandlePath {
// We simply do not build [batch] point get for prefix indexes. This can be optimized.
canConvertPointGet = path.Index.Unique && !path.Index.HasPrefixIndex()
Expand Down
52 changes: 47 additions & 5 deletions planner/core/prepare_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,48 @@ func (s *testPrepareSerialSuite) TestPrepareCacheIndexScan(c *C) {
tk.MustQuery("execute stmt1 using @a, @b").Check(testkit.Rows("1 3", "1 3"))
}

func (s *testPrepareSerialSuite) TestPrepareCachePointGetInsert(c *C) {
defer testleak.AfterTest(c)()
store, dom, err := newStoreWithBootstrap()
c.Assert(err, IsNil)
tk := testkit.NewTestKit(c, store)
orgEnable := core.PreparedPlanCacheEnabled()
defer func() {
dom.Close()
err = store.Close()
c.Assert(err, IsNil)
core.SetPreparedPlanCache(orgEnable)
}()
core.SetPreparedPlanCache(true)
tk.Se, err = session.CreateSession4TestWithOpt(store, &session.Opt{
PreparedPlanCache: kvcache.NewSimpleLRUCache(100, 0.1, math.MaxUint64),
})
c.Assert(err, IsNil)

tk.MustExec("use test")
tk.MustExec("drop table if exists t")
tk.MustExec("create table t1 (a int, b int, primary key(a))")
tk.MustExec("insert into t1 values (1, 1), (2, 2), (3, 3)")

tk.MustExec("create table t2 (a int, b int, primary key(a))")
tk.MustExec(`prepare stmt1 from "insert into t2 select * from t1 where a=?"`)

tk.MustExec("set @a=1")
tk.MustExec("execute stmt1 using @a")
tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("0"))
tk.MustQuery("select * from t2 order by a").Check(testkit.Rows("1 1"))

tk.MustExec("set @a=2")
tk.MustExec("execute stmt1 using @a")
tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("1"))
tk.MustQuery("select * from t2 order by a").Check(testkit.Rows("1 1", "2 2"))

tk.MustExec("set @a=3")
tk.MustExec("execute stmt1 using @a")
tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("1"))
tk.MustQuery("select * from t2 order by a").Check(testkit.Rows("1 1", "2 2", "3 3"))
}

func (s *testPlanSerialSuite) TestPrepareCacheDeferredFunction(c *C) {
defer testleak.AfterTest(c)()
store, dom, err := newStoreWithBootstrap()
Expand Down Expand Up @@ -1107,7 +1149,7 @@ func (s *testPlanSerialSuite) TestPlanCachePointGetAndTableDual(c *C) {
tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("0"))
// Must not reuse the previous TableDual plan.
Copy link
Contributor

@Reminiscent Reminiscent Sep 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Please modify the comment here. Besides, we need to check the plan from the cache. The following changes are the same.

tk.MustQuery("execute s0 using @a0, @a0, @b0").Check(testkit.Rows("0000 7777 1"))
tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("0"))
tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("1"))

tk.MustExec("create table t1(c1 varchar(20), c2 varchar(20), c3 bigint(20), primary key(c1, c2))")
tk.MustExec("insert into t1 values('0000','7777',1)")
Expand All @@ -1118,7 +1160,7 @@ func (s *testPlanSerialSuite) TestPlanCachePointGetAndTableDual(c *C) {
tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("0"))
// Must not reuse the previous PointGet plan.
tk.MustQuery("execute s1 using @a1, @a1, @b1").Check(testkit.Rows("0000 7777 1"))
tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("0"))
tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("1"))

tk.MustExec("create table t2(c1 bigint(20) primary key, c2 varchar(20))")
tk.MustExec("insert into t2 values(1,'7777')")
Expand All @@ -1129,7 +1171,7 @@ func (s *testPlanSerialSuite) TestPlanCachePointGetAndTableDual(c *C) {
tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("0"))
// Must not reuse the previous PointGet plan.
tk.MustQuery("execute s2 using @a2, @b2").Check(testkit.Rows("1 7777"))
tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("0"))
tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("1"))

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)")
Expand All @@ -1140,7 +1182,7 @@ func (s *testPlanSerialSuite) TestPlanCachePointGetAndTableDual(c *C) {
tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("0"))
// Must not reuse the previous IndexMerge with partial PointGet plan.
tk.MustQuery("execute s3 using @a3,@b3").Check(testkit.Rows("2 1 1"))
tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("0"))
tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("1"))

tk.MustExec("prepare s3 from 'select /*+ use_index_merge(t3) */ * from t3 where (c1 >= ? and c1 <= ?) or c2 > 1'")
tk.MustExec("set @a3=1,@b3=3")
Expand Down Expand Up @@ -1199,7 +1241,7 @@ func (s *testPlanSerialSuite) TestIssue23671(c *C) {
tk.MustQuery("execute s1 using @a, @b, @c").Check(testkit.Rows("1 1"))
tk.MustExec("set @a=1, @b=1, @c=10")
tk.MustQuery("execute s1 using @a, @b, @c").Check(testkit.Rows("1 1", "2 2"))
tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("0"))
tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows("1"))
}

func (s *testPlanSerialSuite) TestPartitionTable(c *C) {
Expand Down
6 changes: 2 additions & 4 deletions planner/core/stats.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,8 +343,6 @@ func (ds *DataSource) derivePathStatsAndTryHeuristics() error {
if selected != nil {
ds.possibleAccessPaths[0] = selected
ds.possibleAccessPaths = ds.possibleAccessPaths[:1]
// TODO: Can we make a more careful check on whether the optimization depends on mutable constants?
ds.ctx.GetSessionVars().StmtCtx.OptimDependOnMutableConst = true
Comment on lines -346 to -347
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there some test cases that can cover this change?

if ds.ctx.GetSessionVars().StmtCtx.InVerboseExplain {
var tableName string
if ds.TableAsName.O == "" {
Expand Down Expand Up @@ -532,7 +530,7 @@ func (ds *DataSource) generateIndexMergeOrPaths() error {
dnfItems := expression.FlattenDNFConditions(sf)
for _, item := range dnfItems {
cnfItems := expression.SplitCNFItems(item)
itemPaths := ds.accessPathsForConds(cnfItems, usedIndexCount)
itemPaths := ds.indexMergeAccessPathsForConds(cnfItems, usedIndexCount)
if len(itemPaths) == 0 {
partialPaths = nil
break
Expand Down Expand Up @@ -604,7 +602,7 @@ func (ds *DataSource) isInIndexMergeHints(name string) bool {
}

// accessPathsForConds generates all possible index paths for conditions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the comment here.

func (ds *DataSource) accessPathsForConds(conditions []expression.Expression, usedIndexCount int) []*util.AccessPath {
func (ds *DataSource) indexMergeAccessPathsForConds(conditions []expression.Expression, usedIndexCount int) []*util.AccessPath {
var results = make([]*util.AccessPath, 0, usedIndexCount)
for i := 0; i < usedIndexCount; i++ {
path := &util.AccessPath{}
Expand Down
4 changes: 3 additions & 1 deletion util/ranger/detacher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,9 @@ func ExtractEqAndInCondition(sctx sessionctx.Context, conditions []expression.Ex
// Maybe we can improve it later.
columnValues[i] = &valueInfo{mutable: true}
}
sctx.GetSessionVars().StmtCtx.OptimDependOnMutableConst = true
if sctx.GetSessionVars().StmtCtx.UseCache {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the prepare stmt like prepare stmt for 'select * from t where a > 0 and a < 2' which there are no parameters, we should use the PointGet plan. But here we can't use this plan?

Copy link
Contributor

Choose a reason for hiding this comment

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

In real workloads there may be a small chance to convert a > ? and a < ? to equal condition, so maybe it is acceptable to give up the optimization when prepare-plan-cache is enabled.

return nil, conditions, nil, nil, false
}
}
}
for i, offset := range offsets {
Expand Down