From 2b542e54491b39e85ddca790f98355bff383ee40 Mon Sep 17 00:00:00 2001 From: Adam Storm Date: Mon, 22 Mar 2021 10:53:37 -0400 Subject: [PATCH] sql: Don't require override for multiple MR abstractions in a transaction Previously, we required override_multi_region_zone_config to be specified if multiple add/drop region operations were included in a single transaction. This was due to the fact that the RegionConfig would get updated as part of the first operation, but the zone configurations to which we were comparing were not updated until the transaction committed. This issue was fixed in part by #62200, but this PR finishes the job by creating a function which synthesizes a zone configuration which presents the state of the RegionConfig at the beginning of the transaction. Release note: None --- .../testdata/logic_test/multi_region | 6 -- .../testdata/logic_test/regional_by_row | 12 --- pkg/ccl/multiregionccl/region_test.go | 100 +++++++++++++++--- .../multiregionccl/regional_by_row_test.go | 12 --- pkg/sql/catalog/typedesc/type_desc.go | 24 +++++ pkg/sql/region_util.go | 81 +++++++++++--- 6 files changed, 176 insertions(+), 59 deletions(-) diff --git a/pkg/ccl/logictestccl/testdata/logic_test/multi_region b/pkg/ccl/logictestccl/testdata/logic_test/multi_region index eb95d788809..176bb8ae825 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/multi_region +++ b/pkg/ccl/logictestccl/testdata/logic_test/multi_region @@ -1131,16 +1131,10 @@ txn_database_drop_regions ca-central-1 true {ca-az1,ca-az2,ca-az3} txn_database_drop_regions ap-southeast-2 false {ap-az1,ap-az2,ap-az3} txn_database_drop_regions us-east-1 false {us-az1,us-az2,us-az3} -# Overriding this operation until we get a fix for #60620. When that fix is -# ready, we can construct the view of the zone config as it was at the -# beginning of the transaction, and the checks for override should work -# again, and we won't require an explicit override here. statement ok BEGIN; -SET override_multi_region_zone_config = true; ALTER DATABASE txn_database_drop_regions DROP REGION "us-east-1"; ALTER DATABASE txn_database_drop_regions DROP REGION "ap-southeast-2"; -SET override_multi_region_zone_config = false; COMMIT; query TTBT colnames diff --git a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row index 464be5f6fa4..ed23b6dbf1f 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row +++ b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row @@ -1808,16 +1808,10 @@ CREATE TABLE regional_by_row_as ( FAMILY (cr, pk, i) ) LOCALITY REGIONAL BY ROW AS "cr"; -# Overriding this operation until we get a fix for #60620. When that fix is -# ready, we can construct the view of the zone config as it was at the -# beginning of the transaction, and the checks for override should work -# again, and we won't require an explicit override here. statement ok BEGIN; -SET override_multi_region_zone_config = true; ALTER DATABASE add_regions_in_txn ADD REGION "us-east-1"; ALTER DATABASE add_regions_in_txn ADD REGION "ap-southeast-2"; -SET override_multi_region_zone_config = false; COMMIT; @@ -2067,16 +2061,10 @@ regional_by_row_as CREATE TABLE public.regional_by_row_as ( ) LOCALITY REGIONAL BY ROW AS cr -# Overriding this operation until we get a fix for #60620. When that fix is -# ready, we can construct the view of the zone config as it was at the -# beginning of the transaction, and the checks for override should work -# again, and we won't require an explicit override here. statement ok BEGIN; -SET override_multi_region_zone_config = true; ALTER DATABASE drop_regions ADD REGION "us-east-1"; ALTER DATABASE drop_regions DROP REGION "ap-southeast-2"; -SET override_multi_region_zone_config = false; COMMIT; query TTT diff --git a/pkg/ccl/multiregionccl/region_test.go b/pkg/ccl/multiregionccl/region_test.go index cf540f6b631..ddcdcb2b8b1 100644 --- a/pkg/ccl/multiregionccl/region_test.go +++ b/pkg/ccl/multiregionccl/region_test.go @@ -24,6 +24,94 @@ import ( "github.com/stretchr/testify/require" ) +func TestConcurrentDropRegion(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 + + // 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() + } + }, + }, + } + + _, 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) + }() + + // Wait for the first DROP REGION to make it to the schema changer. + <-waitForFirstDropRegionToReachSchemaChanger + + // Issue the second DROP REGION statement. + _, err = sqlDB.Exec(`ALTER DATABASE db DROP REGION "us-east3"`) + + if err != nil { + t.Error(err) + } + + <-waitForFirstDropRegionToFinish + + // 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() + + const expectedRegion = "us-east1" + var region string + rows.Next() + if err := rows.Scan(®ion); err != nil { + t.Error(err) + } + + if region != expectedRegion { + t.Error(errors.Newf("expected region to be: %q, got %q", expectedRegion, region)) + } + + if rows.Next() { + t.Error(errors.New("unexpected number of rows returned")) + } +} + func TestSettingPrimaryRegionAmidstDrop(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) @@ -68,14 +156,8 @@ func TestSettingPrimaryRegionAmidstDrop(t *testing.T) { // read-only state. <-dropRegionStarted - // Overriding this operation until we get a fix for #60620. When that fix is - // ready, we can construct the view of the zone config as it was at the - // beginning of the transaction, and the checks for override should work - // again, and we won't require an explicit override here. _, err = sqlDB.Exec(` -SET override_multi_region_zone_config = true; ALTER DATABASE db PRIMARY REGION "us-east2"; -SET override_multi_region_zone_config = false; `) if err == nil { @@ -227,17 +309,11 @@ func TestRollbackDuringAddDropRegionAsyncJobFailure(t *testing.T) { "drop-region", `ALTER DATABASE db DROP REGION "us-east2"`, }, - // Overriding this operation until we get a fix for #60620. When that fix is - // ready, we can construct the view of the zone config as it was at the - // beginning of the transaction, and the checks for override should work - // again, and we won't require an explicit override here. { "add-drop-region-in-txn", `BEGIN; - SET override_multi_region_zone_config = true; ALTER DATABASE db DROP REGION "us-east2"; ALTER DATABASE db ADD REGION "us-east3"; - SET override_multi_region_zone_config = false; COMMIT`, }, } diff --git a/pkg/ccl/multiregionccl/regional_by_row_test.go b/pkg/ccl/multiregionccl/regional_by_row_test.go index f09a2af7df6..73aad17a37a 100644 --- a/pkg/ccl/multiregionccl/regional_by_row_test.go +++ b/pkg/ccl/multiregionccl/regional_by_row_test.go @@ -525,31 +525,19 @@ CREATE TABLE db.t(k INT PRIMARY KEY) LOCALITY REGIONAL BY ROW`) t.Error(err) } - // Overriding this operation until we get a fix for #60620. When that fix is - // ready, we can construct the view of the zone config as it was at the - // beginning of the transaction, and the checks for override should work - // again, and we won't require an explicit override here. _, err = sqlDB.Exec(`BEGIN; -SET override_multi_region_zone_config = true; ALTER DATABASE db ADD REGION "us-east3"; ALTER DATABASE db DROP REGION "us-east2"; -SET override_multi_region_zone_config = false; COMMIT;`) require.Error(t, err, "boom") // The cleanup job should kick in and revert the changes that happened to the // type descriptor in the user txn. We should eventually be able to add // "us-east3" and remove "us-east2". - // Overriding this operation until we get a fix for #60620. When that fix is - // ready, we can construct the view of the zone config as it was at the - // beginning of the transaction, and the checks for override should work - // again, and we won't require an explicit override here. testutils.SucceedsSoon(t, func() error { _, err = sqlDB.Exec(`BEGIN; - SET override_multi_region_zone_config = true; ALTER DATABASE db ADD REGION "us-east3"; ALTER DATABASE db DROP REGION "us-east2"; - SET override_multi_region_zone_config = false; COMMIT;`) return err }) diff --git a/pkg/sql/catalog/typedesc/type_desc.go b/pkg/sql/catalog/typedesc/type_desc.go index 314cd9f1c25..3c220007f23 100644 --- a/pkg/sql/catalog/typedesc/type_desc.go +++ b/pkg/sql/catalog/typedesc/type_desc.go @@ -196,6 +196,30 @@ func (desc *Immutable) RegionNames() (descpb.RegionNames, error) { return regions, nil } +// RegionNamesForZoneConfigValidation returns all regions on the multi-region +// enum to make validation with the public zone configs possible. Since the zone +// configs are only updated when a transaction commits, this must ignore all +// regions being added (since they will not be reflected in the zone +// configuration yet), but it must include all region being dropped (since they +// will not be dropped from the zone configuration until they are fully removed +// from the type descriptor, again, at the end of the transaction). +func (desc *Immutable) RegionNamesForZoneConfigValidation() (descpb.RegionNames, error) { + if desc.Kind != descpb.TypeDescriptor_MULTIREGION_ENUM { + return nil, errors.AssertionFailedf( + "can not get regions of a non multi-region enum %d", desc.ID, + ) + } + var regions descpb.RegionNames + for _, member := range desc.EnumMembers { + if member.Capability == descpb.TypeDescriptor_EnumMember_READ_ONLY && + member.Direction == descpb.TypeDescriptor_EnumMember_ADD { + continue + } + regions = append(regions, descpb.RegionName(member.LogicalRepresentation)) + } + return regions, nil +} + // RegionNamesIncludingTransitioning returns all the regions on a multi-region // enum, including `READ ONLY` regions which are in the process of transitioning. func (desc *Immutable) RegionNamesIncludingTransitioning() (descpb.RegionNames, error) { diff --git a/pkg/sql/region_util.go b/pkg/sql/region_util.go index 38be55334ce..45f71d6e9ce 100644 --- a/pkg/sql/region_util.go +++ b/pkg/sql/region_util.go @@ -888,7 +888,14 @@ func (p *planner) CurrentDatabaseRegionConfig( func SynthesizeRegionConfigOffline( ctx context.Context, txn *kv.Txn, dbID descpb.ID, descsCol *descs.Collection, ) (multiregion.RegionConfig, error) { - return synthesizeRegionConfigImpl(ctx, txn, dbID, descsCol, true /* includeOffline */) + return synthesizeRegionConfigImpl( + ctx, + txn, + dbID, + descsCol, + true, /* includeOffline */ + false, /* forZoneConfigValidate */ + ) } // SynthesizeRegionConfig is the public function for the synthesizing region @@ -897,28 +904,64 @@ func SynthesizeRegionConfigOffline( func SynthesizeRegionConfig( ctx context.Context, txn *kv.Txn, dbID descpb.ID, descsCol *descs.Collection, ) (multiregion.RegionConfig, error) { - return synthesizeRegionConfigImpl(ctx, txn, dbID, descsCol, false /* includeOffline */) + return synthesizeRegionConfigImpl( + ctx, + txn, + dbID, + descsCol, + false, /* includeOffline */ + false, /* forZoneConfigValidate */ + ) +} + +// SynthesizeRegionConfigForZoneConfigValidation returns a RegionConfig +// representing the user configured state of a multi-region database by +// coalescing state from both the database descriptor and multi-region type +// descriptor. It avoids the cache and is intended for use by DDL statements. +// Since it is intended to be called for validation of the RegionConfig against +// the current database zone configuration, it omits regions that are in the +// adding state, but includes those that are being dropped. +func SynthesizeRegionConfigForZoneConfigValidation( + ctx context.Context, txn *kv.Txn, dbID descpb.ID, descsCol *descs.Collection, +) (multiregion.RegionConfig, error) { + return synthesizeRegionConfigImpl( + ctx, + txn, + dbID, + descsCol, + false, /* includeOffline */ + true, /* forZoneConfigValidate */ + ) } -// synthesizeRegionConfigImpl returns a RegionConfig representing the user +// SynthesizeRegionConfigImpl returns a RegionConfig representing the user // configured state of a multi-region database by coalescing state from both // the database descriptor and multi-region type descriptor. It avoids the cache -// and is intended for use by DDL statements. +// and is intended for use by DDL statements. It can be called either for a +// traditional construction, which omits all regions in the non-PUBLIC state, or +// for zone configuration validation, which only omits region that are being +// added. func synthesizeRegionConfigImpl( - ctx context.Context, txn *kv.Txn, dbID descpb.ID, descsCol *descs.Collection, includeOffline bool, + ctx context.Context, + txn *kv.Txn, + dbID descpb.ID, + descsCol *descs.Collection, + includeOffline bool, + forZoneConfigValidate bool, ) (multiregion.RegionConfig, error) { + regionConfig := multiregion.RegionConfig{} _, dbDesc, err := descsCol.GetImmutableDatabaseByID(ctx, txn, dbID, tree.DatabaseLookupFlags{ AvoidCached: true, Required: true, IncludeOffline: includeOffline, }) if err != nil { - return multiregion.RegionConfig{}, err + return regionConfig, err } regionEnumID, err := dbDesc.MultiRegionEnumID() if err != nil { - return multiregion.RegionConfig{}, err + return regionConfig, err } regionEnum, err := descsCol.GetImmutableTypeByID( @@ -935,12 +978,17 @@ func synthesizeRegionConfigImpl( if err != nil { return multiregion.RegionConfig{}, err } - regionNames, err := regionEnum.RegionNames() + var regionNames descpb.RegionNames + if forZoneConfigValidate { + regionNames, err = regionEnum.RegionNamesForZoneConfigValidation() + } else { + regionNames, err = regionEnum.RegionNames() + } if err != nil { - return multiregion.RegionConfig{}, err + return regionConfig, err } - regionConfig := multiregion.MakeRegionConfig( + regionConfig = multiregion.MakeRegionConfig( regionNames, dbDesc.RegionConfig.PrimaryRegion, dbDesc.RegionConfig.SurvivalGoal, @@ -1041,7 +1089,7 @@ func (p *planner) validateZoneConfigForMultiRegionDatabaseWasNotModifiedByUser( return nil } - regionConfig, err := SynthesizeRegionConfig(ctx, p.txn, dbDesc.ID, p.Descriptors()) + regionConfig, err := SynthesizeRegionConfigForZoneConfigValidation(ctx, p.txn, dbDesc.ID, p.Descriptors()) if err != nil { return err } @@ -1067,8 +1115,7 @@ func (p *planner) validateZoneConfigForMultiRegionDatabaseWasNotModifiedByUser( ) err = errors.WithDetail(err, "the attempted operation will overwrite "+ "a user modified field") - return errors.WithHint(err, "to override this error and proceed with "+ - "the overwrite, specify \"FORCE\" at the end of the statement") + return errors.WithHint(err, "to proceed with the override, SET override_multi_region_zone_config = true, and reissue the statement") } return nil @@ -1093,8 +1140,6 @@ func (p *planner) validateZoneConfigForMultiRegionTableWasNotModifiedByUser( return nil } - hint := "to proceed with the override, SET override_multi_region_zone_config = true, and reissue the statement" - currentZoneConfig, err := getZoneConfigRaw(ctx, p.txn, p.ExecCfg().Codec, desc.GetID()) if err != nil { return err @@ -1139,7 +1184,8 @@ func (p *planner) validateZoneConfigForMultiRegionTableWasNotModifiedByUser( ) err = errors.WithDetail(err, "the attempted operation will override "+ "the index zone configuration field") - return errors.WithHint(err, hint) + return errors.WithHint(err, "to proceed with the override, SET "+ + "override_multi_region_zone_config = true, and reissue the statement") } } } @@ -1185,7 +1231,8 @@ func (p *planner) validateZoneConfigForMultiRegionTableWasNotModifiedByUser( ) err = errors.WithDetail(err, "the attempted operation will overwrite "+ "a user modified field") - return errors.WithHint(err, hint) + return errors.WithHint(err, "to proceed with the overwrite, SET "+ + "override_multi_region_zone_config = true, and reissue the statement") } return nil