From 14af57def8c36d271bf4fd53594511b7b2a87445 Mon Sep 17 00:00:00 2001 From: arulajmani Date: Wed, 10 Feb 2021 11:23:51 -0500 Subject: [PATCH] sql: add support for dropping the primary region from a multi-region db This patch adds support for dropping the primary region from a multi-region database. Dropping the primary region is allowed only if all other regions have been removed from the database. Put another way, the primary region is always the last region to go and the last region to go is always the primary region. As part of removing the last region from the database, all of the static sql state is also reverted, leaving no detritus from multi-region ways of old. In particular, this means that all GLOBAL and REGIONAL BY TABLE tables lose the locality config from their descriptor and the multi-region type descriptor is removed from the database. Dropping the last region can fail for a few of reasons: - A REGIONAL BY TABLE table is homed explicitly in the region being dropped. - A REGIONAL BY ROW table exists in the database (which must write the last region in all rows of its region column). - A table in the database uses the multi-region enum. All of these fail scenarios can be ascertained by checking if references on the type descriptor exist, without having to go through the general case of enum value removal that happens in the type schema changer. This is what we do. Lastly, it's worth noting this patch doesn't modify zone configurations. Closes #57391 Informs #58333 Release note (sql change): `ALTER DATABASE ... DROP REGION` now allows for dropping the primary region. --- pkg/sql/alter_database.go | 168 ++++++++++++++---- pkg/sql/catalog/dbdesc/database_desc.go | 6 + .../logictest/testdata/logic_test/multiregion | 82 ++++++++- 3 files changed, 218 insertions(+), 38 deletions(-) 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"