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.

Sign up for GitHub

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

Jump to bottom

tiered compaction: fails to meet target file size on many updates to a single key #7243

Closed
Tracked by #7554
jcsp opened this issue Mar 26, 2024 · 4 comments · Fixed by #7671
Closed
Tracked by #7554

tiered compaction: fails to meet target file size on many updates to a single key #7243

jcsp opened this issue Mar 26, 2024 · 4 comments · Fixed by #7671
Assignees
@arpad-m
Labels
c/storage/pageserver Component: storage: pageserver t/bug Issue Type: Bug

Comments

@jcsp
Copy link
Contributor

jcsp commented Mar 26, 2024 •
edited by problame
Loading

We have a test but it's ignored because the code to handle the case hasn't been implemented yet.

neon/pageserver/compaction/tests/tests.rs

Lines 4 to 13 in 045bc6a

/// Test the extreme case that there are so many updates for a single key that
/// even if we produce an extremely narrow delta layer, spanning just that one
/// key, we still too many records to fit in the target file size. We need to
/// split in the LSN dimension too in that case.
///
/// TODO: The code to avoid this problem has not been implemented yet! So the
/// assertion currently fails, but we need to make it not fail.
#[ignore]
#[tokio::test]
async fn test_many_updates_for_single_key() {

@jcsp jcsp mentioned this issue Mar 26, 2024
Closed
10 tasks
@jcsp jcsp added t/bug Issue Type: Bug c/storage/pageserver Component: storage: pageserver labels Mar 26, 2024
@jcsp jcsp changed the title Fix case where a key has so many deltas it exceeds image size limit new compaction: Fix case where a key has so many deltas it exceeds image size limit Mar 26, 2024
@arpad-m arpad-m changed the title new compaction: Fix case where a key has so many deltas it exceeds image size limit new compaction: Fix case where a key has so many deltas it exceeds file size limit Mar 28, 2024
@arpad-m arpad-m mentioned this issue Mar 28, 2024
Closed
@problame problame mentioned this issue Apr 30, 2024
Open
@problame problame changed the title new compaction: Fix case where a key has so many deltas it exceeds file size limit tiered compaction fails to meet target file size on many updates to a single key Apr 30, 2024
@problame problame changed the title tiered compaction fails to meet target file size on many updates to a single key tiered compaction: fails to meet target file size on many updates to a single key Apr 30, 2024
@problame
Copy link
Contributor

problame commented May 6, 2024

Solution is there, needs impl; PR to be created this week, @hlinnaka will review since @problame is on vacation starting Thu.

@arpad-m
Copy link
Member

arpad-m commented May 7, 2024

took a stab at this today and got until an infinite loop: there needs to be some changes in the tier identification logic to ignore stacked single-key deltas, i.e. treat them as one.

Issue #7296 might be related, my changes had to add some additional sorting there...

@problame problame mentioned this issue May 8, 2024
Closed
@problame
Copy link
Contributor

problame commented May 8, 2024

Arpad to publish a draft PR that demonstrates the infinite loop problem.
Then go over the problem with @hlinnaka

@arpad-m arpad-m mentioned this issue May 9, 2024
Merged
@arpad-m
Copy link
Member

arpad-m commented May 9, 2024 •
edited
Loading

Draft PR with the infinite loop problem: #7671

(still has debug prints, I have removed the stuff I have since upstreamed however)

edit: infinite loop problem resolved, it's ready for review now: #7671

arpad-m added a commit that referenced this issue May 13, 2024
@arpad-m @hlinnaka
Tiered compaction: cut deltas along lsn as well if needed (#7671)
3a6fa76 
In general, tiered compaction is splitting delta layers along the key
dimension, but this can only continue until a single key is reached: if
the changes from a single key don't fit into one layer file, we used to
create layer files of unbounded sizes.

This patch implements the method listed as TODO/FIXME in the source
code. It does the following things:

* Make `accum_key_values` take the target size and if one key's
modifications exceed it, make it fill `partition_lsns`, a vector of lsns
to use for partitioning.
* Have `retile_deltas` use that `partition_lsns` to create delta layers
separated by lsn.
* Adjust the `test_many_updates_for_single_key` to allow layer files
below 0.5 the target size. This situation can create arbitarily small
layer files: The amount of data is arbitrary that sits between having
just cut a new delta, and then stumbling upon the key that needs to be
split along lsn. This data will end up in a dedicated layer and it can
be arbitrarily small.
* Ignore single-key delta layers for depth calculation: in theory we
might have only single-key delta layers in a tier, and this might
confuse depth calculation as well, but this should be unlikely.

Fixes #7243

Part of #7554

---------

Co-authored-by: Heikki Linnakangas <heikki@neon.tech>
a-masterov pushed a commit that referenced this issue May 20, 2024
@arpad-m @hlinnaka @a-masterov
Tiered compaction: cut deltas along lsn as well if needed (#7671)
e4fdbb3 
In general, tiered compaction is splitting delta layers along the key
dimension, but this can only continue until a single key is reached: if
the changes from a single key don't fit into one layer file, we used to
create layer files of unbounded sizes.

This patch implements the method listed as TODO/FIXME in the source
code. It does the following things:

* Make `accum_key_values` take the target size and if one key's
modifications exceed it, make it fill `partition_lsns`, a vector of lsns
to use for partitioning.
* Have `retile_deltas` use that `partition_lsns` to create delta layers
separated by lsn.
* Adjust the `test_many_updates_for_single_key` to allow layer files
below 0.5 the target size. This situation can create arbitarily small
layer files: The amount of data is arbitrary that sits between having
just cut a new delta, and then stumbling upon the key that needs to be
split along lsn. This data will end up in a dedicated layer and it can
be arbitrarily small.
* Ignore single-key delta layers for depth calculation: in theory we
might have only single-key delta layers in a tier, and this might
confuse depth calculation as well, but this should be unlikely.

Fixes #7243

Part of #7554

---------

Co-authored-by: Heikki Linnakangas <heikki@neon.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@arpad-m arpad-m

Labels
c/storage/pageserver Component: storage: pageserver t/bug Issue Type: Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Tiered compaction: cut deltas along lsn as well if needed neondatabase/neon
3 participants
@jcsp @problame @arpad-m