sql: add a SET CLUSTER SETTING sql.defaults deprecation notice #80548
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
e-mbrown
requested review from
a team and
msbutler
and removed request for
a team
msbutler
approved these changes
Apr 26, 2022
otan
reviewed
Apr 26, 2022
pkg/sql/set_cluster_setting.go
Outdated
|
|
||
| if strings.Contains(n.name, "sql.defaults") { | ||
| params.p.BufferClientNotice(params.ctx, | ||
| pgnotice.Newf("The `SET CLUSTER SETTING sql.defaults...` syntax is deprecated, please "+ |
There was a problem hiding this comment.
perhaps
params.p.BufferClientNotice(
params.ctx,
errors.WithHintf(
pgnotice.Newf("`SET CLUSTER SETTING %s` syntax is deprecated", n.name),
"use `ALTER ROLE ... SET` which allows finger-grained control. See %s",
docs.URL("alter-role.html#set-default-session-variable-values-for-a-role"),
),
)
There was a problem hiding this comment.
i think we should use a bit softer language than "deprecated" -- usually deprecated means that we don't support it or it will go away eventually
in this case, i'd view the notice as more of a helpful hint. like
"setting a global default; use the `ALTER ROLE ... SET` syntax to control session variable defaults at a finer-grained level"
e-mbrown
force-pushed
the
eb/sqldeflts
branch
2 times, most recently
from
e31f7ea to
431a4ce
Compare
otan
reviewed
Apr 28, 2022
pkg/sql/set_cluster_setting.go
Outdated
| errors.WithHintf( | ||
| pgnotice.Newf("Setting global default %s`;", n.name), | ||
| "use the `ALTER ROLE ... SET` syntax to control session variable defaults at a finer-grained level. See: %s", | ||
| docs.URL("https://www.cockroachlabs.com/docs/stable/alter-role.html#set-default-session-variable-values-for-a-role"), |
There was a problem hiding this comment.
this should just be alter-role.html#set-default-session-variable-values-for-a-role
otan
reviewed
Apr 28, 2022
pkg/sql/set_cluster_setting.go
Outdated
| @@ -193,6 +195,18 @@ func (p *planner) getAndValidateTypedClusterSetting( | |||
| } | |||
|
|
|||
| func (n *setClusterSettingNode) startExec(params runParams) error { | |||
|
|
|||
| if strings.Contains(n.name, "sql.defaults") { | |||
pkg/sql/set_cluster_setting.go
Outdated
| params.p.BufferClientNotice( | ||
| params.ctx, | ||
| errors.WithHintf( | ||
| pgnotice.Newf("Setting global default %s`;", n.name), |
There was a problem hiding this comment.
nit: errors start with lowercase, so "setting global default". remove the semicolon ; as well
otan
reviewed
May 2, 2022
pkg/sql/set_cluster_setting.go
Outdated
| params.p.BufferClientNotice( | ||
| params.ctx, | ||
| errors.WithHintf( | ||
| pgnotice.Newf("setting global default %s`", n.name), |
There was a problem hiding this comment.
maybe" setting global default %s is not recommended" (remove the backtick at the end)
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.
complete! 0 of 0 LGTMs obtained (waiting on 
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #80325
Added a deprecation notice for when
SET CLUSTER SETTINGS sql.defaults...is used. The notice directs users to use the
ALTER ROLEsyntax insteadRelease note: None