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
release-23.1.20-rc: stats: more conservative lower bound for downward-trending forecasts #122990
release-23.1.20-rc: stats: more conservative lower bound for downward-trending forecasts #122990
Conversation
Instead of 0 as a lower bound for predictions, use 1/3 times the lowest prior observation. This will avoid predicting zero rows for a statistic unless we really observed zero rows somewhat recently. Fixes: cockroachdb#119967 Release note (bug fix): Statistics forecasts of zero rows can cause bad plans. This commit changes forecasting to avoid predicting zero rows for most downward-trending statistics.
Thanks for opening a backport. Please check the backport criteria before merging:
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
Also, please add a brief release justification to the body of your PR to justify this |
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 3 of 3 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner)
Fixes for the tests coming up, but this is RFAL. |
fd4a722
to
ad6f459
Compare
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 3 of 3 files at r1, 4 of 4 files at r3, 5 of 5 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @michae2)
pkg/sql/stats/forecast.go
Outdated
"sql.stats.forecasts.max_decrease", | ||
"the most a prediction is allowed to decrease, expressed as the minimum ratio of the prediction "+ | ||
"to the lowest prior observation", | ||
1.0/3.0, |
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.
Should we set this to 1 by default in backports to retain the old behavior? Would that reduce risk of this change?
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.
Good point, I will do that.
ad6f459
to
8294fe2
Compare
Turn three constants used for statistics forecasting into cluster settings. (These must be cluster settings rather than session variables because forecasting happens when reloading table statistics in the stats cache, which can be independent of any session.) Informs: cockroachdb#119967 Release note (sql change): Introduce three new cluster settings for controlling table statistics forecasting: 1. `sql.stats.forecasts.min_observations` is the minimum number of observed statistics required to produce a forecast. 2. `sql.stats.forecasts.min_goodness_of_fit` is the minimum R² (goodness of fit) measurement required from all predictive models to use a forecast. 3. `sql.stats.forecasts.max_decrease` is the most a prediction can decrease, expressed as the minimum ratio of the prediction to the lowest prior observation.
8294fe2
to
15a5b0e
Compare
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 3 of 3 files at r1, 4 of 4 files at r3, 5 of 5 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner)
Backport:
/cc @cockroachdb/release
stats: more conservative lower bound for downward-trending forecasts
Instead of 0 as a lower bound for predictions, use 1/3 times the lowest prior observation. This will avoid predicting zero rows for a statistic unless we really observed zero rows somewhat recently.
Fixes: #119967
Release note (bug fix): Statistics forecasts of zero rows can cause bad plans. This commit changes forecasting to avoid predicting zero rows for most downward-trending statistics.
sql/stats: turn forecasting constants into cluster settings
Turn three constants used for statistics forecasting into cluster settings. (These must be cluster settings rather than session variables because forecasting happens when reloading table statistics in the stats cache, which can be independent of any session.)
Informs: #119967
Release note (sql change): Introduce three new cluster settings for controlling table statistics forecasting:
sql.stats.forecasts.min_observations
is the minimum number of observed statistics required to produce a forecast.sql.stats.forecasts.min_goodness_of_fit
is the minimum R² (goodness of fit) measurement required from all predictive models to use a forecast.sql.stats.forecasts.max_decrease
is the most a prediction can decrease, expressed as the minimum ratio of the prediction to the lowest prior observation.Release justification: specially-requested fix for plan regression bug.