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: drop the layer map lock after planning reads #7215

Merged
merged 11 commits into from Apr 2, 2024

Conversation

VladLazar
Copy link
Contributor

@VladLazar VladLazar commented Mar 23, 2024

Problem

The vectored read path holds the layer map lock while visiting a timeline.

Summary of changes

  • Rework the fringe order to hold Layer on Arc<InMemoryLayer> handles instead of descriptions that are resolved by the layer map at the time of read. Note that previously get_values_reconstruct_data was implemented for the layer description which already knew the lsn range for the read. Now it is implemented on the new ReadableLayer handle and needs to get the lsn range as an argument.
  • Drop the layer map lock after updating the fringe.

Related #6833

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

Copy link

github-actions bot commented Mar 23, 2024

2730 tests run: 2590 passed, 0 failed, 140 skipped (full report)


Code coverage* (full report)

  • functions: 28.2% (6307 of 22365 functions)
  • lines: 47.0% (44298 of 94281 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
b1feb1f at 2024-03-27T16:28:03.162Z :recycle:

@VladLazar VladLazar marked this pull request as ready for review March 23, 2024 16:34
@VladLazar VladLazar requested a review from a team as a code owner March 23, 2024 16:34
Copy link
Contributor

@koivunej koivunej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accidentially hit the pending review comment button, flushing.

pageserver/src/tenant/storage_layer.rs Outdated Show resolved Hide resolved
pageserver/src/tenant/storage_layer.rs Outdated Show resolved Hide resolved
pageserver/src/tenant/storage_layer/inmemory_layer.rs Outdated Show resolved Hide resolved
@VladLazar VladLazar requested a review from koivunej March 26, 2024 17:37
Copy link
Contributor

@problame problame left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we can remove the InMemoryLayerHandle?
It's dead code, but not identified because it's pub.


I haven't reviewed the Ordering / descriptor stuff, as Joonas has already commented on it.
I think it would be nice to minimize this diff by

  1. putting the new types in the same location in the file as the old types and
  2. using the same names

We can improve the names in a later PR.

Then again, this is not a super strongly held request.


The PR description only mentions "rework the fringe a bit", but, doesn't explain why the signature of get_values_reconstruct_data had to change and now includes an LSN range. I think a bit more description is justified.

Specifically, it would be nice to explain what exactly has been reworked.


The core change "hold struct Layer or Arc" without the layer map lock is sound.

pageserver/src/tenant/storage_layer/inmemory_layer.rs Outdated Show resolved Hide resolved
pageserver/src/tenant/layer_map.rs Show resolved Hide resolved
pageserver/src/tenant/storage_layer.rs Outdated Show resolved Hide resolved
pageserver/src/tenant/storage_layer.rs Outdated Show resolved Hide resolved
@VladLazar
Copy link
Contributor Author

Seems like we can remove the InMemoryLayerHandle? It's dead code, but not identified because it's pub.

Good catch. Done.

I haven't reviewed the Ordering / descriptor stuff, as Joonas has already commented on it. I think it would be nice to minimize this diff by

1. putting the new types in the same location in the file as the old types and

Done.

2. using the same names

The old names don't really fit anymore.

We can improve the names in a later PR.

Then again, this is not a super strongly held request.

The PR description only mentions "rework the fringe a bit", but, doesn't explain why the signature of get_values_reconstruct_data had to change and now includes an LSN range. I think a bit more description is justified.

Specifically, it would be nice to explain what exactly has been reworked.

Updated the PR description.

pageserver/src/tenant/storage_layer/inmemory_layer.rs Outdated Show resolved Hide resolved
pageserver/src/tenant/storage_layer.rs Outdated Show resolved Hide resolved
pageserver/src/tenant/timeline.rs Outdated Show resolved Hide resolved
@VladLazar VladLazar merged commit 9957c6a into main Apr 2, 2024
53 checks passed
@VladLazar VladLazar deleted the vlad/drop-layer-map-lock branch April 2, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants