pageserver: fix image layer creation check that inhibited compaction

[VladLazar]

Problem

PR #7230 attempted to introduce a WAL ingest threshold for checking whether enough deltas are stacked to warrant creating a new image layer. However, this check was incorrectly performed at the compaction partition level instead of the timeline level. Hence, it inhibited GC for any keys outside of the first partition.

Summary of Changes

Hoist the check up to the timeline level.

Testing and Deployment

I generated the layer map diagram from the output of test_compaction_smoke and checked that it looks as expected.
Before the fix there's no image layers. For deployment, I suggest we go region by region and monitor pageserver_layers_visited_per_read_global. If people have ideas for any more testing, happy to do it.

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]

2958 tests run: 2837 passed, 0 failed, 121 skipped (full report)

---

Flaky tests (7)

Postgres 16

• test_vm_bit_clear_on_heap_lock: release
• test_wal_restore_initdb: release
• test_wal_restore: release

Postgres 15

• test_fully_custom_config: debug
• test_crafted_wal_end[last_wal_record_xlog_switch]: debug
• test_multi_attach: debug

Postgres 14

• test_crafted_wal_end[simple]: debug

Code coverage* (full report)

• functions: 28.1% (6483 of 23056 functions)
• lines: 46.9% (46049 of 98152 lines)

* collected from Rust tests only

---

_{The comment gets automatically updated with the latest test results
aa2ea71 at 2024-04-26T13:27:03.263Z :recycle:}

[koivunej]

Excellent that the test now shows expected behaviour with the config removed.

Do I understand correctly that only the self.last_image_layer_creation_check_at.store was at the wrong place, but everything could be hoisted as that was the original intention? It is a bit frustrating how non-obviously this place is better, partially due to how the code is organized (the per-partition detail method is above or comes before of per-timeline or the caller).

[VladLazar]

Do I understand correctly that only the self.last_image_layer_creation_check_at.store was at the wrong place, but everything could be hoisted as that was the original intention?

Yes, that's right

[problame]

I'm surprised this doesn't break a bunch of tests. Like, the ones where we write a certain amount of data and then check for an image layer being created.

But 🤷, LGTM

As a side note: I don't like we use "threshold" in the config variable name here; because we already have image_creation_threshold, whose unit is "number of delta layers" whereas this variable's unit is "amount of WAL bytes".
But, I guess it's too late for that.

[VladLazar]

I'm surprised this doesn't break a bunch of tests. Like, the ones where we write a certain amount of data and then check for an image layer being created.

Probably for the same reason why these tests passed in #7230. They don't write a keyspace that's wide enough to get multiple partitions?

[problame]

Good to go from my side.