Merge #146124 #146135

146124: admission: call LowerBound(0) instead of Reset on token bucket for el… r=jeffswenson a=sumeerbhola

…astic CPU

The goal of this periodic operation is to ensure the token bucket is not extremely negative, which can happen if elastic work consumes more CPU than initially granted. However, instead of merely ensuring a lower bound of 0 tokens, it was resetting the token bucket to be full, which would admit another burst.

Observed in
https://cockroachlabs.slack.com/archives/C01SRKWGHG8/p1745891638545989?thread_ts=1745881523.172559&cid=C01SRKWGHG8.

Epic: none

Release note: None

146135: sql: fix DROP REGION error messages r=rafiss a=rafiss

The format arguments were not correct, and the error was misleading for the system database.

Epic: None
Release note: None

Co-authored-by: sumeerbhola <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>

Author: 3 people

pkg/sql/alter_database.go

@@ -442,16 +442,25 @@ func (p *planner) AlterDatabaseDropRegion(
 			)
 		}
 
+		// The system database, once it's been made multi-region, must not be
+		// allowed to go back.
 		isSystemDatabase := dbDesc.ID == keys.SystemDatabaseID
-		if allowDrop := allowDropFinalRegion.Get(&p.execCfg.Settings.SV); !allowDrop ||
-			// The system database, once it's been made multi-region, must not be
-			// allowed to go back.
-			isSystemDatabase {
+		if isSystemDatabase {
 			return nil, pgerror.Newf(
 				pgcode.InvalidDatabaseDefinition,
-				"databases in this cluster must have at least 1 region",
+				"cannot drop %s; system database must have at least 1 region",
 				n.Region,
-				sqlclustersettings.DefaultPrimaryRegionClusterSettingName,
+			)
+		}
+		if allowDrop := allowDropFinalRegion.Get(&p.execCfg.Settings.SV); !allowDrop {
+			return nil, errors.WithHintf(
+				pgerror.Newf(
+					pgcode.InvalidDatabaseDefinition,
+					"cannot drop %s; databases in this cluster must have at least 1 region",
+					n.Region,
+				),
+				"Try enabling the %s cluster setting.",
+				allowDropFinalRegion.Name(),
 			)
 		}
 

pkg/util/admission/elastic_cpu_granter.go

@@ -208,12 +208,12 @@ func (e *elasticCPUGranter) setUtilizationLimit(utilizationLimit float64) {
 	e.mu.utilizationLimit = utilizationLimit
 	e.mu.tb.UpdateConfig(tokenbucket.TokensPerSecond(rate), tokenbucket.Tokens(rate))
 	if now := timeutil.Now(); now.Sub(e.mu.tbLastReset) > 15*time.Second { // TODO(irfansharif): make this is a cluster setting?
-		// Periodically reset the token bucket. This is just defense-in-depth
-		// and at worst, over-admits. We've seen production clusters where the
-		// token bucket was severely in debt and caused wait queue times of
-		// minutes, which can be long enough to fail backups completely
-		// (#102817).
-		e.mu.tb.Reset()
+		// Periodically ensure the tokens are at least 0. This is just
+		// defense-in-depth and at worst, over-admits. We've seen production
+		// clusters where the token bucket was severely in debt and caused wait
+		// queue times of minutes, which can be long enough to fail backups
+		// completely (#102817).
+		e.mu.tb.EnsureLowerBound(0)
 		e.mu.tbLastReset = now
 	}
 	e.metrics.NanosExhaustedDuration.Update(e.mu.tb.Exhausted().Microseconds())