sql: remove sql.defaults.optimizer_use_not_visible_indexes.enabled by michae2 · Pull Request #86634 · cockroachdb/cockroach

michae2 · 2022-08-23T05:10:38Z

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.

cockroach-teamcity · 2022-08-23T05:10:48Z

This change is

mgartner

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")
    }
}

wenyihu6

The first commit Thanks for fixing this!!

Reviewed all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @michae2, @rafiss, and @rytaft)

rafiss

thanks for the change! let's not have a release note on this commit. a user reading the notes won't be able to make sense of it, since we never released a CRDB version that includes this cluster setting

rafiss

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @mgartner and @rytaft)

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?

rafiss

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @mgartner and @rytaft)

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

wenyihu6 · 2022-08-23T15:18:36Z

pkg/sql/exec_util.go line 402 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

using a constant for maxSQLDefaults could have an issue if a setting is retired and reduces the "expected" size of the allowed settings list

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

rafiss

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @mgartner, @rytaft, and @wenyihu6)

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.

wenyihu6 · 2022-08-23T15:44:37Z

pkg/sql/exec_util.go line 402 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

no problem. then perhaps we can make the error message more clear so that someone else doesn't do the same thing.

Adding comments on top of this map can also be useful.

michae2

TFTRs!

Removed the release note (instead I left a comment on cockroachdb/docs#14819).

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @mgartner, @rafiss, @rytaft, and @wenyihu6)

-- commits line 11 at r1:

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.

rafiss

Reviewed 3 of 6 files at r1, 2 of 5 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @mgartner, @rafiss, @rytaft, and @wenyihu6)

michae2 · 2022-08-23T17:26:29Z

bors r=mgartner,wenyihu6,rafiss

craig · 2022-08-23T17:43:48Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

michae2 · 2022-08-23T17:44:59Z

bors r-

craig · 2022-08-23T17:45:02Z

Canceled.

michae2 · 2022-08-23T21:06:05Z

Trying again.

bors r=mgartner,wenyihu6,rafiss

mgartner · 2022-08-23T21:34:44Z

pkg/settings/registry.go

+// associated sql.defaults cluster setting. Instead they can have their default
+// changed with ALTER ROLE ... SET.
 var sqlDefaultSettings = map[string]struct{}{
+	// PLEASE DO NOT ADD NEW SETTINGS TO THIS MAP. THANK YOU.


🖖

Thanks for adding these warnings!

craig · 2022-08-23T23:28:27Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-08-23T23:55:16Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-08-24T00:25:23Z

Build failed:

Bazel Essential CI (Cockroach)

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

michae2 · 2022-08-24T05:19:59Z

Third time's the charm.

bors r=mgartner,wenyihu6,rafiss

michae2 · 2022-08-24T06:09:15Z

bors r-

craig · 2022-08-24T06:09:17Z

Canceled.

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

michae2 · 2022-08-24T18:03:17Z

"It's definitely going to work this time."

bors r=mgartner,wenyihu6,rafiss

mgartner · 2022-08-24T18:23:03Z

The second commit makes progress towards #72434. @michae2 do you think there are other session settings that we should add to the statement bundle, or should we just close #72434?

michae2 · 2022-08-24T18:55:08Z

The second commit makes progress towards #72434. @michae2 do you think there are other session settings that we should add to the statement bundle, or should we just close #72434?

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.

craig · 2022-08-24T21:51:26Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-08-25T00:16:11Z

Build succeeded:

Bazel Essential CI (Cockroach)

michae2 requested review from a team, rafiss, rytaft and wenyihu6 August 23, 2022 05:10

mgartner approved these changes Aug 23, 2022

View reviewed changes

wenyihu6 approved these changes Aug 23, 2022

View reviewed changes

rafiss requested changes Aug 23, 2022

View reviewed changes

rafiss reviewed Aug 23, 2022

View reviewed changes

michae2 mentioned this pull request Aug 23, 2022

sql: setting to disable NotVisible index feature cockroachdb/docs#14819

Closed

michae2 force-pushed the optusenot branch from c31f0f5 to 3f36773 Compare August 23, 2022 16:53

michae2 requested a review from a team as a code owner August 23, 2022 16:53

michae2 commented Aug 23, 2022

View reviewed changes

rafiss approved these changes Aug 23, 2022

View reviewed changes

michae2 force-pushed the optusenot branch from 3f36773 to 5f82c5f Compare August 23, 2022 19:03

mgartner reviewed Aug 23, 2022

View reviewed changes

sql: remove sql.defaults.optimizer_use_not_visible_indexes.enabled

e3601c5

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

michae2 force-pushed the optusenot branch from 5f82c5f to 129f1de Compare August 24, 2022 05:19

michae2 force-pushed the optusenot branch 2 times, most recently from 917c5de to 6208560 Compare August 24, 2022 14:07

sql: add more session variables to statement bundles

96597d8

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

michae2 force-pushed the optusenot branch from 6208560 to 96597d8 Compare August 24, 2022 16:39

michae2 mentioned this pull request Aug 24, 2022

sql/opt: add session setting to disable stats forecast use in optimizer #86834

Merged

craig bot merged commit 2787ff1 into cockroachdb:master Aug 25, 2022

michae2 deleted the optusenot branch August 25, 2022 04:33

Conversation

michae2 commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Aug 23, 2022

Uh oh!

mgartner left a comment

Choose a reason for hiding this comment

Uh oh!

wenyihu6 left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

wenyihu6 commented Aug 23, 2022

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

wenyihu6 commented Aug 23, 2022

Uh oh!

michae2 left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

michae2 commented Aug 23, 2022

Uh oh!

craig bot commented Aug 23, 2022

Uh oh!

michae2 commented Aug 23, 2022

Uh oh!

craig bot commented Aug 23, 2022

Uh oh!

michae2 commented Aug 23, 2022

Uh oh!

mgartner Aug 23, 2022

Choose a reason for hiding this comment

Uh oh!

craig bot commented Aug 23, 2022

Uh oh!

craig bot commented Aug 23, 2022

Uh oh!

craig bot commented Aug 24, 2022

Uh oh!

michae2 commented Aug 24, 2022

Uh oh!

michae2 commented Aug 24, 2022

Uh oh!

craig bot commented Aug 24, 2022

Uh oh!

michae2 commented Aug 24, 2022

Uh oh!

mgartner commented Aug 24, 2022

Uh oh!

michae2 commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

craig bot commented Aug 24, 2022

Uh oh!

craig bot commented Aug 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

michae2 commented Aug 23, 2022 •

edited

Loading

michae2 commented Aug 24, 2022 •

edited

Loading