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

compaction: formatted_sstables_list can potentially cause OOM #14264

Open
vladzcloudius opened this issue Jun 15, 2023 · 3 comments
Open

compaction: formatted_sstables_list can potentially cause OOM #14264

vladzcloudius opened this issue Jun 15, 2023 · 3 comments

Comments

@vladzcloudius
Copy link
Contributor

vladzcloudius commented Jun 15, 2023

Installation details
HEAD: 780aee9

Description

formatted_sstables_list uses std::vector to store std::string objects what are sstables' names for a given compaction unit.

lass formatted_sstables_list {
    bool _include_origin = true;
    std::vector<std::string> _ssts;

With off-strategy compaction + repair thousands and even 100s of thousands sstables is not something impossible.

For 1000 entries the size of a linear vector buffer can become (assuming an sstable object size is 16B (pointer + size_t)) 16B*1000=16KB.

And even this not overly pessimistic case can create unnecessary pressure on seastar memory allocator.
Not to mention more extreme cases mentioned above.

It makes sense to use better suited container instead of std::vector here.
e.g. chunked_vector.

Found by a code review.

@mykaul
Copy link
Contributor

mykaul commented Aug 7, 2023

#13907 probably missing 'Fixes' line, but I believe it fixes this issue and thus can be closed. @raphaelsc ?

@raphaelsc
Copy link
Member

#13907 probably missing 'Fixes' line, but I believe it fixes this issue and thus can be closed. @raphaelsc ?

this is a secondary improvement suggested by Vlad, but the main problem observed was tracked by #14071 which is already closed by the fix

@vladzcloudius
Copy link
Contributor Author

#13907 probably missing 'Fixes' line, but I believe it fixes this issue and thus can be closed. @raphaelsc ?

This GH issue was opened because I noticed this issue while reviewing #13907 (https://github.com/scylladb/scylladb/pull/13907/files#r1229971707)

So, no, #13907 does not resolve this issue.

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

No branches or pull requests

3 participants