Skip to content
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.

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

Add MemoryTracker for the background tasks #46089

Merged
merged 14 commits into from Apr 13, 2023
Merged

Conversation

novikd
Copy link
Member

@novikd novikd commented Feb 6, 2023

Changelog category (leave one):

  • New Feature

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

Add MemoryTracker for the background tasks (merges and mutation). Introduces merges_mutations_memory_usage_soft_limit and merges_mutations_memory_usage_to_ram_ratio settings that represent the soft memory limit for merges and mutations. If this limit is reached ClickHouse won't schedule new merge or mutation tasks. Also MergesMutationsMemoryTracking metric is introduced to allow observing current memory usage of background tasks.
Closes #45710.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@robot-ch-test-poll robot-ch-test-poll added the pr-feature Pull request with new product feature label Feb 6, 2023
programs/server/Server.cpp Outdated Show resolved Hide resolved
src/Storages/StorageMergeTree.cpp Outdated Show resolved Hide resolved
programs/server/Server.cpp Outdated Show resolved Hide resolved
@filimonov
Copy link
Contributor

Check also
#10154 - there is a test there.

@tavplubix tavplubix self-assigned this Feb 10, 2023
programs/server/Server.cpp Outdated Show resolved Hide resolved
Comment on lines +897 to +900
if (!canEnqueueBackgroundTask())
{
if (out_disable_reason)
*out_disable_reason = fmt::format("Current background tasks memory usage ({}) is more than the limit ({})",
Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid that it will be too easy to get "too many parts" if the soft limit is not high enough. Imagine that some big merges are currently executing and consume a big amount of memory. It will be impossible to assign small merges. So it would be better to make the limit flexible and give priority to small merges as we do in getMaxSourcePartsSizeForMerge.

However, it's more complicated for memory usage because memory usage should be O(1) and should not depend on the source parts size. The concern here is that big marges may hold memory for too long.

But there's a chance that it will just work as is (it's hard to predict), so let's just keep it in mind.

@cangyin cangyin mentioned this pull request Mar 23, 2023
1 task
@cangyin
Copy link
Contributor

cangyin commented Mar 26, 2023

sorry for any disturbution.

there may be some misbehaviors for this feature that I observed and want to report to you.

  1. updated value for background_memory_usage_soft_limit is not reflected in system.sever_settings after config hot reload
  2. metric BackgroundMemoryTracking is increasing in the long term:
SELECT
    now(),
    metric,
    formatReadableSize(value),
    description
FROM system.metrics
WHERE metric = 'MergesMutationsMemoryTracking'

Query id: 70f4c034-70b4-4a46-bc0e-6cdaf433c0cc

┌───────────────now()─┬─metric────────────────────────┬─formatReadableSize(value)─┬─description──────────────────────────────────────────────────────────────────────────┐
│ 2023-03-27 00:11:08 │ MergesMutationsMemoryTracking │ 134.15 GiB                │ Total amount of memory (bytes) allocated by background tasks (merges and mutations). │
└─────────────────────┴───────────────────────────────┴───────────────────────────┴──────────────────────────────────────────────────────────────────────────────────────┘

while total available amount of memory is about 32 GiB

@tavplubix tavplubix mentioned this pull request Mar 31, 2023
@novikd
Copy link
Member Author

novikd commented Apr 11, 2023

@cangyin thank you for the feedback. Indeed, I initially forgot to update BackgroundMemoryTracking metric after a background task finished, but I can't reproduce the issue with system.sever_settings you mentioned.

@novikd novikd merged commit 467ecf4 into master Apr 13, 2023
144 checks passed
@novikd novikd deleted the background-memory-tracker branch April 13, 2023 11:29
@tavplubix
Copy link
Member

@novikd, you forgot to check the failed tests before merging

@novikd novikd restored the background-memory-tracker branch April 14, 2023 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-feature Pull request with new product feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Soft memory limits for merges and mutations.
7 participants