Skip to content

Commit

Permalink
sql: update database zone configurations after region drop finalization
Browse files Browse the repository at this point in the history 
Previously, database level zone configurations weren't updated when the
region drop was finalized. This patch adds that support.

This patch also moves setting zone configurations on ADD REGION from
the user txn to the type schema changer. This ensures that zone
configurations are added transactionally -- if the ADD REGION fails for
whatever reason, we no longer leave behind dangling zone config.

Lastly, I've refactored some test setup code that was being duplicated
and improved the tests around rollback to test additional scenarios.

Closes cockroachdb#60435
Closes cockroachdb#60750

Release note: None

Release justification: bug fixes and low-risk updates to new
functionality
  • Loading branch information
@arulajmani
arulajmani committed Feb 24, 2021
1 parent 3bae09b commit 93fc4d0
Show file tree
Hide file tree
Showing 7 changed files with 204 additions and 171 deletions.
32 changes: 32 additions & 0 deletions pkg/ccl/logictestccl/testdata/logic_test/multi_region
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1083,3 +1083,35 @@ ALTER TABLE drop_primary_regions_db.primary SET LOCALITY REGIONAL BY TABLE IN PR

statement ok
ALTER DATABASE drop_primary_regions_db DROP REGION "ca-central-1"

statement ok
CREATE DATABASE zone_config_drop_region PRIMARY REGION "ca-central-1" REGIONS "us-east-1", "ap-southeast-2"

query TT
SHOW ZONE CONFIGURATION FOR DATABASE zone_config_drop_region
----
DATABASE zone_config_drop_region ALTER DATABASE zone_config_drop_region CONFIGURE ZONE USING
range_min_bytes = 134217728,
range_max_bytes = 536870912,
gc.ttlseconds = 90000,
num_replicas = 5,
num_voters = 3,
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
voter_constraints = '[+region=ca-central-1]',
lease_preferences = '[[+region=ca-central-1]]'

statement ok
ALTER DATABASE zone_config_drop_region DROP REGION "us-east-1"

query TT
SHOW ZONE CONFIGURATION FOR DATABASE zone_config_drop_region
----
DATABASE zone_config_drop_region ALTER DATABASE zone_config_drop_region CONFIGURE ZONE USING
range_min_bytes = 134217728,
range_max_bytes = 536870912,
gc.ttlseconds = 90000,
num_replicas = 4,
num_voters = 3,
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1}',
voter_constraints = '[+region=ca-central-1]',
lease_preferences = '[[+region=ca-central-1]]'
24 changes: 12 additions & 12 deletions pkg/ccl/logictestccl/testdata/logic_test/regional_by_row
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1657,9 +1657,9 @@ DATABASE drop_regions ALTER DATABASE drop_regions CONFIGURE ZONE USING
range_min_bytes = 134217728,
range_max_bytes = 536870912,
gc.ttlseconds = 90000,
num_replicas = 5,
num_replicas = 4,
num_voters = 3,
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1}',
voter_constraints = '[+region=ca-central-1]',
lease_preferences = '[[+region=ca-central-1]]'

Expand Down Expand Up @@ -1699,9 +1699,9 @@ DATABASE drop_regions ALTER DATABASE drop_regions CONFIGURE ZONE USING
range_min_bytes = 134217728,
range_max_bytes = 536870912,
gc.ttlseconds = 90000,
num_replicas = 5,
num_replicas = 4,
num_voters = 3,
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1}',
voter_constraints = '[+region=ca-central-1]',
lease_preferences = '[[+region=ca-central-1]]'

Expand Down Expand Up @@ -1749,9 +1749,9 @@ DATABASE drop_regions ALTER DATABASE drop_regions CONFIGURE ZONE USING
range_min_bytes = 134217728,
range_max_bytes = 536870912,
gc.ttlseconds = 90000,
num_replicas = 5,
num_replicas = 4,
num_voters = 3,
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
constraints = '{+region=ca-central-1: 1, +region=us-east-1: 1}',
voter_constraints = '[+region=ca-central-1]',
lease_preferences = '[[+region=ca-central-1]]'

Expand Down Expand Up @@ -1791,9 +1791,9 @@ DATABASE drop_regions ALTER DATABASE drop_regions CONFIGURE ZONE USING
range_min_bytes = 134217728,
range_max_bytes = 536870912,
gc.ttlseconds = 90000,
num_replicas = 5,
num_replicas = 4,
num_voters = 3,
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
constraints = '{+region=ca-central-1: 1, +region=us-east-1: 1}',
voter_constraints = '[+region=ca-central-1]',
lease_preferences = '[[+region=ca-central-1]]'

Expand Down Expand Up @@ -1831,9 +1831,9 @@ DATABASE drop_regions ALTER DATABASE drop_regions CONFIGURE ZONE USING
range_min_bytes = 134217728,
range_max_bytes = 536870912,
gc.ttlseconds = 90000,
num_replicas = 5,
num_replicas = 3,
num_voters = 3,
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
constraints = '{+region=ca-central-1: 1}',
voter_constraints = '[+region=ca-central-1]',
lease_preferences = '[[+region=ca-central-1]]'

Expand All @@ -1844,9 +1844,9 @@ DATABASE drop_regions ALTER DATABASE drop_regions CONFIGURE ZONE USING
range_min_bytes = 134217728,
range_max_bytes = 536870912,
gc.ttlseconds = 90000,
num_replicas = 5,
num_replicas = 3,
num_voters = 3,
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
constraints = '{+region=ca-central-1: 1}',
voter_constraints = '[+region=ca-central-1]',
lease_preferences = '[[+region=ca-central-1]]'

Expand Down
189 changes: 84 additions & 105 deletions pkg/ccl/multiregionccl/region_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,12 @@
package multiregionccl_test

import (
"context"
"fmt"
"testing"

"github.com/cockroachdb/cockroach/pkg/base"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/sql"
"github.com/cockroachdb/cockroach/pkg/sql/sqltestutils"
"github.com/cockroachdb/cockroach/pkg/sql/tests"
"github.com/cockroachdb/cockroach/pkg/testutils"
"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/cockroach/pkg/util/syncutil"
Expand All @@ -32,47 +27,27 @@ func TestSettingPrimaryRegionAmidstDrop(t *testing.T) {
defer log.Scope(t).Close(t)

numServers := 2
serverArgs := make(map[int]base.TestServerArgs)

regionNames := make([]string, numServers)
for i := 0; i < numServers; i++ {
// "us-east1", "us-east2"...
regionNames[i] = fmt.Sprintf("us-east%d", i+1)
}

var mu syncutil.Mutex
dropRegionStarted := make(chan struct{})
waitForPrimaryRegionSwitch := make(chan struct{})
dropRegionFinished := make(chan struct{})
for i := 0; i < numServers; i++ {
serverArgs[i] = base.TestServerArgs{
Knobs: base.TestingKnobs{
SQLTypeSchemaChanger: &sql.TypeSchemaChangerTestingKnobs{
RunBeforeEnumMemberPromotion: func() {
mu.Lock()
defer mu.Unlock()
if dropRegionStarted != nil {
close(dropRegionStarted)
<-waitForPrimaryRegionSwitch
dropRegionStarted = nil
}
},
},
knobs := base.TestingKnobs{
SQLTypeSchemaChanger: &sql.TypeSchemaChangerTestingKnobs{
RunBeforeEnumMemberPromotion: func() {
mu.Lock()
defer mu.Unlock()
if dropRegionStarted != nil {
close(dropRegionStarted)
<-waitForPrimaryRegionSwitch
dropRegionStarted = nil
}
},
Locality: roachpb.Locality{
Tiers: []roachpb.Tier{{Key: "region", Value: regionNames[i]}},
},
}
},
}

tc := serverutils.StartNewTestCluster(t, numServers, base.TestClusterArgs{
ServerArgsPerNode: serverArgs,
})

ctx := context.Background()
defer tc.Stopper().Stop(ctx)

sqlDB := tc.ServerConn(0)
_, sqlDB, cleanup := sql.CreateTestMultiRegionCluster(t, numServers, knobs)
defer cleanup()

// Setup the test.
_, err := sqlDB.Exec(`CREATE DATABASE db WITH PRIMARY REGION "us-east1" REGIONS "us-east2"`)
Expand Down Expand Up @@ -142,44 +117,41 @@ func TestDroppingPrimaryRegionAsyncJobFailure(t *testing.T) {
// Decrease the adopt loop interval so that retries happen quickly.
defer sqltestutils.SetTestJobsAdoptInterval()()

params, _ := tests.CreateTestServerParams()
params.Locality.Tiers = []roachpb.Tier{
{Key: "region", Value: "us-east-1"},
}

// Protects expectedCleanupRuns
var mu syncutil.Mutex
// We need to cleanup 2 times, once for the multi-region type descriptor and
// once for the array alias of the multi-region type descriptor.
haveWePerformedFirstRoundOfCleanup := false
cleanupFinished := make(chan struct{})
params.Knobs.SQLTypeSchemaChanger = &sql.TypeSchemaChangerTestingKnobs{
RunBeforeExec: func() error {
return errors.New("yikes")
},
RunAfterOnFailOrCancel: func() error {
mu.Lock()
defer mu.Unlock()
if haveWePerformedFirstRoundOfCleanup {
close(cleanupFinished)
}
haveWePerformedFirstRoundOfCleanup = true
return nil
knobs := base.TestingKnobs{
SQLTypeSchemaChanger: &sql.TypeSchemaChangerTestingKnobs{
RunBeforeExec: func() error {
return errors.New("yikes")
},
RunAfterOnFailOrCancel: func() error {
mu.Lock()
defer mu.Unlock()
if haveWePerformedFirstRoundOfCleanup {
close(cleanupFinished)
}
haveWePerformedFirstRoundOfCleanup = true
return nil
},
},
}

s, sqlDB, _ := serverutils.StartServer(t, params)
ctx := context.Background()
defer s.Stopper().Stop(ctx)
numServers := 1
_, sqlDB, cleanup := sql.CreateTestMultiRegionCluster(t, numServers, knobs)
defer cleanup()

// Setup the test.
_, err := sqlDB.Exec(`
CREATE DATABASE db WITH PRIMARY REGION "us-east-1";
CREATE DATABASE db WITH PRIMARY REGION "us-east1";
CREATE TABLE db.t(k INT) LOCALITY REGIONAL BY TABLE IN PRIMARY REGION;
`)
require.NoError(t, err)

_, err = sqlDB.Exec(`ALTER DATABASE db DROP REGION "us-east-1"`)
_, err = sqlDB.Exec(`ALTER DATABASE db DROP REGION "us-east1"`)
testutils.IsError(err, "yikes")

<-cleanupFinished
Expand All @@ -192,7 +164,7 @@ CREATE TABLE db.t(k INT) LOCALITY REGIONAL BY TABLE IN PRIMARY REGION;
t.Fatal("expected crdb_internal_region not to be present in system.namespace")
}

_, err = sqlDB.Exec(`ALTER DATABASE db PRIMARY REGION "us-east-1"`)
_, err = sqlDB.Exec(`ALTER DATABASE db PRIMARY REGION "us-east1"`)
require.NoError(t, err)

rows = sqlDB.QueryRow(`SELECT count(*) FROM system.namespace WHERE name = 'crdb_internal_region'`)
Expand All @@ -203,61 +175,68 @@ CREATE TABLE db.t(k INT) LOCALITY REGIONAL BY TABLE IN PRIMARY REGION;
}
}

// TODO(arul): Currently, database level zone configurations aren't rolled back.
// When we fix this limitation we should also update this test to check for that.
func TestRollbackDuringAddRegion(t *testing.T) {
// TestRollbackDuringAddDropRegionAsyncJobFailure ensures that rollback when
// an ADD REGION/DROP REGION fails asynchronously is handled appropriately.
// This also ensures that zone configuration changes on the database are
// transactional.
func TestRollbackDuringAddDropRegionAsyncJobFailure(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

// Decrease the adopt loop interval so that retries happen quickly.
defer sqltestutils.SetTestJobsAdoptInterval()()

numServers := 2
serverArgs := make(map[int]base.TestServerArgs)

regionNames := make([]string, numServers)
for i := 0; i < numServers; i++ {
// "us-east1", "us-east2"...
regionNames[i] = fmt.Sprintf("us-east%d", i+1)
}

for i := 0; i < numServers; i++ {
serverArgs[i] = base.TestServerArgs{
Knobs: base.TestingKnobs{
SQLTypeSchemaChanger: &sql.TypeSchemaChangerTestingKnobs{
RunBeforeExec: func() error {
return errors.New("boom")
},
},
numServers := 3
knobs := base.TestingKnobs{
SQLTypeSchemaChanger: &sql.TypeSchemaChangerTestingKnobs{
RunBeforeMultiRegionUpdates: func() error {
return errors.New("boom")
},
Locality: roachpb.Locality{
Tiers: []roachpb.Tier{{Key: "region", Value: regionNames[i]}},
},
}
},
}

tc := serverutils.StartNewTestCluster(t, numServers, base.TestClusterArgs{
ServerArgsPerNode: serverArgs,
})

ctx := context.Background()
defer tc.Stopper().Stop(ctx)
// Setup
_, sqlDB, cleanup := sql.CreateTestMultiRegionCluster(t, numServers, knobs)
defer cleanup()

sqlDB := tc.ServerConn(0)

_, err := sqlDB.Exec(`CREATE DATABASE db WITH PRIMARY REGION "us-east1"`)
_, err := sqlDB.Exec(`CREATE DATABASE db WITH PRIMARY REGION "us-east1" REGIONS "us-east2"`)
require.NoError(t, err)
if err != nil {
t.Error(err)
}

_, err = sqlDB.Exec(`ALTER DATABASE db ADD REGION "us-east2"`)
require.Error(t, err, "boom")
testCases := []struct {
query string
}{
{`ALTER DATABASE db ADD REGION "us-east3"`},
{`ALTER DATABASE db DROP REGION "us-east2"`},
{`BEGIN; ALTER DATABASE db DROP REGION "us-east2"; ALTER DATABASE db ADD REGION "us-east3"; COMMIT`},
}

var jobStatus string
var jobErr string
row := sqlDB.QueryRow("SELECT status, error FROM [SHOW JOBS] WHERE job_type = 'TYPEDESC SCHEMA CHANGE'")
require.NoError(t, row.Scan(&jobStatus, &jobErr))
require.Regexp(t, "boom", jobErr)
require.Regexp(t, "failed", jobStatus)
for _, tc := range testCases {
var originalZoneConfig string
res := sqlDB.QueryRow(`SELECT raw_config_sql FROM [SHOW ZONE CONFIGURATION FOR DATABASE db]`)
err = res.Scan(&originalZoneConfig)
require.NoError(t, err)

_, err = sqlDB.Exec(tc.query)
testutils.IsError(err, "boom")

var jobStatus string
var jobErr string
row := sqlDB.QueryRow("SELECT status, error FROM [SHOW JOBS] WHERE job_type = 'TYPEDESC SCHEMA CHANGE'")
require.NoError(t, row.Scan(&jobStatus, &jobErr))
require.Regexp(t, "boom", jobErr)
require.Regexp(t, "failed", jobStatus)

// Ensure the zone configuration didn't change
var newZoneConfig string
res = sqlDB.QueryRow(`SELECT raw_config_sql FROM [SHOW ZONE CONFIGURATION FOR DATABASE db]`)
err = res.Scan(&newZoneConfig)
require.NoError(t, err)

if newZoneConfig != originalZoneConfig {
t.Fatalf("expected zone config to not have changed, expected %q found %q",
originalZoneConfig,
newZoneConfig,
)
}
}
}
Loading

0 comments on commit 93fc4d0

Please sign in to comment.