From a70d78f7697c678bbb835e2c861586c0754e303a Mon Sep 17 00:00:00 2001 From: Chengxiong Ruan Date: Sat, 24 Jun 2023 17:13:51 -0400 Subject: [PATCH] sql: re-enable TestDropTableDeleteData This commit re-enable TestDropTableDeleteData test by utilizing the mvcc gc queue settings. Informs: #85876 Release note: None --- pkg/sql/drop_test.go | 74 +++++++++++++-------------------------- pkg/sql/tests/BUILD.bazel | 2 ++ pkg/sql/tests/data.go | 39 +++++++++++++++++++++ 3 files changed, 66 insertions(+), 49 deletions(-) diff --git a/pkg/sql/drop_test.go b/pkg/sql/drop_test.go index b75d6431e6ae..c0deabbd2b95 100644 --- a/pkg/sql/drop_test.go +++ b/pkg/sql/drop_test.go @@ -14,10 +14,10 @@ import ( "context" gosql "database/sql" "fmt" - "math/rand" "net/url" "strings" "testing" + "time" "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/config/zonepb" @@ -679,19 +679,19 @@ func TestDropTable(t *testing.T) { func TestDropTableDeleteData(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - skip.WithIssue(t, 85876) params, _ := tests.CreateTestServerParams() - - defer gcjob.SetSmallMaxGCIntervalForTest()() + // Speed up mvcc queue scan. + params.ScanMaxIdleTime = time.Millisecond s, sqlDB, kvDB := serverutils.StartServer(t, params) defer s.Stopper().Stop(context.Background()) - ctx := context.Background() + + _, err := sqlDB.Exec(`SET CLUSTER SETTING sql.gc_job.wait_for_gc.interval = '1s';`) + require.NoError(t, err) // Disable strict GC TTL enforcement because we're going to shove a zero-value // TTL into the system with AddImmediateGCZoneConfig. defer sqltestutils.DisableGCTTLStrictEnforcement(t, sqlDB)() - sqltestutils.SetShortRangeFeedIntervals(t, sqlDB) const numRows = 2*row.TableTruncateChunkSize + 1 const numKeys = 3 * numRows @@ -723,7 +723,14 @@ func TestDropTableDeleteData(t *testing.T) { tableSpan := descs[i].TableSpan(keys.SystemSQLCodec) tests.CheckKeyCount(t, kvDB, tableSpan, numKeys) - if _, err := sqlDB.Exec(fmt.Sprintf(`DROP TABLE t.%s`, tableName)); err != nil { + + if _, err := sqltestutils.AddImmediateGCZoneConfig(sqlDB, descs[i].GetID()); err != nil { + t.Fatal(err) + } + } + + for i := 0; i < numTables; i++ { + if _, err := sqlDB.Exec(fmt.Sprintf(`DROP TABLE t.%s`, descs[i].GetName())); err != nil { t.Fatal(err) } } @@ -735,54 +742,45 @@ func TestDropTableDeleteData(t *testing.T) { t.Fatal(err) } tableSpan := descs[i].TableSpan(keys.SystemSQLCodec) - tests.CheckKeyCount(t, kvDB, tableSpan, numKeys) + tests.CheckKeyCountIncludingTombstoned(t, s, tableSpan, numKeys) - if err := jobutils.VerifySystemJob(t, sqlRun, i, + if err := jobutils.VerifySystemJob(t, sqlRun, numTables+i, jobspb.TypeNewSchemaChange, jobs.StatusSucceeded, jobs.Record{ Username: username.RootUserName(), Description: fmt.Sprintf(`DROP TABLE t.public.%s`, descs[i].GetName()), - DescriptorIDs: descpb.IDs{ - descs[i].GetID(), - }, }); err != nil { t.Fatal(err) } } - // The closure pushes a zone config reducing the TTL to 0 for descriptor i. - pushZoneCfg := func(i int) { - if _, err := sqltestutils.AddImmediateGCZoneConfig(sqlDB, descs[i].GetID()); err != nil { - t.Fatal(err) - } - } + // Refresh protected timestamp cache immediately to make MVCC GC queue to + // process GC immediately. + _, err = sqlDB.Exec(`SET CLUSTER SETTING kv.protectedts.poll_interval = '1s';`) + require.NoError(t, err) checkTableGCed := func(i int) { testutils.SucceedsSoon(t, func() error { if err := descExists(sqlDB, false, descs[i].GetID()); err != nil { return err } - return zoneExists(sqlDB, nil, descs[i].GetID()) }) + tableSpan := descs[i].TableSpan(keys.SystemSQLCodec) - tests.CheckKeyCount(t, kvDB, tableSpan, 0) + tests.CheckKeyCountIncludingTombstoned(t, s, tableSpan, 0) // Ensure that the job is marked as succeeded. - if err := jobutils.VerifySystemJob(t, sqlRun, i, + if err := jobutils.VerifySystemJob(t, sqlRun, numTables+i, jobspb.TypeNewSchemaChange, jobs.StatusSucceeded, jobs.Record{ Username: username.RootUserName(), Description: fmt.Sprintf(`DROP TABLE t.public.%s`, descs[i].GetName()), - DescriptorIDs: descpb.IDs{ - descs[i].GetID(), - }, }); err != nil { t.Fatal(err) } - // Ensure that the job is marked as succeeded. - // Note that this 2*i+1 nonsense comes + // Ensure that the gc job is marked as succeeded. testutils.SucceedsSoon(t, func() error { - return jobutils.VerifySystemJob(t, sqlRun, 2*i+1, + return jobutils.VerifySystemJob(t, sqlRun, numTables+i, jobspb.TypeSchemaChangeGC, jobs.StatusSucceeded, jobs.Record{ Username: username.NodeUserName(), Description: fmt.Sprintf(`GC for DROP TABLE t.public.%s`, descs[i].GetName()), @@ -793,29 +791,7 @@ func TestDropTableDeleteData(t *testing.T) { }) } - // Push a new zone config for a few tables with TTL=0 so the data - // is deleted immediately. - barrier := rand.Intn(numTables) - for i := 0; i < barrier; i++ { - pushZoneCfg(i) - } - - // Check GC worked! for i := 0; i < numTables; i++ { - if i < barrier { - checkTableGCed(i) - } else { - // Data still present for tables past barrier. - tableSpan := descs[i].TableSpan(keys.SystemSQLCodec) - tests.CheckKeyCount(t, kvDB, tableSpan, numKeys) - } - } - - // Push the rest of the zone configs and check all the data gets GC-ed. - for i := barrier; i < numTables; i++ { - pushZoneCfg(i) - } - for i := barrier; i < numTables; i++ { checkTableGCed(i) } } diff --git a/pkg/sql/tests/BUILD.bazel b/pkg/sql/tests/BUILD.bazel index 8c505338ef28..2b5ac6b8109c 100644 --- a/pkg/sql/tests/BUILD.bazel +++ b/pkg/sql/tests/BUILD.bazel @@ -20,6 +20,8 @@ go_library( "//pkg/roachpb", "//pkg/sql/sem/tree", "//pkg/sql/sqlstats", + "//pkg/storage", + "//pkg/testutils/serverutils", "//pkg/testutils/storageutils", "//pkg/util/syncutil", "@com_github_cockroachdb_errors//:errors", diff --git a/pkg/sql/tests/data.go b/pkg/sql/tests/data.go index 5d4634e3120d..959235e61346 100644 --- a/pkg/sql/tests/data.go +++ b/pkg/sql/tests/data.go @@ -19,6 +19,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/storage" + "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/errors" ) @@ -31,6 +33,43 @@ func CheckKeyCount(t *testing.T, kvDB *kv.DB, span roachpb.Span, numKeys int) { } } +// CheckKeyCountIncludingTombstoned checks that the number of keys (including +// those whose tombstones are marked but not GC'ed yet) in the provided span +// matches the expected number. +func CheckKeyCountIncludingTombstoned( + t *testing.T, s serverutils.TestServerInterface, tableSpan roachpb.Span, expectedNum int, +) { + // Check key count including tombstoned ones. + engines := s.Engines() + if len(engines) != 1 { + t.Fatalf("expecting 1 engine from the test server, but found %d", len(engines)) + } + + keyCount := 0 + it := engines[0].NewMVCCIterator( + storage.MVCCKeyIterKind, + storage.IterOptions{ + LowerBound: tableSpan.Key, + UpperBound: tableSpan.EndKey, + }, + ) + + for it.SeekGE(storage.MVCCKey{Key: tableSpan.Key}); ; it.NextKey() { + ok, err := it.Valid() + if err != nil { + t.Fatal(err) + } + if !ok { + break + } + keyCount++ + } + it.Close() + if keyCount != expectedNum { + t.Fatalf("expecting %d keys, but found %d", expectedNum, keyCount) + } +} + // CheckKeyCountE returns an error if the the number of keys in the // provided span does not match numKeys. func CheckKeyCountE(t *testing.T, kvDB *kv.DB, span roachpb.Span, numKeys int) error {