Track memory consumed by Roaring Bitmaps.#27252
Conversation
robot-clickhouse
added
the
pr-improvement
robot-ch-test-poll1
added
the
submodule changed
|
|
||
| /// Some aggregate functions cannot throw exceptions on allocations (e.g. from C malloc) | ||
| /// but still tracks memory. Check it here. | ||
| CurrentMemoryTracker::check(); |
There was a problem hiding this comment.
Note: memory is not checked while deserializing aggregation states from file or network.
| target_include_directories(roaring SYSTEM BEFORE PUBLIC "${LIBRARY_DIR}/include") | ||
| target_include_directories(roaring SYSTEM BEFORE PUBLIC "${LIBRARY_DIR}/cpp") | ||
|
|
||
| target_compile_definitions(roaring PRIVATE |
There was a problem hiding this comment.
This is the main part of the fix.
| } | ||
| } | ||
|
|
||
| /// Some aggregate functions cannot throw exceptions on allocations (e.g. from C malloc) |
There was a problem hiding this comment.
Is it ok to just return a nullptr when memory limit is exceeded?
There was a problem hiding this comment.
Generally I won't do it, many libraries are not ready (they will either segfault on nullptr dereference or abort).
Maybe Roaring Bitmap is ok but I don't want to "surprise" it.
E.g. I expect it may have memory leak if one of subsequent allocations return nullptr.
There was a problem hiding this comment.
PS. You might noticed, we have removed throwing from operator new.
|
Ok, all tests passed... |
|
@Mergifyio update |
|
Command
|
|
@alexey-milovidov The test does not work with split libraries. The query uses ~11 GB and then the python client uses ~15 GB of memory, until it works (so the test fails). |
|
I did not find a section in skip_list for shared builds. |
The wiki (https://clickhouse.tech/docs/en/development/build/) basically recommends the split build for development, which means that anyone using the development build won't be able to pass the tests. IMO this should either work or be disabled. Note that it is the only stateless test that won't work witha a split build. If it's going to always fail, why recommend (or even support at all) the split build? |
|
In fact I don't recomment split build for development. There are too many issues:
But if split build is important for you, maybe you can convert this test to .sh test that will check system.build_options and succeed unconditionally if it is split build? |
Sure, I'll give it a try. Note that the only reason I'm using a split build is because the wiki implies that is the most convenient for development, so I'll add that information to the wiki too. Really useful. Thanks! |
5 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Memory consumed by bitmap aggregate functions now is taken into account for memory limits. This closes #26555.
Note: it is implemented in quite hacky way: C does not require implicit function declarations; it also depends on linking order.