sql: Don't require override for multiple MR abstractions in a transac…

…tion

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 cockroachdb#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

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

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

pkg/ccl/multiregionccl/region_test.go

@@ -24,6 +24,89 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+func TestConcurrentDropRegion(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	firstDropAtSchemaChange := false
+	waitForFirstDropRegionToReachSchemaChanger := make(chan struct{})
+	waitForSecondDropRegionToReachSchemaChanger := make(chan struct{})
+
+	// 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() {
+				if !firstDropAtSchemaChange {
+					firstDropAtSchemaChange = true
+					close(waitForFirstDropRegionToReachSchemaChanger)
+					<-waitForSecondDropRegionToReachSchemaChanger
+				} else {
+					// We're the second DROP REGION, close the channel to free the first
+					// DROP REGION.
+					close(waitForSecondDropRegionToReachSchemaChanger)
+				}
+			},
+		},
+	}
+
+	_, 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.Fatal(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)
+		}
+	}()
+
+	// 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)
+	}
+
+	// Validate that both DROP REGION statements completed.
+	testutils.SucceedsSoon(t, func() error {
+		rows, err := sqlDB.Query("SELECT region FROM [SHOW REGIONS FROM DATABASE db]")
+		if err != nil {
+			return err
+		}
+		defer rows.Close()
+
+		const expectedRegion = "us-east1"
+		var region string
+		rows.Next()
+		if err := rows.Scan(&region); err != nil {
+			return err
+		}
+
+		if region != expectedRegion {
+			return errors.Newf("expected region to be: %q, got %q", expectedRegion, region)
+		}
+
+		if rows.Next() {
+			return errors.New("unexpected number of rows returned")
+		}
+		return nil
+	})
+}
+
 func TestSettingPrimaryRegionAmidstDrop(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
@@ -68,14 +151,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 +304,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`,
 		},
 	}

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
 	})

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) {

pkg/sql/region_util.go

@@ -916,17 +916,48 @@ func (p *planner) CurrentDatabaseRegionConfig(
 func SynthesizeRegionConfig(
 	ctx context.Context, txn *kv.Txn, dbID descpb.ID, descsCol *descs.Collection,
 ) (multiregion.RegionConfig, error) {
+	return synthesizeRegionConfigImpl(ctx, txn, dbID, descsCol, 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, true /* forZoneConfigValidate */)
+}
+
+// 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. 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,
+	forZoneConfigValidate bool,
+) (multiregion.RegionConfig, error) {
+	regionConfig := multiregion.RegionConfig{}
 	_, dbDesc, err := descsCol.GetImmutableDatabaseByID(ctx, txn, dbID, tree.DatabaseLookupFlags{
 		AvoidCached: true,
 		Required:    true,
 	})
 	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(
@@ -942,12 +973,17 @@ func SynthesizeRegionConfig(
 	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,
@@ -1048,7 +1084,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
 	}
@@ -1074,8 +1110,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
@@ -1100,8 +1135,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
@@ -1146,7 +1179,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")
 				}
 			}
 		}
@@ -1192,7 +1226,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