-
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
release-23.2.6-rc: sql/stats: evict stats cache entry if user-defined types have changed #124854
release-23.2.6-rc: sql/stats: evict stats cache entry if user-defined types have changed #124854
Conversation
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 |
When adding table statistics to the stats cache, we decode histogram upper bounds into datums. If the histogram column uses a user-defined type, we hydrate the type and use this to decode. In statistics builder, these histogram upper bound datums are compared against datums in spans and constraints. The comparisons assume that the datums are of equivalent type, but if the user-defined type has changed sometime after loading the stats cache entry, this might not be true. If the user-defined type has changed, we need to evict and re-load the stats cache entry so that we decode histogram datums with a freshly- hydrated type. (We were already checking UDT versions when building the optTable in sql.(*optCatalog).dataSourceForTable, but the newly-built optTable used the existing table statistics instead of refreshing those, too.) Fixes: cockroachdb#124181 Release note (bug fix): Fix a bug where a change to a user-defined type could cause queries against tables using that type to fail with an error message like: "histogram.go:694: span must be fully contained in the bucket" The change to the user-defined type could come directly from an ALTER TYPE statement, or indirectly from an ALTER DATABASE ADD REGION or DROP REGION statement (which implicitly change the crdb_internal_region type). This bug has existed since UDTs were introduced in v20.2.
2d14052
to
43fe2b0
Compare
I looked in depth at the Bazel Extended CI timeouts, and they seem to be the normal timeouts for stress-race, experienced frequently even before this PR. There is no deadlock like I feared. |
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 11 of 11 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @michae2 and @rafiss)
pkg/sql/stats/stats_cache.go
line 320 at r1 (raw file):
// getTableStatsFromCache is like GetTableStats but assumes that the table ID // is safe to fetch statistics for: non-system, non-virtual, non-view, etc.
nit for follow-up PR: explain the udtCols argument.
Backport 1/3 commits from #124603.
/cc @cockroachdb/release
sql/stats: evict stats cache entry if user-defined types have changed
When adding table statistics to the stats cache, we decode histogram upper bounds into datums. If the histogram column uses a user-defined type, we hydrate the type and use this to decode.
In statistics builder, these histogram upper bound datums are compared against datums in spans and constraints. The comparisons assume that the datums are of equivalent type, but if the user-defined type has changed sometime after loading the stats cache entry, this might not be true.
If the user-defined type has changed, we need to evict and re-load the stats cache entry so that we decode histogram datums with a freshly-hydrated type.
(We were already checking UDT versions when building the optTable in sql.(*optCatalog).dataSourceForTable, but the newly-built optTable used the existing table statistics instead of refreshing those, too.)
Fixes: #124181
Release note (bug fix): Fix a bug where a change to a user-defined type could cause queries against tables using that type to fail with an error message like:
The change to the user-defined type could come directly from an ALTER TYPE statement, or indirectly from an ALTER DATABASE ADD REGION or DROP REGION statement (which implicitly change the crdb_internal_region type).
This bug has existed since UDTs were introduced in v20.2.
Release justification: fix for a serious production issue.