Skip to content

Fix visible drift of MemoryTracking metric #10496

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.

Sign up for GitHub

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

Merged
alexey-milovidov merged 1 commit into from
Apr 25, 2020
Merged

Fix visible drift of MemoryTracking metric #10496

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/Interpreters/AsynchronousMetrics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@
#endif


namespace CurrentMetrics
{
extern const Metric MemoryTracking;
}


namespace DB
{

Expand Down Expand Up @@ -146,6 +152,7 @@ void AsynchronousMetrics::update()
/// Otherwise it might be calculated incorrectly - it can include a "drift" of memory amount.
/// See https://github.com/ClickHouse/ClickHouse/issues/10293
total_memory_tracker.set(data.resident);
CurrentMetrics::set(CurrentMetrics::MemoryTracking, data.resident);
Copy link
Copy Markdown
Member

@azat azat Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still some issues with memory tracking, but now with per-user tracking:

2020.07.06 08:59:07.817896 [ 47193 ] {902cf22b-87e0-4dae-a687-ecd89ba5360c} executeQuery: Code: 241, e.displayText() = DB::Exception: Memory limit (for user) exceeded: would use 437.72 GiB (attempt to allocate chunk of 4200926 bytes), maximum: 437.72 GiB (version 20.6.1.1) (from 10.7.140.7:31318)

Although the server is mostly idle:

SELECT formatReadableSize(memory_usage)
FROM system.processes
SETTINGS max_memory_usage_for_user = 1

┌─formatReadableSize(memory_usage)─┐
│ 289.28 MiB                       │
│ 155.75 MiB                       │
│ 0.00 B                           │
└──────────────────────────────────┘

3 rows in set. Elapsed: 0.002 sec.

@alexey-milovidov how about reseting per-user accounting here too as a temporary solution?

P.S. this version is compiled from sources 811d124 + some of PRs applied (mostly mine), although this should not affect memory tracking.

Copy link
Copy Markdown
Member

@azat azat Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexey-milovidov FWIW it will be nice to print not only per-user limit on error, but per-query too, and looks like it is not that complex (just adding childs into the MemoryTracker), thoughts?

Copy link
Copy Markdown
Member Author

@alexey-milovidov alexey-milovidov Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about reseting per-user accounting here too as a temporary solution?

Ok.

FWIW it will be nice to print not only per-user limit on error, but per-query too, and looks like it is not that complex (just adding childs into the MemoryTracker), thoughts?

Ok, that will be better.

}
#endif

Expand Down