Merge #135168

135168: sql: validate zone config before multi region database DDL r=fqazi a=fqazi

Previously, schema changes for databases involving zone configuration modifications could hang. This occurred because the system didn't validate the existing zone configuration's validity before initiating these operations. Since users can manually modify zone configurations, they might inadvertently introduce invalid states. To address this, this patch validates the zone configuration before the following database operations: setting the primary region, setting the secondary region, adding a region, and dropping a region.

Fixes: #131342

Release note (bug fix): Prevent ALTER DATABASE operations that modify the zone config from hanging if an invalid zone config already exists.

Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com>

pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs

@@ -2102,3 +2102,48 @@ statement ok
 DROP TABLE db1.t1_with_index_zone CASCADE;
 
 subtest end
+
+
+subtest invalid_zone_config
+
+statement ok
+CREATE DATABASE db_invalid_zone_config PRIMARY REGION "ca-central-1" REGIONS "ap-southeast-2"
+
+let $database_id
+select id from system.namespace WHERE name='db_invalid_zone_config'
+
+statement ok
+UPSERT INTO system.zones (id, config)
+VALUES
+	(
+		$database_id,
+		(
+			SELECT
+				crdb_internal.json_to_pb(
+					'cockroach.config.zonepb.ZoneConfig',
+					jsonb_set(
+						crdb_internal.pb_to_json(
+							'cockroach.config.zonepb.ZoneConfig',
+							config
+						),
+						ARRAY['rangeMinBytes'],
+						'-1'::JSONB
+					)
+				)
+			FROM
+				system.zones
+			WHERE
+				id = $database_id
+		)
+	);
+
+statement error RangeMinBytes -1 less than minimum allowed 0
+ALTER DATABASE db_invalid_zone_config ADD REGION 'us-east-1'
+
+statement error RangeMinBytes -1 less than minimum allowed 0
+ALTER DATABASE db_invalid_zone_config DROP REGION 'ap-southeast-2'
+
+statement error RangeMinBytes -1 less than minimum allowed 0
+ALTER DATABASE db_invalid_zone_config SET SECONDARY REGION 'us-east-1'
+
+subtest end

pkg/sql/alter_database.go

@@ -198,6 +198,16 @@ var GetMultiRegionEnumAddValuePlacementCCL = func(
 	)
 }
 
+// validateExistingZoneCfg validates existing zone configs for a given descriptor
+// ID.
+func (p *planner) validateExistingZoneCfg(ctx context.Context, id descpb.ID) error {
+	zc, err := p.Descriptors().GetZoneConfig(ctx, p.txn, id)
+	if err != nil || zc == nil {
+		return err
+	}
+	return zc.ZoneConfigProto().Validate()
+}
+
 func (n *alterDatabaseAddRegionNode) startExec(params runParams) error {
 	if err := params.p.validateZoneConfigForMultiRegionDatabaseWasNotModifiedByUser(
 		params.ctx,
@@ -216,6 +226,13 @@ func (n *alterDatabaseAddRegionNode) startExec(params runParams) error {
 		return err
 	}
 
+	// Validate if the existing zone config of this descriptor is sane,
+	// since otherwise, any modifications for multi-region could fail during
+	// the job phase.
+	if err := params.p.validateExistingZoneCfg(params.ctx, n.desc.ID); err != nil {
+		return err
+	}
+
 	// Get the type descriptor for the multi-region enum.
 	typeDesc, err := params.p.Descriptors().MutableByID(params.p.txn).Type(params.ctx, n.desc.RegionConfig.RegionEnumID)
 	if err != nil {
@@ -721,6 +738,14 @@ func (n *alterDatabaseDropRegionNode) startExec(params runParams) error {
 	if n.n == nil {
 		return nil
 	}
+
+	// Validate if the existing zone config of this descriptor is sane,
+	// since otherwise, any modifications for multi-region could fail during
+	// the job phase.
+	if err := params.p.validateExistingZoneCfg(params.ctx, n.desc.ID); err != nil {
+		return err
+	}
+
 	typeDesc, err := params.p.Descriptors().MutableByID(params.p.txn).Type(params.ctx, n.desc.RegionConfig.RegionEnumID)
 	if err != nil {
 		return err
@@ -1132,6 +1157,13 @@ func (n *alterDatabasePrimaryRegionNode) setInitialPrimaryRegion(params runParam
 
 func (n *alterDatabasePrimaryRegionNode) startExec(params runParams) error {
 
+	// Validate if the existing zone config of this descriptor is sane,
+	// since otherwise, any modifications for multi-region could fail during
+	// the job phase.
+	if err := params.p.validateExistingZoneCfg(params.ctx, n.desc.ID); err != nil {
+		return err
+	}
+
 	// There are two paths to consider here: either this is the first setting of
 	// the primary region, OR we're updating the primary region. In the case where
 	// this is the first setting of the primary region, the call will turn the
@@ -1893,6 +1925,13 @@ func (n *alterDatabaseSecondaryRegion) startExec(params runParams) error {
 		)
 	}
 
+	// Validate if the existing zone config of this descriptor is sane,
+	// since otherwise, any modifications for multi-region could fail during
+	// the job phase.
+	if err := params.p.validateExistingZoneCfg(params.ctx, n.desc.ID); err != nil {
+		return err
+	}
+
 	// Verify that the secondary region is part of the region list.
 	prevRegionConfig, err := SynthesizeRegionConfig(params.ctx, params.p.txn, n.desc.ID, params.p.Descriptors())
 	if err != nil {