-
Notifications
You must be signed in to change notification settings - Fork 409
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
pageserver: fix image layer creation check that inhibited compaction #7420
Conversation
2958 tests run: 2837 passed, 0 failed, 121 skipped (full report)Flaky tests (7)Postgres 16Postgres 15
Postgres 14
Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
aa2ea71 at 2024-04-26T13:27:03.263Z :recycle: |
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. Hoist the check up to the timeline level. We should probably allow compaction to catch up across the fleet before re-enabling this. In the meantime, I'll test with a real tenant to make sure we don't inhibit compaction in an unexpected way again.
df46294
to
4460f98
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
Yes, that's right |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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? |
Good to go from my side. |
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
Checklist before merging