From fc362ff52841678af1ef5bc90fbf50ef9bd28f48 Mon Sep 17 00:00:00 2001 From: wjhuang2016 Date: Wed, 4 Dec 2024 13:45:12 +0800 Subject: [PATCH 1/5] init Signed-off-by: wjhuang2016 --- pkg/ddl/tests/metadatalock/BUILD.bazel | 4 +- pkg/ddl/tests/metadatalock/mdl_test.go | 109 +++++++++++++++++++++++++ pkg/planner/core/plan_cache.go | 8 +- 3 files changed, 119 insertions(+), 2 deletions(-) diff --git a/pkg/ddl/tests/metadatalock/BUILD.bazel b/pkg/ddl/tests/metadatalock/BUILD.bazel index bc8fc8a444520..e0439b0fea460 100644 --- a/pkg/ddl/tests/metadatalock/BUILD.bazel +++ b/pkg/ddl/tests/metadatalock/BUILD.bazel @@ -8,16 +8,18 @@ go_test( "mdl_test.go", ], flaky = True, - shard_count = 36, + shard_count = 37, deps = [ "//pkg/config", "//pkg/ddl", "//pkg/ddl/ingest/testutil", "//pkg/errno", + "//pkg/meta/model", "//pkg/server", "//pkg/testkit", "//pkg/testkit/testfailpoint", "//pkg/testkit/testsetup", + "@com_github_pingcap_failpoint//:failpoint", "@com_github_stretchr_testify//require", "@org_uber_go_goleak//:goleak", ], diff --git a/pkg/ddl/tests/metadatalock/mdl_test.go b/pkg/ddl/tests/metadatalock/mdl_test.go index 262cb745e7a63..7c9d562314ca8 100644 --- a/pkg/ddl/tests/metadatalock/mdl_test.go +++ b/pkg/ddl/tests/metadatalock/mdl_test.go @@ -15,13 +15,17 @@ package metadatalocktest import ( + "context" "fmt" "sync" "testing" "time" + "github.com/pingcap/failpoint" + "github.com/pingcap/tidb/pkg/ddl" ingesttestutil "github.com/pingcap/tidb/pkg/ddl/ingest/testutil" mysql "github.com/pingcap/tidb/pkg/errno" + "github.com/pingcap/tidb/pkg/meta/model" "github.com/pingcap/tidb/pkg/server" "github.com/pingcap/tidb/pkg/testkit" "github.com/pingcap/tidb/pkg/testkit/testfailpoint" @@ -997,6 +1001,111 @@ func TestMDLPreparePlanCacheExecute2(t *testing.T) { tk.MustExec("admin check table t") } +func TestMDLPreparePlanCacheExecuteInsert(t *testing.T) { + store, dom := testkit.CreateMockStoreAndDomain(t) + defer ingesttestutil.InjectMockBackendMgr(t, store)() + + sv := server.CreateMockServer(t, store) + + sv.SetDomain(dom) + dom.InfoSyncer().SetSessionManager(sv) + defer sv.Close() + + conn1 := server.CreateMockConn(t, sv) + tk := testkit.NewTestKitWithSession(t, store, conn1.Context().Session) + conn2 := server.CreateMockConn(t, sv) + tkDDL := testkit.NewTestKitWithSession(t, store, conn2.Context().Session) + conn3 := server.CreateMockConn(t, sv) + tk3 := testkit.NewTestKitWithSession(t, store, conn3.Context().Session) + tk.MustExec("use test") + tk.MustExec("set global tidb_enable_metadata_lock=1") + tk.MustExec("create table t(a int primary key, b int);") + tk.MustExec("create table t2(a int);") + tk.MustExec("insert into t values(1, 1), (2, 2), (3, 3), (4, 4);") + + tk.MustExec(`begin`) + tk.MustExec(`prepare select_stmt from 'select * from t where a = ?';`) + tk.MustExec(`prepare delete_stmt from 'delete from t where a = ?'`) + tk.MustExec(`prepare insert_stmt from 'insert into t values (?, ?)'`) + tk.MustExec(`commit`) + + tk.MustExec(`begin`) + tk.MustExec(`set @a = 4, @b= 4;`) + tk.MustExec(`execute select_stmt using @a;`) + tk.MustExec(`execute delete_stmt using @a;`) + tk.MustExec(`execute insert_stmt using @a, @b;`) + tk.MustExec(`commit`) + + tk.MustExec("begin") + + ch := make(chan struct{}) + + first := true + testfailpoint.EnableCall(t, "github.com/pingcap/tidb/pkg/ddl/onJobUpdated", func(job *model.Job) { + switch job.SchemaState { + case model.StateWriteReorganization: + tbl, _ := dom.InfoSchema().TableByID(context.Background(), job.TableID) + idx := tbl.Meta().FindIndexByName("idx") + switch idx.BackfillState { + case model.BackfillStateRunning: + if first { + tk.MustExec(`begin`) + tk.MustExec(`set @a=4;`) + tk.MustExec(`execute select_stmt using @a;`) + tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("0")) + tk.MustExec(`set @a=9;`) + tk.MustExec(`execute delete_stmt using @a;`) + tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("0")) + tk.MustExec(`set @a=6, @b=4;`) + tk.MustExec(`execute insert_stmt using @a, @b;`) + tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("0")) + tk.MustExec(`commit`) + tk.MustExec(`begin`) + tk.MustExec(`set @a=4;`) + tk.MustExec(`execute select_stmt using @a;`) + tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("1")) + tk.MustExec(`set @a=4;`) + tk.MustExec(`execute delete_stmt using @a;`) + tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("1")) + tk.MustExec(`set @a=4, @b=4;`) + tk.MustExec(`execute insert_stmt using @a, @b;`) + tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("0")) + tk.MustExec(`commit`) + + tk.MustExec("begin") + tk.MustExec("select * from t2") + first = false + tk3.MustExec("insert into test.t values(10000, 1000)") + return + } + } + } + }) + + ddl.MockDMLExecutionMerging = func() { + tk.MustExec(`execute delete_stmt using @a;`) + tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("0")) + tk.MustExec(`execute insert_stmt using @a, @b;`) + //tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("0")) + tk.MustExec("commit") + } + require.NoError(t, failpoint.Enable("github.com/pingcap/tidb/pkg/ddl/mockDMLExecutionMerging", "1*return(true)->return(false)")) + + var wg sync.WaitGroup + wg.Add(1) + go func() { + <-ch + tkDDL.MustExec("alter table test.t add index idx(a);") + wg.Done() + }() + + ch <- struct{}{} + + wg.Wait() + + tk.MustExec("admin check table t") +} + func TestMDLDisable2Enable(t *testing.T) { store, dom := testkit.CreateMockStoreAndDomain(t) sv := server.CreateMockServer(t, store) diff --git a/pkg/planner/core/plan_cache.go b/pkg/planner/core/plan_cache.go index 1f948b42c1d41..6b2fbd4406f12 100644 --- a/pkg/planner/core/plan_cache.go +++ b/pkg/planner/core/plan_cache.go @@ -119,11 +119,17 @@ func planCachePreprocess(ctx context.Context, sctx sessionctx.Context, isNonPrep stmt.tbls[i] = tblByName stmt.RelateVersion[tblByName.Meta().ID] = tblByName.Meta().Revision } - newTbl, err := tryLockMDLAndUpdateSchemaIfNecessary(ctx, sctx.GetPlanCtx(), stmt.dbName[i], stmt.tbls[i], is) + newTbl, err := tryLockMDLAndUpdateSchemaIfNecessary(ctx, sctx.GetPlanCtx(), stmt.dbName[i], tbl, is) if err != nil { schemaNotMatch = true continue } + // We need to keep the newTbl as the newest table info. + // If the MDL is locked by previous statement, then the tbl is the newest table info. + // We don't pass tbl to tryLockMDLAndUpdateSchemaIfNecessary for avoiding nil. + //if tbl != nil && tbl.Meta().Revision > newTbl.Meta().Revision { + // newTbl = tbl + //} // The revision of tbl and newTbl may not be the same. // Example: // The version of stmt.tbls[i] is taken from the prepare statement and is revision v1. From 4931acdc6598e4a0b3396356c81d921839a978a1 Mon Sep 17 00:00:00 2001 From: wjhuang2016 Date: Wed, 4 Dec 2024 15:42:46 +0800 Subject: [PATCH 2/5] try fix Signed-off-by: wjhuang2016 --- 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 6b2fbd4406f12..e1728fc76a779 100644 --- a/pkg/planner/core/plan_cache.go +++ b/pkg/planner/core/plan_cache.go @@ -116,6 +116,7 @@ func planCachePreprocess(ctx context.Context, sctx sessionctx.Context, isNonPrep return plannererrors.ErrSchemaChanged.GenWithStack("Schema change caused error: %s", err.Error()) } delete(stmt.RelateVersion, stmt.tbls[i].Meta().ID) + tbl = tblByName stmt.tbls[i] = tblByName stmt.RelateVersion[tblByName.Meta().ID] = tblByName.Meta().Revision } From 6a6222598657bf70cd4fca74810fe9a72b17b760 Mon Sep 17 00:00:00 2001 From: wjhuang2016 Date: Wed, 4 Dec 2024 16:35:40 +0800 Subject: [PATCH 3/5] refine Signed-off-by: wjhuang2016 --- pkg/ddl/tests/metadatalock/mdl_test.go | 7 ------- pkg/planner/core/plan_cache.go | 8 +------- 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/pkg/ddl/tests/metadatalock/mdl_test.go b/pkg/ddl/tests/metadatalock/mdl_test.go index 7c9d562314ca8..3b60cba7ecd23 100644 --- a/pkg/ddl/tests/metadatalock/mdl_test.go +++ b/pkg/ddl/tests/metadatalock/mdl_test.go @@ -1024,14 +1024,12 @@ func TestMDLPreparePlanCacheExecuteInsert(t *testing.T) { tk.MustExec("insert into t values(1, 1), (2, 2), (3, 3), (4, 4);") tk.MustExec(`begin`) - tk.MustExec(`prepare select_stmt from 'select * from t where a = ?';`) tk.MustExec(`prepare delete_stmt from 'delete from t where a = ?'`) tk.MustExec(`prepare insert_stmt from 'insert into t values (?, ?)'`) tk.MustExec(`commit`) tk.MustExec(`begin`) tk.MustExec(`set @a = 4, @b= 4;`) - tk.MustExec(`execute select_stmt using @a;`) tk.MustExec(`execute delete_stmt using @a;`) tk.MustExec(`execute insert_stmt using @a, @b;`) tk.MustExec(`commit`) @@ -1050,9 +1048,6 @@ func TestMDLPreparePlanCacheExecuteInsert(t *testing.T) { case model.BackfillStateRunning: if first { tk.MustExec(`begin`) - tk.MustExec(`set @a=4;`) - tk.MustExec(`execute select_stmt using @a;`) - tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("0")) tk.MustExec(`set @a=9;`) tk.MustExec(`execute delete_stmt using @a;`) tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("0")) @@ -1062,8 +1057,6 @@ func TestMDLPreparePlanCacheExecuteInsert(t *testing.T) { tk.MustExec(`commit`) tk.MustExec(`begin`) tk.MustExec(`set @a=4;`) - tk.MustExec(`execute select_stmt using @a;`) - tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("1")) tk.MustExec(`set @a=4;`) tk.MustExec(`execute delete_stmt using @a;`) tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("1")) diff --git a/pkg/planner/core/plan_cache.go b/pkg/planner/core/plan_cache.go index e1728fc76a779..089aae98f0a6c 100644 --- a/pkg/planner/core/plan_cache.go +++ b/pkg/planner/core/plan_cache.go @@ -125,18 +125,12 @@ func planCachePreprocess(ctx context.Context, sctx sessionctx.Context, isNonPrep schemaNotMatch = true continue } - // We need to keep the newTbl as the newest table info. - // If the MDL is locked by previous statement, then the tbl is the newest table info. - // We don't pass tbl to tryLockMDLAndUpdateSchemaIfNecessary for avoiding nil. - //if tbl != nil && tbl.Meta().Revision > newTbl.Meta().Revision { - // newTbl = tbl - //} // The revision of tbl and newTbl may not be the same. // Example: // The version of stmt.tbls[i] is taken from the prepare statement and is revision v1. // When stmt.tbls[i] is locked in MDL, the revision of newTbl is also v1. // The revision of tbl is v2. The reason may have other statements trigger "tryLockMDLAndUpdateSchemaIfNecessary" before, leading to tbl revision update. - if stmt.tbls[i].Meta().Revision != newTbl.Meta().Revision || (tbl != nil && tbl.Meta().Revision != newTbl.Meta().Revision) { + if stmt.tbls[i].Meta().Revision != newTbl.Meta().Revision { schemaNotMatch = true } stmt.tbls[i] = newTbl From 4246161effb7d8cd2097a999a9e0d596641880c5 Mon Sep 17 00:00:00 2001 From: wjhuang2016 Date: Fri, 6 Dec 2024 16:50:31 +0800 Subject: [PATCH 4/5] refine Signed-off-by: wjhuang2016 --- pkg/ddl/tests/metadatalock/mdl_test.go | 5 ++--- pkg/planner/core/plan_cache.go | 16 +++++++++------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/pkg/ddl/tests/metadatalock/mdl_test.go b/pkg/ddl/tests/metadatalock/mdl_test.go index 3b60cba7ecd23..232f10383d7c6 100644 --- a/pkg/ddl/tests/metadatalock/mdl_test.go +++ b/pkg/ddl/tests/metadatalock/mdl_test.go @@ -1001,6 +1001,7 @@ func TestMDLPreparePlanCacheExecute2(t *testing.T) { tk.MustExec("admin check table t") } +// TestMDLPreparePlanCacheExecuteInsert makes sure the insert statement handle the schema correctly in plan cache. func TestMDLPreparePlanCacheExecuteInsert(t *testing.T) { store, dom := testkit.CreateMockStoreAndDomain(t) defer ingesttestutil.InjectMockBackendMgr(t, store)() @@ -1057,7 +1058,6 @@ func TestMDLPreparePlanCacheExecuteInsert(t *testing.T) { tk.MustExec(`commit`) tk.MustExec(`begin`) tk.MustExec(`set @a=4;`) - tk.MustExec(`set @a=4;`) tk.MustExec(`execute delete_stmt using @a;`) tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("1")) tk.MustExec(`set @a=4, @b=4;`) @@ -1066,6 +1066,7 @@ func TestMDLPreparePlanCacheExecuteInsert(t *testing.T) { tk.MustExec(`commit`) tk.MustExec("begin") + // Activate txn. tk.MustExec("select * from t2") first = false tk3.MustExec("insert into test.t values(10000, 1000)") @@ -1079,7 +1080,6 @@ func TestMDLPreparePlanCacheExecuteInsert(t *testing.T) { tk.MustExec(`execute delete_stmt using @a;`) tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("0")) tk.MustExec(`execute insert_stmt using @a, @b;`) - //tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("0")) tk.MustExec("commit") } require.NoError(t, failpoint.Enable("github.com/pingcap/tidb/pkg/ddl/mockDMLExecutionMerging", "1*return(true)->return(false)")) @@ -1093,7 +1093,6 @@ func TestMDLPreparePlanCacheExecuteInsert(t *testing.T) { }() ch <- struct{}{} - wg.Wait() tk.MustExec("admin check table t") diff --git a/pkg/planner/core/plan_cache.go b/pkg/planner/core/plan_cache.go index 089aae98f0a6c..d005bed7148ae 100644 --- a/pkg/planner/core/plan_cache.go +++ b/pkg/planner/core/plan_cache.go @@ -16,6 +16,8 @@ package core import ( "context" + "go.uber.org/zap" + "math" "time" "github.com/pingcap/errors" @@ -39,6 +41,7 @@ import ( "github.com/pingcap/tidb/pkg/util/dbterror/plannererrors" "github.com/pingcap/tidb/pkg/util/hint" "github.com/pingcap/tidb/pkg/util/intest" + "github.com/pingcap/tidb/pkg/util/logutil" ) // PlanCacheKeyTestIssue43667 is only for test. @@ -115,24 +118,23 @@ func planCachePreprocess(ctx context.Context, sctx sessionctx.Context, isNonPrep if err != nil { return plannererrors.ErrSchemaChanged.GenWithStack("Schema change caused error: %s", err.Error()) } + // Table ID is changed, for example, drop & create table, truncate table. delete(stmt.RelateVersion, stmt.tbls[i].Meta().ID) tbl = tblByName - stmt.tbls[i] = tblByName - stmt.RelateVersion[tblByName.Meta().ID] = tblByName.Meta().Revision } + // newTbl is the 'should be used' table info for this execution. newTbl, err := tryLockMDLAndUpdateSchemaIfNecessary(ctx, sctx.GetPlanCtx(), stmt.dbName[i], tbl, is) if err != nil { + logutil.BgLogger().Warn("meet error during tryLockMDLAndUpdateSchemaIfNecessary", zap.String("table name", tbl.Meta().Name.String()), zap.Error(err)) + // Invalid the cache key related fields to avoid using plan cache. + stmt.RelateVersion[tbl.Meta().ID] = math.MaxUint64 schemaNotMatch = true continue } - // The revision of tbl and newTbl may not be the same. - // Example: - // The version of stmt.tbls[i] is taken from the prepare statement and is revision v1. - // When stmt.tbls[i] is locked in MDL, the revision of newTbl is also v1. - // The revision of tbl is v2. The reason may have other statements trigger "tryLockMDLAndUpdateSchemaIfNecessary" before, leading to tbl revision update. if stmt.tbls[i].Meta().Revision != newTbl.Meta().Revision { schemaNotMatch = true } + // Update the cache key related fields. stmt.tbls[i] = newTbl stmt.RelateVersion[newTbl.Meta().ID] = newTbl.Meta().Revision } From c965705f98e7dfcb84396d75c1bd2ed2e588bbe7 Mon Sep 17 00:00:00 2001 From: wjhuang2016 Date: Fri, 6 Dec 2024 16:51:37 +0800 Subject: [PATCH 5/5] refine Signed-off-by: wjhuang2016 --- pkg/planner/core/plan_cache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/planner/core/plan_cache.go b/pkg/planner/core/plan_cache.go index d005bed7148ae..a9091d51b47dd 100644 --- a/pkg/planner/core/plan_cache.go +++ b/pkg/planner/core/plan_cache.go @@ -16,7 +16,6 @@ package core import ( "context" - "go.uber.org/zap" "math" "time" @@ -42,6 +41,7 @@ import ( "github.com/pingcap/tidb/pkg/util/hint" "github.com/pingcap/tidb/pkg/util/intest" "github.com/pingcap/tidb/pkg/util/logutil" + "go.uber.org/zap" ) // PlanCacheKeyTestIssue43667 is only for test.