From 9177312ba6bd1b8ba85e77d4490517a7f4c01ec5 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Fri, 14 Feb 2025 18:57:18 +0100 Subject: [PATCH] basebackup: use `Timeline::get` for `get_rel` instead of `get_rel_page_at_lsn` (#10476) I noticed the opportunity to simplify here while working on https://github.com/neondatabase/neon/pull/9353 . The only difference is the zero-fill behavior: if one reads past rel size, `get_rel_page_at_lsn` returns a zeroed page whereas `Timeline::get` returns an error. However, the `endblk` is at most rel size large, because `nblocks` is eq `get_rel_size`, see a few lines above this change. We're using the same LSN (`self.lsn`) for everything, so there is no chance of non-determinism. Refs: - Slack discussion debating correctness: https://neondb.slack.com/archives/C033RQ5SPDH/p1737457010607119 --- pageserver/src/basebackup.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/pageserver/src/basebackup.rs b/pageserver/src/basebackup.rs index 25078b57c8..e03b1bbe96 100644 --- a/pageserver/src/basebackup.rs +++ b/pageserver/src/basebackup.rs @@ -13,7 +13,7 @@ use anyhow::{anyhow, Context}; use bytes::{BufMut, Bytes, BytesMut}; use fail::fail_point; -use pageserver_api::key::Key; +use pageserver_api::key::{rel_block_to_key, Key}; use postgres_ffi::pg_constants; use std::fmt::Write as FmtWrite; use std::time::{Instant, SystemTime}; @@ -501,13 +501,9 @@ where for blknum in startblk..endblk { let img = self .timeline - .get_rel_page_at_lsn( - src, - blknum, - Version::Lsn(self.lsn), - self.ctx, - self.io_concurrency.clone(), - ) + // TODO: investigate using get_vectored for the entire startblk..endblk range. + // But this code path is not on the critical path for most basebackups (?). + .get(rel_block_to_key(src, blknum), self.lsn, self.ctx) .await .map_err(|e| BasebackupError::Server(e.into()))?; segment_data.extend_from_slice(&img[..]);