Skip to content

Fix visible drift of MemoryTracking metric#10496

Merged
alexey-milovidov merged 1 commit intomasterfrom
fix-visible-drift-memory-tracking
Apr 25, 2020
Merged

Fix visible drift of MemoryTracking metric#10496
alexey-milovidov merged 1 commit intomasterfrom
fix-visible-drift-memory-tracking

Conversation

@alexey-milovidov
Copy link
Member

Changelog category (leave one):

  • Non-significant (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Addition to #10293

@blinkov blinkov added the pr-not-for-changelog This PR should not be mentioned in the changelog label Apr 25, 2020
@alexey-milovidov alexey-milovidov merged commit 092efc5 into master Apr 25, 2020
@alexey-milovidov alexey-milovidov deleted the fix-visible-drift-memory-tracking branch April 25, 2020 20:50
@qoega
Copy link
Member

qoega commented Apr 28, 2020

We need to document current approach of memory tracking. Both in parts about limits and metrics.

/// 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
Member

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
Member

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
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants