Merge pull request #114102 from cockroachdb/blathers/backport-release…

…-23.2-113956

release-23.2: sql: only validate new regions when adding/dropping

pkg/ccl/streamingccl/streamingest/replication_stream_e2e_test.go

@@ -1277,6 +1277,67 @@ func TestStreamingRegionalConstraint(t *testing.T) {
 
 }
 
+func TestStreamingMismatchedMRDatabase(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+	skip.UnderStressRace(t, "takes too long under stress race")
+
+	ctx := context.Background()
+	regions := []string{"mars", "venus", "mercury"}
+	args := replicationtestutils.DefaultTenantStreamingClustersArgs
+	args.SrcClusterTestRegions = regions
+	args.SrcNumNodes = 3
+
+	c, cleanup := replicationtestutils.CreateTenantStreamingClusters(ctx, t, args)
+	defer cleanup()
+
+	producerJobID, ingestionJobID := c.StartStreamReplication(ctx)
+	jobutils.WaitForJobToRun(c.T, c.SrcSysSQL, jobspb.JobID(producerJobID))
+	jobutils.WaitForJobToRun(c.T, c.DestSysSQL, jobspb.JobID(ingestionJobID))
+
+	c.SrcTenantSQL.Exec(t, "CREATE DATABASE prim PRIMARY REGION mars")
+	c.SrcTenantSQL.Exec(t, "CREATE TABLE prim.x (id INT PRIMARY KEY, n INT)")
+	c.SrcTenantSQL.Exec(t, "INSERT INTO prim.x VALUES (1, 1)")
+
+	c.SrcTenantSQL.Exec(t, "CREATE DATABASE many PRIMARY REGION mars REGIONS = mars,mercury,venus")
+	c.SrcTenantSQL.Exec(t, "CREATE TABLE many.x (id INT PRIMARY KEY, n INT)")
+	c.SrcTenantSQL.Exec(t, "INSERT INTO many.x VALUES (1, 1)")
+
+	srcTime := c.SrcCluster.Server(0).Clock().Now()
+	c.Cutover(producerJobID, ingestionJobID, srcTime.GoTime(), false)
+
+	cleanupTenant := c.StartDestTenant(ctx, nil)
+	defer func() {
+		require.NoError(t, cleanupTenant())
+	}()
+
+	// Check how MR primitives have replicated to non-mr stand by cluster
+	t.Run("mr db only with primary region", func(t *testing.T) {
+		var res string
+		c.DestTenantSQL.QueryRow(c.T, `SELECT create_statement FROM [SHOW CREATE DATABASE prim]`).Scan(&res)
+		require.Equal(t, "CREATE DATABASE prim PRIMARY REGION mars REGIONS = mars SURVIVE ZONE FAILURE", res)
+
+		var region string
+		c.DestTenantSQL.QueryRow(c.T, "SELECT region FROM [SHOW REGIONS FROM DATABASE prim];").Scan(&region)
+		require.Equal(t, "mars", region)
+
+		c.DestTenantSQL.Exec(t, "INSERT INTO prim.x VALUES (2, 2)")
+
+		c.DestTenantSQL.Exec(c.T, `ALTER DATABASE prim DROP REGION "mars"`)
+	})
+	t.Run("mr db with several regions", func(t *testing.T) {
+		var res string
+		c.DestTenantSQL.QueryRow(c.T, `SELECT create_statement FROM [SHOW CREATE DATABASE many]`).Scan(&res)
+		require.Equal(t, "CREATE DATABASE many PRIMARY REGION mars REGIONS = mars, mercury, venus SURVIVE ZONE FAILURE", res)
+
+		c.DestTenantSQL.Exec(t, "INSERT INTO many.x VALUES (2, 2)")
+
+		// As a sanity check, drop a region on the source and destination cluster.
+		c.SrcTenantSQL.ExecSucceedsSoon(c.T, `ALTER DATABASE many DROP REGION "venus"`)
+		c.DestTenantSQL.ExecSucceedsSoon(c.T, `ALTER DATABASE many DROP REGION "venus"`)
+	})
+}
+
 func checkLocalityRanges(
 	t *testing.T, sysSQL *sqlutils.SQLRunner, codec keys.SQLCodec, tableID uint32, region string,
 ) {

pkg/cmd/roachtest/tests/BUILD.bazel

@@ -99,6 +99,7 @@ go_library(
         "liquibase_blocklist.go",
         "loss_of_quorum_recovery.go",
         "many_splits.go",
+        "mismatched_locality.go",
         "mixed_version_backup.go",
         "mixed_version_cdc.go",
         "mixed_version_change_replicas.go",

pkg/cmd/roachtest/tests/acceptance.go

@@ -82,6 +82,12 @@ func registerAcceptance(r registry.Registry) {
 				fn:            runValidateSystemSchemaAfterVersionUpgrade,
 				timeout:       30 * time.Minute,
 				defaultLeases: true,
+				numNodes:      1,
+			},
+			{
+				name:     "mismatched-locality",
+				fn:       runMismatchedLocalityTest,
+				numNodes: 3,
 			},
 		},
 	}

pkg/cmd/roachtest/tests/mismatched_locality.go

@@ -0,0 +1,85 @@
+// Copyright 2023 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package tests
+
+import (
+	"context"
+
+	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster"
+	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
+	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
+	"github.com/cockroachdb/cockroach/pkg/roachprod/install"
+)
+
+// This test tests that we can add and drop regions even if the locality flags
+// of a node no longer match the regions that already were added to the
+// database.
+func runMismatchedLocalityTest(ctx context.Context, t test.Test, c cluster.Cluster) {
+	// Start 3 nodes with a different localities.
+	startOpts := option.DefaultStartOpts()
+	startOpts.RoachprodOpts.ExtraArgs = []string{"--locality=region=east"}
+	c.Start(ctx, t.L(), startOpts, install.MakeClusterSettings(), c.Nodes(1))
+	startOpts.RoachprodOpts.ExtraArgs = []string{"--locality=region=central"}
+	c.Start(ctx, t.L(), startOpts, install.MakeClusterSettings(), c.Nodes(2))
+	startOpts.RoachprodOpts.ExtraArgs = []string{"--locality=region=west"}
+	c.Start(ctx, t.L(), startOpts, install.MakeClusterSettings(), c.Nodes(3))
+
+	// Add the 3 regions to the database.
+	db := c.Conn(ctx, t.L(), 1)
+	if _, err := db.Exec(`ALTER DATABASE defaultdb PRIMARY REGION "east";`); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := db.Exec(`ALTER DATABASE defaultdb ADD REGION "central";`); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := db.Exec(`ALTER DATABASE defaultdb ADD REGION "west";`); err != nil {
+		t.Fatal(err)
+	}
+
+	// Restart all the nodes with new localities.
+	c.Stop(ctx, t.L(), option.DefaultStopOpts(), c.Nodes(1))
+	startOpts.RoachprodOpts.ExtraArgs = []string{"--locality=region=mars"}
+	c.Start(ctx, t.L(), startOpts, install.MakeClusterSettings(), c.Nodes(1))
+	c.Stop(ctx, t.L(), option.DefaultStopOpts(), c.Nodes(2))
+	startOpts.RoachprodOpts.ExtraArgs = []string{"--locality=region=jupiter"}
+	c.Start(ctx, t.L(), startOpts, install.MakeClusterSettings(), c.Nodes(2))
+	c.Stop(ctx, t.L(), option.DefaultStopOpts(), c.Nodes(3))
+	startOpts.RoachprodOpts.ExtraArgs = []string{"--locality=region=venus"}
+	c.Start(ctx, t.L(), startOpts, install.MakeClusterSettings(), c.Nodes(3))
+
+	// Verify that we can add and drop regions for the database. There's no longer
+	// any node with the old localities, but that's fine.
+	db = c.Conn(ctx, t.L(), 3)
+	if err := WaitFor3XReplication(ctx, t, db); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := db.Exec(`ALTER DATABASE defaultdb ADD REGION "venus";`); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := db.Exec(`ALTER DATABASE defaultdb DROP REGION "central";`); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := db.Exec(`ALTER DATABASE defaultdb ADD REGION "jupiter";`); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := db.Exec(`ALTER DATABASE defaultdb ADD REGION "mars";`); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := db.Exec(`ALTER DATABASE defaultdb SET PRIMARY REGION "mars";`); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := db.Exec(`ALTER DATABASE defaultdb DROP REGION "west";`); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := db.Exec(`ALTER DATABASE defaultdb DROP REGION "east";`); err != nil {
+		t.Fatal(err)
+	}
+}

pkg/sql/alter_database.go

@@ -2326,6 +2326,15 @@ func (n *alterDatabaseSetZoneConfigExtensionNode) startExec(params runParams) er
 		}
 	}
 
+	currentZone := zonepb.NewZoneConfig()
+	if currentZoneConfigWithRaw, err := params.p.Descriptors().GetZoneConfig(
+		params.ctx, params.p.Txn(), n.desc.ID,
+	); err != nil {
+		return err
+	} else if currentZoneConfigWithRaw != nil {
+		currentZone = currentZoneConfigWithRaw.ZoneConfigProto()
+	}
+
 	if deleteZone {
 		switch n.n.LocalityLevel {
 		case tree.LocalityLevelGlobal:
@@ -2388,7 +2397,7 @@ func (n *alterDatabaseSetZoneConfigExtensionNode) startExec(params runParams) er
 		}
 
 		if err := validateZoneAttrsAndLocalities(
-			params.ctx, params.p.InternalSQLTxn().Regions(), params.p.ExecCfg(), newZone,
+			params.ctx, params.p.InternalSQLTxn().Regions(), params.p.ExecCfg(), currentZone, newZone,
 		); err != nil {
 			return err
 		}
@@ -2433,7 +2442,7 @@ func (n *alterDatabaseSetZoneConfigExtensionNode) startExec(params runParams) er
 
 	// Validate if the zone config extension is compatible with the database.
 	dbZoneConfig, err := generateAndValidateZoneConfigForMultiRegionDatabase(
-		params.ctx, params.p.InternalSQLTxn().Regions(), params.ExecCfg(), updatedRegionConfig, true, /* validateLocalities */
+		params.ctx, params.p.InternalSQLTxn().Regions(), params.ExecCfg(), updatedRegionConfig, currentZone, true, /* validateLocalities */
 	)
 	if err != nil {
 		return err

pkg/sql/region_util.go

@@ -896,6 +896,7 @@ func generateAndValidateZoneConfigForMultiRegionDatabase(
 	regionProvider descs.RegionProvider,
 	execConfig *ExecutorConfig,
 	regionConfig multiregion.RegionConfig,
+	currentZoneConfig *zonepb.ZoneConfig,
 	validateLocalities bool,
 ) (zonepb.ZoneConfig, error) {
 	// Build a zone config based on the RegionConfig information.
@@ -916,7 +917,7 @@ func generateAndValidateZoneConfigForMultiRegionDatabase(
 		return zonepb.ZoneConfig{}, err
 	}
 
-	if err := validateZoneAttrsAndLocalities(ctx, regionProvider, execConfig, &dbZoneConfig); err != nil {
+	if err := validateZoneAttrsAndLocalities(ctx, regionProvider, execConfig, currentZoneConfig, &dbZoneConfig); err != nil {
 		// If we are validating localities this is fatal, otherwise let's log any
 		// errors as warnings.
 		if validateLocalities {
@@ -940,8 +941,17 @@ func ApplyZoneConfigFromDatabaseRegionConfig(
 	validateLocalities bool,
 	kvTrace bool,
 ) error {
+	currentZone := zonepb.NewZoneConfig()
+	if currentZoneConfigWithRaw, err := txn.Descriptors().GetZoneConfig(
+		ctx, txn.KV(), dbID,
+	); err != nil {
+		return err
+	} else if currentZoneConfigWithRaw != nil {
+		currentZone = currentZoneConfigWithRaw.ZoneConfigProto()
+	}
+
 	// Build a zone config based on the RegionConfig information.
-	dbZoneConfig, err := generateAndValidateZoneConfigForMultiRegionDatabase(ctx, txn.Regions(), execConfig, regionConfig, validateLocalities)
+	dbZoneConfig, err := generateAndValidateZoneConfigForMultiRegionDatabase(ctx, txn.Regions(), execConfig, regionConfig, currentZone, validateLocalities)
 	if err != nil {
 		return err
 	}
@@ -2660,10 +2670,21 @@ func (zv *zoneConfigValidator) ValidateDbZoneConfig(
 	if err != nil {
 		return err
 	}
+
+	currentZone := zonepb.NewZoneConfig()
+	if currentZoneConfigWithRaw, err := zv.descs.GetZoneConfig(
+		ctx, zv.txn, db.GetID(),
+	); err != nil {
+		return err
+	} else if currentZoneConfigWithRaw != nil {
+		currentZone = currentZoneConfigWithRaw.ZoneConfigProto()
+	}
+
 	_, err = generateAndValidateZoneConfigForMultiRegionDatabase(ctx,
 		zv.regionProvider,
 		zv.execCfg,
 		regionConfig,
+		currentZone,
 		true, /*validateLocalities*/
 	)
 	if err != nil {