Merge #122285

122285: sql,multiregionccl: rename multiregion system DB cluster setting r=rafiss a=rafiss Avoid using the word "preview" in the name. That shouldn't be needed, since the cluster setting is hidden, so users would only know about it if we tell them to enable it. Keeping "preview" in the name isn't a huge problem, but it's just slightly annoying if we decide that we want to keep the setting around even after the feature leaves the "preview" status, since it requires updating docs and any customer code that was using the old name. Also, this removes the mention of the setting in error messages. An error should not refer to an undocumented setting, since a user would have no way of learning more about it. informs: #63365 Epic: CRDB-33032 Release note: None Co-authored-by: Rafi Shamim <[email protected]>
cockroachdb · May 8, 2024 · af7d2f4 · af7d2f4
2 parents ba6b500 + 7ab3f77
commit af7d2f4
Show file tree

Hide file tree

Showing 5 changed files with 11 additions and 14 deletions.
diff --git a/pkg/ccl/logictestccl/testdata/logic_test/multi_region b/pkg/ccl/logictestccl/testdata/logic_test/multi_region
@@ -544,7 +544,7 @@ ALTER DATABASE system PRIMARY REGION "ap-southeast-2"
 # are in the system tenant. Hence the lack of error code checking and the
 # regex matching both possible outcomes.
 skipif config multiregion-9node-3region-3azs-tenant
-statement error user testuser may not modify the system database|Modifying the regions of system database is not supported. Set up your system database as multi-region using the cluster setting `sql.multiregion.preview_multiregion_system_database.enabled` https://www.cockroachlabs.com/docs/stable/cluster-settings
+statement error (user testuser may not modify the system database|modifying the regions of system database is not supported)
 ALTER DATABASE system PRIMARY REGION "ap-southeast-2"
 
 user root

diff --git a/pkg/ccl/multiregionccl/multiregion_system_table_test.go b/pkg/ccl/multiregionccl/multiregion_system_table_test.go
@@ -87,7 +87,7 @@ func TestMrSystemDatabase(t *testing.T) {
 
 	sDB := sqlutils.MakeSQLRunner(systemSQL)
 
-	sDB.Exec(t, `SET CLUSTER SETTING sql.multiregion.preview_multiregion_system_database.enabled = true`)
+	sDB.Exec(t, `SET CLUSTER SETTING sql.multiregion.system_database_multiregion.enabled = true`)
 	sDB.Exec(t, `ALTER DATABASE system SET PRIMARY REGION "us-east1"`)
 	sDB.Exec(t, `ALTER DATABASE system ADD REGION "us-east2"`)
 	sDB.Exec(t, `ALTER DATABASE system ADD REGION "us-east3"`)

diff --git a/pkg/cmd/roachtest/tests/multi_region_system_database.go b/pkg/cmd/roachtest/tests/multi_region_system_database.go
@@ -49,7 +49,7 @@ func registerMultiRegionSystemDatabase(r registry.Registry) {
 
 			require.NoError(t, WaitFor3XReplication(ctx, t, t.L(), conn))
 
-			_, err := conn.ExecContext(ctx, "SET CLUSTER SETTING sql.multiregion.preview_multiregion_system_database.enabled = true")
+			_, err := conn.ExecContext(ctx, "SET CLUSTER SETTING sql.multiregion.system_database_multiregion.enabled = true")
 			require.NoError(t, err)
 
 			_, err = conn.ExecContext(ctx,

diff --git a/pkg/sql/alter_database.go b/pkg/sql/alter_database.go
@@ -583,22 +583,19 @@ func (p *planner) checkPrivilegesForMultiRegionOp(
 	ctx context.Context, desc catalog.Descriptor,
 ) error {
 
-	// Ensure that only secondary tenants may have their system database
-	// set up for multi-region operations. Even then, ensure that only the
-	// node user may configure the system database.
+	// Unless MultiRegionSystemDatabaseEnabled is true, only secondary tenants may
+	// have their system database set up for multi-region operations. Even then,
+	// ensure that only the node user may configure the system database.
 	//
-	// TODO(ajwerner): Adopt the auto-config infrastructure for configuring
-	// multi-region primitives in the system database. For now, we also allow
-	// root to perform the various operations to enable testing.
+	// TODO(rafi): For now, we also allow root to perform the various operations
+	// to enable testing. When MultiRegionSystemDatabaseEnabled leaves preview,
+	// we may want to allow any admin user to perform these operations as well.
 	if desc.GetID() == keys.SystemDatabaseID {
 		if multiRegionSystemDatabase := sqlclustersettings.MultiRegionSystemDatabaseEnabled.Get(&p.execCfg.Settings.SV); !multiRegionSystemDatabase &&
 			p.execCfg.Codec.ForSystemTenant() {
 			return pgerror.Newf(
 				pgcode.FeatureNotSupported,
-				"Modifying the regions of system database is not supported. "+
-					"Set up your system database as multi-region using the cluster setting "+
-					"`sql.multiregion.preview_multiregion_system_database.enabled` "+
-					"https://www.cockroachlabs.com/docs/stable/cluster-settings.",
+				"modifying the regions of system database is not supported",
 			)
 		}
 		if u := p.SessionData().User(); !u.IsNodeUser() && !u.IsRootUser() {

diff --git a/pkg/sql/sqlclustersettings/clustersettings.go b/pkg/sql/sqlclustersettings/clustersettings.go
@@ -83,7 +83,7 @@ var SecondaryTenantsAllZoneConfigsEnabled = settings.RegisterBoolSetting(
 // to be set up to be multi-region.
 var MultiRegionSystemDatabaseEnabled = settings.RegisterBoolSetting(
 	settings.SystemVisible,
-	"sql.multiregion.preview_multiregion_system_database.enabled",
+	"sql.multiregion.system_database_multiregion.enabled",
 	"enable option to set up system database as multi-region",
 	false,
 )