From 5e5abae45f8c4a8d40dee7539be80e5b42d0abdb Mon Sep 17 00:00:00 2001 From: tiancaiamao Date: Thu, 1 Dec 2022 14:32:08 +0800 Subject: [PATCH 1/5] meta/autoid: fix AUTO_ID_CACHE 1 setting affect row id allocator performance --- executor/autoidtest/autoid_test.go | 18 ++++++++++++++++++ meta/autoid/autoid.go | 20 ++++++++++++++++---- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/executor/autoidtest/autoid_test.go b/executor/autoidtest/autoid_test.go index fd74018b162fb..63a360260bf2b 100644 --- a/executor/autoidtest/autoid_test.go +++ b/executor/autoidtest/autoid_test.go @@ -749,3 +749,21 @@ func TestMockAutoIDServiceError(t *testing.T) { // Cover a bug that the autoid client retry non-retryable errors forever cause dead loop. tk.MustExecToErr("insert into t_mock_err values (),()") // mock error, instead of dead loop } + +func TestIssue39528(t *testing.T) { + // When AUTO_ID_CACHE is 1, it should not affect row id setting when autoid and rowid are seperated. + store := testkit.CreateMockStore(t) + tk := testkit.NewTestKit(t, store) + tk.MustExec("use test;") + tk.MustExec("create table issue39528 (id int unsigned key nonclustered auto_increment) shard_row_id_bits=4 auto_id_cache 1;") + tk.MustExec("insert into issue39528 values ()") + tk.MustExec("insert into issue39528 values ()") + + // Make sure the code does not visit tikv on allocate path. + ctx := context.Background() + var codeRun bool + ctx = context.WithValue(ctx, "testIssue39528", &codeRun) + _, err := tk.ExecWithContext(ctx, "insert into issue39528 values ()") + require.NoError(t, err) + require.True(t, codeRun) +} diff --git a/meta/autoid/autoid.go b/meta/autoid/autoid.go index 2a181a088d802..d68059659607e 100644 --- a/meta/autoid/autoid.go +++ b/meta/autoid/autoid.go @@ -616,10 +616,17 @@ func NewAllocator(store kv.Storage, dbID, tbID int64, isUnsigned bool, } // Use the MySQL compatible AUTO_INCREMENT mode. - if allocType == AutoIncrementType && alloc.customStep && alloc.step == 1 && alloc.tbVersion >= model.TableInfoVersion5 { - alloc1 := newSinglePointAlloc(store, dbID, tbID, isUnsigned) - if alloc1 != nil { - return alloc1 + if alloc.customStep && alloc.step == 1 && alloc.tbVersion >= model.TableInfoVersion5 { + if allocType == AutoIncrementType { + alloc1 := newSinglePointAlloc(store, dbID, tbID, isUnsigned) + if alloc1 != nil { + return alloc1 + } + } else if allocType == RowIDAllocType { + // Now that the autoid and rowid allocator are seperated, the AUTO_ID_CACHE 1 setting should not make + // the rowid allocator do not use cache. + alloc.customStep = false + alloc.step = step } } @@ -972,6 +979,11 @@ func (alloc *allocator) alloc4Unsigned(ctx context.Context, n uint64, increment, }() } + if codeRun := ctx.Value("testIssue39528"); codeRun != nil { + *(codeRun.(*bool)) = true + return 0, 0, errors.New("mock error for test") + } + ctx = kv.WithInternalSourceType(ctx, kv.InternalTxnMeta) err := kv.RunInNewTxn(ctx, alloc.store, true, func(ctx context.Context, txn kv.Transaction) error { if span := opentracing.SpanFromContext(ctx); span != nil && span.Tracer() != nil { From e13b800ffd33174de6f666309dffd69e4d538357 Mon Sep 17 00:00:00 2001 From: tiancaiamao Date: Thu, 1 Dec 2022 14:41:34 +0800 Subject: [PATCH 2/5] fix a typo in comment --- meta/autoid/autoid.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/meta/autoid/autoid.go b/meta/autoid/autoid.go index d68059659607e..aba2ad565b617 100644 --- a/meta/autoid/autoid.go +++ b/meta/autoid/autoid.go @@ -623,7 +623,7 @@ func NewAllocator(store kv.Storage, dbID, tbID int64, isUnsigned bool, return alloc1 } } else if allocType == RowIDAllocType { - // Now that the autoid and rowid allocator are seperated, the AUTO_ID_CACHE 1 setting should not make + // Now that the autoid and rowid allocator are separated, the AUTO_ID_CACHE 1 setting should not make // the rowid allocator do not use cache. alloc.customStep = false alloc.step = step From 25d8f0222ecb9ec43a6a6af6fb98c207a7f2f363 Mon Sep 17 00:00:00 2001 From: tiancaiamao Date: Thu, 1 Dec 2022 15:02:34 +0800 Subject: [PATCH 3/5] fix a typo in comment --- executor/autoidtest/autoid_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/executor/autoidtest/autoid_test.go b/executor/autoidtest/autoid_test.go index 63a360260bf2b..4094ab525b2f9 100644 --- a/executor/autoidtest/autoid_test.go +++ b/executor/autoidtest/autoid_test.go @@ -751,7 +751,7 @@ func TestMockAutoIDServiceError(t *testing.T) { } func TestIssue39528(t *testing.T) { - // When AUTO_ID_CACHE is 1, it should not affect row id setting when autoid and rowid are seperated. + // When AUTO_ID_CACHE is 1, it should not affect row id setting when autoid and rowid are separated. store := testkit.CreateMockStore(t) tk := testkit.NewTestKit(t, store) tk.MustExec("use test;") From 1c5388a5870ef9d465f25243761d6577d9e24f09 Mon Sep 17 00:00:00 2001 From: tiancaiamao Date: Thu, 1 Dec 2022 16:00:44 +0800 Subject: [PATCH 4/5] fix typo --- executor/autoidtest/autoid_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/executor/autoidtest/autoid_test.go b/executor/autoidtest/autoid_test.go index 4094ab525b2f9..d191e52e38cdf 100644 --- a/executor/autoidtest/autoid_test.go +++ b/executor/autoidtest/autoid_test.go @@ -759,11 +759,11 @@ func TestIssue39528(t *testing.T) { tk.MustExec("insert into issue39528 values ()") tk.MustExec("insert into issue39528 values ()") - // Make sure the code does not visit tikv on allocate path. ctx := context.Background() var codeRun bool ctx = context.WithValue(ctx, "testIssue39528", &codeRun) _, err := tk.ExecWithContext(ctx, "insert into issue39528 values ()") require.NoError(t, err) - require.True(t, codeRun) + // Make sure the code does not visit tikv on allocate path. + require.False(t, codeRun) } From a5abf4085704cd43eb0d89c6b2c53c015ca28558 Mon Sep 17 00:00:00 2001 From: tiancaiamao Date: Thu, 1 Dec 2022 16:31:28 +0800 Subject: [PATCH 5/5] fix test case --- ddl/db_integration_test.go | 22 +++++----------------- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/ddl/db_integration_test.go b/ddl/db_integration_test.go index 6d2e5cf39c468..4b2468b3be9c4 100644 --- a/ddl/db_integration_test.go +++ b/ddl/db_integration_test.go @@ -3036,21 +3036,6 @@ func TestAutoIncrementForceAutoIDCache(t *testing.T) { return gid } - // Rebase _tidb_row_id. - tk.MustExec("create table t (a int) AUTO_ID_CACHE 1") - tk.MustExec("alter table t force auto_increment = 2;") - tk.MustExec("insert into t values (1),(2);") - tk.MustQuery("select a, _tidb_rowid from t;").Check(testkit.Rows("1 1", "2 2")) - // Cannot set next global ID to 0. - tk.MustExec("alter table t force auto_increment = 0;") - tk.MustExec("alter table t force auto_increment = 1;") - require.Equal(t, uint64(3), getNextGlobalID()) - // inserting new rows can overwrite the existing data. - tk.MustExec("insert into t values (3);") - tk.MustExec("insert into t values (3);") - tk.MustQuery("select a, _tidb_rowid from t;").Check(testkit.Rows("1 1", "2 2", "3 3", "3 4")) - tk.MustExec("drop table if exists t;") - // When AUTO_ID_CACHE is 1, row id and auto increment id use separate allocator, so the behaviour differs. // "Alter table t force auto_increment" has no effect on row id. tk.MustExec("create table t (a int) AUTO_ID_CACHE 1") @@ -3060,11 +3045,14 @@ func TestAutoIncrementForceAutoIDCache(t *testing.T) { // Cannot set next global ID to 0. tk.MustExec("alter table t force auto_increment = 0;") tk.MustExec("alter table t force auto_increment = 1;") - require.Equal(t, uint64(3), getNextGlobalID()) + tk.MustQuery("show table t next_row_id").Check(testkit.Rows( + "auto_inc_force t _tidb_rowid 5001 _TIDB_ROWID", + )) + // inserting new rows can overwrite the existing data. tk.MustExec("insert into t values (3);") tk.MustExec("insert into t values (3);") - tk.MustQuery("select a, _tidb_rowid from t;").Check(testkit.Rows("1 1", "2 2", "3 3", "3 4")) + tk.MustQuery("select a, _tidb_rowid from t;").Check(testkit.Rows("1 1", "2 2", "3 5001", "3 5002")) tk.MustExec("drop table if exists t;") // Rebase auto_increment.