From bc5ade701b405a149cae9595e5471cc5823e6389 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Sun, 28 May 2023 02:39:44 +0300 Subject: [PATCH] Enable sanity check that disk_consistent_lsn is valid on created timeline. This required changing a bunch of unit tests to use a valid initdb LSN. --- pageserver/src/tenant.rs | 65 ++++++++++++------- .../src/tenant/remote_timeline_client.rs | 3 +- pageserver/src/tenant/timeline.rs | 5 ++ .../walreceiver/connection_manager.rs | 2 +- 4 files changed, 49 insertions(+), 26 deletions(-) diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index ad061b7002..95393c380a 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -11,7 +11,7 @@ //! parent timeline, and the last LSN that has been written to disk. //! -use anyhow::{bail, Context}; +use anyhow::{bail, ensure, Context}; use futures::FutureExt; use pageserver_api::models::TimelineState; use remote_storage::DownloadError; @@ -199,11 +199,11 @@ impl UninitializedTimeline<'_> { })?; // Check that the caller initialized disk_consistent_lsn - // - // TODO: many our unit tests violate this. - //let new_disk_consistent_lsn = new_timeline.get_disk_consistent_lsn(); - //anyhow::ensure!(new_disk_consistent_lsn.is_valid(), - // "new timeline {tenant_id}/{timeline_id} has invalid disk_consistent_lsn"); + let new_disk_consistent_lsn = new_timeline.get_disk_consistent_lsn(); + ensure!( + new_disk_consistent_lsn.is_valid(), + "new timeline {tenant_id}/{timeline_id} has invalid disk_consistent_lsn" + ); let mut timelines = self.owning_tenant.timelines.lock().unwrap(); match timelines.entry(timeline_id) { @@ -1250,6 +1250,13 @@ impl Tenant { ctx: &RequestContext, ) -> anyhow::Result> { let uninit_tl = self.create_empty_timeline(new_timeline_id, initdb_lsn, pg_version, ctx)?; + + // Normally, you would need to import some data to the timeline before making it + // available. But our unit tests have no such initial data. + uninit_tl + .raw_timeline()? + .set_disk_consistent_lsn(initdb_lsn); + let tl = uninit_tl.finish_creation()?; // The non-test code would call tl.activate() here. tl.set_state(TimelineState::Active); @@ -3459,7 +3466,8 @@ mod tests { #[tokio::test] async fn test_basic() -> anyhow::Result<()> { let (tenant, ctx) = TenantHarness::create("test_basic")?.load().await; - let tline = tenant.create_test_timeline(TIMELINE_ID, Lsn(0), DEFAULT_PG_VERSION, &ctx)?; + let tline = + tenant.create_test_timeline(TIMELINE_ID, Lsn(0x05), DEFAULT_PG_VERSION, &ctx)?; let writer = tline.writer(); writer.put(*TEST_KEY, Lsn(0x10), &Value::Image(TEST_IMG("foo at 0x10")))?; @@ -3492,9 +3500,9 @@ mod tests { let (tenant, ctx) = TenantHarness::create("no_duplicate_timelines")? .load() .await; - let _ = tenant.create_test_timeline(TIMELINE_ID, Lsn(0), DEFAULT_PG_VERSION, &ctx)?; + let _ = tenant.create_test_timeline(TIMELINE_ID, Lsn(0x10), DEFAULT_PG_VERSION, &ctx)?; - match tenant.create_empty_timeline(TIMELINE_ID, Lsn(0), DEFAULT_PG_VERSION, &ctx) { + match tenant.create_empty_timeline(TIMELINE_ID, Lsn(0x10), DEFAULT_PG_VERSION, &ctx) { Ok(_) => panic!("duplicate timeline creation should fail"), Err(e) => assert_eq!( e.to_string(), @@ -3523,7 +3531,8 @@ mod tests { use std::str::from_utf8; let (tenant, ctx) = TenantHarness::create("test_branch")?.load().await; - let tline = tenant.create_test_timeline(TIMELINE_ID, Lsn(0), DEFAULT_PG_VERSION, &ctx)?; + let tline = + tenant.create_test_timeline(TIMELINE_ID, Lsn(0x10), DEFAULT_PG_VERSION, &ctx)?; let writer = tline.writer(); #[allow(non_snake_case)] @@ -3620,7 +3629,8 @@ mod tests { TenantHarness::create("test_prohibit_branch_creation_on_garbage_collected_data")? .load() .await; - let tline = tenant.create_test_timeline(TIMELINE_ID, Lsn(0), DEFAULT_PG_VERSION, &ctx)?; + let tline = + tenant.create_test_timeline(TIMELINE_ID, Lsn(0x10), DEFAULT_PG_VERSION, &ctx)?; make_some_layers(tline.as_ref(), Lsn(0x20)).await?; // this removes layers before lsn 40 (50 minus 10), so there are two remaining layers, image and delta for 31-50 @@ -3707,7 +3717,8 @@ mod tests { TenantHarness::create("test_get_branchpoints_from_an_inactive_timeline")? .load() .await; - let tline = tenant.create_test_timeline(TIMELINE_ID, Lsn(0), DEFAULT_PG_VERSION, &ctx)?; + let tline = + tenant.create_test_timeline(TIMELINE_ID, Lsn(0x10), DEFAULT_PG_VERSION, &ctx)?; make_some_layers(tline.as_ref(), Lsn(0x20)).await?; tenant @@ -3755,7 +3766,8 @@ mod tests { TenantHarness::create("test_retain_data_in_parent_which_is_needed_for_child")? .load() .await; - let tline = tenant.create_test_timeline(TIMELINE_ID, Lsn(0), DEFAULT_PG_VERSION, &ctx)?; + let tline = + tenant.create_test_timeline(TIMELINE_ID, Lsn(0x10), DEFAULT_PG_VERSION, &ctx)?; make_some_layers(tline.as_ref(), Lsn(0x20)).await?; tenant @@ -3778,7 +3790,8 @@ mod tests { TenantHarness::create("test_parent_keeps_data_forever_after_branching")? .load() .await; - let tline = tenant.create_test_timeline(TIMELINE_ID, Lsn(0), DEFAULT_PG_VERSION, &ctx)?; + let tline = + tenant.create_test_timeline(TIMELINE_ID, Lsn(0x10), DEFAULT_PG_VERSION, &ctx)?; make_some_layers(tline.as_ref(), Lsn(0x20)).await?; tenant @@ -3831,7 +3844,7 @@ mod tests { { let (tenant, ctx) = harness.load().await; let tline = - tenant.create_test_timeline(TIMELINE_ID, Lsn(0), DEFAULT_PG_VERSION, &ctx)?; + tenant.create_test_timeline(TIMELINE_ID, Lsn(0x10), DEFAULT_PG_VERSION, &ctx)?; make_some_layers(tline.as_ref(), Lsn(0x20)).await?; @@ -3868,7 +3881,8 @@ mod tests { let harness = TenantHarness::create(TEST_NAME)?; let (tenant, ctx) = harness.load().await; - let tline = tenant.create_test_timeline(TIMELINE_ID, Lsn(0), DEFAULT_PG_VERSION, &ctx)?; + let tline = + tenant.create_test_timeline(TIMELINE_ID, Lsn(0x10), DEFAULT_PG_VERSION, &ctx)?; drop(tline); drop(tenant); @@ -3906,7 +3920,8 @@ mod tests { #[tokio::test] async fn test_images() -> anyhow::Result<()> { let (tenant, ctx) = TenantHarness::create("test_images")?.load().await; - let tline = tenant.create_test_timeline(TIMELINE_ID, Lsn(0), DEFAULT_PG_VERSION, &ctx)?; + let tline = + tenant.create_test_timeline(TIMELINE_ID, Lsn(0x05), DEFAULT_PG_VERSION, &ctx)?; let writer = tline.writer(); writer.put(*TEST_KEY, Lsn(0x10), &Value::Image(TEST_IMG("foo at 0x10")))?; @@ -3971,7 +3986,8 @@ mod tests { #[tokio::test] async fn test_bulk_insert() -> anyhow::Result<()> { let (tenant, ctx) = TenantHarness::create("test_bulk_insert")?.load().await; - let tline = tenant.create_test_timeline(TIMELINE_ID, Lsn(0), DEFAULT_PG_VERSION, &ctx)?; + let tline = + tenant.create_test_timeline(TIMELINE_ID, Lsn(0x05), DEFAULT_PG_VERSION, &ctx)?; let mut lsn = Lsn(0x10); @@ -4013,7 +4029,8 @@ mod tests { #[tokio::test] async fn test_random_updates() -> anyhow::Result<()> { let (tenant, ctx) = TenantHarness::create("test_random_updates")?.load().await; - let tline = tenant.create_test_timeline(TIMELINE_ID, Lsn(0), DEFAULT_PG_VERSION, &ctx)?; + let tline = + tenant.create_test_timeline(TIMELINE_ID, Lsn(0x10), DEFAULT_PG_VERSION, &ctx)?; const NUM_KEYS: usize = 1000; @@ -4025,7 +4042,7 @@ mod tests { // a read sees the latest page version. let mut updated = [Lsn(0); NUM_KEYS]; - let mut lsn = Lsn(0); + let mut lsn = Lsn(0x10); #[allow(clippy::needless_range_loop)] for blknum in 0..NUM_KEYS { lsn = Lsn(lsn.0 + 0x10); @@ -4087,7 +4104,7 @@ mod tests { .load() .await; let mut tline = - tenant.create_test_timeline(TIMELINE_ID, Lsn(0), DEFAULT_PG_VERSION, &ctx)?; + tenant.create_test_timeline(TIMELINE_ID, Lsn(0x10), DEFAULT_PG_VERSION, &ctx)?; const NUM_KEYS: usize = 1000; @@ -4099,7 +4116,7 @@ mod tests { // a read sees the latest page version. let mut updated = [Lsn(0); NUM_KEYS]; - let mut lsn = Lsn(0); + let mut lsn = Lsn(0x10); #[allow(clippy::needless_range_loop)] for blknum in 0..NUM_KEYS { lsn = Lsn(lsn.0 + 0x10); @@ -4170,7 +4187,7 @@ mod tests { .load() .await; let mut tline = - tenant.create_test_timeline(TIMELINE_ID, Lsn(0), DEFAULT_PG_VERSION, &ctx)?; + tenant.create_test_timeline(TIMELINE_ID, Lsn(0x10), DEFAULT_PG_VERSION, &ctx)?; const NUM_KEYS: usize = 100; const NUM_TLINES: usize = 50; @@ -4179,7 +4196,7 @@ mod tests { // Track page mutation lsns across different timelines. let mut updated = [[Lsn(0); NUM_KEYS]; NUM_TLINES]; - let mut lsn = Lsn(0); + let mut lsn = Lsn(0x10); #[allow(clippy::needless_range_loop)] for idx in 0..NUM_TLINES { diff --git a/pageserver/src/tenant/remote_timeline_client.rs b/pageserver/src/tenant/remote_timeline_client.rs index c4640307d0..e18a86349e 100644 --- a/pageserver/src/tenant/remote_timeline_client.rs +++ b/pageserver/src/tenant/remote_timeline_client.rs @@ -1264,7 +1264,8 @@ mod tests { let harness = TenantHarness::create(test_name)?; let (tenant, ctx) = runtime.block_on(harness.load()); // create an empty timeline directory - let _ = tenant.create_test_timeline(TIMELINE_ID, Lsn(0), DEFAULT_PG_VERSION, &ctx)?; + let _ = + tenant.create_test_timeline(TIMELINE_ID, Lsn(0x10), DEFAULT_PG_VERSION, &ctx)?; let remote_fs_dir = harness.conf.workdir.join("remote_fs"); std::fs::create_dir_all(remote_fs_dir)?; diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 3673b74bf1..cb7129499e 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -581,6 +581,11 @@ impl Timeline { self.disk_consistent_lsn.load() } + #[cfg(test)] + pub(super) fn set_disk_consistent_lsn(&self, lsn: Lsn) { + self.disk_consistent_lsn.store(lsn) + } + pub fn get_remote_consistent_lsn(&self) -> Option { if let Some(remote_client) = &self.remote_client { remote_client.last_uploaded_consistent_lsn() diff --git a/pageserver/src/tenant/timeline/walreceiver/connection_manager.rs b/pageserver/src/tenant/timeline/walreceiver/connection_manager.rs index e235fab425..88c317f369 100644 --- a/pageserver/src/tenant/timeline/walreceiver/connection_manager.rs +++ b/pageserver/src/tenant/timeline/walreceiver/connection_manager.rs @@ -1324,7 +1324,7 @@ mod tests { async fn dummy_state(harness: &TenantHarness<'_>) -> ConnectionManagerState { let (tenant, ctx) = harness.load().await; let timeline = tenant - .create_test_timeline(TIMELINE_ID, Lsn(0), crate::DEFAULT_PG_VERSION, &ctx) + .create_test_timeline(TIMELINE_ID, Lsn(0x10), crate::DEFAULT_PG_VERSION, &ctx) .expect("Failed to create an empty timeline for dummy wal connection manager"); ConnectionManagerState {