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

pageserver: work around #9185 in layer visibility calculation #9400

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jcsp
Copy link
Collaborator

@jcsp jcsp commented Oct 15, 2024

Problem

The layer visibility logic considers a layer visible if serving a read from the head LSN or a branch point would hit the layer. It presumes that an image layer is sufficient to prevent reads reaching a delta.

However, getpage logic doesn't quite match this expectation, and will sometimes read deltas even if an image layer is available: #9185

Closes: #9058

Summary of changes

  • In update_layer_visibility, use the start_lsn of the oldest InMemoryLayer as a read point, so that we will continue to regard delta layers as visible until they are covered by an image layer and that image layer doesn't intersect with an InMemoryLayer's LSN range.

Because we only call update_layer_visibility after generating image layers or at tenant load, this means that such deltas will remain "visible" until the next time one of those things happens. This degrades the effectiveness of the layer visibility contribution to eviction and heatmap generation, but relieves us of the volume of misleading "became visible" log messages (#9058)

This code should be removed when #9185 is fixed.

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

@jcsp jcsp added t/bug Issue Type: Bug c/storage/pageserver Component: storage: pageserver labels Oct 15, 2024
Copy link

5202 tests run: 4995 passed, 0 failed, 207 skipped (full report)


Code coverage* (full report)

  • functions: 31.4% (7551 of 24068 functions)
  • lines: 49.2% (60429 of 122907 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
0ed9897 at 2024-10-15T18:14:35.832Z :recycle:

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 t/bug Issue Type: Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate unexpectedly high volume of "became visible" logs
1 participant