Merge #156228 #156275 #156288

156228: multiregion: allow zone config extensions to remove read replicas r=rafiss a=rafiss

Previously, zone config extensions validated that num_replicas could not be set lower than the total number of replicas required for the database's survival goal (which includes both voters and non-voting read replicas). This prevented users from removing read replicas to reduce storage costs while maintaining the required number of voters for the survival goal.

The survival goal only depends on the number of voting replicas (num_voters), not the total number of replicas. Non-voting replicas are used for follower reads and do not participate in Raft consensus or affect availability guarantees.

This commit changes the validation in ExtendZoneConfigWithGlobal() and ExtendZoneConfigWithRegionalIn() to check that num_replicas is not set lower than num_voters instead of the total numReplicas. This allows users to set num_replicas equal to num_voters, effectively removing read replicas while maintaining the required voter count for their survival goal.

For example, in a SURVIVE ZONE FAILURE database with 3 regions, users can now reduce num_replicas from 5 (default: 3 voters + 2 read replicas) to 3 (just the required voters), saving storage while maintaining zone failure tolerance.

Also fixes a typo in the error message ("num_replica" -> "num_replicas").

Resolves: #105210
Epic: CRDB-45964

Release note (bug fix): Fixed a bug where zone config extensions incorrectly prevented users from removing non-voting read replicas from multi-region databases. Users can now set num_replicas equal to num_voters to remove read replicas while maintaining the required number of voting replicas for their database's survival goal. This allows reducing storage costs without compromising availability guarantees.

156275: catalog/lease: fix race condition with testing knob r=fqazi a=fqazi

Previously, the testing knob TestingDisableRangeFeedCheckpoint was read without any mutex, which could lead to a race condition in certain tests as detected by the race detector. To address this, this patch moves the field into the lease manager as an atomic, since testing knobs are copied, so they cannot store mutexes or atomics.

Fixes: #156182

Release note: None

156288: raftstorebench: prevent iter use-after-close r=tbg a=tbg

The `statsLoop` could still be running. In the linked test flake below, we see it call `(*pebble).GetMetrics` when the engines are already closed.

Rearrange the code so that we wait for `statsLoop` to terminate properly before exiting the test.

Closes #156138.

Epic: none

Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Faizan Qazi <[email protected]>
Co-authored-by: Tobias Grieger <[email protected]>

Author: 4 people

pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_config_extensions

@@ -290,10 +290,100 @@ statement error pq: zone config extension cannot unset the home region \(ca-cent
 ALTER DATABASE "mr-zone-configs" ALTER LOCALITY REGIONAL IN "ca-central-1" CONFIGURE ZONE USING
   lease_preferences = '[[+region=ap-southeast-2]]'
 
-statement error pq: zone config extension cannot set num_replicas 4 that is lower than the one required for the survival goal: 5 with goal REGION_FAILURE
+statement error pq: cannot set num_replicas below 5 \(num_voters required for survival goal REGION_FAILURE\), got 4
 ALTER DATABASE "mr-zone-configs" ALTER LOCALITY GLOBAL CONFIGURE ZONE USING
   num_replicas = 4
 
-statement error pq: zone config extension cannot set num_voters 4 that is lower than the one required for the survival goal: 5 with goal REGION_FAILURE
+statement error pq: cannot set num_voters below 5 \(num_voters required for survival goal REGION_FAILURE\), got 4
 ALTER DATABASE "mr-zone-configs" ALTER LOCALITY GLOBAL CONFIGURE ZONE USING
   num_voters = 4
+
+# Test that num_replicas can be set to num_voters (removing read replicas).
+# This is allowed because survival goals depend on num_voters, not total replicas.
+
+# SURVIVE REGION FAILURE with num_voters=5 should allow num_replicas=5 (removing read replicas).
+statement ok
+ALTER DATABASE "mr-zone-configs" ALTER LOCALITY GLOBAL CONFIGURE ZONE USING
+  num_replicas = 5
+
+query TT
+SHOW ZONE CONFIGURATION FOR DATABASE "mr-zone-configs"
+----
+DATABASE "mr-zone-configs"  ALTER DATABASE "mr-zone-configs" CONFIGURE ZONE USING
+                              range_min_bytes = 134217728,
+                              range_max_bytes = 536870912,
+                              gc.ttlseconds = 14400,
+                              num_replicas = 9,
+                              num_voters = 5,
+                              constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
+                              voter_constraints = '{+region=ca-central-1: 2}',
+                              lease_preferences = '[[+region=ca-central-1], [+region=ap-southeast-2]]'
+
+statement ok
+ALTER DATABASE "mr-zone-configs" ALTER LOCALITY REGIONAL CONFIGURE ZONE USING
+  num_replicas = 5
+
+query TT
+SHOW ZONE CONFIGURATION FOR DATABASE "mr-zone-configs"
+----
+DATABASE "mr-zone-configs"  ALTER DATABASE "mr-zone-configs" CONFIGURE ZONE USING
+                              range_min_bytes = 134217728,
+                              range_max_bytes = 536870912,
+                              gc.ttlseconds = 14400,
+                              num_replicas = 5,
+                              num_voters = 5,
+                              constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
+                              voter_constraints = '{+region=ca-central-1: 2}',
+                              lease_preferences = '[[+region=ca-central-1], [+region=ap-southeast-2]]'
+
+# num_replicas below num_voters should still fail.
+statement error pq: cannot set num_replicas below 5 \(num_voters required for survival goal REGION_FAILURE\), got 3
+ALTER DATABASE "mr-zone-configs" ALTER LOCALITY REGIONAL CONFIGURE ZONE USING
+  num_replicas = 3
+
+# Test SURVIVE ZONE FAILURE case.
+statement ok
+CREATE DATABASE "mr-zone-survival"
+  primary region "ca-central-1"
+  regions "ap-southeast-2", "us-east-1"
+  survive zone failure
+
+statement ok
+use "mr-zone-survival"
+
+# SURVIVE ZONE with num_voters=3, num_replicas=5 by default.
+query TT
+SHOW ZONE CONFIGURATION FOR DATABASE "mr-zone-survival"
+----
+DATABASE "mr-zone-survival"  ALTER DATABASE "mr-zone-survival" CONFIGURE ZONE USING
+                               range_min_bytes = 134217728,
+                               range_max_bytes = 536870912,
+                               gc.ttlseconds = 14400,
+                               num_replicas = 5,
+                               num_voters = 3,
+                               constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
+                               voter_constraints = '[+region=ca-central-1]',
+                               lease_preferences = '[[+region=ca-central-1]]'
+
+# Reduce num_replicas to num_voters (3) - removing read replicas.
+statement ok
+ALTER DATABASE "mr-zone-survival" ALTER LOCALITY REGIONAL CONFIGURE ZONE USING
+  num_replicas = 3
+
+query TT
+SHOW ZONE CONFIGURATION FOR DATABASE "mr-zone-survival"
+----
+DATABASE "mr-zone-survival"  ALTER DATABASE "mr-zone-survival" CONFIGURE ZONE USING
+                               range_min_bytes = 134217728,
+                               range_max_bytes = 536870912,
+                               gc.ttlseconds = 14400,
+                               num_replicas = 3,
+                               num_voters = 3,
+                               constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
+                               voter_constraints = '[+region=ca-central-1]',
+                               lease_preferences = '[[+region=ca-central-1]]'
+
+# num_replicas below num_voters should still fail.
+statement error pq: cannot set num_replicas below 3 \(num_voters required for survival goal ZONE_FAILURE\), got 2
+ALTER DATABASE "mr-zone-survival" ALTER LOCALITY REGIONAL CONFIGURE ZONE USING
+  num_replicas = 2

pkg/kv/kvserver/raftstorebench/bench.go

@@ -28,26 +28,32 @@ func Run(t T, cfg Config) Result {
 
 	q := makeReplicaQueue(cfg.NumReplicas)
 
-	var wg sync.WaitGroup
 	s := newAggStats()
 
-	statsCtx, stopStats := context.WithCancel(context.Background())
-	defer stopStats()
-	go statsLoop(t, statsCtx, cfg, s, raftEng, smEng)
+	statsCtx, cancelCtx := context.WithCancel(context.Background())
+	defer cancelCtx()
+	var bgWG sync.WaitGroup
+	bgWG.Add(1)
+	go func() {
+		defer bgWG.Done()
+		statsLoop(t, statsCtx, cfg, s, raftEng, smEng)
+	}()
 
 	o := writeOptions{
 		cfg: cfg, smEng: smEng, raftEng: raftEng,
 		keyLen: keyLen, valueLen: valueLen, batchLen: batchLen,
 	}
 	var durabilityCallbackCount atomic.Int64
 	tStartWorkers := timeutil.Now()
+
+	var workerWG sync.WaitGroup
 	for i := 0; i < cfg.NumWorkers; i++ {
-		wg.Add(1)
+		workerWG.Add(1)
 		w := &worker{
 			t: t, s: s, o: o, rng: rand.New(rand.NewSource(int64(i))),
 			durabilityCallbackCount: &durabilityCallbackCount,
 		}
-		go w.run(t, q, &wg)
+		go w.run(t, q, &workerWG)
 	}
 	logf(t, "started workers")
 
@@ -63,7 +69,9 @@ func Run(t T, cfg Config) Result {
 		var bytesFlushed uint64
 		var n int
 		notifyCh := make(chan struct{}, 1)
+		bgWG.Add(1)
 		go func() {
+			defer bgWG.Done()
 			for {
 				select {
 				case <-statsCtx.Done():
@@ -87,8 +95,9 @@ func Run(t T, cfg Config) Result {
 		})
 	}
 
-	wg.Wait()
-	stopStats()
+	workerWG.Wait()
+	cancelCtx()
+	bgWG.Wait() // make sure all goroutines are stopped by time engine closes
 	duration := timeutil.Since(tStartWorkers)
 	logf(t, "done working")
 

pkg/sql/catalog/lease/lease.go

@@ -1567,6 +1567,11 @@ type Manager struct {
 	bytesMonitor *mon.BytesMonitor
 	// boundAccount tracks the memory usage from leased descriptors.
 	boundAccount *mon.ConcurrentBoundAccount
+
+	// TestingDisableRangeFeedCheckpoint is used to disable rangefeed checkpoints.
+	// Ideally, this would be inside StorageTestingKnobs, but those get copied into
+	// the lease manager.
+	TestingDisableRangeFeedCheckpoint atomic.Bool
 }
 
 const leaseConcurrencyLimit = 5
@@ -2506,7 +2511,7 @@ func (m *Manager) watchForUpdates(ctx context.Context) {
 		ctx context.Context, ev *kvpb.RangeFeedValue,
 	) {
 		// Skip updates if rangefeeds are disabled.
-		if m.testingKnobs.DisableRangeFeedCheckpoint {
+		if m.TestingDisableRangeFeedCheckpoint.Load() {
 			return
 		}
 		// Check first if it is the special descriptor metadata key used to track
@@ -2572,7 +2577,7 @@ func (m *Manager) watchForUpdates(ctx context.Context) {
 	}
 
 	handleCheckpoint := func(ctx context.Context, checkpoint *kvpb.RangeFeedCheckpoint) {
-		if m.testingKnobs.DisableRangeFeedCheckpoint {
+		if m.TestingDisableRangeFeedCheckpoint.Load() {
 			return
 		}
 		// Track checkpoints that occur from the rangefeed to make sure progress
@@ -3150,7 +3155,7 @@ func (m *Manager) TestingSetDisableRangeFeedCheckpointFn(disable bool) chan stru
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	m.mu.rangeFeedCheckpoints = 0
-	m.testingKnobs.DisableRangeFeedCheckpoint = disable
+	m.TestingDisableRangeFeedCheckpoint.Store(disable)
 	if disable {
 		m.testingKnobs.RangeFeedResetChannel = make(chan struct{})
 	} else {

pkg/sql/catalog/lease/testutils.go

@@ -61,9 +61,6 @@ type ManagerTestingKnobs struct {
 	// To disable the deletion of orphaned leases at server startup.
 	DisableDeleteOrphanedLeases bool
 
-	// DisableRangeFeedCheckpoint is used to disable rangefeed checkpoints.
-	DisableRangeFeedCheckpoint bool
-
 	// RangeFeedReset channel is closed to indicate that the range feed
 	// has been reset.
 	RangeFeedResetChannel chan struct{}

pkg/sql/catalog/multiregion/region_config.go

@@ -183,27 +183,24 @@ func (r *RegionConfig) ZoneConfigExtensions() descpb.ZoneConfigExtensions {
 // extensions to the provided zone configuration, returning the updated config.
 func (r *RegionConfig) ExtendZoneConfigWithGlobal(zc zonepb.ZoneConfig) (zonepb.ZoneConfig, error) {
 	if ext := r.zoneCfgExtensions.Global; ext != nil {
-		var numVoters, numReplicas int32
+		var numVoters int32
 		if zc.NumVoters != nil {
 			numVoters = *zc.NumVoters
 		}
-		if zc.NumReplicas != nil {
-			numReplicas = *zc.NumReplicas
-		}
 		zc = extendZoneCfg(zc, *ext)
 		// TODO(janexing): to ensure that the zone config extension won't break the
 		// survival goal, here we only add restriction on the number of voters and
 		// replicas. We may want to consider adding constraint to their distribution
 		// across zones/regions as well.
 		if zc.NumVoters != nil && *zc.NumVoters < numVoters {
-			return zonepb.ZoneConfig{}, errors.Newf("zone config extension "+
-				"cannot set num_voters %v that is lower than the one required for the "+
-				"survival goal: %v with goal %v\n", *zc.NumVoters, numVoters, r.SurvivalGoal())
+			return zonepb.ZoneConfig{}, errors.Newf(
+				"cannot set num_voters below %v (num_voters required for survival goal %v), got %v",
+				numVoters, r.SurvivalGoal(), *zc.NumVoters)
 		}
-		if zc.NumReplicas != nil && *zc.NumReplicas < numReplicas {
-			return zonepb.ZoneConfig{}, errors.Newf("zone config extension "+
-				"cannot set num_replicas %v that is lower than the one required for the "+
-				"survival goal: %v with goal %v\n", *zc.NumReplicas, numReplicas, r.SurvivalGoal())
+		if zc.NumReplicas != nil && *zc.NumReplicas < numVoters {
+			return zonepb.ZoneConfig{}, errors.Newf(
+				"cannot set num_replicas below %v (num_voters required for survival goal %v), got %v",
+				numVoters, r.SurvivalGoal(), *zc.NumReplicas)
 		}
 	}
 	return zc, nil
@@ -215,13 +212,10 @@ func (r *RegionConfig) ExtendZoneConfigWithGlobal(zc zonepb.ZoneConfig) (zonepb.
 func (r *RegionConfig) ExtendZoneConfigWithRegionalIn(
 	zc zonepb.ZoneConfig, region catpb.RegionName,
 ) (zonepb.ZoneConfig, error) {
-	var numVoters, numReplicas int32
+	var numVoters int32
 	if zc.NumVoters != nil {
 		numVoters = *zc.NumVoters
 	}
-	if zc.NumReplicas != nil {
-		numReplicas = *zc.NumReplicas
-	}
 
 	if ext := r.zoneCfgExtensions.Regional; ext != nil {
 		if ext.LeasePreferences != nil {
@@ -242,14 +236,14 @@ func (r *RegionConfig) ExtendZoneConfigWithRegionalIn(
 	// replicas. We may want to consider adding constraint to their distribution
 	// across zones/regions as well.
 	if zc.NumVoters != nil && *zc.NumVoters < numVoters {
-		return zonepb.ZoneConfig{}, errors.Newf("zone config extension "+
-			"cannot set num_voters %v that is lower than the one required for the "+
-			"survival goal: %v with goal %v\n", *zc.NumVoters, numVoters, r.SurvivalGoal())
-	}
-	if zc.NumReplicas != nil && *zc.NumReplicas < numReplicas {
-		return zonepb.ZoneConfig{}, errors.Newf("zone config extension "+
-			"cannot set num_replica %v that is lower than the one required for the "+
-			"survival goal: %v with goal %v\n", *zc.NumReplicas, numReplicas, r.SurvivalGoal())
+		return zonepb.ZoneConfig{}, errors.Newf(
+			"cannot set num_voters below %v (num_voters required for survival goal %v), got %v",
+			numVoters, r.SurvivalGoal(), *zc.NumVoters)
+	}
+	if zc.NumReplicas != nil && *zc.NumReplicas < numVoters {
+		return zonepb.ZoneConfig{}, errors.Newf(
+			"cannot set num_replicas below %v (num_voters required for survival goal %v), got %v",
+			numVoters, r.SurvivalGoal(), *zc.NumReplicas)
 	}
 	return zc, nil
 }