Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: default index recommendations to be off for logic tests #74154

Closed
wants to merge 2 commits into from

Conversation

nehageorge
Copy link

@nehageorge nehageorge commented Dec 21, 2021

sql: refactor GlobalDefault for session variables

This commit refactors pkg/sql/vars.go to use globalFalse and
globalTrue as the setting GlobalDefault where possible.

Release note: None

sql: default index recommendations to be off for logic tests

Previously, we did not have a cluster setting for index recommendations.
We add one in this commit in order to configure index recommendations to
be off for logic tests. This is to avoid flaky tests, as the index
recommendation output can vary depending on the best plan chosen by the
optimizer.

However, this cluster setting is temporary and will be migrated to
ALTER ROLE ALL for 22.1 (along with other sql.defaults cluster settings).

Fixes: #74069.

Release note: None

@nehageorge nehageorge requested a review from a team as a code owner December 21, 2021 16:30
@cockroach-teamcity
Copy link
Member

This change is Reviewable

This commit refactors `pkg/sql/vars.go` to use globalFalse and
globalTrue as the setting GlobalDefault where possible.

Release note: None
Previously, we did not have a cluster setting for index recommendations.
We add one in this commit in order to configure index recommendations to
be off for logic tests. This is to avoid flaky tests, as the index
recommendation output can vary depending on the best plan chosen by the
optimizer.

However, this cluster setting is temporary and will be migrated to
`ALTER ROLE ALL` for 22.1 (along with other `sql.defaults` cluster settings).

Fixes: cockroachdb#74069.

Release note: None
Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r1, 48 of 48 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @nehageorge)

@mgartner mgartner requested a review from rafiss December 21, 2021 16:51
@nehageorge
Copy link
Author

Closing this in favour of #74159. We want to avoid adding new cluster settings as we add the new ALTER ROLE ALL syntax in 22.1.

@nehageorge nehageorge closed this Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql/opt/exec/execbuilder: TestExecBuild failed
3 participants