Skip to content

Commit eca41cb

Browse files
qw4990qw4990
authored
planner: fix the issue of reusing wrong point-plan for "select ... for update" (#54661) (#57239)
close #54652
1 parent f65629b commit eca41cb

File tree

5 files changed

+60
-3
lines changed

5 files changed

+60
-3
lines changed

‎planner/core/plan_cache.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
package core
1616

1717
import (
18+
"bytes"
1819
"context"
1920

2021
"github.com/pingcap/errors"
@@ -109,6 +110,7 @@ func planCachePreprocess(ctx context.Context, sctx sessionctx.Context, isNonPrep
109110
stmtAst.CachedPlan = nil
110111
stmt.Executor = nil
111112
stmt.ColumnInfos = nil
113+
stmt.planCacheKey = nil
112114
// If the schema version has changed we need to preprocess it again,
113115
// if this time it failed, the real reason for the error is schema changed.
114116
// Example:
@@ -191,7 +193,7 @@ func GetPlanFromSessionPlanCache(ctx context.Context, sctx sessionctx.Context,
191193
}
192194
}
193195

194-
if stmtCtx.UseCache && stmtAst.CachedPlan != nil { // special code path for fast point plan
196+
if stmtCtx.UseCache && stmtAst.CachedPlan != nil && bytes.Equal(stmt.planCacheKey.Hash(), cacheKey.Hash()) { // special code path for fast point plan
195197
if plan, names, ok, err := getCachedPointPlan(stmtAst, sessVars, stmtCtx); ok {
196198
return plan, names, err
197199
}
@@ -338,6 +340,7 @@ func generateNewPlan(ctx context.Context, sctx sessionctx.Context, isNonPrepared
338340
}
339341
cached := NewPlanCacheValue(p, names, stmtCtx.TblInfo2UnionScan, matchOpts, &stmtCtx.StmtHints)
340342
stmt.NormalizedPlan, stmt.PlanDigest = NormalizePlan(p)
343+
stmt.planCacheKey = cacheKey
341344
stmtCtx.SetPlan(p)
342345
stmtCtx.SetPlanDigest(stmt.NormalizedPlan, stmt.PlanDigest)
343346
sctx.GetSessionPlanCache().Put(cacheKey, cached, matchOpts)
@@ -837,6 +840,7 @@ func IsPointPlanShortPathOK(sctx sessionctx.Context, is infoschema.InfoSchema, s
837840
if stmtAst.SchemaVersion != is.SchemaMetaVersion() {
838841
stmtAst.CachedPlan = nil
839842
stmt.ColumnInfos = nil
843+
stmt.planCacheKey = nil
840844
return false, nil
841845
}
842846
// maybe we'd better check cached plan type here, current

‎planner/core/plan_cache_test.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2567,3 +2567,26 @@ func TestIndexRange(t *testing.T) {
25672567
tk.MustQuery(`SELECT t0.* FROM t0 WHERE (id = 1 or id = 9223372036854775808);`).Check(testkit.Rows("1"))
25682568
tk.MustQuery("SELECT t1.c0 FROM t1 WHERE t1.c0!=BIN(-1);").Check(testkit.Rows("1"))
25692569
}
2570+
2571+
func TestIssue54652(t *testing.T) {
2572+
store := testkit.CreateMockStore(t)
2573+
tk := testkit.NewTestKit(t, store)
2574+
2575+
tk.MustExec(`use test`)
2576+
tk.MustExec(`create table t (pk int, a int, primary key(pk))`)
2577+
tk.MustExec(`set autocommit=on`)
2578+
tk.MustQuery(`select @@autocommit`).Check(testkit.Rows("1"))
2579+
tk.MustExec(`set @pk=1`)
2580+
2581+
tk.MustExec(`prepare st from 'select * from t where pk=? for update'`)
2582+
tk.MustExec(`execute st using @pk`)
2583+
tk.MustExec(`execute st using @pk`)
2584+
tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("1"))
2585+
2586+
tk.MustExec(`begin`)
2587+
tk.MustExec(`execute st using @pk`)
2588+
tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("0")) // can't reuse since it's in txn now.
2589+
tk.MustExec(`execute st using @pk`)
2590+
tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("1")) // can reuse since it's in txn now.
2591+
tk.MustExec(`commit`)
2592+
}

‎planner/core/plan_cache_utils.go‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"unsafe"
2323

2424
"github.com/pingcap/errors"
25+
"github.com/pingcap/tidb/config"
2526
"github.com/pingcap/tidb/expression"
2627
"github.com/pingcap/tidb/infoschema"
2728
"github.com/pingcap/tidb/kv"
@@ -224,12 +225,20 @@ type planCacheKey struct {
224225
TiDBSuperReadOnly bool
225226
ExprBlacklistTS int64 // expr-pushdown-blacklist can affect query optimization, so we need to consider it in plan cache.
226227

228+
// status related to Txn
229+
inTxn bool
230+
autoCommit bool
231+
pessAutoCommit bool
232+
227233
memoryUsage int64 // Do not include in hash
228234
hash []byte
229235
}
230236

231237
// Hash implements Key interface.
232238
func (key *planCacheKey) Hash() []byte {
239+
if key == nil {
240+
return nil
241+
}
233242
if len(key.hash) == 0 {
234243
if key.hash == nil {
235244
key.hash = make([]byte, 0, len(key.stmtText)*2)
@@ -256,10 +265,20 @@ func (key *planCacheKey) Hash() []byte {
256265
key.hash = append(key.hash, hack.Slice(strconv.FormatBool(key.restrictedReadOnly))...)
257266
key.hash = append(key.hash, hack.Slice(strconv.FormatBool(key.TiDBSuperReadOnly))...)
258267
key.hash = codec.EncodeInt(key.hash, key.ExprBlacklistTS)
268+
key.hash = append(key.hash, bool2Byte(key.inTxn))
269+
key.hash = append(key.hash, bool2Byte(key.autoCommit))
270+
key.hash = append(key.hash, bool2Byte(key.pessAutoCommit))
259271
}
260272
return key.hash
261273
}
262274

275+
func bool2Byte(flag bool) byte {
276+
if flag {
277+
return '1'
278+
}
279+
return '0'
280+
}
281+
263282
const emptyPlanCacheKeySize = int64(unsafe.Sizeof(planCacheKey{}))
264283

265284
// MemoryUsage return the memory usage of planCacheKey
@@ -326,6 +345,9 @@ func NewPlanCacheKey(sessionVars *variable.SessionVars, stmtText, stmtDB string,
326345
restrictedReadOnly: variable.RestrictedReadOnly.Load(),
327346
TiDBSuperReadOnly: variable.VarTiDBSuperReadOnly.Load(),
328347
ExprBlacklistTS: exprBlacklistTS,
348+
inTxn: sessionVars.InTxn(),
349+
autoCommit: sessionVars.IsAutocommit(),
350+
pessAutoCommit: config.GetGlobalConfig().PessimisticTxn.PessimisticAutoCommit.Load(),
329351
}
330352
for k, v := range sessionVars.IsolationReadEngines {
331353
key.isolationReadEngines[k] = v
@@ -467,6 +489,9 @@ type PlanCacheStmt struct {
467489
// NormalizedSQL4PC: select * from `test` . `t` where `a` > ? and `b` < ? --> schema name is added,
468490
// StmtText: select * from t where a>1 and b <? --> just format the original query;
469491
StmtText string
492+
493+
// the cache key for point-get statement, have to check whether the cache key changes before reusing this plan for safety.
494+
planCacheKey kvcache.Key
470495
}
471496

472497
// GetPreparedStmt extract the prepared statement from the execute statement.

‎planner/core/plan_cache_utils_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,5 +49,5 @@ func TestCacheKey(t *testing.T) {
4949
if err != nil {
5050
t.Fail()
5151
}
52-
require.Equal(t, []byte{0x74, 0x65, 0x73, 0x74, 0x80, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x73, 0x65, 0x6c, 0x65, 0x63, 0x74, 0x20, 0x31, 0x80, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x80, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x80, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x80, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x74, 0x69, 0x64, 0x62, 0x74, 0x69, 0x6b, 0x76, 0x74, 0x69, 0x66, 0x6c, 0x61, 0x73, 0x68, 0x7f, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x66, 0x61, 0x6c, 0x73, 0x65, 0x66, 0x61, 0x6c, 0x73, 0x65, 0x66, 0x61, 0x6c, 0x73, 0x65, 0x80, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, key.Hash())
52+
require.Equal(t, []byte{0x74, 0x65, 0x73, 0x74, 0x80, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x73, 0x65, 0x6c, 0x65, 0x63, 0x74, 0x20, 0x31, 0x80, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x80, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x80, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x80, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x74, 0x69, 0x64, 0x62, 0x74, 0x69, 0x6b, 0x76, 0x74, 0x69, 0x66, 0x6c, 0x61, 0x73, 0x68, 0x7f, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x66, 0x61, 0x6c, 0x73, 0x65, 0x66, 0x61, 0x6c, 0x73, 0x65, 0x66, 0x61, 0x6c, 0x73, 0x65, 0x80, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x30, 0x31, 0x30}, key.Hash())
5353
}

‎planner/core/prepare_test.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1263,9 +1263,12 @@ func TestPlanCacheUnionScan(t *testing.T) {
12631263
tk.MustQuery("execute stmt1 using @p0").Check(testkit.Rows())
12641264
tk.MustExec("begin")
12651265
tk.MustQuery("execute stmt1 using @p0").Check(testkit.Rows())
1266+
cnt := pb.GetCounter().GetValue()
1267+
require.Equal(t, float64(0), cnt) // can't reuse the plan created outside the txn
1268+
tk.MustQuery("execute stmt1 using @p0").Check(testkit.Rows())
12661269
err := counter.Write(pb)
12671270
require.NoError(t, err)
1268-
cnt := pb.GetCounter().GetValue()
1271+
cnt = pb.GetCounter().GetValue()
12691272
require.Equal(t, float64(1), cnt)
12701273
tk.MustExec("insert into t1 values(1)")
12711274
// Cached plan is invalid now, it is not chosen and removed.
@@ -1297,6 +1300,8 @@ func TestPlanCacheUnionScan(t *testing.T) {
12971300
tk.MustQuery("execute stmt2 using @p0").Check(testkit.Rows())
12981301
tk.MustExec("begin")
12991302
tk.MustQuery("execute stmt2 using @p0").Check(testkit.Rows())
1303+
require.Equal(t, float64(3), cnt) // can't reuse the plan created outside the txn
1304+
tk.MustQuery("execute stmt2 using @p0").Check(testkit.Rows())
13001305
err = counter.Write(pb)
13011306
require.NoError(t, err)
13021307
cnt = pb.GetCounter().GetValue()

0 commit comments

Comments
 (0)