Fix per-user memory tracker drift from system log elements - #110708
Conversation
…r drift System log elements (query_log, query_views_log, part_log, text_log, ...) are built in a query thread but destroyed later by the SystemLog flush thread, which cannot credit the freed bytes back to the per-user MemoryTracker. Memory an element owns that was charged to the user therefore leaked onto the per-user tracker permanently while the user had a running query, eventually tripping max_memory_usage_for_user with phantom memory. SystemLogBase::add now takes a callback that fills the element in place: it is default-constructed under a MemoryTrackerBlockerInThread and moved into the queue, so its allocations are charged to the global total tracker only. SystemLogQueue::push is private, so add() is the single entry point (this also covers text_log via OwnSplitChannel). Log elements are made self-contained (they own all their memory, with no shared or borrowed heap): profile snapshots stored by value, changed settings and async-read counters as owning name->value maps, ClientInfo addresses as optional<SocketAddress>, error_log traces as UInt64, and BackupLogElement copies only the loggable fields out of BackupOperationInfo. Coordination::ErrorCounter drops its redundant (always mutex-guarded) atomics so it is a plain trivially-copyable value. A compile-time check (Common/SelfContained.h, using boost::pfr) enforces that every system log element is self-contained and fails the build with a clear message if one (transitively) holds a shared_ptr, weak_ptr, raw pointer or exception_ptr. Adds a differential integration regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Workflow [PR], commit [899287e] Summary: ❌
AI ReviewSummaryThis PR moves system-log element construction under a memory-tracker blocker and replaces several shared/borrowed carriers with owned copies, which addresses the main Findings
Final VerdictStatus: |
clickhouse-gh
Bot
added
the
pr-bugfix
Converting SystemLog add(elem) to the fill-in-place add([&]{...}) callback
moved guarded-member access into a lambda, which Clang's thread-safety
analysis cannot see the lock through. Read/move the guarded previous values
into locals around the callback in ErrorLog and MetricLog. Also consume the
unused sleeper request in the drift integration test to satisfy ruff.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The new fill-in-place SystemLogBase::add default-constructs the element, which tidy's member-init check flags when an aggregate's implicit constructor leaves POD fields uninitialized. Value-initialize the element (also safer, since the fill callback need not set every field); this covers every log element except the KafkaDetails alternative of a default-constructed std::variant, which needs its own in-class initializers. Also pass current_address (now an optional) directly to Session::authenticate instead of dereferencing and re-wrapping it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
…llectors The mutex is held; thread-safety analysis simply cannot see the lock through the add() callback. Access the guarded previous_values / previous_profile_events directly inside the callback and suppress the false positive on that access with TSA_SUPPRESS_WARNING_FOR_*, rather than copying or moving the state out and back. This also removes the exception-safety hazard of leaving the member moved-out if the callback throws. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Field can hold a CustomType alternative backed by a shared_ptr, so whitelisting Field/Array/Map/Tuple as self-contained leaves means the gate cannot prove the invariant for elements carrying them. Record this as a deliberate tripwire limitation: the elements that use these types never store a CustomType, and the runtime sanitizer covers the rest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ntained-elements # Conflicts: # src/Core/Settings.cpp # src/Core/Settings.h
Storing the socket address by value made ClientInfo.h include <Poco/Net/SocketAddress.h>, whose socket headers transitively pull in <termios.h> on ppc64le, defining the CR1/CR2/CR3 macros. These collide with parameter names in LLVM's ConstantRange.h in translation units that include ClientInfo.h (via Context.h) before the LLVM headers, e.g. FunctionsConversion.cpp. Undef the macros at the source of the inclusion, matching the existing workaround in Native.cpp. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
📊 Cloud Performance Report ✅ AI verdict: This PR reworks how system-log elements are built and enqueued (in-place fill under a memory-tracker blocker, a compile-time self-containment gate, and value-typed ErrorCounter/ClientInfo members) — logging plumbing, not the query-execution hot path. ClickBench Q4 showed a −15.6% median improvement but CPU time is essentially unchanged (~2690 ms both sides), so the delta is run-to-run variance and was correctly kept as no_change within master's current variance band. No real per-query regression or improvement is attributable to this change. clickbench🟢 No significant changes tpch_adapted_1_official🟢 No significant changes Debug info
|
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 767/849 (90.34%) · Uncovered code |
|
All three failures classified. Here's the summary. PR #110708 — CI failure summary PR: SystemLog per-user memory-tracker drift fix (makes system log elements self-contained; related to issue #81541, escalation 7909). Test: test_replicated_database/test.py::test_replicated_table_structure_alter |
64cf1c2
|
Found via ClickGap automated review. Please close or comment if this is incorrect or needs adjustment. TL;DRFix per-user memory tracker drift from system log elements ClickGap verified: reproduced it with the script below on ReproductionShell reproducer — run against a local ClickHouse build ( On a server with query_log + query_views_log enabled:
CREATE TABLE t (k UInt64, s String) ENGINE = MergeTree ORDER BY k;
CREATE MATERIALIZED VIEW mv1 ENGINE = SummingMergeTree ORDER BY k AS SELECT k, count() AS c FROM t GROUP BY k;
CREATE MATERIALIZED VIEW mv2 ENGINE = SummingMergeTree ORDER BY s AS SELECT s, count() AS c FROM t GROUP BY s;
CREATE USER u IDENTIFIED WITH no_password; GRANT INSERT ON *.* TO u;
-- keep the user alive so its per-user tracker is not reset to zero:
(background) clickhouse-client --user u --query "SELECT sleepEachRow(1) FROM numbers(600) SETTINGS max_block_size=1 FORMAT Null"
-- 50 inserts that each produce large query_log/query_views_log elements:
for i in $(seq 1 50); do clickhouse-client --user u --log_queries=1 --log_query_views=1 --log_comment="$(printf 'x%.0s' $(seq 1 100000))" --query "INSERT INTO t VALUES ($i,'v')"; done
SYSTEM FLUSH LOGS;
SELECT memory_usage FROM system.user_processes WHERE user = 'u';
Observed symptom: the user's memory_usage floor is inflated by ~tens of MB (roughly the total size of the logged elements) and never comes back down even after SYSTEM FLUSH LOGS destroys those elements; a user with a continuous workload eventually trips max_memory_usage_for_user with this phantom memory. The same workload with logging disabled leaves only sub-MB noise.Bisect
Affects
Suggested next stepThe fix has landed on master; the affected release branches above still need the backport. Component: cc fix PR reviewer: @SmitaRKulkarni ClickGap · Finding: Triage metadata |
robot-ch-test-poll
added
the
pr-synced-to-cloud
5 participants
System log elements (
query_log,query_views_log,part_log,text_log, ...) are built in a query thread but destroyed later by theSystemLogflush thread, which cannot credit the freed bytes back to the per-user memory tracker. Any memory an element owned that was charged to the user leaked onto the per-user tracker permanently while the user had at least one running query, eventually trippingmax_memory_usage_for_userwith phantom memory.This makes system log elements self-contained and builds them under a memory-tracker blocker (via a fill-in-place
SystemLogBase::add), so their allocations are charged only to the global total tracker and are freed with the element by the flush thread. A compile-time check (Common/SelfContained.h, usingboost::pfr) enforces self-containment and fails the build with a clear message if a system log element ever holds ashared_ptr,weak_ptr, raw pointer orexception_ptr.This fixes the system-log-element contributor to the per-user memory tracker drift. Other contributors to the same skew (e.g. memory from the
String→LowCardinality(String)conversion during INSERT) are separate and out of scope here.Related: #81541
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fixed drift of the per-user memory tracker caused by system log records (e.g.
query_log,query_views_log) being accounted to the query that produced them but freed later by a background thread, which could eventually trigger a falsemax_memory_usage_for_userlimit for users with continuous workloads.