Clamp max_streams_for_merge_tree_reading to avoid std::length_error

Project: ClickHouse/ClickHouse | Language: C++ | Concepts: error-handling, configuration-management, testing-strategy | Difficulty: mid
Link: https://github.com/ClickHouse/ClickHouse/pull/110936

The problem

If the max_streams_for_merge_tree_reading setting was set to a very large value (near UINT64_MAX), it could flow unchecked into code that calls std::vector::reserve() with that value. This would throw a std::length_error (because the requested size exceeds what a vector can hold), causing the ClickHouse server to abort with a LOGICAL_ERROR. This could be triggered by a user query with a pathological setting.

Why it's non-trivial

Simply checking for large values at the crash site (where reserve() is called) would only fix one instance and could miss other code paths that use the same setting. The setting can also be amplified through other configuration ratios, so a robust fix requires bounding the value at ingestion (when the setting is read/applied), ensuring all downstream consumers are protected.

The approach

1. Clamp the setting at ingestion

In doSettingsSanityCheckClamp, the code now checks if the value is above a safe ceiling (256 * getNumberOfCPUCoresToUse(), same as max_threads) and clamps it if necessary:

+    /// Same ceiling as max_threads: an unbounded value drives pipes.reserve()/resize() in
+    /// ReadFromMergeTree, where reserve() throws std::length_error when the requested size exceeds
+    /// the vector max_size.
+    if (UInt64 max_streams_for_merge_tree_reading = current_settings[Setting::max_streams_for_merge_tree_reading];
+        max_streams_for_merge_tree_reading > max_threads_max_value)
+    {
+        if (log)
+            LOG_WARNING(
+                log,
+                "Sanity check: 'max_streams_for_merge_tree_reading' value is too high ({}). Reduced to {}",
+                max_streams_for_merge_tree_reading,
+                max_threads_max_value);
+        current_settings[Setting::max_streams_for_merge_tree_reading] = max_threads_max_value;
+    }

This ensures that any absurdly high value is reduced before it can cause problems, and logs a warning for visibility.

2. Bound the effective number of streams in the main read path

In the ReadFromMergeTree constructor, after applying the setting, the code explicitly clamps the number of streams to the same ceiling. This is important because the effective stream count can be further amplified by other settings (like max_streams_to_max_threads_ratio):

+    /// `requested_num_streams` drives pipes.reserve()/resize() downstream, which throws
+    /// std::length_error when unbounded. It can be amplified past any setting clamp via
+    /// `max_streams_to_max_threads_ratio`, so bound the effective value here too.
+    requested_num_streams = std::min<size_t>(requested_num_streams, 256 * getNumberOfCPUCoresToUse());

This provides a second layer of defense, making sure even indirect amplification can't cause an overflow.

3. Add a regression test

A new stateless test ensures that even with a pathological value, the server does not crash and the setting is clamped as intended:

+-- A pathological max_streams_for_merge_tree_reading must not throw std::length_error from
+-- pipes.reserve in groupPartitionsByStreams (which aborts the server in debug/sanitizer builds).
+-- EXPLAIN PIPELINE exercises the streaming read path without running the streaming query forever.
...
+SET max_streams_for_merge_tree_reading = 9223372036854775807;
+SELECT
+    toUInt64(value) = 256 * toUInt64(extract((SELECT default FROM system.settings WHERE name = 'max_threads'), 'auto\\(([0-9]+)\\)')) AS clamped_to_256x_cores,
+    toUInt64(value) < toUInt64(9223372036854775807) AS reduced_from_requested
+FROM system.settings
+WHERE name = 'max_streams_for_merge_tree_reading';

This checks both that the server doesn't crash and that the value is actually clamped.

From the review thread

No notable review discussion on this PR.

Takeaways

• Clamp dangerous configuration values at ingestion, not just at use sites. This ensures all code paths are protected, not just the ones you know about.
• Defensive programming: always bound user-amplifiable parameters before they reach memory allocation. Even if a value seems safe in one context, it might be multiplied or propagated in ways that break invariants elsewhere.
• Automated regression tests are crucial for catching and preventing catastrophic failures from edge-case settings. Explicit tests for pathological values help ensure the system fails safely, not catastrophically.