Compaction: sort on slices directly instead of kmerge

[arpad-m]

Problem

The k-merge in pageserver compaction currently relies on iterators over the keys and also over the values. This approach does not support async code because we are using iterators and those don't support async in general. Also, the k-merge implementation we use doesn't support async either. Instead, as we already load all the keys into memory, just do sorting in-memory.

Summary of changes

The PR can be read commit-by-commit, but most importantly, it:

• Stops using kmerge in compaction, using slice sorting instead.
• Makes load_keys and load_val_refs async, using Handle::block_on in the compaction code as we don't want to turn the compaction function, called inside spawn_blocking, into an async fn.

Builds on top of #4836, part of #4743

Checklist before requesting a review

• I have performed a self-review of my code.
• If it is a core feature, I have added thorough tests.
• Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
• If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

• Do not forget to reformat commit message to not include the above checklist

[github-actions]

1264 tests run: 1213 passed, 0 failed, 51 skipped (full report)

---

Flaky tests (1)

Postgres 15

• test_crafted_wal_end[wal_record_crossing_segment_followed_by_small_one]: debug

[koivunej]

I understand this already implements what I suggested; a single walk to get sizes and valuerefs. Looking good.

Always enjoy a decrease in:

• amount of virtual calls
• amount of duplicated in-mem contents
• amount of LOC

[arpad-m]

I understand this already implements what I suggested; a single walk to get sizes and valuerefs. Looking good.

Indeed I have pushed a commit to merge the two functions, but now there is a failure in tenant::tests::test_random_updates, and I can reproduce it locally so it seems to be valid. I am still investigating, but the load_keys function is doing some early returning if last.0 == delta_key.key() holds, while the load_val_refs function doesn't do that.

[arpad-m]

Mhh yeah if I print the size of the arrays, it seems that the key search has way less items than the value search:

val refs len: 1000, becoming  1000
val refs len: 1000, becoming  2000
val refs len: 1000, becoming  3000
val refs len: 1000, becoming  4000
val refs len: 1000, becoming  5000
val refs len: 1000, becoming  6000
val refs len: 1000, becoming  7000
val refs len: 1000, becoming  8000
val refs len: 1000, becoming  9000
val refs len: 1000, becoming 10000
keys     len:  641, becoming   641
keys     len:  635, becoming  1276
keys     len:  653, becoming  1929
keys     len:  619, becoming  2548
keys     len:  627, becoming  3175
keys     len:  629, becoming  3804
keys     len:  631, becoming  4435
keys     len:  627, becoming  5062
keys     len:  625, becoming  5687
keys     len:  642, becoming  6329

The failure is gone if I remove the return true from the load_keys function, but that might mean that we now iterate over more keys than needed...

[arpad-m]

Okay, more investigation reveals that the code in question comes from #1927, more specifically the commit "Optimize stoage keys iterator" (link). It feels weird to me that we early exit the search and also that we extend the length of the last key to the entire rest of the file.

[arpad-m]

I've pushed a commit which I hope fixes the test failures.... but I don't understand why this prior optimization worked. I think due to the reordering, it's not possible to have a similarly optimized state where a chunk of the keys are gone when we have a shared list (for the tenant::tests::test_random_updates it's roughly half as observable above)... not sure if then having a shared list is worth it.

[arpad-m]

I've looked at what is done with the keys, and am now more confident that all the merging did was to ensure that blobs for the same key (but different lsns) would end up in the same layer file. To preserve that, I've added a commit to coalesce the iterator. It won't have precisely the same coalescing merging though: first, the old approach would not merge key pairs from different files. in the new vec we have no good way to distinguish files so instead we just look at the size: if the combined key reaches the target file size, we don't merge. This might still create files larger than the target file size, however it will stay below the double of the target file size.

[arpad-m]

I've pushed an earlier state of the PR (with two separate vecs) to this branch to enable faster merging. This will unblock some downstream refactors and separates the more involved changes from the ones that just do a refactor. For the changes that originally were part of this PR, see #4888.