diff --git a/pageserver/src/layered_repository.rs b/pageserver/src/layered_repository.rs index 0aef3afacc..17795f8f1e 100644 --- a/pageserver/src/layered_repository.rs +++ b/pageserver/src/layered_repository.rs @@ -1424,15 +1424,21 @@ impl LayeredTimeline { } // 2. Is it needed by a child branch? + // NOTE With that wee would keep data that + // might be referenced by child branches forever. + // We can track this in child timeline GC and delete parent layers when + // they are no longer needed. This might be complicated with long inheritance chains. for retain_lsn in &retain_lsns { - // start_lsn is inclusive and end_lsn is exclusive - if l.get_start_lsn() <= *retain_lsn && *retain_lsn < l.get_end_lsn() { + // start_lsn is inclusive + if &l.get_start_lsn() <= retain_lsn { info!( - "keeping {} {}-{} because it's needed by branch point {}", + "keeping {} {}-{} because it's still might be referenced by child branch forked at {} is_dropped: {} is_incremental: {}", seg, l.get_start_lsn(), l.get_end_lsn(), - *retain_lsn + retain_lsn, + l.is_dropped(), + l.is_incremental(), ); if seg.rel.is_relation() { result.ondisk_relfiles_needed_by_branches += 1; diff --git a/pageserver/src/layered_repository/delta_layer.rs b/pageserver/src/layered_repository/delta_layer.rs index 8b17af251e..d9d0ed37d3 100644 --- a/pageserver/src/layered_repository/delta_layer.rs +++ b/pageserver/src/layered_repository/delta_layer.rs @@ -203,7 +203,7 @@ impl Layer for DeltaLayer { let page_version_reader = inner .book .as_ref() - .unwrap() + .expect("should be loaded in load call above") .chapter_reader(PAGE_VERSIONS_CHAPTER)?; // Scan the metadata BTreeMap backwards, starting from the given entry. @@ -530,7 +530,7 @@ impl DeltaLayer { *inner = DeltaLayerInner { loaded: true, - book: None, + book: Some(book), page_version_metas, relsizes, }; diff --git a/pageserver/src/repository.rs b/pageserver/src/repository.rs index 7ba608e001..17c65d3e91 100644 --- a/pageserver/src/repository.rs +++ b/pageserver/src/repository.rs @@ -829,6 +829,23 @@ mod tests { .to_string() .contains("tried to request a page version that was garbage collected")), } + Ok(()) + } + + #[test] + fn test_retain_data_in_parent_which_is_needed_for_child() -> Result<()> { + let repo = + RepoHarness::create("test_retain_data_in_parent_which_is_needed_for_child")?.load(); + let tline = repo.create_empty_timeline(TIMELINE_ID)?; + + make_some_layers(&tline)?; + + repo.branch_timeline(TIMELINE_ID, NEW_TIMELINE_ID, Lsn(0x40))?; + let newtline = repo.get_timeline(NEW_TIMELINE_ID)?; + + // this removes layers before lsn 40 (50 minus 10), so there are two remaining layers, image and delta for 31-50 + repo.gc_iteration(Some(TIMELINE_ID), 0x10, false)?; + assert!(newtline.get_page_at_lsn(TESTREL_A, 0, Lsn(0x25)).is_ok()); Ok(()) }