Skip to content

Commit

Permalink
fix: vectored get returns incorrect result on inexact materialized pa…
Browse files Browse the repository at this point in the history
…ge cache hit (#8050)

# Problem

Suppose our vectored get starts with an inexact materialized page cache
hit ("cached lsn") that is shadowed by a newer image layer image layer.
Like so:


```
    <inmemory layers>

    +-+ < delta layer
    | |
   -|-|----- < image layer
    | |
    | |
   -|-|----- < cached lsn for requested key
    +_+
```

The correct visitation order is
1. inmemory layers
2. delta layer records in LSN range `[image_layer.lsn,
oldest_inmemory_layer.lsn_range.start)`
3. image layer

However, the vectored get code, when it visits the delta layer, it
(incorrectly!) returns with state `Complete`.

The reason why it returns is that it calls `on_lsn_advanced` with
`self.lsn_range.start`, i.e., the layer's LSN range.

Instead, it should use `lsn_range.start`, i.e., the LSN range from the
correct visitation order listed above.

# Solution

Use `lsn_range.start` instead of `self.lsn_range.start`.

# Refs

discovered by & fixes #6967

Co-authored-by: Vlad Lazar <vlad@neon.tech>
  • Loading branch information
problame and VladLazar committed Jun 13, 2024
1 parent d25f7e3 commit 8271954
Showing 1 changed file with 2 additions and 4 deletions.
6 changes: 2 additions & 4 deletions pageserver/src/tenant/storage_layer/delta_layer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,6 @@ pub struct DeltaLayerInner {
// values copied from summary
index_start_blk: u32,
index_root_blk: u32,
lsn_range: Range<Lsn>,

file: VirtualFile,
file_id: FileId,
Expand Down Expand Up @@ -785,7 +784,6 @@ impl DeltaLayerInner {
file_id,
index_start_blk: actual_summary.index_start_blk,
index_root_blk: actual_summary.index_root_blk,
lsn_range: actual_summary.lsn_range,
max_vectored_read_bytes,
}))
}
Expand Down Expand Up @@ -911,7 +909,7 @@ impl DeltaLayerInner {

let reads = Self::plan_reads(
&keyspace,
lsn_range,
lsn_range.clone(),
data_end_offset,
index_reader,
planner,
Expand All @@ -924,7 +922,7 @@ impl DeltaLayerInner {
self.do_reads_and_update_state(reads, reconstruct_state, ctx)
.await;

reconstruct_state.on_lsn_advanced(&keyspace, self.lsn_range.start);
reconstruct_state.on_lsn_advanced(&keyspace, lsn_range.start);

Ok(())
}
Expand Down

1 comment on commit 8271954

@github-actions
Copy link

Choose a reason for hiding this comment

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

3304 tests run: 3153 passed, 0 failed, 151 skipped (full report)


Code coverage* (full report)

  • functions: 31.5% (6632 of 21060 functions)
  • lines: 48.6% (51623 of 106254 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
8271954 at 2024-06-13T19:43:31.774Z :recycle:

Please sign in to comment.