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
Fix excessive memory usage for FINAL (due to too much streams usage) #50429
Conversation
This is an automated comment for commit 228ebab with description of existing statuses. It's updated for the latest CI running
|
Previously it could create MergeTreeInOrder for each mark, however this could be very suboptimal, due to each MergeTreeInOrder has some memory overhead. Now, by collapsing all marks for one part together it is more memory effiecient. I've tried the query from the altinity wiki [1] and it decreases memory usage twice: SELECT * FROM repl_tbl FINAL WHERE key IN (SELECT toUInt32(number) FROM numbers(1000000) WHERE number % 50000 = 0) FORMAT Null - upstream: MemoryTracker: Peak memory usage (for query): 520.27 MiB. - patched: MemoryTracker: Peak memory usage (for query): 260.95 MiB. [1]: https://kb.altinity.com/engines/mergetree-table-engine-family/replacingmergetree/#multiple-keys And it could be not 2x and even more or less, it depends on the gaps in marks for reading (for example in my setup the memory usage increased a lot, from ~16GiB of RAM to >64GiB due to lots of marks and gaps). Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
162ebc8
to
228ebab
Compare
|
|
result_layers.emplace_back(); | ||
auto & current_layer = result_layers.emplace_back(); | ||
/// Map part_idx into index inside layer, used to merge marks from the same part into one reader | ||
std::unordered_map<size_t, size_t> part_idx_in_layer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't it be just ssize_t last_part_idx
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this will not be enough, since parts_ranges_queue
is sorted by primary key value not by the part, plus there are modifications of this queue inside this loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, I misunderstood your approach initially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice improvement, thanks
result_layers.emplace_back(); | ||
auto & current_layer = result_layers.emplace_back(); | ||
/// Map part_idx into index inside layer, used to merge marks from the same part into one reader | ||
std::unordered_map<size_t, size_t> part_idx_in_layer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, I misunderstood your approach initially.
thanks for the fix! do we know the plan this can be released / backported? since the LTS release 23.3 contains #47801, we had observed critical memory issues with it. |
…lickHouse#50429) Previously it could create MergeTreeInOrder for each mark, however this could be very suboptimal, due to each MergeTreeInOrder has some memory overhead. Now, by collapsing all marks for one part together it is more memory effiecient. I've tried the query from the altinity wiki [1] and it decreases memory usage twice: SELECT * FROM repl_tbl FINAL WHERE key IN (SELECT toUInt32(number) FROM numbers(1000000) WHERE number % 50000 = 0) FORMAT Null - upstream: MemoryTracker: Peak memory usage (for query): 520.27 MiB. - patched: MemoryTracker: Peak memory usage (for query): 260.95 MiB. [1]: https://kb.altinity.com/engines/mergetree-table-engine-family/replacingmergetree/#multiple-keys And it could be not 2x and even more or less, it depends on the gaps in marks for reading (for example in my setup the memory usage increased a lot, from ~16GiB of RAM to >64GiB due to lots of marks and gaps). Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
…l-memory-usage 23.3 Backport of ClickHouse#50429 - Fix excessive memory usage for FINAL (due to too much streams usage)
…lickHouse#50429) Previously it could create MergeTreeInOrder for each mark, however this could be very suboptimal, due to each MergeTreeInOrder has some memory overhead. Now, by collapsing all marks for one part together it is more memory effiecient. I've tried the query from the altinity wiki [1] and it decreases memory usage twice: SELECT * FROM repl_tbl FINAL WHERE key IN (SELECT toUInt32(number) FROM numbers(1000000) WHERE number % 50000 = 0) FORMAT Null - upstream: MemoryTracker: Peak memory usage (for query): 520.27 MiB. - patched: MemoryTracker: Peak memory usage (for query): 260.95 MiB. [1]: https://kb.altinity.com/engines/mergetree-table-engine-family/replacingmergetree/#multiple-keys And it could be not 2x and even more or less, it depends on the gaps in marks for reading (for example in my setup the memory usage increased a lot, from ~16GiB of RAM to >64GiB due to lots of marks and gaps). Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
@azat This isn't a bug fix, and it shouldn't have been backported. |
@alexey-milovidov I've marked it as bug fix to run tests on previous version, and AFAIK there is no automatic backports of the bug fixes anyway, so this had been done explicitly and not by me. |
And what was the problem with the backport? Did it create any bugs? |
@azat excessive backports compromise stability by stealing attention from important changes. |
AFAIK, there was a discussion and complain, about why people stuck to old versions of ClickHouse and not newer one, which is still in 1 year LTS support. And if "newer" version of ClickHouse have regressions compared to older ones, which makes them 10-100 times slower for them, it's the answer why this (keeping to older versions) will continue to happen and people will look away from using newer releases. And no, upgrade to latest version, which have all fixes is not an answer for them, #49410 #55643 |
We should not do these types of backports. |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix excessive memory usage for FINAL (due to too much streams usage)
Previously it could create MergeTreeInOrder for each mark, however this could be very suboptimal, due to each MergeTreeInOrder has some memory overhead.
Now, by collapsing all marks for one part together it is more memory effiecient.
I've tried the query from the altinity wiki 1 and it decreases memory usage twice:
And it could be not 2x and even more or less, it depends on the gaps in marks for reading (for example in my setup the memory usage increased a lot, from ~16GiB of RAM to >64GiB due to lots of marks and gaps).
Cc: @nickitat
Cc: @UnamedRus (you may want to run your queries)
Note: this pops up after a proper marks skipping for FINAL in #47801
P.S. Marked as bug fix to run tests on previous version.