-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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/stats: add table setting to disable generation of stats forecasts #86986
Conversation
Add a new cluster setting `sql.stats.forecasts.enabled` that can be used to disable generation of statistics forecasts in the stats cache. Changing this setting will cause the stats cache to gradually evict and reload all cached statistics as they are requested. Fixes: cockroachdb#86350 Release justification: Low-risk update to new functionality. Release note (sql change): Add a new cluster setting `sql.stats.forecasts.enabled` which controls whether statistics forecasts are generated by default for all tables. (Note that this is different from the session setting `optimizer_use_forecasts` which controls whether statistics forecasts are used when optimizing the current query. If forecasting is disabled, then even if `optimizer_use_forecasts` is true for a given query it won't have any forecasts to use.)
Note that the first commit is from #86932 and can be ignored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 8 of 8 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @michae2 and @msirek)
pkg/sql/opt/exec/execbuilder/testdata/forecast
line 743 at r2 (raw file):
statement ok CREATE TABLE d (d DATE PRIMARY KEY) WITH (sql_stats_automatic_collection_enabled = false, sql_stats_forecasts_enabled = false)
Is it worth also testing the other direction, that we can explicitly enable stats on a table even if the cluster setting is false?
Previously, rytaft (Rebecca Taft) wrote…
(by stats I mean forecasts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @michae2)
Add a new storage parameter (a.k.a. table setting) which can be used to override cluster setting `sql.stats.forecasts.enabled`. This builds on work done earlier in cockroachdb#79025 and cockroachdb#86932 so we don't have to do much, just wire it all together. Fixes: cockroachdb#86353 Release justification: Low-risk update to new functionality. Release note (sql change): Add a new table setting `sql_stats_forecasts_enabled` which controls whether statistics forecasts are generated for a specific table. When set, this overrides cluster setting `sql.stats.forecasts.enabled`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the Saturday reviews!
bors r=rytaft,msirek
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @rytaft)
pkg/sql/opt/exec/execbuilder/testdata/forecast
line 743 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
(by stats I mean forecasts)
Yes, definitely worth testing, and good news is that I did sneakily include a test like this by adding sql_stats_forecasts_enabled = true
to CREATE TABLE c
up above. But that was too subtle, so I've moved it to an ALTER TABLE c
down below with a comment.
Build succeeded: |
Add a new storage parameter (a.k.a. table setting) which can be used to
override cluster setting
sql.stats.forecasts.enabled
. This builds onwork done earlier in #79025 and #86932 so we don't have to do much, just
wire it all together.
Fixes: #86353
Release justification: Low-risk update to new functionality.
Release note (sql change): Add a new table setting
sql_stats_forecasts_enabled
which controls whether statisticsforecasts are generated for a specific table. When set, this overrides
cluster setting
sql.stats.forecasts.enabled
.