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

Epic: productionize tiered compaction #7554

Closed
11 of 16 tasks
problame opened this issue Apr 30, 2024 · 2 comments
Closed
11 of 16 tasks

Epic: productionize tiered compaction #7554

problame opened this issue Apr 30, 2024 · 2 comments
Assignees
Labels
c/storage/pageserver Component: storage: pageserver

Comments

@problame
Copy link
Contributor

problame commented Apr 30, 2024

https://www.notion.so/neondatabase/Compaction-2024Q2-eca9b06aa1ae4c62bdf6cf40ab002eb6?pvs=4

Fix bugs (continued from Q1)

Preview Give feedback
  1. c/storage/pageserver t/bug
    arpad-m
  2. c/storage/pageserver
    arpad-m
  3. c/storage/pageserver
    arpad-m
  4. c/storage/pageserver t/bug
    arpad-m
  5. 1 of 3
    c/storage/pageserver
    arpad-m
  6. c/storage/pageserver

basic functional testing

Preview Give feedback
  1. c/storage/pageserver
    problame
  2. 1 of 1
    problame

Must get done before ship

Preview Give feedback
  1. c/storage/pageserver t/bug triaged

Backlog

Preview Give feedback
  1. c/storage/pageserver
    arpad-m
  2. c/storage/pageserver
    skyzh
@problame problame self-assigned this Apr 30, 2024
@problame problame added the c/storage/pageserver Component: storage: pageserver label Apr 30, 2024
arpad-m added a commit that referenced this issue May 10, 2024
The old test based on the immutable `target_file_size` that was a
parameter to the function.

It makes no sense to go further once `current_level_target_height` has
reached `u64::MAX`, as lsn's are u64 typed. In practice, we should only
run into this if there is a bug, as the practical lsn range usually ends
much earlier.

Testing on `target_file_size` makes less sense, it basically implements
an invocation mode that turns off the looping and only runs one
iteration of it.
@hlinnaka agrees that `current_level_target_height` is better here.

Part of #7554
arpad-m added a commit that referenced this issue May 13, 2024
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>
arpad-m added a commit that referenced this issue May 15, 2024
Adds a test that is a reproducer for many tiered compaction bugs,
both ones that have since been fixed as well as still unfxied ones:
* (now fixed) #7296 
* #7707 
* #7759
* Likely also #7244 but I haven't tried that.

The key ordering bug can be reproduced by switching to
`merge_delta_keys` instead of `merge_delta_keys_buffered`, so reverting
a big part of #7661, although it only sometimes reproduces (30-50% of
cases).

part of #7554
a-masterov pushed a commit that referenced this issue May 20, 2024
The old test based on the immutable `target_file_size` that was a
parameter to the function.

It makes no sense to go further once `current_level_target_height` has
reached `u64::MAX`, as lsn's are u64 typed. In practice, we should only
run into this if there is a bug, as the practical lsn range usually ends
much earlier.

Testing on `target_file_size` makes less sense, it basically implements
an invocation mode that turns off the looping and only runs one
iteration of it.
@hlinnaka agrees that `current_level_target_height` is better here.

Part of #7554
a-masterov pushed a commit that referenced this issue May 20, 2024
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
Adds a test that is a reproducer for many tiered compaction bugs,
both ones that have since been fixed as well as still unfxied ones:
* (now fixed) #7296 
* #7707 
* #7759
* Likely also #7244 but I haven't tried that.

The key ordering bug can be reproduced by switching to
`merge_delta_keys` instead of `merge_delta_keys_buffered`, so reverting
a big part of #7661, although it only sometimes reproduces (30-50% of
cases).

part of #7554
@problame problame changed the title Epic: productionize tiered compaction Epic: 2024Q2 compaction work Jun 10, 2024
@problame problame changed the title Epic: 2024Q2 compaction work Epic: productionize tiered compaction Jun 10, 2024
@problame
Copy link
Contributor Author

productinizing tiered compaction is on hold, remaining Q2 work tracked in #8001

@problame problame assigned problame, skyzh and arpad-m and unassigned problame Jun 10, 2024
@problame
Copy link
Contributor Author

Closing, we're not planning to continue working on this in 2024.

@problame problame closed this as not planned Won't fix, can't repro, duplicate, stale Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/storage/pageserver Component: storage: pageserver
Projects
None yet
Development

No branches or pull requests

3 participants