From a1f602cac06561d75183fccb1e072c9e19e2361f Mon Sep 17 00:00:00 2001 From: e-mbrown Date: Mon, 25 Apr 2022 13:23:44 -0400 Subject: [PATCH] sql: add a SET CLUSTER SETTING deprecation notice Release note (sql change): The `ALTER ROLE` syntax` allows users to set default values for session variables making `SET CLUSTER SETTINGS sql.defaults...` redundant. This PR adds a notice to `SET CLUSTER SETTINGS sql.defaults...` that directs the user to use the `ALTER ROLE` syntax instead. --- .../backupccl/testdata/backup-restore/multiregion | 6 ++++++ pkg/sql/set_cluster_setting.go | 14 ++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/pkg/ccl/backupccl/testdata/backup-restore/multiregion b/pkg/ccl/backupccl/testdata/backup-restore/multiregion index f49c4c423194..078b1c601bae 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/multiregion +++ b/pkg/ccl/backupccl/testdata/backup-restore/multiregion @@ -95,6 +95,8 @@ DROP DATABASE no_region_db_2; exec-sql SET CLUSTER SETTING sql.defaults.primary_region = 'non-existent-region'; ---- +NOTICE: setting global default sql.defaults.primary_region is not recommended +HINT: use the `ALTER ROLE ... SET` syntax to control session variable defaults at a finer-grained level. See: https://www.cockroachlabs.com/docs/v22.1/alter-role.html#set-default-session-variable-values-for-a-role exec-sql RESTORE DATABASE no_region_db FROM LATEST IN 'nodelocal://1/no_region_database_backup/'; @@ -107,6 +109,8 @@ set the default PRIMARY REGION to a region that exists (see SHOW REGIONS FROM CL exec-sql SET CLUSTER SETTING sql.defaults.primary_region = 'eu-central-1'; ---- +NOTICE: setting global default sql.defaults.primary_region is not recommended +HINT: use the `ALTER ROLE ... SET` syntax to control session variable defaults at a finer-grained level. See: https://www.cockroachlabs.com/docs/v22.1/alter-role.html#set-default-session-variable-values-for-a-role exec-sql RESTORE DATABASE no_region_db FROM LATEST IN 'nodelocal://1/no_region_database_backup/'; @@ -146,6 +150,8 @@ new-server name=s4 share-io-dir=s1 allow-implicit-access localities=eu-central-1 exec-sql SET CLUSTER SETTING sql.defaults.primary_region = 'eu-north-1'; ---- +NOTICE: setting global default sql.defaults.primary_region is not recommended +HINT: use the `ALTER ROLE ... SET` syntax to control session variable defaults at a finer-grained level. See: https://www.cockroachlabs.com/docs/v22.1/alter-role.html#set-default-session-variable-values-for-a-role exec-sql RESTORE FROM LATEST IN 'nodelocal://1/no_region_cluster_backup/'; diff --git a/pkg/sql/set_cluster_setting.go b/pkg/sql/set_cluster_setting.go index 0ab76f88400e..08589dd7a267 100644 --- a/pkg/sql/set_cluster_setting.go +++ b/pkg/sql/set_cluster_setting.go @@ -20,6 +20,7 @@ import ( "time" "github.com/cockroachdb/cockroach/pkg/clusterversion" + "github.com/cockroachdb/cockroach/pkg/docs" "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security" @@ -31,6 +32,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/paramparse" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice" "github.com/cockroachdb/cockroach/pkg/sql/roleoption" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" @@ -193,6 +195,18 @@ func (p *planner) getAndValidateTypedClusterSetting( } func (n *setClusterSettingNode) startExec(params runParams) error { + + if strings.HasPrefix(n.name, "sql.defaults") { + params.p.BufferClientNotice( + params.ctx, + errors.WithHintf( + pgnotice.Newf("setting global default %s is not recommended", n.name), + "use the `ALTER ROLE ... SET` syntax to control session variable defaults at a finer-grained level. See: %s", + docs.URL("alter-role.html#set-default-session-variable-values-for-a-role"), + ), + ) + } + if !params.extendedEvalCtx.TxnIsSingleStmt { return errors.Errorf("SET CLUSTER SETTING cannot be used inside a multi-statement transaction") }