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
Make sanity check of settings worse #63119
Conversation
This is an automated comment for commit 5355dec with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page
Successful checks
|
if (max_threads > getNumberOfPhysicalCPUCores() * 65536) | ||
throw Exception(ErrorCodes::INVALID_SETTING_VALUE, "Sanity check: Too many threads requested ({})", max_threads); | ||
UInt64 max_threads_max_value = 256 * getNumberOfPhysicalCPUCores(); | ||
if (max_threads > max_threads_max_value) |
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.
Maybe log the fact that clamping happened, so when somebody gets surprised, we can check the logs.
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.
Logs can rotate quite quickly. I would love to see this information in system.errors
as well.
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.
I'll add some logs. Adding to system.errors
might come in the future since we need extra code to support this (right now we only log exceptions) and I'd rather not backport that part.
|
||
EXPLAIN PIPELINE SELECT zero + 1 AS x FROM system.zeros SETTINGS max_block_size = 9223372036854775806, max_rows_to_read = 20, read_overflow_mode = 'break'; -- { serverError INVALID_SETTING_VALUE } | ||
EXPLAIN PIPELINE SELECT zero + 1 AS x FROM system.zeros LIMIT 10 SETTINGS max_block_size = 9223372036854775806, max_rows_to_read = 20, read_overflow_mode = 'break'; |
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.
Maybe we can do something like:
SET max_block_size =9223372036854775806;
SELECT value FROM system.settings WHERE name = 'max_block_size';
415a7be
…6f746fb25d73a43a27f6f39c361a5 Cherry pick #63119 to 24.2: Make sanity check of settings worse
…6f746fb25d73a43a27f6f39c361a5 Cherry pick #63119 to 24.3: Make sanity check of settings worse
Backport #63119 to 24.2: Make sanity check of settings worse
Backport #63119 to 24.3: Make sanity check of settings worse
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Documentation entry for user-facing changes
When I introduced setting sanity checks in 24.2 (#60138) I didn't anticipate anybody except the fuzzer would be using such trash values. I was wrong, and this led to problems during upgrades.
Introducing a setting to allow skipping sanity checks is stupid, as the sanity checks are necessary to prevent crashes that otherwise would require hundreds to different checks whenever some settings are used, so I intend to introduce 2 modes of sanity checks: the one introduced initially, where we throw and error, and the one I'm using to replace temporarily now where we clamp the values to something "valid" (still unreasonable in most cases).
This will be done in 2 stages:
compatibility
setting.