diff --git a/pkg/sql/alter_database.go b/pkg/sql/alter_database.go index e43317d8805e..3ca09f834f44 100644 --- a/pkg/sql/alter_database.go +++ b/pkg/sql/alter_database.go @@ -21,6 +21,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv" "github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/roleoption" @@ -249,8 +250,10 @@ func (n *alterDatabaseAddRegionNode) Values() tree.Datums { return tree func (n *alterDatabaseAddRegionNode) Close(context.Context) {} type alterDatabaseDropRegionNode struct { - n *tree.AlterDatabaseDropRegion - desc *dbdesc.Mutable + n *tree.AlterDatabaseDropRegion + desc *dbdesc.Mutable + removingPrimaryRegion bool + toDrop []*typedesc.Mutable } // AlterDatabaseDropRegion transforms a tree.AlterDatabaseDropRegion into a plan node. @@ -280,15 +283,92 @@ func (p *planner) AlterDatabaseDropRegion( return nil, pgerror.New(pgcode.InvalidDatabaseDefinition, "database has no regions to drop") } + removingPrimaryRegion := false + var toDrop []*typedesc.Mutable + if dbDesc.RegionConfig.PrimaryRegion == descpb.RegionName(n.Region) { - return nil, errors.WithHintf( - errors.Newf("cannot drop region %q", dbDesc.RegionConfig.PrimaryRegion), - "You must designate another region as the primary region or remove all "+ - "other regions before attempting to drop region %q", n.Region, - ) + removingPrimaryRegion = true + + typeID, err := dbDesc.MultiRegionEnumID() + if err != nil { + return nil, err + } + typeDesc, err := p.Descriptors().GetMutableTypeVersionByID(ctx, p.txn, typeID) + if err != nil { + return nil, err + } + regions, err := typeDesc.RegionNames() + if err != nil { + return nil, err + } + if len(regions) != 1 { + return nil, errors.WithHintf( + errors.Newf("cannot drop region %q", dbDesc.RegionConfig.PrimaryRegion), + "You must designate another region as the primary region using "+ + "ALTER DATABASE %s SET PRIMARY REGION or remove all other regions before "+ + "attempting to drop region %q", dbDesc.GetName(), n.Region, + ) + } + + // When the last region is removed from the database, we also clean up + // detritus multi-region type descriptor. This includes both the + // type descriptor and its array counterpart. + toDrop = append(toDrop, typeDesc) + arrayTypeDesc, err := p.Descriptors().GetMutableTypeVersionByID(ctx, p.txn, typeDesc.ArrayTypeID) + if err != nil { + return nil, err + } + toDrop = append(toDrop, arrayTypeDesc) + for _, desc := range toDrop { + // canDropTypeDesc ensures that there are no references to tables on the + // type descriptor. This is what we expect when dropping the final + // (primary) region from a database, as REGIONAL BY ROW tables and + // REGIONAL BY TABLE tables (homed explicitly in the final region) will + // store a reference on the type descriptor. It is sufficient to simply + // check for stored references and not go through the validation that + // happens in the type_schema changer in this scenario. + if err := p.canDropTypeDesc(ctx, desc, tree.DropRestrict); err != nil { + return nil, errors.Wrapf( + err, "error removing primary region from database %s", dbDesc.Name) + } + } } - return &alterDatabaseDropRegionNode{n, dbDesc}, nil + return &alterDatabaseDropRegionNode{ + n, + dbDesc, + removingPrimaryRegion, + toDrop, + }, nil +} + +// removeLocalityConfigFromAllTablesInDB removes the locality config from all +// tables under the supplied database. +func removeLocalityConfigFromAllTablesInDB( + ctx context.Context, p *planner, desc *dbdesc.Immutable, +) error { + if !desc.IsMultiRegion() { + return errors.AssertionFailedf( + "cannot remove locality configs from tables in non multi-region database with ID %d", + desc.GetID(), + ) + } + b := p.Txn().NewBatch() + if err := forEachTableDesc(ctx, p, desc, hideVirtual, + func(immutable *dbdesc.Immutable, _ string, desc catalog.TableDescriptor) error { + mutDesc, err := p.Descriptors().GetMutableTableByID(ctx, p.txn, desc.GetID(), tree.ObjectLookupFlags{}) + if err != nil { + return err + } + mutDesc.LocalityConfig = nil + if err := p.writeSchemaChangeToBatch(ctx, mutDesc, b); err != nil { + return err + } + return nil + }); err != nil { + return err + } + return p.Txn().Run(ctx, b) } func (n *alterDatabaseDropRegionNode) startExec(params runParams) error { @@ -301,36 +381,54 @@ func (n *alterDatabaseDropRegionNode) startExec(params runParams) error { return err } - // dropEnumValue tries to remove the region value from the multi-region type - // descriptor. Among other things, it validates that the region is not in - // use by any tables. A region is considered "in use" if either a REGIONAL BY - // TABLE table is explicitly homed in that region or a row in a REGIONAL BY - // ROW table is homed in that region. The type schema changer is responsible - // for all the requisite validation. - if err := params.p.dropEnumValue(params.ctx, typeDesc, tree.EnumValue(n.n.Region)); err != nil { - return err - } + if n.removingPrimaryRegion { + for _, desc := range n.toDrop { + jobDesc := fmt.Sprintf("drop multi-region enum with ID %d", desc.ID) + err := params.p.dropTypeImpl(params.ctx, desc, jobDesc, true /* queueJob */) + if err != nil { + return err + } + } - idx := 0 - found := false - for i, region := range n.desc.RegionConfig.Regions { - if region.Name == descpb.RegionName(n.n.Region) { - idx = i - found = true - break + err = removeLocalityConfigFromAllTablesInDB(params.ctx, params.p, &n.desc.Immutable) + if err != nil { + return errors.Wrap(err, "error removing locality configs from tables") + } + + n.desc.UnsetMultiRegionConfig() + } else { + // dropEnumValue tries to remove the region value from the multi-region type + // descriptor. Among other things, it validates that the region is not in + // use by any tables. A region is considered "in use" if either a REGIONAL BY + // TABLE table is explicitly homed in that region or a row in a REGIONAL BY + // ROW table is homed in that region. The type schema changer is responsible + // for all the requisite validation. + if err := params.p.dropEnumValue(params.ctx, typeDesc, tree.EnumValue(n.n.Region)); err != nil { + return err + } + + // Remove the region from the database descriptor as well. + idx := 0 + found := false + for i, region := range n.desc.RegionConfig.Regions { + if region.Name == descpb.RegionName(n.n.Region) { + idx = i + found = true + break + } } - } - if !found { - // This shouldn't happen and is simply a sanity check to ensure the database - // descriptor regions and multi-region enum regions are indeed consistent. - return errors.AssertionFailedf( - "attempting to drop region %s not on database descriptor %d but found on type descriptor", - n.n.Region, n.desc.GetID(), - ) - } - n.desc.RegionConfig.Regions = append(n.desc.RegionConfig.Regions[:idx], - n.desc.RegionConfig.Regions[idx+1:]...) + if !found { + // This shouldn't happen and is simply a sanity check to ensure the database + // descriptor regions and multi-region enum regions are indeed consistent. + return errors.AssertionFailedf( + "attempting to drop region %s not on database descriptor %d but found on type descriptor", + n.n.Region, n.desc.GetID(), + ) + } + n.desc.RegionConfig.Regions = append(n.desc.RegionConfig.Regions[:idx], + n.desc.RegionConfig.Regions[idx+1:]...) + } if err := params.p.writeNonDropDatabaseChange( params.ctx, diff --git a/pkg/sql/catalog/dbdesc/database_desc.go b/pkg/sql/catalog/dbdesc/database_desc.go index caf54be4ec9b..e9c5b9958a4f 100644 --- a/pkg/sql/catalog/dbdesc/database_desc.go +++ b/pkg/sql/catalog/dbdesc/database_desc.go @@ -387,3 +387,9 @@ func (desc *Mutable) SetOffline(reason string) { func (desc *Mutable) AddDrainingName(name descpb.NameInfo) { desc.DrainingNames = append(desc.DrainingNames, name) } + +// UnsetMultiRegionConfig removes the stored multi-region config from the +// database descriptor. +func (desc *Mutable) UnsetMultiRegionConfig() { + desc.RegionConfig = nil +} diff --git a/pkg/sql/logictest/testdata/logic_test/multiregion b/pkg/sql/logictest/testdata/logic_test/multiregion index 7e0575cee2dc..72f63ad8355f 100644 --- a/pkg/sql/logictest/testdata/logic_test/multiregion +++ b/pkg/sql/logictest/testdata/logic_test/multiregion @@ -841,7 +841,7 @@ ALTER DATABASE non_multi_region_db DROP REGION "ca-central-1" statement ok CREATE DATABASE drop_region_db PRIMARY REGION "ca-central-1" REGIONS "ap-southeast-2", "us-east-1" -statement error pq: cannot drop region "ca-central-1"\nHINT: You must designate another region as the primary region or remove all other regions before attempting to drop region "ca-central-1" +statement error pq: cannot drop region "ca-central-1"\nHINT: You must designate another region as the primary region using ALTER DATABASE drop_region_db SET PRIMARY REGION or remove all other regions before attempting to drop region "ca-central-1" ALTER DATABASE drop_region_db DROP REGION "ca-central-1" statement ok @@ -891,7 +891,7 @@ CREATE TABLE start_off_non_multi_region.public.t(a INT); ALTER DATABASE start_off_non_multi_region PRIMARY REGION "ca-central-1"; ALTER DATABASE start_off_non_multi_region ADD REGION "ap-southeast-2" -statement error pq: cannot drop region "ca-central-1"\nHINT: You must designate another region as the primary region or remove all other regions before attempting to drop region "ca-central-1" +statement error pq: cannot drop region "ca-central-1"\nHINT: You must designate another region as the primary region using ALTER DATABASE start_off_non_multi_region SET PRIMARY REGION or remove all other regions before attempting to drop region "ca-central-1" ALTER DATABASE start_off_non_multi_region DROP REGION "ca-central-1" statement ok @@ -914,7 +914,6 @@ 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} - statement ok BEGIN; ALTER DATABASE txn_database_drop_regions DROP REGION "us-east-1"; @@ -964,3 +963,80 @@ ALTER DATABASE drop_regions_alter_patterns DROP REGION "us-east-1" statement ok DROP TABLE east; DROP TABLE southeast; + +statement ok +CREATE DATABASE drop_primary_regions_db PRIMARY REGION "ca-central-1" REGIONS "ap-southeast-2", "us-east-1" + +statement ok +CREATE TABLE drop_primary_regions_db.primary(k INT PRIMARY KEY) LOCALITY REGIONAL BY TABLE IN PRIMARY REGION; +CREATE TABLE drop_primary_regions_db.east(k INT PRIMARY KEY) LOCALITY REGIONAL BY TABLE IN "us-east-1"; +CREATE TABLE drop_primary_regions_db.ca(k INT PRIMARY KEY) LOCALITY REGIONAL BY TABLE IN "ca-central-1" + +statement ok +ALTER DATABASE drop_primary_regions_db PRIMARY REGION "us-east-1" + +statement error pq: could not remove enum value "ca-central-1" as it is the home region for table "ca" +ALTER DATABASE drop_primary_regions_db DROP REGION "ca-central-1" + +statement ok +DROP TABLE drop_primary_regions_db.ca + +# As the primary region has been changed to "us-east-1", this drop should succeed. +statement ok +ALTER DATABASE drop_primary_regions_db DROP REGION "ca-central-1" + +# Cannot drop the primary region yet, as there are other regions in the db. +statement error pq: cannot drop region "us-east-1"\nHINT: You must designate another region as the primary region using ALTER DATABASE drop_primary_regions_db SET PRIMARY REGION or remove all other regions before attempting to drop region "us-east-1" +ALTER DATABASE drop_primary_regions_db DROP REGION "us-east-1" + +statement ok +ALTER DATABASE drop_primary_regions_db DROP REGION "ap-southeast-2" + +statement error pq: error removing primary region from database drop_primary_regions_db: cannot drop type "crdb_internal_region" because other objects \(\[drop_primary_regions_db.public.east\]\) still depend on it +ALTER DATABASE drop_primary_regions_db DROP REGION "us-east-1" + +statement ok +DROP TABLE drop_primary_regions_db.east + +# This should succeed, even though the table "primary" hasn't been dropped, as +# homing RTTs in primary regions does not block drops. +statement ok +ALTER DATABASE drop_primary_regions_db DROP REGION "us-east-1" + +# Accessing the table should work without an issue even after the database is +# no longer multi-region. +statement ok +SELECT * FROM drop_primary_regions_db.primary + +# Adding a region to the multi-region database should be possible as well, +# with no trace that this database was ever a multi-region db before. +statement ok +ALTER DATABASE drop_primary_regions_db PRIMARY REGION "ca-central-1" + +query TT colnames +SHOW ZONE CONFIGURATION FOR TABLE drop_primary_regions_db.primary +---- +target raw_config_sql +DATABASE drop_primary_regions_db ALTER DATABASE drop_primary_regions_db CONFIGURE ZONE USING + range_min_bytes = 134217728, + range_max_bytes = 536870912, + gc.ttlseconds = 90000, + num_replicas = 3, + num_voters = 3, + constraints = '{+region=ca-central-1: 1}', + voter_constraints = '[+region=ca-central-1]', + lease_preferences = '[[+region=ca-central-1]]' + + + +statement ok +ALTER TABLE drop_primary_regions_db.primary SET LOCALITY REGIONAL BY TABLE IN "ca-central-1" + +statement error pq: error removing primary region from database drop_primary_regions_db: cannot drop type "crdb_internal_region" because other objects \(\[drop_primary_regions_db.public."primary"\]\) still depend on it +ALTER DATABASE drop_primary_regions_db DROP REGION "ca-central-1" + +statement ok +ALTER TABLE drop_primary_regions_db.primary SET LOCALITY REGIONAL BY TABLE IN PRIMARY REGION + +statement ok +ALTER DATABASE drop_primary_regions_db DROP REGION "ca-central-1"