iframe-proxy

mstetsyuk · 2026-06-03T13:47:54Z

Changelog category (leave one):

Improvement

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

Add an asynchronous metric UntrackedMemory reporting the sum of all threads' untracked memory counters. This value is also included in MEMORY_LIMIT_EXCEEDED exception messages.

clickhouse-gh · 2026-06-03T13:48:38Z

mstetsyuk · 2026-06-03T13:50:59Z

Sanitizer output: https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=7272570617579b425019e36f8e9bd549b96449fc&name_0=MasterCI&name_1=Stateless%20tests%20%28amd_tsan%2C%20s3%20storage%2C%20parallel%2C%202%2F2%29&name_1=Stateless%20tests%20%28amd_tsan%2C%20s3%20storage%2C%20parallel%2C%202%2F2%29

One line fix: https://github.com/ClickHouse/ClickHouse/pull/106386/changes#diff-6289db0bca42b99004d2a57bbbb8bcbd6ede5b54e60ef2f5d451beefc822e64aR21

clickhouse-gh · 2026-06-03T13:56:03Z

+
+-- Value is an Int64 sum; can be negative if threads have net-freed since last flush,
+-- but the metric must exist and be queryable as a number.
+SELECT isFinite(value) FROM system.asynchronous_metrics WHERE metric = 'UntrackedMemory';


The coverage here only proves that UntrackedMemory exists and is finite. It would still pass if UntrackedMemoryRegistry::sum always returned 0, if counters were not removed on thread teardown, or if MEMORY_LIMIT_EXCEEDED exceptions stopped including the registry value. Since the PR contract is specifically that the metric reports the sum of per-thread untracked counters and that this value is surfaced in the exception text, please add focused coverage for those semantics, for example a small unit test that creates multiple UntrackedMemoryCounters with positive and negative deltas and verifies sum before and after one counter is destroyed, plus a minimal memory-limit exception check for the new message field.

azat · 2026-06-04T12:15:41Z

+
+UntrackedMemoryRegistry & UntrackedMemoryRegistry::instance()
+{
+    static UntrackedMemoryRegistry & registry = *new UntrackedMemoryRegistry;


I definitely deserves a comment, but

Why do we have this problem at all (https://pastila.nl/?002ee642/fd2d1b214537fab3b4f7a93806a4cb46#p4P1X/SxYgpEYdM+OhsgRg==GCM)? thread pool should be joined before

Oh, I see, looks like it came from clickhouse-benchmark, which does not wait for threads, and I think this is the correct fix. And we also add here "GlobalThreadPool::instance().finished())"

Also side question, why ASan does not report this leak?

Added a comment.

Also side question, why ASan does not report this leak?

I checked the assembly code, there's always a reference with the address of the registry, it's not optimised away in any way. So LSan sees that there's a pointer to the memory and doesn't flag it as a leak.

mstetsyuk · 2026-06-05T22:55:03Z

build broken here: #106102

…rg::alter PR ClickHouse#106102 (merged 2026-06-05 17:43 UTC) refactored Iceberg::alter to declare last_version and compression_method outside an if/else that assigns them in both branches. Clang-tidy cppcoreguidelines-init-variables flags the bare declarations and Build (arm_tidy) is built with -warnings-as-errors, so the build fails for every PR whose CI ran on master after this commit. CIDB shows 30+ unrelated PRs hitting this in the last 2 days (e.g. ClickHouse#89360, ClickHouse#103540, ClickHouse#106120, ClickHouse#106386, ClickHouse#106404, ClickHouse#106522, ClickHouse#105102, ClickHouse#106590). Initialize both variables to safe defaults at declaration. They are unconditionally overwritten in both if and else branches before use, so behavior is unchanged. The new defaults are only relevant if a future code path skips both assignments, in which case last_version=0 and compression_method=CompressionMethod::None are sane no-op values (the same defaults the old structured-binding form would produce through default-construction of the destructured aggregate). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit d72cac5)

…rg::alter PR #106102 (merged 2026-06-05 17:43 UTC) refactored Iceberg::alter to declare last_version and compression_method outside an if/else that assigns them in both branches. Clang-tidy cppcoreguidelines-init-variables flags the bare declarations and Build (arm_tidy) is built with -warnings-as-errors, so the build fails for every PR whose CI ran on master after this commit. CIDB shows 30+ unrelated PRs hitting this in the last 2 days (e.g. #89360, #103540, #106120, #106386, #106404, #106522, #105102, #106590). Initialize both variables to safe defaults at declaration. They are unconditionally overwritten in both if and else branches before use, so behavior is unchanged. The new defaults are only relevant if a future code path skips both assignments, in which case last_version=0 and compression_method=CompressionMethod::None are sane no-op values (the same defaults the old structured-binding form would produce through default-construction of the destructured aggregate). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit d72cac5)

…rg::alter PR ClickHouse#106102 (merged 2026-06-05 17:43 UTC) refactored Iceberg::alter to declare last_version and compression_method outside an if/else that assigns them in both branches. Clang-tidy cppcoreguidelines-init-variables flags the bare declarations and Build (arm_tidy) is built with -warnings-as-errors, so the build fails for every PR whose CI ran on master after this commit. CIDB shows 30+ unrelated PRs hitting this in the last 2 days (e.g. ClickHouse#89360, ClickHouse#103540, ClickHouse#106120, ClickHouse#106386, ClickHouse#106404, ClickHouse#106522, ClickHouse#105102, ClickHouse#106590). Initialize both variables to safe defaults at declaration. They are unconditionally overwritten in both if and else branches before use, so behavior is unchanged. The new defaults are only relevant if a future code path skips both assignments, in which case last_version=0 and compression_method=CompressionMethod::None are sane no-op values (the same defaults the old structured-binding form would produce through default-construction of the destructured aggregate). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit d72cac5)

…rg::alter PR #106102 (merged 2026-06-05 17:43 UTC) refactored Iceberg::alter to declare last_version and compression_method outside an if/else that assigns them in both branches. Clang-tidy cppcoreguidelines-init-variables flags the bare declarations and Build (arm_tidy) is built with -warnings-as-errors, so the build fails for every PR whose CI ran on master after this commit. CIDB shows 30+ unrelated PRs hitting this in the last 2 days (e.g. #89360, #103540, #106120, #106386, #106404, #106522, #105102, #106590). Initialize both variables to safe defaults at declaration. They are unconditionally overwritten in both if and else branches before use, so behavior is unchanged. The new defaults are only relevant if a future code path skips both assignments, in which case last_version=0 and compression_method=CompressionMethod::None are sane no-op values (the same defaults the old structured-binding form would produce through default-construction of the destructured aggregate). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit d72cac5)

…rg::alter PR ClickHouse#106102 (merged 2026-06-05 17:43 UTC) refactored Iceberg::alter to declare last_version and compression_method outside an if/else that assigns them in both branches. Clang-tidy cppcoreguidelines-init-variables flags the bare declarations and Build (arm_tidy) is built with -warnings-as-errors, so the build fails for every PR whose CI ran on master after this commit. CIDB shows 30+ unrelated PRs hitting this in the last 2 days (e.g. ClickHouse#89360, ClickHouse#103540, ClickHouse#106120, ClickHouse#106386, ClickHouse#106404, ClickHouse#106522, ClickHouse#105102, ClickHouse#106590). Initialize both variables to safe defaults at declaration. They are unconditionally overwritten in both if and else branches before use, so behavior is unchanged. The new defaults are only relevant if a future code path skips both assignments, in which case last_version=0 and compression_method=CompressionMethod::None are sane no-op values (the same defaults the old structured-binding form would produce through default-construction of the destructured aggregate). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit d72cac5)

…rg::alter PR #106102 (merged 2026-06-05 17:43 UTC) refactored Iceberg::alter to declare last_version and compression_method outside an if/else that assigns them in both branches. Clang-tidy cppcoreguidelines-init-variables flags the bare declarations and Build (arm_tidy) is built with -warnings-as-errors, so the build fails for every PR whose CI ran on master after this commit. CIDB shows 30+ unrelated PRs hitting this in the last 2 days (e.g. #89360, #103540, #106120, #106386, #106404, #106522, #105102, #106590). Initialize both variables to safe defaults at declaration. They are unconditionally overwritten in both if and else branches before use, so behavior is unchanged. The new defaults are only relevant if a future code path skips both assignments, in which case last_version=0 and compression_method=CompressionMethod::None are sane no-op values (the same defaults the old structured-binding form would produce through default-construction of the destructured aggregate). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit d72cac5)

azat · 2026-06-08T15:18:16Z

+    /// where the main thread finishes, destructing static objects,
+    /// without joining the threads that may still use this method.


But we should not have such code, if we do not join threads we may have problems with other global variables. So I still think we need to join threads in benchmark utility instead

Actually, you're right. I've added GlobalThreadPool::shutdown calls to all the binaries that use it.

This is still not covered by the current code. src/Common/tests/gtest_main.cpp:24 returns after RUN_ALL_TESTS() without calling GlobalThreadPool::shutdown, while src/Common/tests/gtest_thread_pool_global_full.cpp:24 and src/Common/tests/gtest_thread_pool_global_full.cpp:60 call GlobalThreadPool::instance and can leave idle global-pool workers alive until static destruction. Those workers own ThreadStatus, so if they are only joined from GlobalThreadPool::the_instance static destruction, UntrackedMemoryRegistry::instance can already have been destroyed.

Please either make the registry intentionally process-lifetime, or add a common shutdown path for gtest binaries and any other entrypoint that can use the global pool.

This is still not covered by the current fix. The failing stack from the 6a12cd47 ASan/UBSan report is not a GlobalThreadPool worker: base/poco/Foundation/src/ThreadPool.cpp:190 creates a ThreadStatus inside every PooledThread, and Poco::ThreadPool::defaultPool is backed by the namespace-scope singleton at base/poco/Foundation/src/ThreadPool.cpp:527. GlobalThreadPool::shutdown() does not stop that pool.

So with UntrackedMemoryRegistry::instance() still being the destructible function-local static at src/Common/UntrackedMemoryRegistry.cpp:21, a pooled thread can still run UntrackedMemoryCounter::~UntrackedMemoryCounter after the registry has been destroyed; the CI stack shows exactly UntrackedMemoryRegistry::remove from Poco::PooledThread::run. Please make the registry process-lifetime, or add a teardown path that stops every ThreadStatus-owning pool before the registry can be destroyed.

azat · 2026-06-11T12:01:02Z

Looks like unit tests are now hunged in GlobalThreadPool::shutdown

mstetsyuk · 2026-06-12T17:02:21Z

Found a shutdown bug in server and local binaries. To make sure we are able to backport it ad hoc, I've consolidated all the join-related fixes/improvements here: #107291.

After that is merged, this PR will be a clean revert of the revert.

azat · 2026-06-22T14:08:38Z

@mstetsyuk Let's rebase/merge master? After #107291 has been merged

This reverts commit 3930f8a.

The Upgrade check (amd_release) server.log scan fails on benign background-mutation errors left behind by 02597_column_update_tricky_expression_and_replication: an `ALTER TABLE test UPDATE d = d + throwIf(1)` with mutations_sync=0 leaves an intentionally-failing mutation that the upgraded server retries on restart before the test's later KILL MUTATION cleans it up, logging FUNCTION_THROW_IF_VALUE_IS_NON_ZERO at <Error>. This is the issue ClickHouse#39174 class (bad mutation, not a backward incompatibility). The CANNOT_PARSE_TEXT sibling ('x' as UInt64) is already in the allow-list on master; this adds the throwIf counterpart. The narrow inner-text 'Value passed to throwIf function is non-zero' matches all three emitted line shapes (MutatePlainMergeTreeTask, its executeStep(), and the MergeTreeBackgroundExecutor wrapper) without masking a genuinely different background-mutation error. Validated against real leaked upgrade_error_messages.txt (159 lines from PR ClickHouse#106386, 255 from PR ClickHouse#105991): without the entry every line leaks and the gate fails; with it 0 lines leak; a control LOGICAL_ERROR in the same MutatePlainMergeTreeTask still surfaces. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

# Conflicts: # src/Common/CurrentMemoryTracker.cpp # src/Common/ThreadStatus.cpp

clickhouse-gh · 2026-06-29T22:02:56Z

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered by tests: 115/123 (93.50%) | Lost baseline coverage: none · Uncovered code

Full report · Diff report

mstetsyuk requested a review from azat June 3, 2026 13:48

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

clickhouse-gh Bot reviewed Jun 3, 2026

View reviewed changes

azat self-assigned this Jun 3, 2026

azat reviewed Jun 4, 2026

View reviewed changes

mstetsyuk requested a review from azat June 4, 2026 19:06

groeneai mentioned this pull request Jun 4, 2026

Add integration tests for attach/detach and restart of Iceberg tables #91583

Merged

1 task

clickhouse-gh Bot added the submodule changed At least one submodule changed in this PR. label Jun 4, 2026