From aa919571327e46822cb561975f66468541ae76e3 Mon Sep 17 00:00:00 2001 From: qw4990 Date: Tue, 16 Jul 2024 15:58:04 +0800 Subject: [PATCH 1/7] fixup --- pkg/planner/core/plan_cache.go | 3 ++- pkg/planner/core/plan_cache_test.go | 21 +++++++++++++++++++++ pkg/planner/core/plan_cache_utils.go | 22 +++++++++++++++++----- 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/pkg/planner/core/plan_cache.go b/pkg/planner/core/plan_cache.go index a20e0fe1b7c7e..dba3c03f4e5be 100644 --- a/pkg/planner/core/plan_cache.go +++ b/pkg/planner/core/plan_cache.go @@ -219,7 +219,7 @@ func GetPlanFromPlanCache(ctx context.Context, sctx sessionctx.Context, if stmtCtx.UseCache() { var cachedVal *PlanCacheValue var hit, isPointPlan bool - if stmt.PointGet.pointPlan != nil { // if it's PointGet Plan, no need to use paramTypes + if stmt.PointGet.pointPlan != nil && stmt.PointGet.planCacheKey == cacheKey { // if it's PointGet Plan, no need to use paramTypes cachedVal = &PlanCacheValue{ Plan: stmt.PointGet.pointPlan, OutputColumns: stmt.PointGet.columnNames, @@ -322,6 +322,7 @@ func generateNewPlan(ctx context.Context, sctx sessionctx.Context, isNonPrepared stmt.PointGet.pointPlan = p stmt.PointGet.columnNames = names stmt.PointGet.pointPlanHints = stmtCtx.StmtHints.Clone() + stmt.PointGet.planCacheKey = cacheKey } } sessVars.FoundInPlanCache = false diff --git a/pkg/planner/core/plan_cache_test.go b/pkg/planner/core/plan_cache_test.go index aa5ab2abd4502..f29c10bfe369c 100644 --- a/pkg/planner/core/plan_cache_test.go +++ b/pkg/planner/core/plan_cache_test.go @@ -1907,3 +1907,24 @@ func TestInstancePlanCacheAcrossSession(t *testing.T) { tk2.MustQuery(`execute st using @a`).Sort().Check(testkit.Rows(`1`, `2`, `3`)) tk2.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("1")) } + +func TestIssue54652(t *testing.T) { + store := testkit.CreateMockStore(t) + tk := testkit.NewTestKit(t, store) + + tk.MustExec(`use test`) + tk.MustExec(`create table t (pk int, a int, primary key(pk))`) + tk.MustExec(`set autocommit=on`) + tk.MustQuery(`select @@autocommit`).Check(testkit.Rows("1")) + tk.MustExec(`set @pk=1`) + + tk.MustExec(`prepare st from 'select * from t where pk=? for update'`) + tk.MustExec(`execute st using @pk`) + tk.MustExec(`execute st using @pk`) + tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("1")) + + tk.MustExec(`begin`) + tk.MustExec(`execute st using @pk`) + tk.MustQuery(`select @@last_plan_from_cache`).Check(testkit.Rows("0")) // can't reuse since it's in txn now. + tk.MustExec(`commit`) +} diff --git a/pkg/planner/core/plan_cache_utils.go b/pkg/planner/core/plan_cache_utils.go index 93044f0566afe..a2da0d37cce3f 100644 --- a/pkg/planner/core/plan_cache_utils.go +++ b/pkg/planner/core/plan_cache_utils.go @@ -17,6 +17,7 @@ package core import ( "cmp" "context" + "github.com/pingcap/tidb/pkg/config" "math" "slices" "sort" @@ -330,11 +331,7 @@ func NewPlanCacheKey(sctx sessionctx.Context, stmt *PlanCacheStmt) (key, binding } // this variable might affect the plan - if vars.ForeignKeyChecks { - hash = append(hash, '1') - } else { - hash = append(hash, '0') - } + hash = append(hash, bool2Byte(vars.ForeignKeyChecks)) // "limit ?" can affect the cached plan: "limit 1" and "limit 10000" should use different plans. if len(stmt.limits) > 0 { @@ -386,9 +383,23 @@ func NewPlanCacheKey(sctx sessionctx.Context, stmt *PlanCacheStmt) (key, binding hash = codec.EncodeInt(hash, id) } } + + // txn status + hash = append(hash, '|') + hash = append(hash, bool2Byte(vars.InTxn())) + hash = append(hash, bool2Byte(vars.IsAutocommit())) + hash = append(hash, bool2Byte(config.GetGlobalConfig().PessimisticTxn.PessimisticAutoCommit.Load())) + return string(hash), binding, true, "", nil } +func bool2Byte(flag bool) byte { + if flag { + return '1' + } + return '0' +} + // PlanCacheValue stores the cached Statement and StmtNode. type PlanCacheValue struct { Plan base.Plan // not-read-only, session might update it before reusing @@ -514,6 +525,7 @@ type PointGetExecutorCache struct { pointPlan base.Plan pointPlanHints *hint.StmtHints columnNames types.NameSlice + planCacheKey string ColumnInfos any // Executor is only used for point get scene. From 8e6bafc2eaee75ef134ed33011b903b5ab74d306 Mon Sep 17 00:00:00 2001 From: qw4990 Date: Tue, 16 Jul 2024 16:05:33 +0800 Subject: [PATCH 2/7] fixup --- pkg/planner/core/plan_cache_utils.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/planner/core/plan_cache_utils.go b/pkg/planner/core/plan_cache_utils.go index a2da0d37cce3f..2af7501f54d96 100644 --- a/pkg/planner/core/plan_cache_utils.go +++ b/pkg/planner/core/plan_cache_utils.go @@ -17,7 +17,6 @@ package core import ( "cmp" "context" - "github.com/pingcap/tidb/pkg/config" "math" "slices" "sort" @@ -26,6 +25,7 @@ import ( "github.com/pingcap/errors" "github.com/pingcap/tidb/pkg/bindinfo" + "github.com/pingcap/tidb/pkg/config" "github.com/pingcap/tidb/pkg/domain" "github.com/pingcap/tidb/pkg/expression" "github.com/pingcap/tidb/pkg/infoschema" @@ -525,6 +525,8 @@ type PointGetExecutorCache struct { pointPlan base.Plan pointPlanHints *hint.StmtHints columnNames types.NameSlice + + // the cache key for this statement, have to check whether the cache key changes before reusing this plan for safety. planCacheKey string ColumnInfos any From 9676e8a80dace67f2266f8764d9e0afd4051fb9e Mon Sep 17 00:00:00 2001 From: qw4990 Date: Tue, 16 Jul 2024 16:11:47 +0800 Subject: [PATCH 3/7] make linter happy --- pkg/planner/core/plan_cache_utils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/planner/core/plan_cache_utils.go b/pkg/planner/core/plan_cache_utils.go index 2af7501f54d96..3a1b61ec1aeba 100644 --- a/pkg/planner/core/plan_cache_utils.go +++ b/pkg/planner/core/plan_cache_utils.go @@ -527,7 +527,7 @@ type PointGetExecutorCache struct { columnNames types.NameSlice // the cache key for this statement, have to check whether the cache key changes before reusing this plan for safety. - planCacheKey string + planCacheKey string ColumnInfos any // Executor is only used for point get scene. From 44846610469f01d4a67a34594823e7d8a058491e Mon Sep 17 00:00:00 2001 From: qw4990 Date: Tue, 16 Jul 2024 16:44:13 +0800 Subject: [PATCH 4/7] make linter happy --- pkg/ddl/tests/metadatalock/mdl_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/ddl/tests/metadatalock/mdl_test.go b/pkg/ddl/tests/metadatalock/mdl_test.go index 467affc1b974d..d2730c2479c5a 100644 --- a/pkg/ddl/tests/metadatalock/mdl_test.go +++ b/pkg/ddl/tests/metadatalock/mdl_test.go @@ -935,7 +935,7 @@ func TestMDLPreparePlanCacheExecute(t *testing.T) { tk.MustQuery("select * from t2") tk.MustExec(`set @a = 2, @b=4;`) tk.MustExec(`execute stmt_test_1 using @a, @b;`) - tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("1")) + tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("0")) // The plan is from cache, the metadata lock should be added to block the DDL. ch <- struct{}{} From 751986bf920868c6d95ff4da59f8686eec78ff1a Mon Sep 17 00:00:00 2001 From: qw4990 Date: Tue, 16 Jul 2024 18:12:22 +0800 Subject: [PATCH 5/7] fixup --- pkg/planner/core/tests/prepare/prepare_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/planner/core/tests/prepare/prepare_test.go b/pkg/planner/core/tests/prepare/prepare_test.go index 1981d9f16d284..77a40d3eb8915 100644 --- a/pkg/planner/core/tests/prepare/prepare_test.go +++ b/pkg/planner/core/tests/prepare/prepare_test.go @@ -756,10 +756,12 @@ func TestPlanCacheUnionScan(t *testing.T) { tk.MustQuery("execute stmt1 using @p0").Check(testkit.Rows()) tk.MustExec("begin") tk.MustQuery("execute stmt1 using @p0").Check(testkit.Rows()) + cnt := pb.GetCounter().GetValue() + require.Equal(t, float64(0), cnt) // can't reuse the plan created outside the txn + tk.MustQuery("execute stmt1 using @p0").Check(testkit.Rows()) err := counter.Write(pb) require.NoError(t, err) - cnt := pb.GetCounter().GetValue() - require.Equal(t, float64(1), cnt) + require.Equal(t, float64(0), cnt) tk.MustExec("insert into t1 values(1)") // Cached plan is invalid now, it is not chosen and removed. tk.MustQuery("execute stmt1 using @p0").Check(testkit.Rows( @@ -790,6 +792,8 @@ func TestPlanCacheUnionScan(t *testing.T) { tk.MustQuery("execute stmt2 using @p0").Check(testkit.Rows()) tk.MustExec("begin") tk.MustQuery("execute stmt2 using @p0").Check(testkit.Rows()) + require.Equal(t, float64(3), cnt) // can't reuse the plan created outside the txn + tk.MustQuery("execute stmt2 using @p0").Check(testkit.Rows()) err = counter.Write(pb) require.NoError(t, err) cnt = pb.GetCounter().GetValue() From 3a0dcbb5785c460e3ae49dca22ce069af0ab7a64 Mon Sep 17 00:00:00 2001 From: qw4990 Date: Tue, 16 Jul 2024 18:16:19 +0800 Subject: [PATCH 6/7] fixup --- pkg/planner/core/plan_cache.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/planner/core/plan_cache.go b/pkg/planner/core/plan_cache.go index dba3c03f4e5be..71d78db328714 100644 --- a/pkg/planner/core/plan_cache.go +++ b/pkg/planner/core/plan_cache.go @@ -135,6 +135,7 @@ func planCachePreprocess(ctx context.Context, sctx sessionctx.Context, isNonPrep // Cached plan in prepared struct does NOT have a "cache key" with // schema version like prepared plan cache key stmt.PointGet.pointPlan = nil + stmt.PointGet.planCacheKey = "" stmt.PointGet.columnNames = nil stmt.PointGet.pointPlanHints = nil stmt.PointGet.Executor = nil From 4a9e0706ab7a9a826ee238674264fabbd0f7193a Mon Sep 17 00:00:00 2001 From: qw4990 Date: Tue, 16 Jul 2024 18:26:56 +0800 Subject: [PATCH 7/7] fixup --- pkg/ddl/tests/metadatalock/mdl_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/ddl/tests/metadatalock/mdl_test.go b/pkg/ddl/tests/metadatalock/mdl_test.go index d2730c2479c5a..4f5bbc7d622ed 100644 --- a/pkg/ddl/tests/metadatalock/mdl_test.go +++ b/pkg/ddl/tests/metadatalock/mdl_test.go @@ -934,8 +934,12 @@ func TestMDLPreparePlanCacheExecute(t *testing.T) { tk.MustQuery("select * from t2") tk.MustExec(`set @a = 2, @b=4;`) - tk.MustExec(`execute stmt_test_1 using @a, @b;`) + tk.MustExec(`execute stmt_test_1 using @a, @b;`) // can't reuse the prior plan created outside this txn. + tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("0")) + tk.MustExec(`execute stmt_test_1 using @a, @b;`) // can't reuse the prior plan since this table becomes dirty. tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("0")) + tk.MustExec(`execute stmt_test_1 using @a, @b;`) // can't reuse the prior plan now. + tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("1")) // The plan is from cache, the metadata lock should be added to block the DDL. ch <- struct{}{}