From d83b25c8aabb98a9824948b56050b3d5485e6118 Mon Sep 17 00:00:00 2001 From: arulajmani Date: Tue, 30 Mar 2021 16:44:51 -0400 Subject: [PATCH] sql: add tests for concurrent add/drop region operations This patch generalizes the setup in what was previously `TestConcurrentDropRegion` and extends it to all combinations of add/drop region on a multi-region database. The only change is that I've added a regional by row table into the test setup mixer, so as to excercise the repartitioning semantics. Previously, there was a limitation with concurrent add/drop regions where both the operations were bound to fail in the repartitioning phase. This limitation was fixed in #60620, but we never had a regression test for it. Adding a regional by row table during the test setup serves as one. Closes #62813 Release note: None --- pkg/ccl/multiregionccl/region_test.go | 230 ++++++++++++++++++-------- pkg/sql/type_change.go | 5 - 2 files changed, 159 insertions(+), 76 deletions(-) diff --git a/pkg/ccl/multiregionccl/region_test.go b/pkg/ccl/multiregionccl/region_test.go index ddcdcb2b8b14..1bb87a9b5349 100644 --- a/pkg/ccl/multiregionccl/region_test.go +++ b/pkg/ccl/multiregionccl/region_test.go @@ -24,91 +24,179 @@ import ( "github.com/stretchr/testify/require" ) -func TestConcurrentDropRegion(t *testing.T) { +// TestConcurrentAddDropRegions tests all combinations of add/drop as if they +// were executed by two concurrent sessions. The general sketch of the test is +// as follows: +// - First operation is executed and blocks before the enum members are promoted. +// - The second operation starts once the first operation has reached the type +// schema changer. It continues to completion. It may succeed/fail depending +// on the specific test setup. +// - The first operation is resumed and allowed to complete. We expect it to +// succeed. +// - Verify database regions are as expected. +// Operations act on a multi-region database that contains a REGIONAL BY ROW +// table, so as to exercise the repartitioning semantics. +func TestConcurrentAddDropRegions(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - firstDropAtSchemaChange := false - secondDropAtSchemaChange := false - waitForFirstDropRegionToReachSchemaChanger := make(chan struct{}) - waitForSecondDropRegionToReachSchemaChanger := make(chan struct{}) - waitForFirstDropRegionToFinish := make(chan struct{}) - var mu syncutil.Mutex + skip.UnderRace(t, "times out under race") - // Pause the first drop region in the type schema changer before it does enum - // member promotion. Then release it when the second drop region gets to the - // same state. This validates that the second drop region can complete without - // requiring an override (something which was not possible before #62354). - knobs := base.TestingKnobs{ - SQLTypeSchemaChanger: &sql.TypeSchemaChangerTestingKnobs{ - RunBeforeEnumMemberPromotion: func() { - mu.Lock() - if !firstDropAtSchemaChange { - firstDropAtSchemaChange = true - close(waitForFirstDropRegionToReachSchemaChanger) - mu.Unlock() - <-waitForSecondDropRegionToReachSchemaChanger - } else if !secondDropAtSchemaChange { - secondDropAtSchemaChange = true - // We're the second DROP REGION, close the channel to free the first - // DROP REGION. - close(waitForSecondDropRegionToReachSchemaChanger) - mu.Unlock() - } - }, + testCases := []struct { + name string + firstOp string + secondOp string + expectedRegions []string + secondOpErr string + }{ + { + "concurrent-drops", + `ALTER DATABASE db DROP REGION "us-east2"`, + `ALTER DATABASE db DROP REGION "us-east3"`, + []string{"us-east1"}, + "", + }, + { + "concurrent-adds", + `ALTER DATABASE db ADD REGION "us-east4"`, + `ALTER DATABASE db ADD REGION "us-east5"`, + []string{"us-east1", "us-east2", "us-east3", "us-east4", "us-east5"}, + "", + }, + { + "concurrent-add-drop", + `ALTER DATABASE db ADD REGION "us-east4"`, + `ALTER DATABASE db DROP REGION "us-east3"`, + []string{"us-east1", "us-east2", "us-east4"}, + "", + }, + { + "concurrent-drop-add", + `ALTER DATABASE db DROP REGION "us-east2"`, + `ALTER DATABASE db ADD REGION "us-east5"`, + []string{"us-east1", "us-east3", "us-east5"}, + "", + }, + { + "concurrent-add-same-region", + `ALTER DATABASE db ADD REGION "us-east5"`, + `ALTER DATABASE db ADD REGION "us-east5"`, + []string{"us-east1", "us-east2", "us-east3", "us-east5"}, + `region "us-east5" already added to database`, + }, + { + "concurrent-drop-same-region", + `ALTER DATABASE db DROP REGION "us-east2"`, + `ALTER DATABASE db DROP REGION "us-east2"`, + []string{"us-east1", "us-east3"}, + `enum value "us-east2" is already being dropped`, + }, + { + "concurrent-add-drop-same-region", + `ALTER DATABASE db ADD REGION "us-east5"`, + `ALTER DATABASE db DROP REGION "us-east5"`, + []string{"us-east1", "us-east2", "us-east3", "us-east5"}, + `enum value "us-east5" is being added, try again later`, + }, + { + "concurrent-drop-add-same-region", + `ALTER DATABASE db DROP REGION "us-east2"`, + `ALTER DATABASE db ADD REGION "us-east2"`, + []string{"us-east1", "us-east3"}, + `enum value "us-east2" is being dropped, try again later`, }, } - _, sqlDB, cleanup := multiregionccltestutils.TestingCreateMultiRegionCluster( - t, 3 /* numServers */, knobs, nil, /* baseDir */ - ) - defer cleanup() - - _, err := sqlDB.Exec(`CREATE DATABASE db WITH PRIMARY REGION "us-east1" REGIONS "us-east2", "us-east3"`) - if err != nil { - t.Error(err) - } - - // Send the first DROP REGION statement on its way. - go func() { - if _, err := sqlDB.Exec(`ALTER DATABASE db DROP REGION "us-east2"`); err != nil { - t.Error(err) - } - close(waitForFirstDropRegionToFinish) - }() + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + firstOp := true + firstOpStarted := make(chan struct{}) + secondOpFinished := make(chan struct{}) + firstOpFinished := make(chan struct{}) + var mu syncutil.Mutex + + knobs := base.TestingKnobs{ + SQLTypeSchemaChanger: &sql.TypeSchemaChangerTestingKnobs{ + RunBeforeEnumMemberPromotion: func() { + mu.Lock() + if firstOp { + firstOp = false + close(firstOpStarted) + mu.Unlock() + // Don't promote any members before the second operation reaches + // the schema changer as well. + <-secondOpFinished + } else { + mu.Unlock() + } + }, + }, + } - // Wait for the first DROP REGION to make it to the schema changer. - <-waitForFirstDropRegionToReachSchemaChanger + _, sqlDB, cleanup := multiregionccltestutils.TestingCreateMultiRegionCluster( + t, 5 /* numServers */, knobs, nil, /* baseDir */ + ) + defer cleanup() - // Issue the second DROP REGION statement. - _, err = sqlDB.Exec(`ALTER DATABASE db DROP REGION "us-east3"`) + // Create a multi-region database with a REGIONAL BY ROW table inside of it + // which needs to be re-partitioned on add/drop operations. + _, err := sqlDB.Exec(` +CREATE DATABASE db WITH PRIMARY REGION "us-east1" REGIONS "us-east2", "us-east3"; +CREATE TABLE db.rbr () LOCALITY REGIONAL BY ROW`) + require.NoError(t, err) - if err != nil { - t.Error(err) - } + go func() { + if _, err := sqlDB.Exec(tc.firstOp); err != nil { + t.Error(err) + } + close(firstOpFinished) + }() + + // Wait for the first operation to reach the type schema changer. + <-firstOpStarted + + // Start the second operation. + _, err = sqlDB.Exec(tc.secondOp) + if tc.secondOpErr == "" { + require.NoError(t, err) + } else { + require.True(t, testutils.IsError(err, tc.secondOpErr)) + } + close(secondOpFinished) - <-waitForFirstDropRegionToFinish + <-firstOpFinished - // Validate that both DROP REGION statements completed. - rows, err := sqlDB.Query("SELECT region FROM [SHOW REGIONS FROM DATABASE db]") - if err != nil { - t.Error(err) - } - defer rows.Close() + dbRegions := make([]string, 0, len(tc.expectedRegions)) + rows, err := sqlDB.Query("SELECT region FROM [SHOW REGIONS FROM DATABASE db]") + require.NoError(t, err) + for { + done := rows.Next() + if !done { + break + } + var region string + err := rows.Scan(®ion) + require.NoError(t, err) - const expectedRegion = "us-east1" - var region string - rows.Next() - if err := rows.Scan(®ion); err != nil { - t.Error(err) - } + dbRegions = append(dbRegions, region) + } - if region != expectedRegion { - t.Error(errors.Newf("expected region to be: %q, got %q", expectedRegion, region)) - } + if len(dbRegions) != len(tc.expectedRegions) { + t.Fatalf("unexpected number of regions, expected: %v found %v", + tc.expectedRegions, + dbRegions, + ) + } - if rows.Next() { - t.Error(errors.New("unexpected number of rows returned")) + for i, expectedRegion := range tc.expectedRegions { + if expectedRegion != dbRegions[i] { + t.Fatalf("unexpected regions, expected: %v found %v", + tc.expectedRegions, + dbRegions, + ) + } + } + }) } } @@ -164,7 +252,7 @@ ALTER DATABASE db PRIMARY REGION "us-east2"; t.Fatalf("expected error, found nil") } if !testutils.IsError(err, `"us-east2" has not been added to the database`) { - t.Fatalf(`expected err, got %v`, err) + t.Fatalf(`expected secondOpErr, got %v`, err) } close(waitForPrimaryRegionSwitch) diff --git a/pkg/sql/type_change.go b/pkg/sql/type_change.go index 71f9274f1335..5ece1344834a 100644 --- a/pkg/sql/type_change.go +++ b/pkg/sql/type_change.go @@ -459,11 +459,6 @@ func performMultiRegionFinalization( // all REGIONAL BY ROW tables in the enclosing database such that there is a // partition and corresponding zone configuration for all PUBLIC enum members // (regions). -// This currently doesn't work too well if there are READ ONLY member on the -// type. This is because we create the partitioning clause based on the regions -// on the database descriptor, which may include the READ ONLY member, and -// partitioning on a READ ONLY member doesn't work. This will go away once -// https://github.com/cockroachdb/cockroach/issues/60620 is fixed. func repartitionRegionalByRowTables( ctx context.Context, typeDesc *typedesc.Immutable,