Merge #131303

131303: sql: Exempt SET CLUSTER SETTING from throttling r=fqazi a=spilchen This change prevents the SET CLUSTER SETTING ... command from being throttled. Previously, it was subject to throttling, but since this command is used to install a new license and disable throttling, we want to ensure that the action itself is not throttled. This adds an exception to allow license updates. This will be backported to versions 24.2, 24.1, 23.2, and 23.1. Epic: CRDB-39988 Closes CRDB-42308 Release note: None Co-authored-by: Matt Spilchen <[email protected]>
cockroachdb · Sep 26, 2024 · a355dbe · a355dbe
2 parents 08e0006 + 3ac4dd7
commit a355dbe
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 14 deletions.
diff --git a/pkg/server/license/enforcer_test.go b/pkg/server/license/enforcer_test.go
@@ -324,19 +324,24 @@ func TestThrottleErrorMsg(t *testing.T) {
 		overThreshold   bool
 		throttleCheckTS time.Time
 		telemetryTS     time.Time
+		sql             string
 		errRE           string
 	}{
 		// NB: license expires at t30d and grace period ends at t60d.
-		{"at-threshold-valid-license-and-telemetry", true, t10d, t10d, ""},
-		{"at-threshold-expired-license-in-grace-valid-telemetry", true, t60d, t55d, ""},
+		{"at-threshold-valid-license-and-telemetry", true, t10d, t10d, "SELECT 1", ""},
+		{"at-threshold-expired-license-in-grace-valid-telemetry", true, t60d, t55d, "SELECT 1", ""},
 		{"at-threshold-expired-license-past-grace-valid-telemetry", true, t60d1ms, t55d,
-			"License expired .*. The maximum number of .*"},
-		{"below-threshold-expired-license-past-grace-valid-telemetry", false, t60d1ms, t55d, ""},
-		{"at-threshold-expired-license-in-grace-invalid-telemetry", true, t55d, t10d,
+			"SELECT 1", "License expired .*. The maximum number of .*"},
+		{"below-threshold-expired-license-past-grace-valid-telemetry", false, t60d1ms, t55d, "SELECT 1", ""},
+		{"at-threshold-expired-license-in-grace-invalid-telemetry", true, t55d, t10d, "SELECT 1",
 			"The maximum number of concurrently open transactions has been reached because the license requires diagnostic reporting"},
-		{"at-threshold-valid-license-invalid-telemetry", true, t10d, t0d,
+		{"at-threshold-valid-license-invalid-telemetry", true, t10d, t0d, "SELECT 1",
 			"The maximum number of concurrently open transactions has been reached because the license requires diagnostic reporting"},
-		{"below-threshold-invalid-telemetry", false, t10d, t0d, ""},
+		{"below-threshold-invalid-telemetry", false, t10d, t0d, "SELECT 1", ""},
+		{"at-threshold-expired-license-past-grace-valid-telemetry-SET_CLUSTER", true, t60d1ms, t55d,
+			"SET CLUSTER SETTING cluster.label = ''", ""},
+		{"at-threshold-expired-license-past-grace-invalid-telemetry-SET_CLUSTER", true, t60d1ms, t10d,
+			"SET CLUSTER SETTING cluster.label = ''", ""},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
 			// Adjust the throttle check time for this test unit
@@ -358,9 +363,9 @@ func TestThrottleErrorMsg(t *testing.T) {
 			// It may or may not be throttled, depending on tc parms.
 			tdb := sqlutils.MakeSQLRunner(sqlDB)
 			if tc.errRE != "" {
-				tdb.ExpectErr(t, tc.errRE, "SELECT 1")
+				tdb.ExpectErr(t, tc.errRE, tc.sql)
 			} else {
-				tdb.Exec(t, "SELECT 1")
+				tdb.Exec(t, tc.sql)
 			}
 
 			// Confirm that internal connections are never throttled

diff --git a/pkg/sql/conn_executor_exec.go b/pkg/sql/conn_executor_exec.go
@@ -505,11 +505,13 @@ func (ex *connExecutor) execStmtInOpenState(
 		}
 
 		// Enforce license policies. Throttling can occur if there is no valid
-		// license or if it has expired.
-		if notice, err := ex.server.cfg.LicenseEnforcer.MaybeFailIfThrottled(ctx, curOpen); err != nil {
-			return makeErrEvent(err)
-		} else if notice != nil {
-			res.BufferNotice(notice)
+		// license or if the existing one has expired.
+		if isSQLOkayToThrottle(ast) {
+			if notice, err := ex.server.cfg.LicenseEnforcer.MaybeFailIfThrottled(ctx, curOpen); err != nil {
+				return makeErrEvent(err)
+			} else if notice != nil {
+				res.BufferNotice(notice)
+			}
 		}
 	}
 
@@ -3446,3 +3448,16 @@ func (ex *connExecutor) execWithProfiling(
 	}
 	return err
 }
+
+// isSQLOkayToThrottle will return true if the given statement is allowed to be throttled.
+func isSQLOkayToThrottle(ast tree.Statement) bool {
+	switch ast.(type) {
+	// We do not throttle the SET CLUSTER command, as this is how a new license
+	// would be installed to disable throttling. We want to avoid the situation
+	// where the action to disable throttling is itself throttled.
+	case *tree.SetClusterSetting:
+		return false
+	default:
+		return true
+	}
+}