iframe-proxy

azat · 2026-03-30T16:47:19Z

Changelog category (leave one):

Performance Improvement

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Add dynamic untracked memory limits for more precise memory tracking

Original comment from @serxa:

The current memory tracker has an error of up to untracked_memory_limit = 4MB per thread. Consider a ClickHouse server running with 8GB RAM and a standard max_server_memory_usage_to_ram_ratio = 0.9 server setting. If there are 1000 threads (and every thread has 800KB on average) OOM can happen before any query is stopped with MEMORY_LIMIT_EXCEEDED.

We need total untracked memory to be proportional to current memory usage regardless of the number of threads. To make it fit into 10% RAM we want an error to be less than e.g. 6%. To achieve it we need untracked_memory_limit to be updated dynamically and be proportional to current thread memory usage for threads with small memory footprint: untracked_memory_limit = clamp(already_tracked / 16, 4KB, 4MB)

Resubmit of: #64423 (by @serxa)

Note: marked as Performance Improvement for run perf tests and memory accounting can be also "performance imporvement"

clickhouse-gh · 2026-03-30T16:48:03Z

Workflow [PR], commit [9f12504]

Summary: ❌

job_name	test_name	status	info
Unit tests (asan_ubsan, function_prop_fuzzer)		FAIL
Unit tests (msan, function_prop_fuzzer)		FAIL
Performance Comparison (amd_release, master_head, 4/6)		FAIL
	apply_patch_parts_join #6::old	FAIL	query history
	apply_patch_parts_join #6::new	FAIL	query history
Performance Comparison (arm_release, master_head, 4/6)		FAIL
	join_reserve #0::old	FAIL	query history
	join_reserve #0::new	FAIL	query history
	join_runtime_filter #9::old	FAIL	query history
	join_runtime_filter #9::new	FAIL	query history
	join_runtime_filter #10::old	FAIL	query history
	join_runtime_filter #10::new	FAIL	query history
	lazyMaterialization #15::old	FAIL	query history
	lazyMaterialization #15::new	FAIL	query history
	logical_functions_small #10::old	FAIL	query history
	logical_functions_small #10::new	FAIL	query history
	6 more test cases not shown

AI Review

Summary

This PR introduces dynamic per-thread untracked_memory_limit (via new min_untracked_memory) and adds new MemoryTracker* profile events. The memory-tracking direction looks good, but there are correctness issues in the new metrics accounting, and the new default dynamic path is not covered by tests, so this should not merge yet.

Missing context

⚠️ No CI logs/results were provided in this review context.

Findings

❌ Blockers

[src/Common/CurrentMemoryTracker.cpp:89-91] MemoryTrackerAllocations and MemoryTrackerAllocationsBytes are incremented before memory_tracker->allocImpl. If allocImpl throws (e.g. MEMORY_LIMIT_EXCEEDED), no flush occurs but counters are already increased, so the metrics overcount under pressure.
- Suggested fix: increment both counters only after successful allocImpl.
[src/Common/CurrentMemoryTracker.cpp:112-113] check increments MemoryTrackerAllocations, but this path does not flush bytes to parent trackers. This mixes frequent limit checks into a flush counter and decouples it from MemoryTrackerAllocationsBytes.
- Suggested fix: remove this increment from check, or introduce a dedicated profile event for limit-check calls.

Tests

⚠️ The touched tests mostly pin min_untracked_memory to 4Mi, so they preserve old behavior and do not validate the new default dynamic path (4KiB lower bound). Add at least one test that keeps defaults and verifies expected behavior of dynamic untracked limits.

ClickHouse Rules

Item	Status	Notes
Deletion logging	➖
Serialization versioning	➖
Core-area scrutiny	✅
No test removal	✅
Experimental gate	➖
No magic constants	✅
Backward compatibility	✅
`SettingsChangesHistory.cpp`	✅
PR metadata quality	✅
Safe rollout	⚠️	New default behavior path is not directly covered by tests in this PR.
Compilation time	✅
No large/binary files	✅

Final Verdict

Status: ⚠️ Request changes
Minimum required actions:
1. Fix MemoryTrackerAllocations* accounting so failed allocImpl calls are not counted as flushed allocations.
2. Stop counting check calls in MemoryTrackerAllocations, or add a separate event for checks.
3. Add coverage for the new default dynamic min_untracked_memory behavior (without forcing 4Mi).

alexey-milovidov · 2026-04-07T00:28:20Z

The Stress test (arm_msan) failure is fixed by #101239, which should be merged first. After it is merged, please update the branch to include the fix.

alexey-milovidov · 2026-04-09T03:26:47Z

The MSan stress test failure (MemorySanitizer: use-of-uninitialized-value, STID 4179-5154 or 4148-3044) is a known pre-existing issue unrelated to this PR. Fix: #102158

alexey-milovidov · 2026-04-09T21:01:33Z

The Can't adjust last granule error in CI is a known issue. The fix is in #101641

azat · 2026-04-17T12:58:40Z

@serxa PTAL

azat · 2026-04-18T09:54:22Z

Here is also a way for untracked_memory introspection - #103065

azat · 2026-04-22T15:54:01Z

I've looked at performance regressions:

group_by_multiple_strings.5 - select key1_8, key2_8, sum(value) from test_group_by_strings group by key1_8, key2_8 settings max_threads = 32 - it is just enters MemoryTracker more often, because this query before uses less then 4MiB (peak was 399840), so before this patch it never enters MemoryTracker, now it enters few times, we can add metric I guess (since if we enter MemoryTracker, it should not be that a big deal)
functions_geo.1 - SELECT count() FROM numbers(1000000) WHERE NOT ignore(geohashDecode(toString(number % 1000000))) - same (peak on some thread was 131335)

So I think this is acceptable, I'm going to add profile event (hope it will not slows down even more)

Note: I've tried to add metrics for all allocations, and it slows down - #85545, hence only when MemoryTracker is called

The current memory tracker has an error of up to `untracked_memory_limit = 4MB` per thread. Consider a ClickHouse server running with 8GB RAM and a standard `max_server_memory_usage_to_ram_ratio = 0.9` server setting. If there are 1000 threads (and every thread has 800KB on average) OOM can happen before any query is stopped with `MEMORY_LIMIT_EXCEEDED`. We need total untracked memory to be proportional to current memory usage regardless of the number of threads. To make it fit into 10% RAM we want an error to be less than e.g. 6%. To achieve it we need `untracked_memory_limit` to be updated dynamically and be proportional to current thread memory usage for threads with small memory footprint: `untracked_memory_limit = clamp(already_tracked / 16, 4KB, 4MB)` Resubmit of: ClickHouse#64423 Co-authored-by: Azat Khuzhin <a3at.mail@gmail.com>

clickhouse-gh · 2026-04-23T08:31:36Z

-        if (current_thread->untracked_memory < -current_thread->untracked_memory_limit)
+        /// Use `max_untracked_memory` (not `untracked_memory_limit`) to create hysteresis and avoid track/untrack cycles
+        if (current_thread->untracked_memory < -current_thread->max_untracked_memory)
        {


Using max_untracked_memory as the flush threshold for deallocations can permanently overcount memory after short-lived spikes.

Concrete trace:

min_untracked_memory = 4KiB, max_untracked_memory = 4MiB.

Allocate 1MiB -> allocation flushes (because current limit is small), so memory_tracker increases by 1MiB.

Free 1MiB -> untracked_memory = -1MiB, but this branch does not flush because -1MiB > -max_untracked_memory.

Next alloc/free cycles of 1MiB keep untracked_memory oscillating between 0 and -1MiB, so the previously tracked 1MiB is never released to parent trackers.

This means query/global trackers can stay overestimated by up to ~max_untracked_memory per thread for a long time, which can trigger premature MEMORY_LIMIT_EXCEEDED under concurrency. I think the deallocation flush condition should stay tied to the current dynamic limit (or another dynamic lower bound), not a fixed max_untracked_memory.

Indeed, valid point

I think initially different bounds for alloc/dealloc were introduced to avoid too many tracker invocations in corner cases, but now it seems to be unavoidable when I think about it

I also thought about this for awhile and decided to try this option, plus, I don't see too much benefits - https://pastila.nl/?009ef774/015773a63df85587f4c9889ec3f9186f#0Ruws2sMXI1d4WKqcrjt0A==GCM

And also maybe we can speedup MemoryTracker - #103464

clickhouse-gh · 2026-04-23T18:39:48Z

@@ -0,0 +1,3 @@
+profiles:
+  default:
+    min_untracked_memory: 4Mi


All updated tests in this PR pin min_untracked_memory back to 4Mi, so the new default dynamic behavior (4KiB lower bound) is not covered.

Could we add at least one test that keeps defaults and verifies the new path (reduced untracked error / earlier memory-limit reaction) to protect against regressions?

clickhouse-gh · 2026-04-23T21:25:53Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	83.80%	83.80%	+0.00%
Functions	91.10%	91.10%	+0.00%
Branches	76.20%	76.20%	+0.00%

Changed lines: 100.00% (62/62) · Uncovered code

Full report · Diff report

vadimskipin · 2026-05-03T17:53:10Z

Take a look at this code: https://github.com/ClickHouse/vskipin-experiments/blob/master/src/util/memory-tracker.h
I believe it resolves the problem with untracked memory completely.

azat · 2026-05-28T14:38:11Z

Here is an alternative - #106055

azat · 2026-07-03T13:31:30Z

Alternative has been merged and it looks better

serxa · 2026-07-03T13:32:46Z

azat requested a review from serxa March 30, 2026 16:47

clickhouse-gh Bot added the pr-improvement Pull request with some product improvements label Mar 30, 2026

azat added the memory When memory usage is higher than expected label Mar 30, 2026

azat force-pushed the precise-memtracking branch 2 times, most recently from 9d23adc to f1f1f1b Compare April 1, 2026 09:23

serxa approved these changes Apr 17, 2026

View reviewed changes

serxa self-assigned this Apr 17, 2026

azat force-pushed the precise-memtracking branch from 8ad391b to b5433a6 Compare April 18, 2026 22:07

azat force-pushed the precise-memtracking branch from b5433a6 to aa50660 Compare April 22, 2026 16:10

clickhouse-gh Bot added pr-performance Pull request with some performance improvements and removed pr-improvement Pull request with some product improvements labels Apr 22, 2026

clickhouse-gh Bot reviewed Apr 22, 2026

View reviewed changes

Comment thread src/Common/CurrentMemoryTracker.cpp

clickhouse-gh Bot reviewed Apr 22, 2026

View reviewed changes

Comment thread src/Common/ProfileEvents.cpp Outdated

azat force-pushed the precise-memtracking branch 3 times, most recently from 7efdb6c to 10b62fe Compare April 22, 2026 21:51

clickhouse-gh Bot reviewed Apr 22, 2026

View reviewed changes

Comment thread src/Common/CurrentMemoryTracker.cpp

groeneai mentioned this pull request Apr 23, 2026

Fix BSONEachRow rollback crash on malformed nullable array elements #103418

Merged

1 task

serxa and others added 3 commits April 23, 2026 10:23

Fix tests after introducing min_untracked_memory

130df8e

Add MemoryTracker* metrics

1e19934

azat force-pushed the precise-memtracking branch from 10b62fe to 1e19934 Compare April 23, 2026 08:24

clickhouse-gh Bot reviewed Apr 23, 2026

View reviewed changes

Flush deallocations based on untracked_memory_limit

9f12504

clickhouse-gh Bot reviewed Apr 23, 2026

View reviewed changes

groeneai mentioned this pull request Apr 23, 2026

Fix SIMD NUL false match in find_first_symbols_or_null (extractKeyValuePairs LOGICAL_ERROR, STID 6488-5f6c) #103389

Closed

1 task

azat marked this pull request as draft May 4, 2026 15:19

azat closed this Jul 3, 2026

azat deleted the precise-memtracking branch July 3, 2026 13:31

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

azat commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Uh oh!

clickhouse-gh Bot commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

Missing context

Findings

Tests

ClickHouse Rules

Final Verdict

Uh oh!

alexey-milovidov commented Apr 7, 2026

Uh oh!

alexey-milovidov commented Apr 9, 2026

Uh oh!

alexey-milovidov commented Apr 9, 2026

Uh oh!

azat commented Apr 17, 2026

Uh oh!

azat commented Apr 18, 2026

Uh oh!

azat commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

clickhouse-gh Bot Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

serxa Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

serxa Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

azat Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot commented Apr 23, 2026

LLVM Coverage Report

Uh oh!

vadimskipin commented May 3, 2026

Uh oh!

azat commented May 28, 2026

Uh oh!

azat commented Jul 3, 2026

Uh oh!

serxa commented Jul 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

azat commented Mar 30, 2026 •

edited

Loading

clickhouse-gh Bot commented Mar 30, 2026 •

edited

Loading

azat commented Apr 22, 2026 •

edited

Loading