sql: remove sql.defaults.optimizer_use_not_visible_indexes.enabled #86634
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 6 of 6 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @michae2, @rafiss, @rytaft, and @wenyihu6)
-- commits line 11 at r1:
I don't think a release note is necessary because the setting is not included in any releases.
pkg/sql/exec_util.go line 402 at r1 (raw file):
var optUseNotVisibleIndexesClusterMode = settings.RegisterBoolSetting( settings.TenantWritable, "sql.defaults.optimizer_use_not_visible_indexes.enabled",
Looks like the check added in #80575 isn't stern enough. Maybe we should add something like this to pkg/settings/registry.go:
// WARNING: Nevertincrease this constant.
const maxSQLDefaults = 48;
func init() {
if len(sqlDefaultSettings) > maxSQLDefaults {
panic("new sql.defaults cluster settings are not needed now that `ALTER ROLE ... SET` syntax is supported")
}
}
There was a problem hiding this comment.
The first commit 
Reviewed all commit messages.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec_util.go line 402 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Looks like the check added in #80575 isn't stern enough. Maybe we should add something like this to
pkg/settings/registry.go:// WARNING: Nevertincrease this constant. const maxSQLDefaults = 48; func init() { if len(sqlDefaultSettings) > maxSQLDefaults { panic("new sql.defaults cluster settings are not needed now that `ALTER ROLE ... SET` syntax is supported") } }
i don't quite understand why the existing check didn't work. do you know?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec_util.go line 402 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
i don't quite understand why the existing check didn't work. do you know?
using a constant for maxSQLDefaults could have an issue if a setting is retired and reduces the "expected" size of the allowed settings list
|
Previously, rafiss (Rafi Shamim) wrote…
That was my bad. I was following the existing pattern for other session variables and didn't realize that this has been deprecated. The commit got around the check by adding the session variable to this map. Any session variables added to this map doesn't throw an error |
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec_util.go line 402 at r1 (raw file):
Previously, wenyihu6 (Wenyi Hu) wrote…
That was my bad. I was following the existing pattern for other session variables and didn't realize that this has been deprecated. The commit got around the check by adding the session variable to this map. Any session variables added to this map doesn't throw an error
no problem. then perhaps we can make the error message more clear so that someone else doesn't do the same thing.
|
Previously, rafiss (Rafi Shamim) wrote…
Adding comments on top of this map can also be useful. |
There was a problem hiding this comment.
TFTRs!
Removed the release note (instead I left a comment on cockroachdb/docs#14819).
Reviewable status:
Previously, mgartner (Marcus Gartner) wrote…
I don't think a release note is necessary because the setting is not included in any releases.
Done.
pkg/sql/exec_util.go line 402 at r1 (raw file):
Previously, wenyihu6 (Wenyi Hu) wrote…
Adding comments on top of this map can also be useful.
I tried making the error message more clear, and added some comments.
|
bors r=mgartner,wenyihu6,rafiss |
|
Build failed (retrying...): |
|
bors r- |
|
Canceled. |
|
Trying again. bors r=mgartner,wenyihu6,rafiss |
| var sqlDefaultSettings = map[string]struct{}{ | ||
| // PLEASE DO NOT ADD NEW SETTINGS TO THIS MAP. THANK YOU. |
There was a problem hiding this comment.
🖖
Thanks for adding these warnings!
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build failed: |
In 22.2 the use of cluster settings for session setting defaults is deprecated, but I forgot this when reviewing cockroachdb#86033. Remove the unreleased default cluster setting we added. Release note: None
|
Third time's the charm. bors r=mgartner,wenyihu6,rafiss |
|
bors r- |
|
Canceled. |
michae2
force-pushed
the
optusenot
branch
2 times, most recently
from
917c5de to
6208560
Compare
Add some missing session variables to statement diagnostic bundles. This isn't quite everything, but it's better than what we had before. Release note: None
|
"It's definitely going to work this time." bors r=mgartner,wenyihu6,rafiss |
This was just a quick-and-dirty attempt to add more while I was in the area. I think I probably missed a few, and something more systematic (i.e. automatically adding all session settings) would be better. So I left the TODO in the code, and I think we should leave #72434 open. |
|
Build failed (retrying...): |
|
Build succeeded: |
sql: remove sql.defaults.optimizer_use_not_visible_indexes.enabled
In 22.2 the use of cluster settings for session setting defaults is
deprecated, but I forgot this when reviewing #86033. Remove the
unreleased default cluster setting we added.
Release note: None
sql: add more session variables to statement bundles
Add some missing session variables to statement diagnostic bundles. This
isn't quite everything, but it's better than what we had before.
Release note: None
Release justification: low-risk bug fix.