Skip to content

Commit

Permalink
sql: validate zone config before multi region database DDL
Browse files Browse the repository at this point in the history
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: cockroachdb#131342

Release note (bug fix): Prevent ALTER DATABASE operations that modify
the zone config from hanging if an invalid zone config already exists.
  • Loading branch information
fqazi committed Nov 14, 2024
1 parent 2f324cc commit 7498055
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 0 deletions.
47 changes: 47 additions & 0 deletions pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs
Original file line number Diff line number Diff line change
Expand Up @@ -2102,3 +2102,50 @@ 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
28 changes: 28 additions & 0 deletions pkg/sql/alter_database.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -216,6 +226,10 @@ func (n *alterDatabaseAddRegionNode) startExec(params runParams) error {
return err
}

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 {
Expand Down Expand Up @@ -721,6 +735,11 @@ func (n *alterDatabaseDropRegionNode) startExec(params runParams) error {
if n.n == nil {
return nil
}

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
Expand Down Expand Up @@ -1132,6 +1151,11 @@ func (n *alterDatabasePrimaryRegionNode) setInitialPrimaryRegion(params runParam

func (n *alterDatabasePrimaryRegionNode) startExec(params runParams) error {

// Validate if the existing zone config is good.
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
Expand Down Expand Up @@ -1893,6 +1917,10 @@ func (n *alterDatabaseSecondaryRegion) startExec(params runParams) error {
)
}

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 {
Expand Down

0 comments on commit 7498055

Please sign in to comment.