Revert "revert the cancellation sensitivity change for the flush task, doesn't work because flush task can't disambiguate orderly shutdown"

This reverts commit 42922cebe0.

compute_tools/src/bin/compute_ctl.rs

@@ -29,12 +29,13 @@
 //! ```sh
 //! compute_ctl -D /var/db/postgres/compute \
 //!             -C 'postgresql://cloud_admin@localhost/postgres' \
-//!             -c /var/db/postgres/configs/config.json \
+//!             -S /var/db/postgres/specs/current.json \
 //!             -b /usr/local/bin/postgres \
 //!             -r http://pg-ext-s3-gateway \
 //! ```
 use std::ffi::OsString;
 use std::fs::File;
+use std::path::Path;
 use std::process::exit;
 use std::sync::mpsc;
 use std::thread;
@@ -42,7 +43,8 @@ use std::time::Duration;
 
 use anyhow::{Context, Result};
 use clap::Parser;
-use compute_api::responses::ComputeConfig;
+use compute_api::responses::ComputeCtlConfig;
+use compute_api::spec::ComputeSpec;
 use compute_tools::compute::{
     BUILD_TAG, ComputeNode, ComputeNodeParams, forward_termination_signal,
 };
@@ -116,10 +118,8 @@ struct Cli {
     #[arg(long)]
     pub set_disk_quota_for_fs: Option<String>,
 
-    // TODO(tristan957): remove alias after compatibility tests are no longer
-    // an issue
-    #[arg(short = 'c', long, alias = "spec-path")]
-    pub config: Option<OsString>,
+    #[arg(short = 'S', long, group = "spec-path")]
+    pub spec_path: Option<OsString>,
 
     #[arg(short = 'i', long, group = "compute-id")]
     pub compute_id: String,
@@ -127,9 +127,8 @@ struct Cli {
     #[arg(
         short = 'p',
         long,
-        conflicts_with = "config",
-        value_name = "CONTROL_PLANE_API_BASE_URL",
-        requires = "compute-id"
+        conflicts_with = "spec-path",
+        value_name = "CONTROL_PLANE_API_BASE_URL"
     )]
     pub control_plane_uri: Option<String>,
 }
@@ -139,7 +138,7 @@ fn main() -> Result<()> {
 
     let scenario = failpoint_support::init();
 
-    // For historical reasons, the main thread that processes the config and launches postgres
+    // For historical reasons, the main thread that processes the spec and launches postgres
     // is synchronous, but we always have this tokio runtime available and we "enter" it so
     // that you can use tokio::spawn() and tokio::runtime::Handle::current().block_on(...)
     // from all parts of compute_ctl.
@@ -155,7 +154,7 @@ fn main() -> Result<()> {
 
     let connstr = Url::parse(&cli.connstr).context("cannot parse connstr as a URL")?;
 
-    let config = get_config(&cli)?;
+    let cli_spec = try_spec_from_cli(&cli)?;
 
     let compute_node = ComputeNode::new(
         ComputeNodeParams {
@@ -176,7 +175,8 @@ fn main() -> Result<()> {
             #[cfg(target_os = "linux")]
             vm_monitor_addr: cli.vm_monitor_addr,
         },
-        config,
+        cli_spec.spec,
+        cli_spec.compute_ctl_config,
     )?;
 
     let exit_code = compute_node.run()?;
@@ -201,17 +201,27 @@ async fn init() -> Result<()> {
     Ok(())
 }
 
-fn get_config(cli: &Cli) -> Result<ComputeConfig> {
-    // First, read the config from the path if provided
-    if let Some(ref config) = cli.config {
-        let file = File::open(config)?;
-        return Ok(serde_json::from_reader(&file)?);
+fn try_spec_from_cli(cli: &Cli) -> Result<CliSpecParams> {
+    // First, read spec from the path if provided
+    if let Some(ref spec_path) = cli.spec_path {
+        let file = File::open(Path::new(spec_path))?;
+        return Ok(CliSpecParams {
+            spec: Some(serde_json::from_reader(file)?),
+            compute_ctl_config: ComputeCtlConfig::default(),
+        });
     }
 
-    // If the config wasn't provided in the CLI arguments, then retrieve it from
+    if cli.control_plane_uri.is_none() {
+        panic!("must specify --control-plane-uri");
+    };
+
+    // If the spec wasn't provided in the CLI arguments, then retrieve it from
     // the control plane
-    match get_config_from_control_plane(cli.control_plane_uri.as_ref().unwrap(), &cli.compute_id) {
-        Ok(config) => Ok(config),
+    match get_spec_from_control_plane(cli.control_plane_uri.as_ref().unwrap(), &cli.compute_id) {
+        Ok(resp) => Ok(CliSpecParams {
+            spec: resp.0,
+            compute_ctl_config: resp.1,
+        }),
         Err(e) => {
             error!(
                 "cannot get response from control plane: {}\n\
@@ -223,6 +233,13 @@ fn get_config(cli: &Cli) -> Result<ComputeConfig> {
     }
 }
 
+struct CliSpecParams {
+    /// If a spec was provided via CLI or file, the [`ComputeSpec`]
+    spec: Option<ComputeSpec>,
+    #[allow(dead_code)]
+    compute_ctl_config: ComputeCtlConfig,
+}
+
 fn deinit_and_exit(exit_code: Option<i32>) -> ! {
     // Shutdown trace pipeline gracefully, so that it has a chance to send any
     // pending traces before we exit. Shutting down OTEL tracing provider may

compute_tools/src/compute.rs

@@ -11,7 +11,7 @@ use std::{env, fs};
 use anyhow::{Context, Result};
 use chrono::{DateTime, Utc};
 use compute_api::privilege::Privilege;
-use compute_api::responses::{ComputeConfig, ComputeCtlConfig, ComputeMetrics, ComputeStatus};
+use compute_api::responses::{ComputeCtlConfig, ComputeMetrics, ComputeStatus};
 use compute_api::spec::{
     ComputeAudit, ComputeFeature, ComputeMode, ComputeSpec, ExtVersion, PgIdent,
 };
@@ -303,7 +303,11 @@ struct StartVmMonitorResult {
 }
 
 impl ComputeNode {
-    pub fn new(params: ComputeNodeParams, config: ComputeConfig) -> Result<Self> {
+    pub fn new(
+        params: ComputeNodeParams,
+        cli_spec: Option<ComputeSpec>,
+        compute_ctl_config: ComputeCtlConfig,
+    ) -> Result<Self> {
         let connstr = params.connstr.as_str();
         let conn_conf = postgres::config::Config::from_str(connstr)
             .context("cannot build postgres config from connstr")?;
@@ -311,8 +315,8 @@ impl ComputeNode {
             .context("cannot build tokio postgres config from connstr")?;
 
         let mut new_state = ComputeState::new();
-        if let Some(spec) = config.spec {
-            let pspec = ParsedSpec::try_from(spec).map_err(|msg| anyhow::anyhow!(msg))?;
+        if let Some(cli_spec) = cli_spec {
+            let pspec = ParsedSpec::try_from(cli_spec).map_err(|msg| anyhow::anyhow!(msg))?;
             new_state.pspec = Some(pspec);
         }
 
@@ -323,7 +327,7 @@ impl ComputeNode {
             state: Mutex::new(new_state),
             state_changed: Condvar::new(),
             ext_download_progress: RwLock::new(HashMap::new()),
-            compute_ctl_config: config.compute_ctl_config,
+            compute_ctl_config,
         })
     }
 

compute_tools/src/http/middleware/authorize.rs

@@ -11,7 +11,7 @@ use futures::future::BoxFuture;
 use http::{Request, Response, StatusCode};
 use jsonwebtoken::{Algorithm, DecodingKey, TokenData, Validation, jwk::JwkSet};
 use tower_http::auth::AsyncAuthorizeRequest;
-use tracing::{debug, warn};
+use tracing::warn;
 
 use crate::http::{JsonResponse, extract::RequestId};
 
@@ -92,7 +92,7 @@ impl AsyncAuthorizeRequest<Body> for Authorize {
             if data.claims.compute_id != compute_id {
                 return Err(JsonResponse::error(
                     StatusCode::UNAUTHORIZED,
-                    "invalid compute ID in authorization token claims",
+                    "invalid claims in authorization token",
                 ));
             }
 
@@ -112,14 +112,12 @@ impl Authorize {
         token: &str,
         validation: &Validation,
     ) -> Result<TokenData<ComputeClaims>> {
-        debug!("verifying token {}", token);
-
         for jwk in jwks.keys.iter() {
             let decoding_key = match DecodingKey::from_jwk(jwk) {
                 Ok(key) => key,
                 Err(e) => {
                     warn!(
-                        "failed to construct decoding key from {}: {}",
+                        "Failed to construct decoding key from {}: {}",
                         jwk.common.key_id.as_ref().unwrap(),
                         e
                     );
@@ -132,7 +130,7 @@ impl Authorize {
                 Ok(data) => return Ok(data),
                 Err(e) => {
                     warn!(
-                        "failed to decode authorization token using {}: {}",
+                        "Failed to decode authorization token using {}: {}",
                         jwk.common.key_id.as_ref().unwrap(),
                         e
                     );
@@ -142,6 +140,6 @@ impl Authorize {
             }
         }
 
-        Err(anyhow!("failed to verify authorization token"))
+        Err(anyhow!("Failed to verify authorization token"))
     }
 }

compute_tools/src/metrics.rs

@@ -19,13 +19,13 @@ pub(crate) static INSTALLED_EXTENSIONS: Lazy<UIntGaugeVec> = Lazy::new(|| {
 // but for all our APIs we defined a 'slug'/method/operationId in the OpenAPI spec.
 // And it's fair to call it a 'RPC' (Remote Procedure Call).
 pub enum CPlaneRequestRPC {
-    GetConfig,
+    GetSpec,
 }
 
 impl CPlaneRequestRPC {
     pub fn as_str(&self) -> &str {
         match self {
-            CPlaneRequestRPC::GetConfig => "GetConfig",
+            CPlaneRequestRPC::GetSpec => "GetSpec",
         }
     }
 }

compute_tools/src/spec.rs

@@ -3,8 +3,9 @@ use std::path::Path;
 
 use anyhow::{Result, anyhow, bail};
 use compute_api::responses::{
-    ComputeConfig, ControlPlaneComputeStatus, ControlPlaneConfigResponse,
+    ComputeCtlConfig, ControlPlaneComputeStatus, ControlPlaneSpecResponse,
 };
+use compute_api::spec::ComputeSpec;
 use reqwest::StatusCode;
 use tokio_postgres::Client;
 use tracing::{error, info, instrument};
@@ -20,7 +21,7 @@ use crate::params::PG_HBA_ALL_MD5;
 fn do_control_plane_request(
     uri: &str,
     jwt: &str,
-) -> Result<ControlPlaneConfigResponse, (bool, String, String)> {
+) -> Result<ControlPlaneSpecResponse, (bool, String, String)> {
     let resp = reqwest::blocking::Client::new()
         .get(uri)
         .header("Authorization", format!("Bearer {}", jwt))
@@ -28,14 +29,14 @@ fn do_control_plane_request(
         .map_err(|e| {
             (
                 true,
-                format!("could not perform request to control plane: {:?}", e),
+                format!("could not perform spec request to control plane: {:?}", e),
                 UNKNOWN_HTTP_STATUS.to_string(),
             )
         })?;
 
     let status = resp.status();
     match status {
-        StatusCode::OK => match resp.json::<ControlPlaneConfigResponse>() {
+        StatusCode::OK => match resp.json::<ControlPlaneSpecResponse>() {
             Ok(spec_resp) => Ok(spec_resp),
             Err(e) => Err((
                 true,
@@ -68,35 +69,40 @@ fn do_control_plane_request(
     }
 }
 
-/// Request config from the control-plane by compute_id. If
-/// `NEON_CONTROL_PLANE_TOKEN` env variable is set, it will be used for
-/// authorization.
-pub fn get_config_from_control_plane(base_uri: &str, compute_id: &str) -> Result<ComputeConfig> {
+/// Request spec from the control-plane by compute_id. If `NEON_CONTROL_PLANE_TOKEN`
+/// env variable is set, it will be used for authorization.
+pub fn get_spec_from_control_plane(
+    base_uri: &str,
+    compute_id: &str,
+) -> Result<(Option<ComputeSpec>, ComputeCtlConfig)> {
     let cp_uri = format!("{base_uri}/compute/api/v2/computes/{compute_id}/spec");
-    let jwt: String = std::env::var("NEON_CONTROL_PLANE_TOKEN").unwrap_or_default();
+    let jwt: String = match std::env::var("NEON_CONTROL_PLANE_TOKEN") {
+        Ok(v) => v,
+        Err(_) => "".to_string(),
+    };
     let mut attempt = 1;
 
-    info!("getting config from control plane: {}", cp_uri);
+    info!("getting spec from control plane: {}", cp_uri);
 
     // Do 3 attempts to get spec from the control plane using the following logic:
     // - network error -> then retry
     // - compute id is unknown or any other error -> bail out
     // - no spec for compute yet (Empty state) -> return Ok(None)
-    // - got config -> return Ok(Some(config))
+    // - got spec -> return Ok(Some(spec))
     while attempt < 4 {
         let result = match do_control_plane_request(&cp_uri, &jwt) {
-            Ok(config_resp) => {
+            Ok(spec_resp) => {
                 CPLANE_REQUESTS_TOTAL
                     .with_label_values(&[
-                        CPlaneRequestRPC::GetConfig.as_str(),
+                        CPlaneRequestRPC::GetSpec.as_str(),
                         &StatusCode::OK.to_string(),
                     ])
                     .inc();
-                match config_resp.status {
-                    ControlPlaneComputeStatus::Empty => Ok(config_resp.into()),
+                match spec_resp.status {
+                    ControlPlaneComputeStatus::Empty => Ok((None, spec_resp.compute_ctl_config)),
                     ControlPlaneComputeStatus::Attached => {
-                        if config_resp.spec.is_some() {
-                            Ok(config_resp.into())
+                        if let Some(spec) = spec_resp.spec {
+                            Ok((Some(spec), spec_resp.compute_ctl_config))
                         } else {
                             bail!("compute is attached, but spec is empty")
                         }
@@ -105,7 +111,7 @@ pub fn get_config_from_control_plane(base_uri: &str, compute_id: &str) -> Result
             }
             Err((retry, msg, status)) => {
                 CPLANE_REQUESTS_TOTAL
-                    .with_label_values(&[CPlaneRequestRPC::GetConfig.as_str(), &status])
+                    .with_label_values(&[CPlaneRequestRPC::GetSpec.as_str(), &status])
                     .inc();
                 if retry {
                     Err(anyhow!(msg))
@@ -116,7 +122,7 @@ pub fn get_config_from_control_plane(base_uri: &str, compute_id: &str) -> Result
         };
 
         if let Err(e) = &result {
-            error!("attempt {} to get config failed with: {}", attempt, e);
+            error!("attempt {} to get spec failed with: {}", attempt, e);
         } else {
             return result;
         }
@@ -127,13 +133,13 @@ pub fn get_config_from_control_plane(base_uri: &str, compute_id: &str) -> Result
 
     // All attempts failed, return error.
     Err(anyhow::anyhow!(
-        "Exhausted all attempts to retrieve the config from the control plane"
+        "Exhausted all attempts to retrieve the spec from the control plane"
     ))
 }
 
 /// Check `pg_hba.conf` and update if needed to allow external connections.
 pub fn update_pg_hba(pgdata_path: &Path) -> Result<()> {
-    // XXX: consider making it a part of config.json
+    // XXX: consider making it a part of spec.json
     let pghba_path = pgdata_path.join("pg_hba.conf");
 
     if config::line_in_file(&pghba_path, PG_HBA_ALL_MD5)? {
@@ -147,7 +153,7 @@ pub fn update_pg_hba(pgdata_path: &Path) -> Result<()> {
 
 /// Create a standby.signal file
 pub fn add_standby_signal(pgdata_path: &Path) -> Result<()> {
-    // XXX: consider making it a part of config.json
+    // XXX: consider making it a part of spec.json
     let signalfile = pgdata_path.join("standby.signal");
 
     if !signalfile.exists() {

control_plane/src/endpoint.rs

@@ -29,7 +29,7 @@
 //!     compute.log               - log output of `compute_ctl` and `postgres`
 //!     endpoint.json             - serialized `EndpointConf` struct
 //!     postgresql.conf           - postgresql settings
-//!     config.json                 - passed to `compute_ctl`
+//!     spec.json                 - passed to `compute_ctl`
 //!     pgdata/
 //!         postgresql.conf       - copy of postgresql.conf created by `compute_ctl`
 //!         zenith.signal
@@ -46,9 +46,7 @@ use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH};
 
 use anyhow::{Context, Result, anyhow, bail};
 use compute_api::requests::ConfigurationRequest;
-use compute_api::responses::{
-    ComputeConfig, ComputeCtlConfig, ComputeStatus, ComputeStatusResponse,
-};
+use compute_api::responses::{ComputeCtlConfig, ComputeStatus, ComputeStatusResponse};
 use compute_api::spec::{
     Cluster, ComputeAudit, ComputeFeature, ComputeMode, ComputeSpec, Database, PgIdent,
     RemoteExtSpec, Role,
@@ -621,101 +619,90 @@ impl Endpoint {
             remote_extensions = None;
         };
 
-        // Create config file
-        let config = {
-            let mut spec = ComputeSpec {
-                skip_pg_catalog_updates: self.skip_pg_catalog_updates,
-                format_version: 1.0,
-                operation_uuid: None,
-                features: self.features.clone(),
-                swap_size_bytes: None,
-                disk_quota_bytes: None,
-                disable_lfc_resizing: None,
-                cluster: Cluster {
-                    cluster_id: None, // project ID: not used
-                    name: None,       // project name: not used
-                    state: None,
-                    roles: if create_test_user {
-                        vec![Role {
-                            name: PgIdent::from_str("test").unwrap(),
-                            encrypted_password: None,
-                            options: None,
-                        }]
-                    } else {
-                        Vec::new()
-                    },
-                    databases: if create_test_user {
-                        vec![Database {
-                            name: PgIdent::from_str("neondb").unwrap(),
-                            owner: PgIdent::from_str("test").unwrap(),
-                            options: None,
-                            restrict_conn: false,
-                            invalid: false,
-                        }]
-                    } else {
-                        Vec::new()
-                    },
-                    settings: None,
-                    postgresql_conf: Some(postgresql_conf.clone()),
-                },
-                delta_operations: None,
-                tenant_id: Some(self.tenant_id),
-                timeline_id: Some(self.timeline_id),
-                project_id: None,
-                branch_id: None,
-                endpoint_id: Some(self.endpoint_id.clone()),
-                mode: self.mode,
-                pageserver_connstring: Some(pageserver_connstring),
-                safekeepers_generation: safekeepers_generation.map(|g| g.into_inner()),
-                safekeeper_connstrings,
-                storage_auth_token: auth_token.clone(),
-                remote_extensions,
-                pgbouncer_settings: None,
-                shard_stripe_size: Some(shard_stripe_size),
-                local_proxy_config: None,
-                reconfigure_concurrency: self.reconfigure_concurrency,
-                drop_subscriptions_before_start: self.drop_subscriptions_before_start,
-                audit_log_level: ComputeAudit::Disabled,
-                logs_export_host: None::<String>,
-            };
-
-            // this strange code is needed to support respec() in tests
-            if self.cluster.is_some() {
-                debug!("Cluster is already set in the endpoint spec, using it");
-                spec.cluster = self.cluster.clone().unwrap();
-
-                debug!("spec.cluster {:?}", spec.cluster);
-
-                // fill missing fields again
-                if create_test_user {
-                    spec.cluster.roles.push(Role {
+        // Create spec file
+        let mut spec = ComputeSpec {
+            skip_pg_catalog_updates: self.skip_pg_catalog_updates,
+            format_version: 1.0,
+            operation_uuid: None,
+            features: self.features.clone(),
+            swap_size_bytes: None,
+            disk_quota_bytes: None,
+            disable_lfc_resizing: None,
+            cluster: Cluster {
+                cluster_id: None, // project ID: not used
+                name: None,       // project name: not used
+                state: None,
+                roles: if create_test_user {
+                    vec![Role {
                         name: PgIdent::from_str("test").unwrap(),
                         encrypted_password: None,
                         options: None,
-                    });
-                    spec.cluster.databases.push(Database {
+                    }]
+                } else {
+                    Vec::new()
+                },
+                databases: if create_test_user {
+                    vec![Database {
                         name: PgIdent::from_str("neondb").unwrap(),
                         owner: PgIdent::from_str("test").unwrap(),
                         options: None,
                         restrict_conn: false,
                         invalid: false,
-                    });
-                }
-                spec.cluster.postgresql_conf = Some(postgresql_conf);
-            }
-
-            ComputeConfig {
-                spec: Some(spec),
-                compute_ctl_config: ComputeCtlConfig::default(),
-            }
+                    }]
+                } else {
+                    Vec::new()
+                },
+                settings: None,
+                postgresql_conf: Some(postgresql_conf.clone()),
+            },
+            delta_operations: None,
+            tenant_id: Some(self.tenant_id),
+            timeline_id: Some(self.timeline_id),
+            project_id: None,
+            branch_id: None,
+            endpoint_id: Some(self.endpoint_id.clone()),
+            mode: self.mode,
+            pageserver_connstring: Some(pageserver_connstring),
+            safekeepers_generation: safekeepers_generation.map(|g| g.into_inner()),
+            safekeeper_connstrings,
+            storage_auth_token: auth_token.clone(),
+            remote_extensions,
+            pgbouncer_settings: None,
+            shard_stripe_size: Some(shard_stripe_size),
+            local_proxy_config: None,
+            reconfigure_concurrency: self.reconfigure_concurrency,
+            drop_subscriptions_before_start: self.drop_subscriptions_before_start,
+            audit_log_level: ComputeAudit::Disabled,
+            logs_export_host: None::<String>,
         };
 
-        // TODO(tristan957): Remove the write to spec.json after compatibility
-        // tests work themselves out
+        // this strange code is needed to support respec() in tests
+        if self.cluster.is_some() {
+            debug!("Cluster is already set in the endpoint spec, using it");
+            spec.cluster = self.cluster.clone().unwrap();
+
+            debug!("spec.cluster {:?}", spec.cluster);
+
+            // fill missing fields again
+            if create_test_user {
+                spec.cluster.roles.push(Role {
+                    name: PgIdent::from_str("test").unwrap(),
+                    encrypted_password: None,
+                    options: None,
+                });
+                spec.cluster.databases.push(Database {
+                    name: PgIdent::from_str("neondb").unwrap(),
+                    owner: PgIdent::from_str("test").unwrap(),
+                    options: None,
+                    restrict_conn: false,
+                    invalid: false,
+                });
+            }
+            spec.cluster.postgresql_conf = Some(postgresql_conf);
+        }
+
         let spec_path = self.endpoint_path().join("spec.json");
-        std::fs::write(spec_path, serde_json::to_string_pretty(&config.spec)?)?;
-        let config_path = self.endpoint_path().join("config.json");
-        std::fs::write(config_path, serde_json::to_string_pretty(&config)?)?;
+        std::fs::write(spec_path, serde_json::to_string_pretty(&spec)?)?;
 
         // Open log file. We'll redirect the stdout and stderr of `compute_ctl` to it.
         let logfile = std::fs::OpenOptions::new()
@@ -723,16 +710,6 @@ impl Endpoint {
             .append(true)
             .open(self.endpoint_path().join("compute.log"))?;
 
-        // TODO(tristan957): Remove when compatibility tests are no longer an
-        // issue
-        let old_compute_ctl = {
-            let mut cmd = Command::new(self.env.neon_distrib_dir.join("compute_ctl"));
-            let help_output = cmd.arg("--help").output()?;
-            let help_output = String::from_utf8_lossy(&help_output.stdout);
-
-            !help_output.contains("--config")
-        };
-
         // Launch compute_ctl
         let conn_str = self.connstr("cloud_admin", "postgres");
         println!("Starting postgres node at '{}'", conn_str);
@@ -751,18 +728,9 @@ impl Endpoint {
         ])
         .args(["--pgdata", self.pgdata().to_str().unwrap()])
         .args(["--connstr", &conn_str])
-        // TODO(tristan957): Change this to --config when compatibility tests
-        // are no longer an issue
         .args([
             "--spec-path",
-            self.endpoint_path()
-                .join(if old_compute_ctl {
-                    "spec.json"
-                } else {
-                    "config.json"
-                })
-                .to_str()
-                .unwrap(),
+            self.endpoint_path().join("spec.json").to_str().unwrap(),
         ])
         .args([
             "--pgbin",
@@ -905,12 +873,10 @@ impl Endpoint {
         stripe_size: Option<ShardStripeSize>,
         safekeepers: Option<Vec<NodeId>>,
     ) -> Result<()> {
-        let (mut spec, compute_ctl_config) = {
-            let config_path = self.endpoint_path().join("config.json");
-            let file = std::fs::File::open(config_path)?;
-            let config: ComputeConfig = serde_json::from_reader(file)?;
-
-            (config.spec.unwrap(), config.compute_ctl_config)
+        let mut spec: ComputeSpec = {
+            let spec_path = self.endpoint_path().join("spec.json");
+            let file = std::fs::File::open(spec_path)?;
+            serde_json::from_reader(file)?
         };
 
         let postgresql_conf = self.read_postgresql_conf()?;
@@ -960,7 +926,7 @@ impl Endpoint {
             .body(
                 serde_json::to_string(&ConfigurationRequest {
                     spec,
-                    compute_ctl_config,
+                    compute_ctl_config: ComputeCtlConfig::default(),
                 })
                 .unwrap(),
             )

control_plane/src/local_env.rs

@@ -980,7 +980,7 @@ fn generate_ssl_ca_cert(cert_path: &Path, key_path: &Path) -> anyhow::Result<()>
     // -out rootCA.crt -keyout rootCA.key
     let keygen_output = Command::new("openssl")
         .args([
-            "req", "-x509", "-newkey", "ed25519", "-nodes", "-days", "36500",
+            "req", "-x509", "-newkey", "rsa:2048", "-nodes", "-days", "36500",
         ])
         .args(["-subj", "/CN=Neon Local CA"])
         .args(["-out", cert_path.to_str().unwrap()])
@@ -1010,7 +1010,7 @@ fn generate_ssl_cert(
     // -subj "/CN=localhost" -addext "subjectAltName=DNS:localhost,IP:127.0.0.1"
     let keygen_output = Command::new("openssl")
         .args(["req", "-new", "-nodes"])
-        .args(["-newkey", "ed25519"])
+        .args(["-newkey", "rsa:2048"])
         .args(["-subj", "/CN=localhost"])
         .args(["-addext", "subjectAltName=DNS:localhost,IP:127.0.0.1"])
         .args(["-keyout", key_path.to_str().unwrap()])

control_plane/src/pageserver.rs

@@ -535,11 +535,6 @@ impl PageServerNode {
                 .map(|x| x.parse::<bool>())
                 .transpose()
                 .context("Failed to parse 'gc_compaction_enabled' as bool")?,
-            gc_compaction_verification: settings
-                .remove("gc_compaction_verification")
-                .map(|x| x.parse::<bool>())
-                .transpose()
-                .context("Failed to parse 'gc_compaction_verification' as bool")?,
             gc_compaction_initial_threshold_kb: settings
                 .remove("gc_compaction_initial_threshold_kb")
                 .map(|x| x.parse::<u64>())

docker-compose/compute_wrapper/shell/compute.sh

@@ -11,8 +11,8 @@ generate_id() {
 
 PG_VERSION=${PG_VERSION:-14}
 
-CONFIG_FILE_ORG=/var/db/postgres/configs/config.json
-CONFIG_FILE=/tmp/config.json
+SPEC_FILE_ORG=/var/db/postgres/specs/spec.json
+SPEC_FILE=/tmp/spec.json
 
 echo "Waiting pageserver become ready."
 while ! nc -z pageserver 6400; do
@@ -20,7 +20,7 @@ while ! nc -z pageserver 6400; do
 done
 echo "Page server is ready."
 
-cp ${CONFIG_FILE_ORG} ${CONFIG_FILE}
+cp ${SPEC_FILE_ORG} ${SPEC_FILE}
 
  if [ -n "${TENANT_ID:-}" ] && [ -n "${TIMELINE_ID:-}" ]; then
    tenant_id=${TENANT_ID}
@@ -73,27 +73,17 @@ else
   ulid_extension=ulid
 fi
 echo "Adding pgx_ulid"
-shared_libraries=$(jq -r '.spec.cluster.settings[] | select(.name=="shared_preload_libraries").value' ${CONFIG_FILE})
-sed -i "s/${shared_libraries}/${shared_libraries},${ulid_extension}/" ${CONFIG_FILE}
+shared_libraries=$(jq -r '.cluster.settings[] | select(.name=="shared_preload_libraries").value' ${SPEC_FILE})
+sed -i "s/${shared_libraries}/${shared_libraries},${ulid_extension}/" ${SPEC_FILE}
 echo "Overwrite tenant id and timeline id in spec file"
-sed -i "s/TENANT_ID/${tenant_id}/" ${CONFIG_FILE}
-sed -i "s/TIMELINE_ID/${timeline_id}/" ${CONFIG_FILE}
+sed -i "s/TENANT_ID/${tenant_id}/" ${SPEC_FILE}
+sed -i "s/TIMELINE_ID/${timeline_id}/" ${SPEC_FILE}
 
-cat ${CONFIG_FILE}
-
-# TODO(tristan957): Remove these workarounds for backwards compatibility after
-# the next compute release. That includes these next few lines and the
-# --spec-path in the compute_ctl invocation.
-if compute_ctl --help | grep --quiet -- '--config'; then
-  SPEC_PATH="$CONFIG_FILE"
-else
-  jq '.spec' < "$CONFIG_FILE" > /tmp/spec.json
-  SPEC_PATH=/tmp/spec.json
-fi
+cat ${SPEC_FILE}
 
 echo "Start compute node"
 /usr/local/bin/compute_ctl --pgdata /var/db/postgres/compute \
      -C "postgresql://cloud_admin@localhost:55433/postgres"  \
      -b /usr/local/bin/postgres                              \
      --compute-id "compute-$RANDOM"                          \
-     --spec-path "$SPEC_PATH"
+     -S ${SPEC_FILE}

docker-compose/compute_wrapper/var/db/postgres/configs/config.json

@@ -1,148 +0,0 @@
-{
-    "spec": {
-        "format_version": 1.0,
-
-        "timestamp": "2022-10-12T18:00:00.000Z",
-        "operation_uuid": "0f657b36-4b0f-4a2d-9c2e-1dcd615e7d8c",
-
-        "cluster": {
-            "cluster_id": "docker_compose",
-            "name": "docker_compose_test",
-            "state": "restarted",
-            "roles": [
-                {
-                    "name": "cloud_admin",
-                    "encrypted_password": "b093c0d3b281ba6da1eacc608620abd8",
-                    "options": null
-                }
-            ],
-            "databases": [
-            ],
-            "settings": [
-                {
-                    "name": "fsync",
-                    "value": "off",
-                    "vartype": "bool"
-                },
-                {
-                    "name": "wal_level",
-                    "value": "logical",
-                    "vartype": "enum"
-                },
-                {
-                    "name": "wal_log_hints",
-                    "value": "on",
-                    "vartype": "bool"
-                },
-                {
-                    "name": "log_connections",
-                    "value": "on",
-                    "vartype": "bool"
-                },
-                {
-                    "name": "port",
-                    "value": "55433",
-                    "vartype": "integer"
-                },
-                {
-                    "name": "shared_buffers",
-                    "value": "1MB",
-                    "vartype": "string"
-                },
-                {
-                    "name": "max_connections",
-                    "value": "100",
-                    "vartype": "integer"
-                },
-                {
-                    "name": "listen_addresses",
-                    "value": "0.0.0.0",
-                    "vartype": "string"
-                },
-                {
-                    "name": "max_wal_senders",
-                    "value": "10",
-                    "vartype": "integer"
-                },
-                {
-                    "name": "max_replication_slots",
-                    "value": "10",
-                    "vartype": "integer"
-                },
-                {
-                    "name": "wal_sender_timeout",
-                    "value": "5s",
-                    "vartype": "string"
-                },
-                {
-                    "name": "wal_keep_size",
-                    "value": "0",
-                    "vartype": "integer"
-                },
-                {
-                    "name": "password_encryption",
-                    "value": "md5",
-                    "vartype": "enum"
-                },
-                {
-                    "name": "restart_after_crash",
-                    "value": "off",
-                    "vartype": "bool"
-                },
-                {
-                    "name": "synchronous_standby_names",
-                    "value": "walproposer",
-                    "vartype": "string"
-                },
-                {
-                    "name": "shared_preload_libraries",
-                    "value": "neon,pg_cron,timescaledb,pg_stat_statements",
-                    "vartype": "string"
-                },
-                {
-                    "name": "neon.safekeepers",
-                    "value": "safekeeper1:5454,safekeeper2:5454,safekeeper3:5454",
-                    "vartype": "string"
-                },
-                {
-                    "name": "neon.timeline_id",
-                    "value": "TIMELINE_ID",
-                    "vartype": "string"
-                },
-                {
-                    "name": "neon.tenant_id",
-                    "value": "TENANT_ID",
-                    "vartype": "string"
-                },
-                {
-                    "name": "neon.pageserver_connstring",
-                    "value": "host=pageserver port=6400",
-                    "vartype": "string"
-                },
-                {
-                    "name": "max_replication_write_lag",
-                    "value": "500MB",
-                    "vartype": "string"
-                },
-                {
-                    "name": "max_replication_flush_lag",
-                    "value": "10GB",
-                    "vartype": "string"
-                },
-                {
-                    "name": "cron.database",
-                    "value": "postgres",
-                    "vartype": "string"
-                }
-            ]
-        },
-
-        "delta_operations": [
-        ]
-    },
-    "compute_ctl_config": {
-        "jwks": {
-            "keys": []
-        }
-    }
-}

docker-compose/compute_wrapper/var/db/postgres/specs/spec.json

@@ -0,0 +1,141 @@
+{
+    "format_version": 1.0,
+
+    "timestamp": "2022-10-12T18:00:00.000Z",
+    "operation_uuid": "0f657b36-4b0f-4a2d-9c2e-1dcd615e7d8c",
+
+    "cluster": {
+        "cluster_id": "docker_compose",
+        "name": "docker_compose_test",
+        "state": "restarted",
+        "roles": [
+            {
+                "name": "cloud_admin",
+                "encrypted_password": "b093c0d3b281ba6da1eacc608620abd8",
+                "options": null
+            }
+        ],
+        "databases": [
+        ],
+        "settings": [
+            {
+                "name": "fsync",
+                "value": "off",
+                "vartype": "bool"
+            },
+            {
+                "name": "wal_level",
+                "value": "logical",
+                "vartype": "enum"
+            },
+            {
+                "name": "wal_log_hints",
+                "value": "on",
+                "vartype": "bool"
+            },
+            {
+                "name": "log_connections",
+                "value": "on",
+                "vartype": "bool"
+            },
+            {
+                "name": "port",
+                "value": "55433",
+                "vartype": "integer"
+            },
+            {
+                "name": "shared_buffers",
+                "value": "1MB",
+                "vartype": "string"
+            },
+            {
+                "name": "max_connections",
+                "value": "100",
+                "vartype": "integer"
+            },
+            {
+                "name": "listen_addresses",
+                "value": "0.0.0.0",
+                "vartype": "string"
+            },
+            {
+                "name": "max_wal_senders",
+                "value": "10",
+                "vartype": "integer"
+            },
+            {
+                "name": "max_replication_slots",
+                "value": "10",
+                "vartype": "integer"
+            },
+            {
+                "name": "wal_sender_timeout",
+                "value": "5s",
+                "vartype": "string"
+            },
+            {
+                "name": "wal_keep_size",
+                "value": "0",
+                "vartype": "integer"
+            },
+            {
+                "name": "password_encryption",
+                "value": "md5",
+                "vartype": "enum"
+            },
+            {
+                "name": "restart_after_crash",
+                "value": "off",
+                "vartype": "bool"
+            },
+            {
+                "name": "synchronous_standby_names",
+                "value": "walproposer",
+                "vartype": "string"
+            },
+            {
+                "name": "shared_preload_libraries",
+                "value": "neon,pg_cron,timescaledb,pg_stat_statements",
+                "vartype": "string"
+            },
+            {
+                "name": "neon.safekeepers",
+                "value": "safekeeper1:5454,safekeeper2:5454,safekeeper3:5454",
+                "vartype": "string"
+            },
+            {
+                "name": "neon.timeline_id",
+                "value": "TIMELINE_ID",
+                "vartype": "string"
+            },
+            {
+                "name": "neon.tenant_id",
+                "value": "TENANT_ID",
+                "vartype": "string"
+            },
+            {
+                "name": "neon.pageserver_connstring",
+                "value": "host=pageserver port=6400",
+                "vartype": "string"
+            },
+            {
+                "name": "max_replication_write_lag",
+                "value": "500MB",
+                "vartype": "string"
+            },
+            {
+                "name": "max_replication_flush_lag",
+                "value": "10GB",
+                "vartype": "string"
+            },
+            {
+                "name": "cron.database",
+                "value": "postgres",
+                "vartype": "string"
+            }
+        ]
+    },
+
+    "delta_operations": [
+    ]
+}

docker-compose/docker-compose.yml

@@ -159,7 +159,7 @@ services:
       #- RUST_BACKTRACE=1
     # Mount the test files directly, for faster editing cycle.
     volumes:
-      - ./compute_wrapper/var/db/postgres/configs/:/var/db/postgres/configs/
+      - ./compute_wrapper/var/db/postgres/specs/:/var/db/postgres/specs/
       - ./compute_wrapper/shell/:/shell/
     ports:
       - 55433:55433 # pg protocol handler

libs/compute_api/src/responses.rs

@@ -14,32 +14,6 @@ pub struct GenericAPIError {
     pub error: String,
 }
 
-/// All configuration parameters necessary for a compute. When
-/// [`ComputeConfig::spec`] is provided, it means that the compute is attached
-/// to a tenant. [`ComputeConfig::compute_ctl_config`] will always be provided
-/// and contains parameters necessary for operating `compute_ctl` independently
-/// of whether a tenant is attached to the compute or not.
-///
-/// This also happens to be the body of `compute_ctl`'s /configure request.
-#[derive(Debug, Deserialize, Serialize)]
-pub struct ComputeConfig {
-    /// The compute spec
-    pub spec: Option<ComputeSpec>,
-
-    /// The compute_ctl configuration
-    #[allow(dead_code)]
-    pub compute_ctl_config: ComputeCtlConfig,
-}
-
-impl From<ControlPlaneConfigResponse> for ComputeConfig {
-    fn from(value: ControlPlaneConfigResponse) -> Self {
-        Self {
-            spec: value.spec,
-            compute_ctl_config: value.compute_ctl_config,
-        }
-    }
-}
-
 #[derive(Debug, Clone, Serialize)]
 pub struct ExtensionInstallResponse {
     pub extension: PgIdent,
@@ -187,7 +161,7 @@ pub struct TlsConfig {
 
 /// Response of the `/computes/{compute_id}/spec` control-plane API.
 #[derive(Deserialize, Debug)]
-pub struct ControlPlaneConfigResponse {
+pub struct ControlPlaneSpecResponse {
     pub spec: Option<ComputeSpec>,
     pub status: ControlPlaneComputeStatus,
     pub compute_ctl_config: ComputeCtlConfig,

libs/compute_api/src/spec.rs

@@ -1,8 +1,8 @@
-//! The ComputeSpec contains all the information needed to start up
-//! the right version of PostgreSQL, and connect it to the storage nodes.
-//! It can be passed as part of the `config.json`, or the control plane can
-//! provide it by calling the compute_ctl's `/compute_ctl` endpoint, or
-//! compute_ctl can fetch it by calling the control plane's API.
+//! `ComputeSpec` represents the contents of the spec.json file.
+//!
+//! The spec.json file is used to pass information to 'compute_ctl'. It contains
+//! all the information needed to start up the right version of PostgreSQL,
+//! and connect it to the storage nodes.
 use std::collections::HashMap;
 
 use indexmap::IndexMap;

libs/pageserver_api/src/config.rs

@@ -207,10 +207,6 @@ pub struct PageServicePipeliningConfigPipelined {
     /// Causes runtime errors if larger than max get_vectored batch size.
     pub max_batch_size: NonZeroUsize,
     pub execution: PageServiceProtocolPipelinedExecutionStrategy,
-    // The default below is such that new versions of the software can start
-    // with the old configuration.
-    #[serde(default)]
-    pub batching: PageServiceProtocolPipelinedBatchingStrategy,
 }
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
@@ -220,19 +216,6 @@ pub enum PageServiceProtocolPipelinedExecutionStrategy {
     Tasks,
 }
 
-#[derive(Default, Debug, Clone, Copy, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
-#[serde(rename_all = "kebab-case")]
-pub enum PageServiceProtocolPipelinedBatchingStrategy {
-    /// All get page requests in a batch will be at the same LSN
-    #[default]
-    UniformLsn,
-    /// Get page requests in a batch may be at different LSN
-    ///
-    /// One key cannot be present more than once at different LSNs in
-    /// the same batch.
-    ScatteredLsn,
-}
-
 #[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
 #[serde(tag = "mode", rename_all = "kebab-case")]
 pub enum GetVectoredConcurrentIo {
@@ -469,8 +452,6 @@ pub struct TenantConfigToml {
     // gc-compaction related configs
     /// Enable automatic gc-compaction trigger on this tenant.
     pub gc_compaction_enabled: bool,
-    /// Enable verification of gc-compaction results.
-    pub gc_compaction_verification: bool,
     /// The initial threshold for gc-compaction in KB. Once the total size of layers below the gc-horizon is above this threshold,
     /// gc-compaction will be triggered.
     pub gc_compaction_initial_threshold_kb: u64,
@@ -632,12 +613,9 @@ impl Default for ConfigToml {
             page_service_pipelining: if !cfg!(test) {
                 PageServicePipeliningConfig::Serial
             } else {
-                // Do not turn this into the default until scattered reads have been
-                // validated and rolled-out fully.
                 PageServicePipeliningConfig::Pipelined(PageServicePipeliningConfigPipelined {
                     max_batch_size: NonZeroUsize::new(32).unwrap(),
                     execution: PageServiceProtocolPipelinedExecutionStrategy::ConcurrentFutures,
-                    batching: PageServiceProtocolPipelinedBatchingStrategy::ScatteredLsn,
                 })
             },
             get_vectored_concurrent_io: if !cfg!(test) {
@@ -714,7 +692,6 @@ pub mod tenant_conf_defaults {
     // image layers should be created.
     pub const DEFAULT_IMAGE_LAYER_CREATION_CHECK_THRESHOLD: u8 = 2;
     pub const DEFAULT_GC_COMPACTION_ENABLED: bool = false;
-    pub const DEFAULT_GC_COMPACTION_VERIFICATION: bool = true;
     pub const DEFAULT_GC_COMPACTION_INITIAL_THRESHOLD_KB: u64 = 5 * 1024 * 1024; // 5GB
     pub const DEFAULT_GC_COMPACTION_RATIO_PERCENT: u64 = 100;
 }
@@ -769,7 +746,6 @@ impl Default for TenantConfigToml {
             wal_receiver_protocol_override: None,
             rel_size_v2_enabled: false,
             gc_compaction_enabled: DEFAULT_GC_COMPACTION_ENABLED,
-            gc_compaction_verification: DEFAULT_GC_COMPACTION_VERIFICATION,
             gc_compaction_initial_threshold_kb: DEFAULT_GC_COMPACTION_INITIAL_THRESHOLD_KB,
             gc_compaction_ratio_percent: DEFAULT_GC_COMPACTION_RATIO_PERCENT,
             sampling_ratio: None,

libs/pageserver_api/src/models.rs

@@ -576,8 +576,6 @@ pub struct TenantConfigPatch {
     #[serde(skip_serializing_if = "FieldPatch::is_noop")]
     pub gc_compaction_enabled: FieldPatch<bool>,
     #[serde(skip_serializing_if = "FieldPatch::is_noop")]
-    pub gc_compaction_verification: FieldPatch<bool>,
-    #[serde(skip_serializing_if = "FieldPatch::is_noop")]
     pub gc_compaction_initial_threshold_kb: FieldPatch<u64>,
     #[serde(skip_serializing_if = "FieldPatch::is_noop")]
     pub gc_compaction_ratio_percent: FieldPatch<u64>,
@@ -698,9 +696,6 @@ pub struct TenantConfig {
     #[serde(skip_serializing_if = "Option::is_none")]
     pub gc_compaction_enabled: Option<bool>,
 
-    #[serde(skip_serializing_if = "Option::is_none")]
-    pub gc_compaction_verification: Option<bool>,
-
     #[serde(skip_serializing_if = "Option::is_none")]
     pub gc_compaction_initial_threshold_kb: Option<u64>,
 
@@ -749,7 +744,6 @@ impl TenantConfig {
             mut wal_receiver_protocol_override,
             mut rel_size_v2_enabled,
             mut gc_compaction_enabled,
-            mut gc_compaction_verification,
             mut gc_compaction_initial_threshold_kb,
             mut gc_compaction_ratio_percent,
             mut sampling_ratio,
@@ -841,9 +835,6 @@ impl TenantConfig {
         patch
             .gc_compaction_enabled
             .apply(&mut gc_compaction_enabled);
-        patch
-            .gc_compaction_verification
-            .apply(&mut gc_compaction_verification);
         patch
             .gc_compaction_initial_threshold_kb
             .apply(&mut gc_compaction_initial_threshold_kb);
@@ -885,7 +876,6 @@ impl TenantConfig {
             wal_receiver_protocol_override,
             rel_size_v2_enabled,
             gc_compaction_enabled,
-            gc_compaction_verification,
             gc_compaction_initial_threshold_kb,
             gc_compaction_ratio_percent,
             sampling_ratio,
@@ -984,9 +974,6 @@ impl TenantConfig {
             gc_compaction_enabled: self
                 .gc_compaction_enabled
                 .unwrap_or(global_conf.gc_compaction_enabled),
-            gc_compaction_verification: self
-                .gc_compaction_verification
-                .unwrap_or(global_conf.gc_compaction_verification),
             gc_compaction_initial_threshold_kb: self
                 .gc_compaction_initial_threshold_kb
                 .unwrap_or(global_conf.gc_compaction_initial_threshold_kb),

pageserver/Cargo.toml

@@ -10,8 +10,6 @@ default = []
 # which adds some runtime cost to run tests on outage conditions
 testing = ["fail/failpoints", "pageserver_api/testing", "wal_decoder/testing", "pageserver_client/testing"]
 
-fuzz-read-path = ["testing"]
-
 [dependencies]
 anyhow.workspace = true
 arc-swap.workspace = true

pageserver/src/basebackup.rs

@@ -34,7 +34,7 @@ use utils::lsn::Lsn;
 use crate::context::RequestContext;
 use crate::pgdatadir_mapping::Version;
 use crate::tenant::storage_layer::IoConcurrency;
-use crate::tenant::timeline::{GetVectoredError, VersionedKeySpaceQuery};
+use crate::tenant::timeline::GetVectoredError;
 use crate::tenant::{PageReconstructError, Timeline};
 
 #[derive(Debug, thiserror::Error)]
@@ -353,10 +353,9 @@ where
             let mut slru_builder = SlruSegmentsBuilder::new(&mut self.ar);
 
             for part in slru_partitions.parts {
-                let query = VersionedKeySpaceQuery::uniform(part, self.lsn);
                 let blocks = self
                     .timeline
-                    .get_vectored(query, self.io_concurrency.clone(), self.ctx)
+                    .get_vectored(part, self.lsn, self.io_concurrency.clone(), self.ctx)
                     .await?;
 
                 for (key, block) in blocks {

pageserver/src/metrics.rs

@@ -17,7 +17,7 @@ use metrics::{
 use once_cell::sync::Lazy;
 use pageserver_api::config::{
     PageServicePipeliningConfig, PageServicePipeliningConfigPipelined,
-    PageServiceProtocolPipelinedBatchingStrategy, PageServiceProtocolPipelinedExecutionStrategy,
+    PageServiceProtocolPipelinedExecutionStrategy,
 };
 use pageserver_api::models::InMemoryLayerInfo;
 use pageserver_api::shard::TenantShardId;
@@ -1714,28 +1714,6 @@ pub enum SmgrQueryType {
     Test,
 }
 
-#[derive(
-    Debug,
-    Clone,
-    Copy,
-    IntoStaticStr,
-    strum_macros::EnumCount,
-    strum_macros::EnumIter,
-    strum_macros::FromRepr,
-    enum_map::Enum,
-)]
-#[strum(serialize_all = "snake_case")]
-pub enum GetPageBatchBreakReason {
-    BatchFull,
-    NonBatchableRequest,
-    NonUniformLsn,
-    SamePageAtDifferentLsn,
-    NonUniformTimeline,
-    ExecutorSteal,
-    #[cfg(feature = "testing")]
-    NonUniformKey,
-}
-
 pub(crate) struct SmgrQueryTimePerTimeline {
     global_started: [IntCounter; SmgrQueryType::COUNT],
     global_latency: [Histogram; SmgrQueryType::COUNT],
@@ -1747,8 +1725,6 @@ pub(crate) struct SmgrQueryTimePerTimeline {
     per_timeline_flush_in_progress_micros: IntCounter,
     global_batch_wait_time: Histogram,
     per_timeline_batch_wait_time: Histogram,
-    global_batch_break_reason: [IntCounter; GetPageBatchBreakReason::COUNT],
-    per_timeline_batch_break_reason: GetPageBatchBreakReasonTimelineMetrics,
     throttling: Arc<tenant_throttling::Pagestream>,
 }
 
@@ -1882,55 +1858,12 @@ static PAGE_SERVICE_BATCH_SIZE_PER_TENANT_TIMELINE: Lazy<HistogramVec> = Lazy::n
     .expect("failed to define a metric")
 });
 
-static PAGE_SERVICE_BATCH_BREAK_REASON_GLOBAL: Lazy<IntCounterVec> = Lazy::new(|| {
-    register_int_counter_vec!(
-        // it's a counter, but, name is prepared to extend it to a histogram of queue depth
-        "pageserver_page_service_batch_break_reason_global",
-        "Reason for breaking batches of get page requests",
-        &["reason"],
-    )
-    .expect("failed to define a metric")
-});
-
-struct GetPageBatchBreakReasonTimelineMetrics {
-    map: EnumMap<GetPageBatchBreakReason, IntCounter>,
-}
-
-impl GetPageBatchBreakReasonTimelineMetrics {
-    fn new(tenant_id: &str, shard_slug: &str, timeline_id: &str) -> Self {
-        GetPageBatchBreakReasonTimelineMetrics {
-            map: EnumMap::from_array(std::array::from_fn(|reason_idx| {
-                let reason = GetPageBatchBreakReason::from_usize(reason_idx);
-                PAGE_SERVICE_BATCH_BREAK_REASON_PER_TENANT_TIMELINE.with_label_values(&[
-                    tenant_id,
-                    shard_slug,
-                    timeline_id,
-                    reason.into(),
-                ])
-            })),
-        }
-    }
-
-    fn inc(&self, reason: GetPageBatchBreakReason) {
-        self.map[reason].inc()
-    }
-}
-
-static PAGE_SERVICE_BATCH_BREAK_REASON_PER_TENANT_TIMELINE: Lazy<IntCounterVec> = Lazy::new(|| {
-    register_int_counter_vec!(
-        "pageserver_page_service_batch_break_reason",
-        "Reason for breaking batches of get page requests",
-        &["tenant_id", "shard_id", "timeline_id", "reason"],
-    )
-    .expect("failed to define a metric")
-});
-
 pub(crate) static PAGE_SERVICE_CONFIG_MAX_BATCH_SIZE: Lazy<IntGaugeVec> = Lazy::new(|| {
     register_int_gauge_vec!(
         "pageserver_page_service_config_max_batch_size",
         "Configured maximum batch size for the server-side batching functionality of page_service. \
          Labels expose more of the configuration parameters.",
-        &["mode", "execution", "batching"]
+        &["mode", "execution"]
     )
     .expect("failed to define a metric")
 });
@@ -1938,11 +1871,10 @@ pub(crate) static PAGE_SERVICE_CONFIG_MAX_BATCH_SIZE: Lazy<IntGaugeVec> = Lazy::
 fn set_page_service_config_max_batch_size(conf: &PageServicePipeliningConfig) {
     PAGE_SERVICE_CONFIG_MAX_BATCH_SIZE.reset();
     let (label_values, value) = match conf {
-        PageServicePipeliningConfig::Serial => (["serial", "-", "-"], 1),
+        PageServicePipeliningConfig::Serial => (["serial", "-"], 1),
         PageServicePipeliningConfig::Pipelined(PageServicePipeliningConfigPipelined {
             max_batch_size,
             execution,
-            batching,
         }) => {
             let mode = "pipelined";
             let execution = match execution {
@@ -1951,12 +1883,7 @@ fn set_page_service_config_max_batch_size(conf: &PageServicePipeliningConfig) {
                 }
                 PageServiceProtocolPipelinedExecutionStrategy::Tasks => "tasks",
             };
-            let batching = match batching {
-                PageServiceProtocolPipelinedBatchingStrategy::UniformLsn => "uniform-lsn",
-                PageServiceProtocolPipelinedBatchingStrategy::ScatteredLsn => "scattered-lsn",
-            };
-
-            ([mode, execution, batching], max_batch_size.get())
+            ([mode, execution], max_batch_size.get())
         }
     };
     PAGE_SERVICE_CONFIG_MAX_BATCH_SIZE
@@ -2052,15 +1979,6 @@ impl SmgrQueryTimePerTimeline {
             .get_metric_with_label_values(&[&tenant_id, &shard_slug, &timeline_id])
             .unwrap();
 
-        let global_batch_break_reason = std::array::from_fn(|i| {
-            let reason = GetPageBatchBreakReason::from_usize(i);
-            PAGE_SERVICE_BATCH_BREAK_REASON_GLOBAL
-                .get_metric_with_label_values(&[reason.into()])
-                .unwrap()
-        });
-        let per_timeline_batch_break_reason =
-            GetPageBatchBreakReasonTimelineMetrics::new(&tenant_id, &shard_slug, &timeline_id);
-
         let global_flush_in_progress_micros =
             PAGE_SERVICE_SMGR_FLUSH_INPROGRESS_MICROS_GLOBAL.clone();
         let per_timeline_flush_in_progress_micros = PAGE_SERVICE_SMGR_FLUSH_INPROGRESS_MICROS
@@ -2078,8 +1996,6 @@ impl SmgrQueryTimePerTimeline {
             per_timeline_flush_in_progress_micros,
             global_batch_wait_time,
             per_timeline_batch_wait_time,
-            global_batch_break_reason,
-            per_timeline_batch_break_reason,
             throttling: pagestream_throttle_metrics,
         }
     }
@@ -2108,16 +2024,9 @@ impl SmgrQueryTimePerTimeline {
     }
 
     /// TODO: do something about this? seems odd, we have a similar call on SmgrOpTimer
-    pub(crate) fn observe_getpage_batch_start(
-        &self,
-        batch_size: usize,
-        break_reason: GetPageBatchBreakReason,
-    ) {
+    pub(crate) fn observe_getpage_batch_start(&self, batch_size: usize) {
         self.global_batch_size.observe(batch_size as f64);
         self.per_timeline_batch_size.observe(batch_size as f64);
-
-        self.global_batch_break_reason[break_reason.into_usize()].inc();
-        self.per_timeline_batch_break_reason.inc(break_reason);
     }
 }
 
@@ -3483,15 +3392,6 @@ impl TimelineMetrics {
             shard_id,
             timeline_id,
         ]);
-
-        for reason in GetPageBatchBreakReason::iter() {
-            let _ = PAGE_SERVICE_BATCH_BREAK_REASON_PER_TENANT_TIMELINE.remove_label_values(&[
-                tenant_id,
-                shard_id,
-                timeline_id,
-                reason.into(),
-            ]);
-        }
     }
 }
 
@@ -4370,7 +4270,6 @@ pub fn preinitialize_metrics(
     [
         &BACKGROUND_LOOP_PERIOD_OVERRUN_COUNT,
         &SMGR_QUERY_STARTED_GLOBAL,
-        &PAGE_SERVICE_BATCH_BREAK_REASON_GLOBAL,
     ]
     .into_iter()
     .for_each(|c| {

pageserver/src/page_service.rs

@@ -18,7 +18,7 @@ use itertools::Itertools;
 use once_cell::sync::OnceCell;
 use pageserver_api::config::{
     PageServicePipeliningConfig, PageServicePipeliningConfigPipelined,
-    PageServiceProtocolPipelinedBatchingStrategy, PageServiceProtocolPipelinedExecutionStrategy,
+    PageServiceProtocolPipelinedExecutionStrategy,
 };
 use pageserver_api::key::rel_block_to_key;
 use pageserver_api::models::{
@@ -58,8 +58,8 @@ use crate::context::{
     DownloadBehavior, PerfInstrumentFutureExt, RequestContext, RequestContextBuilder,
 };
 use crate::metrics::{
-    self, COMPUTE_COMMANDS_COUNTERS, ComputeCommandKind, GetPageBatchBreakReason, LIVE_CONNECTIONS,
-    SmgrOpTimer, TimelineMetrics,
+    self, COMPUTE_COMMANDS_COUNTERS, ComputeCommandKind, LIVE_CONNECTIONS, SmgrOpTimer,
+    TimelineMetrics,
 };
 use crate::pgdatadir_mapping::Version;
 use crate::span::{
@@ -641,7 +641,6 @@ impl std::fmt::Display for BatchedPageStreamError {
 struct BatchedGetPageRequest {
     req: PagestreamGetPageRequest,
     timer: SmgrOpTimer,
-    effective_request_lsn: Lsn,
     ctx: RequestContext,
 }
 
@@ -671,8 +670,8 @@ enum BatchedFeMessage {
     GetPage {
         span: Span,
         shard: timeline::handle::WeakHandle<TenantManagerTypes>,
+        effective_request_lsn: Lsn,
         pages: smallvec::SmallVec<[BatchedGetPageRequest; 1]>,
-        batch_break_reason: GetPageBatchBreakReason,
     },
     DbSize {
         span: Span,
@@ -725,119 +724,6 @@ impl BatchedFeMessage {
             BatchedFeMessage::RespondError { .. } => {}
         }
     }
-
-    fn should_break_batch(
-        &self,
-        other: &BatchedFeMessage,
-        max_batch_size: NonZeroUsize,
-        batching_strategy: PageServiceProtocolPipelinedBatchingStrategy,
-    ) -> Option<GetPageBatchBreakReason> {
-        match (self, other) {
-            (
-                BatchedFeMessage::GetPage {
-                    shard: accum_shard,
-                    pages: accum_pages,
-                    ..
-                },
-                BatchedFeMessage::GetPage {
-                    shard: this_shard,
-                    pages: this_pages,
-                    ..
-                },
-            ) => {
-                assert_eq!(this_pages.len(), 1);
-                if accum_pages.len() >= max_batch_size.get() {
-                    trace!(%max_batch_size, "stopping batching because of batch size");
-                    assert_eq!(accum_pages.len(), max_batch_size.get());
-
-                    return Some(GetPageBatchBreakReason::BatchFull);
-                }
-                if !accum_shard.is_same_handle_as(this_shard) {
-                    trace!("stopping batching because timeline object mismatch");
-                    // TODO: we _could_ batch & execute each shard seperately (and in parallel).
-                    // But the current logic for keeping responses in order does not support that.
-
-                    return Some(GetPageBatchBreakReason::NonUniformTimeline);
-                }
-
-                match batching_strategy {
-                    PageServiceProtocolPipelinedBatchingStrategy::UniformLsn => {
-                        if let Some(last_in_batch) = accum_pages.last() {
-                            if last_in_batch.effective_request_lsn
-                                != this_pages[0].effective_request_lsn
-                            {
-                                trace!(
-                                    accum_lsn = %last_in_batch.effective_request_lsn,
-                                    this_lsn = %this_pages[0].effective_request_lsn,
-                                    "stopping batching because LSN changed"
-                                );
-
-                                return Some(GetPageBatchBreakReason::NonUniformLsn);
-                            }
-                        }
-                    }
-                    PageServiceProtocolPipelinedBatchingStrategy::ScatteredLsn => {
-                        // The read path doesn't curently support serving the same page at different LSNs.
-                        // While technically possible, it's uncertain if the complexity is worth it.
-                        // Break the batch if such a case is encountered.
-                        let same_page_different_lsn = accum_pages.iter().any(|batched| {
-                            batched.req.rel == this_pages[0].req.rel
-                                && batched.req.blkno == this_pages[0].req.blkno
-                                && batched.effective_request_lsn
-                                    != this_pages[0].effective_request_lsn
-                        });
-
-                        if same_page_different_lsn {
-                            trace!(
-                                rel=%this_pages[0].req.rel,
-                                blkno=%this_pages[0].req.blkno,
-                                lsn=%this_pages[0].effective_request_lsn,
-                                "stopping batching because same page was requested at different LSNs"
-                            );
-
-                            return Some(GetPageBatchBreakReason::SamePageAtDifferentLsn);
-                        }
-                    }
-                }
-
-                None
-            }
-            #[cfg(feature = "testing")]
-            (
-                BatchedFeMessage::Test {
-                    shard: accum_shard,
-                    requests: accum_requests,
-                    ..
-                },
-                BatchedFeMessage::Test {
-                    shard: this_shard,
-                    requests: this_requests,
-                    ..
-                },
-            ) => {
-                assert!(this_requests.len() == 1);
-                if accum_requests.len() >= max_batch_size.get() {
-                    trace!(%max_batch_size, "stopping batching because of batch size");
-                    assert_eq!(accum_requests.len(), max_batch_size.get());
-                    return Some(GetPageBatchBreakReason::BatchFull);
-                }
-                if !accum_shard.is_same_handle_as(this_shard) {
-                    trace!("stopping batching because timeline object mismatch");
-                    // TODO: we _could_ batch & execute each shard seperately (and in parallel).
-                    // But the current logic for keeping responses in order does not support that.
-                    return Some(GetPageBatchBreakReason::NonUniformTimeline);
-                }
-                let this_batch_key = this_requests[0].req.batch_key;
-                let accum_batch_key = accum_requests[0].req.batch_key;
-                if this_requests[0].req.batch_key != accum_requests[0].req.batch_key {
-                    trace!(%accum_batch_key, %this_batch_key, "stopping batching because batch key changed");
-                    return Some(GetPageBatchBreakReason::NonUniformKey);
-                }
-                None
-            }
-            (_, _) => Some(GetPageBatchBreakReason::NonBatchableRequest),
-        }
-    }
 }
 
 impl PageServerHandler {
@@ -1139,32 +1025,34 @@ impl PageServerHandler {
                 .await?;
 
                 // We're holding the Handle
-                let effective_request_lsn = match Self::effective_request_lsn(
+                // TODO: if we actually need to wait for lsn here, it delays the entire batch which doesn't need to wait
+                let res = Self::wait_or_get_last_lsn(
                     &shard,
-                    shard.get_last_record_lsn(),
                     req.hdr.request_lsn,
                     req.hdr.not_modified_since,
                     &shard.get_applied_gc_cutoff_lsn(),
-                ) {
+                    &ctx,
+                )
+                .maybe_perf_instrument(&ctx, |current_perf_span| {
+                    info_span!(
+                        target: PERF_TRACE_TARGET,
+                        parent: current_perf_span,
+                        "WAIT_LSN",
+                    )
+                })
+                .await;
+
+                let effective_request_lsn = match res {
                     Ok(lsn) => lsn,
                     Err(e) => {
                         return respond_error!(span, e);
                     }
                 };
-
                 BatchedFeMessage::GetPage {
                     span,
                     shard: shard.downgrade(),
-                    pages: smallvec::smallvec![BatchedGetPageRequest {
-                        req,
-                        timer,
-                        effective_request_lsn,
-                        ctx,
-                    }],
-                    // The executor grabs the batch when it becomes idle.
-                    // Hence, [`GetPageBatchBreakReason::ExecutorSteal`] is the
-                    // default reason for breaking the batch.
-                    batch_break_reason: GetPageBatchBreakReason::ExecutorSteal,
+                    effective_request_lsn,
+                    pages: smallvec::smallvec![BatchedGetPageRequest { req, timer, ctx }],
                 }
             }
             #[cfg(feature = "testing")]
@@ -1190,7 +1078,6 @@ impl PageServerHandler {
     #[instrument(skip_all, level = tracing::Level::TRACE)]
     #[allow(clippy::boxed_local)]
     fn pagestream_do_batch(
-        batching_strategy: PageServiceProtocolPipelinedBatchingStrategy,
         max_batch_size: NonZeroUsize,
         batch: &mut Result<BatchedFeMessage, QueryError>,
         this_msg: Result<BatchedFeMessage, QueryError>,
@@ -1202,58 +1089,89 @@ impl PageServerHandler {
             Err(e) => return Err(Err(e)),
         };
 
-        let eligible_batch = match batch {
-            Ok(b) => b,
-            Err(_) => {
-                return Err(Ok(this_msg));
-            }
-        };
-
-        let batch_break =
-            eligible_batch.should_break_batch(&this_msg, max_batch_size, batching_strategy);
-
-        match batch_break {
-            Some(reason) => {
-                if let BatchedFeMessage::GetPage {
-                    batch_break_reason, ..
-                } = eligible_batch
-                {
-                    *batch_break_reason = reason;
+        match (&mut *batch, this_msg) {
+            // something batched already, let's see if we can add this message to the batch
+            (
+                Ok(BatchedFeMessage::GetPage {
+                    span: _,
+                    shard: accum_shard,
+                    pages: accum_pages,
+                    effective_request_lsn: accum_lsn,
+                }),
+                BatchedFeMessage::GetPage {
+                    span: _,
+                    shard: this_shard,
+                    pages: this_pages,
+                    effective_request_lsn: this_lsn,
+                },
+            ) if (|| {
+                assert_eq!(this_pages.len(), 1);
+                if accum_pages.len() >= max_batch_size.get() {
+                    trace!(%accum_lsn, %this_lsn, %max_batch_size, "stopping batching because of batch size");
+                    assert_eq!(accum_pages.len(), max_batch_size.get());
+                    return false;
                 }
-
-                Err(Ok(this_msg))
-            }
-            None => {
+                if !accum_shard.is_same_handle_as(&this_shard) {
+                    trace!(%accum_lsn, %this_lsn, "stopping batching because timeline object mismatch");
+                    // TODO: we _could_ batch & execute each shard seperately (and in parallel).
+                    // But the current logic for keeping responses in order does not support that.
+                    return false;
+                }
+                // the vectored get currently only supports a single LSN, so, bounce as soon
+                // as the effective request_lsn changes
+                if *accum_lsn != this_lsn {
+                    trace!(%accum_lsn, %this_lsn, "stopping batching because LSN changed");
+                    return false;
+                }
+                true
+            })() =>
+            {
                 // ok to batch
-                match (eligible_batch, this_msg) {
-                    (
-                        BatchedFeMessage::GetPage {
-                            pages: accum_pages, ..
-                        },
-                        BatchedFeMessage::GetPage {
-                            pages: this_pages, ..
-                        },
-                    ) => {
-                        accum_pages.extend(this_pages);
-                        Ok(())
-                    }
-                    #[cfg(feature = "testing")]
-                    (
-                        BatchedFeMessage::Test {
-                            requests: accum_requests,
-                            ..
-                        },
-                        BatchedFeMessage::Test {
-                            requests: this_requests,
-                            ..
-                        },
-                    ) => {
-                        accum_requests.extend(this_requests);
-                        Ok(())
-                    }
-                    // Shape guaranteed by [`BatchedFeMessage::should_break_batch`]
-                    _ => unreachable!(),
+                accum_pages.extend(this_pages);
+                Ok(())
+            }
+            #[cfg(feature = "testing")]
+            (
+                Ok(BatchedFeMessage::Test {
+                    shard: accum_shard,
+                    requests: accum_requests,
+                    ..
+                }),
+                BatchedFeMessage::Test {
+                    shard: this_shard,
+                    requests: this_requests,
+                    ..
+                },
+            ) if (|| {
+                assert!(this_requests.len() == 1);
+                if accum_requests.len() >= max_batch_size.get() {
+                    trace!(%max_batch_size, "stopping batching because of batch size");
+                    assert_eq!(accum_requests.len(), max_batch_size.get());
+                    return false;
                 }
+                if !accum_shard.is_same_handle_as(&this_shard) {
+                    trace!("stopping batching because timeline object mismatch");
+                    // TODO: we _could_ batch & execute each shard seperately (and in parallel).
+                    // But the current logic for keeping responses in order does not support that.
+                    return false;
+                }
+                let this_batch_key = this_requests[0].req.batch_key;
+                let accum_batch_key = accum_requests[0].req.batch_key;
+                if this_requests[0].req.batch_key != accum_requests[0].req.batch_key {
+                    trace!(%accum_batch_key, %this_batch_key, "stopping batching because batch key changed");
+                    return false;
+                }
+                true
+            })() =>
+            {
+                // ok to batch
+                accum_requests.extend(this_requests);
+                Ok(())
+            }
+            // something batched already but this message is unbatchable
+            (_, this_msg) => {
+                // by default, don't continue batching
+                Err(Ok(this_msg))
             }
         }
     }
@@ -1475,8 +1393,8 @@ impl PageServerHandler {
             BatchedFeMessage::GetPage {
                 span,
                 shard,
+                effective_request_lsn,
                 pages,
-                batch_break_reason,
             } => {
                 fail::fail_point!("ps::handle-pagerequest-message::getpage");
                 let (shard, ctx) = upgrade_handle_and_set_context!(shard);
@@ -1487,9 +1405,9 @@ impl PageServerHandler {
                         let res = self
                             .handle_get_page_at_lsn_request_batched(
                                 &shard,
+                                effective_request_lsn,
                                 pages,
                                 io_concurrency,
-                                batch_break_reason,
                                 &ctx,
                             )
                             .instrument(span.clone())
@@ -1806,7 +1724,6 @@ impl PageServerHandler {
         let PageServicePipeliningConfigPipelined {
             max_batch_size,
             execution,
-            batching: batching_strategy,
         } = pipelining_config;
 
         // Macro to _define_ a pipeline stage.
@@ -1858,7 +1775,7 @@ impl PageServerHandler {
                     exit |= read_res.is_err();
                     let could_send = batch_tx
                         .send(read_res, |batch, res| {
-                            Self::pagestream_do_batch(batching_strategy, max_batch_size, batch, res)
+                            Self::pagestream_do_batch(max_batch_size, batch, res)
                         })
                         .await;
                     exit |= could_send.is_err();
@@ -1954,39 +1871,7 @@ impl PageServerHandler {
         ctx: &RequestContext,
     ) -> Result<Lsn, PageStreamError> {
         let last_record_lsn = timeline.get_last_record_lsn();
-        let effective_request_lsn = Self::effective_request_lsn(
-            timeline,
-            last_record_lsn,
-            request_lsn,
-            not_modified_since,
-            latest_gc_cutoff_lsn,
-        )?;
 
-        if effective_request_lsn > last_record_lsn {
-            timeline
-                .wait_lsn(
-                    not_modified_since,
-                    crate::tenant::timeline::WaitLsnWaiter::PageService,
-                    timeline::WaitLsnTimeout::Default,
-                    ctx,
-                )
-                .await?;
-
-            // Since we waited for 'effective_request_lsn' to arrive, that is now the last
-            // record LSN. (Or close enough for our purposes; the last-record LSN can
-            // advance immediately after we return anyway)
-        }
-
-        Ok(effective_request_lsn)
-    }
-
-    fn effective_request_lsn(
-        timeline: &Timeline,
-        last_record_lsn: Lsn,
-        request_lsn: Lsn,
-        not_modified_since: Lsn,
-        latest_gc_cutoff_lsn: &RcuReadGuard<Lsn>,
-    ) -> Result<Lsn, PageStreamError> {
         // Sanity check the request
         if request_lsn < not_modified_since {
             return Err(PageStreamError::BadRequest(
@@ -2021,7 +1906,19 @@ impl PageServerHandler {
             }
         }
 
+        // Wait for WAL up to 'not_modified_since' to arrive, if necessary
         if not_modified_since > last_record_lsn {
+            timeline
+                .wait_lsn(
+                    not_modified_since,
+                    crate::tenant::timeline::WaitLsnWaiter::PageService,
+                    timeline::WaitLsnTimeout::Default,
+                    ctx,
+                )
+                .await?;
+            // Since we waited for 'not_modified_since' to arrive, that is now the last
+            // record LSN. (Or close enough for our purposes; the last-record LSN can
+            // advance immediately after we return anyway)
             Ok(not_modified_since)
         } else {
             // It might be better to use max(not_modified_since, latest_gc_cutoff_lsn)
@@ -2176,16 +2073,16 @@ impl PageServerHandler {
     async fn handle_get_page_at_lsn_request_batched(
         &mut self,
         timeline: &Timeline,
+        effective_lsn: Lsn,
         requests: smallvec::SmallVec<[BatchedGetPageRequest; 1]>,
         io_concurrency: IoConcurrency,
-        batch_break_reason: GetPageBatchBreakReason,
         ctx: &RequestContext,
     ) -> Vec<Result<(PagestreamBeMessage, SmgrOpTimer), BatchedPageStreamError>> {
         debug_assert_current_span_has_tenant_and_timeline_id();
 
         timeline
             .query_metrics
-            .observe_getpage_batch_start(requests.len(), batch_break_reason);
+            .observe_getpage_batch_start(requests.len());
 
         // If a page trace is running, submit an event for this request.
         if let Some(page_trace) = timeline.page_trace.load().as_ref() {
@@ -2195,81 +2092,20 @@ impl PageServerHandler {
                 // Ignore error (trace buffer may be full or tracer may have disconnected).
                 _ = page_trace.try_send(PageTraceEvent {
                     key,
-                    effective_lsn: batch.effective_request_lsn,
+                    effective_lsn,
                     time,
                 });
             }
         }
 
-        // If any request in the batch needs to wait for LSN, then do so now.
-        let mut perf_instrument = false;
-        let max_effective_lsn = requests
-            .iter()
-            .map(|req| {
-                if req.ctx.has_perf_span() {
-                    perf_instrument = true;
-                }
-
-                req.effective_request_lsn
-            })
-            .max()
-            .expect("batch is never empty");
-
-        let ctx = match perf_instrument {
-            true => RequestContextBuilder::from(ctx)
-                .root_perf_span(|| {
-                    info_span!(
-                        target: PERF_TRACE_TARGET,
-                        "GET_VECTORED",
-                        tenant_id = %timeline.tenant_shard_id.tenant_id,
-                        timeline_id = %timeline.timeline_id,
-                        shard = %timeline.tenant_shard_id.shard_slug(),
-                        %max_effective_lsn
-                    )
-                })
-                .attached_child(),
-            false => ctx.attached_child(),
-        };
-
-        let last_record_lsn = timeline.get_last_record_lsn();
-        if max_effective_lsn > last_record_lsn {
-            if let Err(e) = timeline
-                .wait_lsn(
-                    max_effective_lsn,
-                    crate::tenant::timeline::WaitLsnWaiter::PageService,
-                    timeline::WaitLsnTimeout::Default,
-                    &ctx,
-                )
-                .maybe_perf_instrument(&ctx, |current_perf_span| {
-                    info_span!(
-                        target: PERF_TRACE_TARGET,
-                        parent: current_perf_span,
-                        "WAIT_LSN",
-                    )
-                })
-                .await
-            {
-                return Vec::from_iter(requests.into_iter().map(|req| {
-                    Err(BatchedPageStreamError {
-                        err: PageStreamError::from(e.clone()),
-                        req: req.req.hdr,
-                    })
-                }));
-            }
-        }
-
         let results = timeline
             .get_rel_page_at_lsn_batched(
-                requests.iter().map(|p| {
-                    (
-                        &p.req.rel,
-                        &p.req.blkno,
-                        p.effective_request_lsn,
-                        p.ctx.attached_child(),
-                    )
-                }),
+                requests
+                    .iter()
+                    .map(|p| (&p.req.rel, &p.req.blkno, p.ctx.attached_child())),
+                effective_lsn,
                 io_concurrency,
-                &ctx,
+                ctx,
             )
             .await;
         assert_eq!(results.len(), requests.len());

pageserver/src/pgdatadir_mapping.rs

@@ -6,7 +6,7 @@
 //! walingest.rs handles a few things like implicit relation creation and extension.
 //! Clarify that)
 //!
-use std::collections::{HashMap, HashSet, hash_map};
+use std::collections::{BTreeMap, HashMap, HashSet, hash_map};
 use std::ops::{ControlFlow, Range};
 
 use crate::walingest::{WalIngestError, WalIngestErrorKind};
@@ -14,6 +14,7 @@ use crate::{PERF_TRACE_TARGET, ensure_walingest};
 use anyhow::Context;
 use bytes::{Buf, Bytes, BytesMut};
 use enum_map::Enum;
+use itertools::Itertools;
 use pageserver_api::key::{
     AUX_FILES_KEY, CHECKPOINT_KEY, CONTROLFILE_KEY, CompactKey, DBDIR_KEY, Key, RelDirExists,
     TWOPHASEDIR_KEY, dbdir_key_range, rel_block_to_key, rel_dir_to_key, rel_key_range,
@@ -21,7 +22,7 @@ use pageserver_api::key::{
     repl_origin_key, repl_origin_key_range, slru_block_to_key, slru_dir_to_key,
     slru_segment_key_range, slru_segment_size_to_key, twophase_file_key, twophase_key_range,
 };
-use pageserver_api::keyspace::{KeySpaceRandomAccum, SparseKeySpace};
+use pageserver_api::keyspace::SparseKeySpace;
 use pageserver_api::models::RelSizeMigration;
 use pageserver_api::record::NeonWalRecord;
 use pageserver_api::reltag::{BlockNumber, RelTag, SlruKind};
@@ -40,7 +41,7 @@ use wal_decoder::serialized_batch::{SerializedValueBatch, ValueMeta};
 
 use super::tenant::{PageReconstructError, Timeline};
 use crate::aux_file;
-use crate::context::{PerfInstrumentFutureExt, RequestContext};
+use crate::context::{PerfInstrumentFutureExt, RequestContext, RequestContextBuilder};
 use crate::keyspace::{KeySpace, KeySpaceAccum};
 use crate::metrics::{
     RELSIZE_CACHE_ENTRIES, RELSIZE_CACHE_HITS, RELSIZE_CACHE_MISSES, RELSIZE_CACHE_MISSES_OLD,
@@ -50,7 +51,7 @@ use crate::span::{
     debug_assert_current_span_has_tenant_and_timeline_id_no_shard_id,
 };
 use crate::tenant::storage_layer::IoConcurrency;
-use crate::tenant::timeline::{GetVectoredError, VersionedKeySpaceQuery};
+use crate::tenant::timeline::GetVectoredError;
 
 /// Max delta records appended to the AUX_FILES_KEY (for aux v1). The write path will write a full image once this threshold is reached.
 pub const MAX_AUX_FILE_DELTAS: usize = 1024;
@@ -206,9 +207,10 @@ impl Timeline {
                 let pages: smallvec::SmallVec<[_; 1]> = smallvec::smallvec![(tag, blknum)];
                 let res = self
                     .get_rel_page_at_lsn_batched(
-                        pages.iter().map(|(tag, blknum)| {
-                            (tag, blknum, effective_lsn, ctx.attached_child())
-                        }),
+                        pages
+                            .iter()
+                            .map(|(tag, blknum)| (tag, blknum, ctx.attached_child())),
+                        effective_lsn,
                         io_concurrency.clone(),
                         ctx,
                     )
@@ -246,7 +248,8 @@ impl Timeline {
     /// The ordering of the returned vec corresponds to the ordering of `pages`.
     pub(crate) async fn get_rel_page_at_lsn_batched(
         &self,
-        pages: impl ExactSizeIterator<Item = (&RelTag, &BlockNumber, Lsn, RequestContext)>,
+        pages: impl ExactSizeIterator<Item = (&RelTag, &BlockNumber, RequestContext)>,
+        effective_lsn: Lsn,
         io_concurrency: IoConcurrency,
         ctx: &RequestContext,
     ) -> Vec<Result<Bytes, PageReconstructError>> {
@@ -259,13 +262,11 @@ impl Timeline {
         let mut result = Vec::with_capacity(pages.len());
         let result_slots = result.spare_capacity_mut();
 
-        let mut keys_slots: HashMap<Key, smallvec::SmallVec<[(usize, RequestContext); 1]>> =
-            HashMap::with_capacity(pages.len());
+        let mut keys_slots: BTreeMap<Key, smallvec::SmallVec<[(usize, RequestContext); 1]>> =
+            BTreeMap::default();
 
-        let mut req_keyspaces: HashMap<Lsn, KeySpaceRandomAccum> =
-            HashMap::with_capacity(pages.len());
-
-        for (response_slot_idx, (tag, blknum, lsn, ctx)) in pages.enumerate() {
+        let mut perf_instrument = false;
+        for (response_slot_idx, (tag, blknum, ctx)) in pages.enumerate() {
             if tag.relnode == 0 {
                 result_slots[response_slot_idx].write(Err(PageReconstructError::Other(
                     RelationError::InvalidRelnode.into(),
@@ -276,14 +277,14 @@ impl Timeline {
             }
 
             let nblocks = match self
-                .get_rel_size(*tag, Version::Lsn(lsn), &ctx)
+                .get_rel_size(*tag, Version::Lsn(effective_lsn), &ctx)
                 .maybe_perf_instrument(&ctx, |crnt_perf_span| {
                     info_span!(
                         target: PERF_TRACE_TARGET,
                         parent: crnt_perf_span,
                         "GET_REL_SIZE",
                         reltag=%tag,
-                        lsn=%lsn,
+                        lsn=%effective_lsn,
                     )
                 })
                 .await
@@ -299,7 +300,7 @@ impl Timeline {
             if *blknum >= nblocks {
                 debug!(
                     "read beyond EOF at {} blk {} at {}, size is {}: returning all-zeros page",
-                    tag, blknum, lsn, nblocks
+                    tag, blknum, effective_lsn, nblocks
                 );
                 result_slots[response_slot_idx].write(Ok(ZERO_PAGE.clone()));
                 slots_filled += 1;
@@ -308,29 +309,46 @@ impl Timeline {
 
             let key = rel_block_to_key(*tag, *blknum);
 
+            if ctx.has_perf_span() {
+                perf_instrument = true;
+            }
+
             let key_slots = keys_slots.entry(key).or_default();
             key_slots.push((response_slot_idx, ctx));
-
-            let acc = req_keyspaces.entry(lsn).or_default();
-            acc.add_key(key);
         }
 
-        let query: Vec<(Lsn, KeySpace)> = req_keyspaces
-            .into_iter()
-            .map(|(lsn, acc)| (lsn, acc.to_keyspace()))
-            .collect();
+        let keyspace = {
+            // add_key requires monotonicity
+            let mut acc = KeySpaceAccum::new();
+            for key in keys_slots
+                .keys()
+                // in fact it requires strong monotonicity
+                .dedup()
+            {
+                acc.add_key(*key);
+            }
+            acc.to_keyspace()
+        };
+
+        let ctx = match perf_instrument {
+            true => RequestContextBuilder::from(ctx)
+                .root_perf_span(|| {
+                    info_span!(
+                        target: PERF_TRACE_TARGET,
+                        "GET_VECTORED",
+                        tenant_id = %self.tenant_shard_id.tenant_id,
+                        timeline_id = %self.timeline_id,
+                        lsn = %effective_lsn,
+                        shard = %self.tenant_shard_id.shard_slug(),
+                    )
+                })
+                .attached_child(),
+            false => ctx.attached_child(),
+        };
 
-        let query = VersionedKeySpaceQuery::scattered(query);
         let res = self
-            .get_vectored(query, io_concurrency, ctx)
-            .maybe_perf_instrument(ctx, |current_perf_span| {
-                info_span!(
-                    target: PERF_TRACE_TARGET,
-                    parent: current_perf_span,
-                    "GET_BATCH",
-                    batch_size = %page_count,
-                )
-            })
+            .get_vectored(keyspace, effective_lsn, io_concurrency, &ctx)
+            .maybe_perf_instrument(&ctx, |current_perf_span| current_perf_span.clone())
             .await;
 
         match res {
@@ -360,12 +378,12 @@ impl Timeline {
                         // There is no standardized way to express that the batched span followed from N request spans.
                         // So, abuse the system and mark the request contexts as follows_from the batch span, so we get
                         // some linkage in our trace viewer. It allows us to answer: which GET_VECTORED did this GET_PAGE wait for.
-                        req_ctx.perf_follows_from(ctx);
+                        req_ctx.perf_follows_from(&ctx);
                         slots_filled += 1;
                     }
 
                     result_slots[first_slot].write(res);
-                    first_req_ctx.perf_follows_from(ctx);
+                    first_req_ctx.perf_follows_from(&ctx);
                     slots_filled += 1;
                 }
             }
@@ -404,7 +422,7 @@ impl Timeline {
                         }
                     };
 
-                    req_ctx.perf_follows_from(ctx);
+                    req_ctx.perf_follows_from(&ctx);
                     result_slots[*slot].write(err);
                 }
 
@@ -643,9 +661,8 @@ impl Timeline {
 
         let mut segment = BytesMut::with_capacity(n_blocks as usize * BLCKSZ as usize);
         for batch in batches.parts {
-            let query = VersionedKeySpaceQuery::uniform(batch, lsn);
             let blocks = self
-                .get_vectored(query, io_concurrency.clone(), ctx)
+                .get_vectored(batch, lsn, io_concurrency.clone(), ctx)
                 .await?;
 
             for (_key, block) in blocks {
@@ -882,9 +899,8 @@ impl Timeline {
             );
 
             for batch in batches.parts.into_iter().rev() {
-                let query = VersionedKeySpaceQuery::uniform(batch, probe_lsn);
                 let blocks = self
-                    .get_vectored(query, io_concurrency.clone(), ctx)
+                    .get_vectored(batch, probe_lsn, io_concurrency.clone(), ctx)
                     .await?;
 
                 for (_key, clog_page) in blocks.into_iter().rev() {

pageserver/src/tenant.rs

@@ -5933,20 +5933,12 @@ mod tests {
     use models::CompactLsnRange;
     use pageserver_api::key::{AUX_KEY_PREFIX, Key, NON_INHERITED_RANGE, RELATION_SIZE_PREFIX};
     use pageserver_api::keyspace::KeySpace;
-    #[cfg(feature = "testing")]
-    use pageserver_api::keyspace::KeySpaceRandomAccum;
     use pageserver_api::models::{CompactionAlgorithm, CompactionAlgorithmSettings};
     #[cfg(feature = "testing")]
     use pageserver_api::record::NeonWalRecord;
     use pageserver_api::value::Value;
     use pageserver_compaction::helpers::overlaps_with;
-    #[cfg(feature = "testing")]
-    use rand::SeedableRng;
-    #[cfg(feature = "testing")]
-    use rand::rngs::StdRng;
     use rand::{Rng, thread_rng};
-    #[cfg(feature = "testing")]
-    use std::ops::Range;
     use storage_layer::{IoConcurrency, PersistentLayerKey};
     use tests::storage_layer::ValuesReconstructState;
     use tests::timeline::{GetVectoredError, ShutdownMode};
@@ -5956,7 +5948,7 @@ mod tests {
     use timeline::InMemoryLayerTestDesc;
     #[cfg(feature = "testing")]
     use timeline::compaction::{KeyHistoryRetention, KeyLogAtLsn};
-    use timeline::{CompactOptions, DeltaLayerTestDesc, VersionedKeySpaceQuery};
+    use timeline::{CompactOptions, DeltaLayerTestDesc};
     use utils::id::TenantId;
 
     use super::*;
@@ -5968,318 +5960,6 @@ mod tests {
     static TEST_KEY: Lazy<Key> =
         Lazy::new(|| Key::from_slice(&hex!("010000000033333333444444445500000001")));
 
-    #[cfg(feature = "testing")]
-    struct TestTimelineSpecification {
-        start_lsn: Lsn,
-        last_record_lsn: Lsn,
-
-        in_memory_layers_shape: Vec<(Range<Key>, Range<Lsn>)>,
-        delta_layers_shape: Vec<(Range<Key>, Range<Lsn>)>,
-        image_layers_shape: Vec<(Range<Key>, Lsn)>,
-
-        gap_chance: u8,
-        will_init_chance: u8,
-    }
-
-    #[cfg(feature = "testing")]
-    struct Storage {
-        storage: HashMap<(Key, Lsn), Value>,
-        start_lsn: Lsn,
-    }
-
-    #[cfg(feature = "testing")]
-    impl Storage {
-        fn get(&self, key: Key, lsn: Lsn) -> Bytes {
-            use bytes::BufMut;
-
-            let mut crnt_lsn = lsn;
-            let mut got_base = false;
-
-            let mut acc = Vec::new();
-
-            while crnt_lsn >= self.start_lsn {
-                if let Some(value) = self.storage.get(&(key, crnt_lsn)) {
-                    acc.push(value.clone());
-
-                    match value {
-                        Value::WalRecord(NeonWalRecord::Test { will_init, .. }) => {
-                            if *will_init {
-                                got_base = true;
-                                break;
-                            }
-                        }
-                        Value::Image(_) => {
-                            got_base = true;
-                            break;
-                        }
-                        _ => unreachable!(),
-                    }
-                }
-
-                crnt_lsn = crnt_lsn.checked_sub(1u64).unwrap();
-            }
-
-            assert!(
-                got_base,
-                "Input data was incorrect. No base image for {key}@{lsn}"
-            );
-
-            tracing::debug!("Wal redo depth for {key}@{lsn} is {}", acc.len());
-
-            let mut blob = BytesMut::new();
-            for value in acc.into_iter().rev() {
-                match value {
-                    Value::WalRecord(NeonWalRecord::Test { append, .. }) => {
-                        blob.extend_from_slice(append.as_bytes());
-                    }
-                    Value::Image(img) => {
-                        blob.put(img);
-                    }
-                    _ => unreachable!(),
-                }
-            }
-
-            blob.into()
-        }
-    }
-
-    #[cfg(feature = "testing")]
-    #[allow(clippy::too_many_arguments)]
-    async fn randomize_timeline(
-        tenant: &Arc<Tenant>,
-        new_timeline_id: TimelineId,
-        pg_version: u32,
-        spec: TestTimelineSpecification,
-        random: &mut rand::rngs::StdRng,
-        ctx: &RequestContext,
-    ) -> anyhow::Result<(Arc<Timeline>, Storage, Vec<Lsn>)> {
-        let mut storage: HashMap<(Key, Lsn), Value> = HashMap::default();
-        let mut interesting_lsns = vec![spec.last_record_lsn];
-
-        for (key_range, lsn_range) in spec.in_memory_layers_shape.iter() {
-            let mut lsn = lsn_range.start;
-            while lsn < lsn_range.end {
-                let mut key = key_range.start;
-                while key < key_range.end {
-                    let gap = random.gen_range(1..=100) <= spec.gap_chance;
-                    let will_init = random.gen_range(1..=100) <= spec.will_init_chance;
-
-                    if gap {
-                        continue;
-                    }
-
-                    let record = if will_init {
-                        Value::WalRecord(NeonWalRecord::wal_init(format!("[wil_init {key}@{lsn}]")))
-                    } else {
-                        Value::WalRecord(NeonWalRecord::wal_append(format!("[delta {key}@{lsn}]")))
-                    };
-
-                    storage.insert((key, lsn), record);
-
-                    key = key.next();
-                }
-                lsn = Lsn(lsn.0 + 1);
-            }
-
-            // Stash some interesting LSN for future use
-            for offset in [0, 5, 100].iter() {
-                if *offset == 0 {
-                    interesting_lsns.push(lsn_range.start);
-                } else {
-                    let below = lsn_range.start.checked_sub(*offset);
-                    match below {
-                        Some(v) if v >= spec.start_lsn => {
-                            interesting_lsns.push(v);
-                        }
-                        _ => {}
-                    }
-
-                    let above = Lsn(lsn_range.start.0 + offset);
-                    interesting_lsns.push(above);
-                }
-            }
-        }
-
-        for (key_range, lsn_range) in spec.delta_layers_shape.iter() {
-            let mut lsn = lsn_range.start;
-            while lsn < lsn_range.end {
-                let mut key = key_range.start;
-                while key < key_range.end {
-                    let gap = random.gen_range(1..=100) <= spec.gap_chance;
-                    let will_init = random.gen_range(1..=100) <= spec.will_init_chance;
-
-                    if gap {
-                        continue;
-                    }
-
-                    let record = if will_init {
-                        Value::WalRecord(NeonWalRecord::wal_init(format!("[wil_init {key}@{lsn}]")))
-                    } else {
-                        Value::WalRecord(NeonWalRecord::wal_append(format!("[delta {key}@{lsn}]")))
-                    };
-
-                    storage.insert((key, lsn), record);
-
-                    key = key.next();
-                }
-                lsn = Lsn(lsn.0 + 1);
-            }
-
-            // Stash some interesting LSN for future use
-            for offset in [0, 5, 100].iter() {
-                if *offset == 0 {
-                    interesting_lsns.push(lsn_range.start);
-                } else {
-                    let below = lsn_range.start.checked_sub(*offset);
-                    match below {
-                        Some(v) if v >= spec.start_lsn => {
-                            interesting_lsns.push(v);
-                        }
-                        _ => {}
-                    }
-
-                    let above = Lsn(lsn_range.start.0 + offset);
-                    interesting_lsns.push(above);
-                }
-            }
-        }
-
-        for (key_range, lsn) in spec.image_layers_shape.iter() {
-            let mut key = key_range.start;
-            while key < key_range.end {
-                let blob = Bytes::from(format!("[image {key}@{lsn}]"));
-                let record = Value::Image(blob.clone());
-                storage.insert((key, *lsn), record);
-
-                key = key.next();
-            }
-
-            // Stash some interesting LSN for future use
-            for offset in [0, 5, 100].iter() {
-                if *offset == 0 {
-                    interesting_lsns.push(*lsn);
-                } else {
-                    let below = lsn.checked_sub(*offset);
-                    match below {
-                        Some(v) if v >= spec.start_lsn => {
-                            interesting_lsns.push(v);
-                        }
-                        _ => {}
-                    }
-
-                    let above = Lsn(lsn.0 + offset);
-                    interesting_lsns.push(above);
-                }
-            }
-        }
-
-        let in_memory_test_layers = {
-            let mut acc = Vec::new();
-
-            for (key_range, lsn_range) in spec.in_memory_layers_shape.iter() {
-                let mut data = Vec::new();
-
-                let mut lsn = lsn_range.start;
-                while lsn < lsn_range.end {
-                    let mut key = key_range.start;
-                    while key < key_range.end {
-                        if let Some(record) = storage.get(&(key, lsn)) {
-                            data.push((key, lsn, record.clone()));
-                        }
-
-                        key = key.next();
-                    }
-                    lsn = Lsn(lsn.0 + 1);
-                }
-
-                acc.push(InMemoryLayerTestDesc {
-                    data,
-                    lsn_range: lsn_range.clone(),
-                    is_open: false,
-                })
-            }
-
-            acc
-        };
-
-        let delta_test_layers = {
-            let mut acc = Vec::new();
-
-            for (key_range, lsn_range) in spec.delta_layers_shape.iter() {
-                let mut data = Vec::new();
-
-                let mut lsn = lsn_range.start;
-                while lsn < lsn_range.end {
-                    let mut key = key_range.start;
-                    while key < key_range.end {
-                        if let Some(record) = storage.get(&(key, lsn)) {
-                            data.push((key, lsn, record.clone()));
-                        }
-
-                        key = key.next();
-                    }
-                    lsn = Lsn(lsn.0 + 1);
-                }
-
-                acc.push(DeltaLayerTestDesc {
-                    data,
-                    lsn_range: lsn_range.clone(),
-                    key_range: key_range.clone(),
-                })
-            }
-
-            acc
-        };
-
-        let image_test_layers = {
-            let mut acc = Vec::new();
-
-            for (key_range, lsn) in spec.image_layers_shape.iter() {
-                let mut data = Vec::new();
-
-                let mut key = key_range.start;
-                while key < key_range.end {
-                    if let Some(record) = storage.get(&(key, *lsn)) {
-                        let blob = match record {
-                            Value::Image(blob) => blob.clone(),
-                            _ => unreachable!(),
-                        };
-
-                        data.push((key, blob));
-                    }
-
-                    key = key.next();
-                }
-
-                acc.push((*lsn, data));
-            }
-
-            acc
-        };
-
-        let tline = tenant
-            .create_test_timeline_with_layers(
-                new_timeline_id,
-                spec.start_lsn,
-                pg_version,
-                ctx,
-                in_memory_test_layers,
-                delta_test_layers,
-                image_test_layers,
-                spec.last_record_lsn,
-            )
-            .await?;
-
-        Ok((
-            tline,
-            Storage {
-                storage,
-                start_lsn: spec.start_lsn,
-            },
-            interesting_lsns,
-        ))
-    }
-
     #[tokio::test]
     async fn test_basic() -> anyhow::Result<()> {
         let (tenant, ctx) = TenantHarness::create("test_basic").await?.load().await;
@@ -7106,11 +6786,10 @@ mod tests {
         for read in reads {
             info!("Doing vectored read on {:?}", read);
 
-            let query = VersionedKeySpaceQuery::uniform(read.clone(), reads_lsn);
-
             let vectored_res = tline
                 .get_vectored_impl(
-                    query,
+                    read.clone(),
+                    reads_lsn,
                     &mut ValuesReconstructState::new(io_concurrency.clone()),
                     &ctx,
                 )
@@ -7189,11 +6868,10 @@ mod tests {
         };
         let read_lsn = child_timeline.get_last_record_lsn();
 
-        let query = VersionedKeySpaceQuery::uniform(aux_keyspace.clone(), read_lsn);
-
         let vectored_res = child_timeline
             .get_vectored_impl(
-                query,
+                aux_keyspace.clone(),
+                read_lsn,
                 &mut ValuesReconstructState::new(io_concurrency.clone()),
                 &ctx,
             )
@@ -7339,12 +7017,10 @@ mod tests {
         let read = KeySpace {
             ranges: vec![key_near_gap..gap_at_key.next(), key_near_end..current_key],
         };
-
-        let query = VersionedKeySpaceQuery::uniform(read.clone(), current_lsn);
-
         let results = child_timeline
             .get_vectored_impl(
-                query,
+                read.clone(),
+                current_lsn,
                 &mut ValuesReconstructState::new(io_concurrency.clone()),
                 &ctx,
             )
@@ -7475,16 +7151,12 @@ mod tests {
         }
 
         for query_lsn in query_lsns {
-            let query = VersionedKeySpaceQuery::uniform(
-                KeySpace {
-                    ranges: vec![child_gap_at_key..child_gap_at_key.next()],
-                },
-                query_lsn,
-            );
-
             let results = child_timeline
                 .get_vectored_impl(
-                    query,
+                    KeySpace {
+                        ranges: vec![child_gap_at_key..child_gap_at_key.next()],
+                    },
+                    query_lsn,
                     &mut ValuesReconstructState::new(io_concurrency.clone()),
                     &ctx,
                 )
@@ -7983,11 +7655,10 @@ mod tests {
             }
 
             let mut cnt = 0;
-            let query = VersionedKeySpaceQuery::uniform(keyspace.clone(), lsn);
-
             for (key, value) in tline
                 .get_vectored_impl(
-                    query,
+                    keyspace.clone(),
+                    lsn,
                     &mut ValuesReconstructState::new(io_concurrency.clone()),
                     &ctx,
                 )
@@ -8194,9 +7865,8 @@ mod tests {
             io_concurrency: IoConcurrency,
         ) -> anyhow::Result<(BTreeMap<Key, Result<Bytes, PageReconstructError>>, usize)> {
             let mut reconstruct_state = ValuesReconstructState::new(io_concurrency);
-            let query = VersionedKeySpaceQuery::uniform(keyspace.clone(), lsn);
             let res = tline
-                .get_vectored_impl(query, &mut reconstruct_state, ctx)
+                .get_vectored_impl(keyspace.clone(), lsn, &mut reconstruct_state, ctx)
                 .await?;
             Ok((res, reconstruct_state.get_delta_layers_visited() as usize))
         }
@@ -8493,10 +8163,13 @@ mod tests {
 
         // test vectored scan on parent timeline
         let mut reconstruct_state = ValuesReconstructState::new(io_concurrency.clone());
-        let query =
-            VersionedKeySpaceQuery::uniform(KeySpace::single(Key::metadata_key_range()), lsn);
         let res = tline
-            .get_vectored_impl(query, &mut reconstruct_state, &ctx)
+            .get_vectored_impl(
+                KeySpace::single(Key::metadata_key_range()),
+                lsn,
+                &mut reconstruct_state,
+                &ctx,
+            )
             .await?;
 
         assert_eq!(
@@ -8516,10 +8189,13 @@ mod tests {
 
         // test vectored scan on child timeline
         let mut reconstruct_state = ValuesReconstructState::new(io_concurrency.clone());
-        let query =
-            VersionedKeySpaceQuery::uniform(KeySpace::single(Key::metadata_key_range()), lsn);
         let res = child
-            .get_vectored_impl(query, &mut reconstruct_state, &ctx)
+            .get_vectored_impl(
+                KeySpace::single(Key::metadata_key_range()),
+                lsn,
+                &mut reconstruct_state,
+                &ctx,
+            )
             .await?;
 
         assert_eq!(
@@ -8553,9 +8229,13 @@ mod tests {
         let io_concurrency =
             IoConcurrency::spawn_from_conf(tline.conf, tline.gate.enter().unwrap());
         let mut reconstruct_state = ValuesReconstructState::new(io_concurrency);
-        let query = VersionedKeySpaceQuery::uniform(KeySpace::single(key..key.next()), lsn);
         let mut res = tline
-            .get_vectored_impl(query, &mut reconstruct_state, ctx)
+            .get_vectored_impl(
+                KeySpace::single(key..key.next()),
+                lsn,
+                &mut reconstruct_state,
+                ctx,
+            )
             .await?;
         Ok(res.pop_last().map(|(k, v)| {
             assert_eq!(k, key);
@@ -9577,7 +9257,6 @@ mod tests {
                 &[Lsn(0x20), Lsn(0x40), Lsn(0x50)],
                 3,
                 None,
-                true,
             )
             .await
             .unwrap();
@@ -9702,15 +9381,7 @@ mod tests {
             ),
         ];
         let res = tline
-            .generate_key_retention(
-                key,
-                &history,
-                Lsn(0x60),
-                &[Lsn(0x40), Lsn(0x50)],
-                3,
-                None,
-                true,
-            )
+            .generate_key_retention(key, &history, Lsn(0x60), &[Lsn(0x40), Lsn(0x50)], 3, None)
             .await
             .unwrap();
         let expected_res = KeyHistoryRetention {
@@ -9789,7 +9460,6 @@ mod tests {
                 &[],
                 3,
                 Some((key, Lsn(0x10), Bytes::copy_from_slice(b"0x10"))),
-                true,
             )
             .await
             .unwrap();
@@ -9838,7 +9508,6 @@ mod tests {
                 &[Lsn(0x30)],
                 3,
                 Some((key, Lsn(0x10), Bytes::copy_from_slice(b"0x10"))),
-                true,
             )
             .await
             .unwrap();
@@ -10689,13 +10358,14 @@ mod tests {
             )
             .await?;
 
-        let query = VersionedKeySpaceQuery::uniform(
-            KeySpace::single(get_key(0)..get_key(10)),
-            delta_layer_end_lsn,
-        );
-
+        let keyspace = KeySpace::single(get_key(0)..get_key(10));
         let results = tline
-            .get_vectored(query, IoConcurrency::sequential(), &ctx)
+            .get_vectored(
+                keyspace,
+                delta_layer_end_lsn,
+                IoConcurrency::sequential(),
+                &ctx,
+            )
             .await
             .expect("No vectored errors");
         for (key, res) in results {
@@ -10843,13 +10513,9 @@ mod tests {
             )
             .await?;
 
-        let query = VersionedKeySpaceQuery::uniform(
-            KeySpace::single(get_key(0)..get_key(10)),
-            last_record_lsn,
-        );
-
+        let keyspace = KeySpace::single(get_key(0)..get_key(10));
         let results = tline
-            .get_vectored(query, IoConcurrency::sequential(), &ctx)
+            .get_vectored(keyspace, last_record_lsn, IoConcurrency::sequential(), &ctx)
             .await
             .expect("No vectored errors");
         for (key, res) in results {
@@ -10863,214 +10529,6 @@ mod tests {
         Ok(())
     }
 
-    // A randomized read path test. Generates a layer map according to a deterministic
-    // specification. Fills the (key, LSN) space in random manner and then performs
-    // random scattered queries validating the results against in-memory storage.
-    //
-    // See this internal Notion page for a diagram of the layer map:
-    // https://www.notion.so/neondatabase/Read-Path-Unit-Testing-Fuzzing-1d1f189e0047806c8e5cd37781b0a350?pvs=4
-    //
-    // A fuzzing mode is also supported. In this mode, the test will use a random
-    // seed instead of a hardcoded one. Use it in conjunction with `cargo stress`
-    // to run multiple instances in parallel:
-    //
-    // $ RUST_BACKTRACE=1 RUST_LOG=INFO \
-    //   cargo stress --package=pageserver --features=testing,fuzz-read-path --release -- test_read_path
-    #[cfg(feature = "testing")]
-    #[tokio::test]
-    async fn test_read_path() -> anyhow::Result<()> {
-        use rand::seq::SliceRandom;
-
-        let seed = if cfg!(feature = "fuzz-read-path") {
-            let seed: u64 = thread_rng().r#gen();
-            seed
-        } else {
-            // Use a hard-coded seed when not in fuzzing mode.
-            // Note that with the current approach results are not reproducible
-            // accross platforms and Rust releases.
-            const SEED: u64 = 0;
-            SEED
-        };
-
-        let mut random = StdRng::seed_from_u64(seed);
-
-        let (queries, will_init_chance, gap_chance) = if cfg!(feature = "fuzz-read-path") {
-            const QUERIES: u64 = 5000;
-            let will_init_chance: u8 = random.gen_range(0..=10);
-            let gap_chance: u8 = random.gen_range(0..=50);
-
-            (QUERIES, will_init_chance, gap_chance)
-        } else {
-            const QUERIES: u64 = 1000;
-            const WILL_INIT_CHANCE: u8 = 1;
-            const GAP_CHANCE: u8 = 5;
-
-            (QUERIES, WILL_INIT_CHANCE, GAP_CHANCE)
-        };
-
-        let harness = TenantHarness::create("test_read_path").await?;
-        let (tenant, ctx) = harness.load().await;
-
-        tracing::info!("Using random seed: {seed}");
-        tracing::info!(%will_init_chance, %gap_chance, "Fill params");
-
-        // Define the layer map shape. Note that this part is not randomized.
-
-        const KEY_DIMENSION_SIZE: u32 = 99;
-        let start_key = Key::from_hex("110000000033333333444444445500000000").unwrap();
-        let end_key = start_key.add(KEY_DIMENSION_SIZE);
-        let total_key_range = start_key..end_key;
-        let total_key_range_size = end_key.to_i128() - start_key.to_i128();
-        let total_start_lsn = Lsn(104);
-        let last_record_lsn = Lsn(504);
-
-        assert!(total_key_range_size % 3 == 0);
-
-        let in_memory_layers_shape = vec![
-            (total_key_range.clone(), Lsn(304)..Lsn(400)),
-            (total_key_range.clone(), Lsn(400)..last_record_lsn),
-        ];
-
-        let delta_layers_shape = vec![
-            (
-                start_key..(start_key.add((total_key_range_size / 3) as u32)),
-                Lsn(200)..Lsn(304),
-            ),
-            (
-                (start_key.add((total_key_range_size / 3) as u32))
-                    ..(start_key.add((total_key_range_size * 2 / 3) as u32)),
-                Lsn(200)..Lsn(304),
-            ),
-            (
-                (start_key.add((total_key_range_size * 2 / 3) as u32))
-                    ..(start_key.add(total_key_range_size as u32)),
-                Lsn(200)..Lsn(304),
-            ),
-        ];
-
-        let image_layers_shape = vec![
-            (
-                start_key.add((total_key_range_size * 2 / 3 - 10) as u32)
-                    ..start_key.add((total_key_range_size * 2 / 3 + 10) as u32),
-                Lsn(456),
-            ),
-            (
-                start_key.add((total_key_range_size / 3 - 10) as u32)
-                    ..start_key.add((total_key_range_size / 3 + 10) as u32),
-                Lsn(256),
-            ),
-            (total_key_range.clone(), total_start_lsn),
-        ];
-
-        let specification = TestTimelineSpecification {
-            start_lsn: total_start_lsn,
-            last_record_lsn,
-            in_memory_layers_shape,
-            delta_layers_shape,
-            image_layers_shape,
-            gap_chance,
-            will_init_chance,
-        };
-
-        // Create and randomly fill in the layers according to the specification
-        let (tline, storage, interesting_lsns) = randomize_timeline(
-            &tenant,
-            TIMELINE_ID,
-            DEFAULT_PG_VERSION,
-            specification,
-            &mut random,
-            &ctx,
-        )
-        .await?;
-
-        // Now generate queries based on the interesting lsns that we've collected.
-        //
-        // While there's still room in the query, pick and interesting LSN and a random
-        // key. Then roll the dice to see if the next key should also be included in
-        // the query. When the roll fails, break the "batch" and pick another point in the
-        // (key, LSN) space.
-
-        const PICK_NEXT_CHANCE: u8 = 50;
-        for _ in 0..queries {
-            let query = {
-                let mut keyspaces_at_lsn: HashMap<Lsn, KeySpaceRandomAccum> = HashMap::default();
-                let mut used_keys: HashSet<Key> = HashSet::default();
-
-                while used_keys.len() < Timeline::MAX_GET_VECTORED_KEYS as usize {
-                    let selected_lsn = interesting_lsns.choose(&mut random).expect("not empty");
-                    let mut selected_key = start_key.add(random.gen_range(0..KEY_DIMENSION_SIZE));
-
-                    while used_keys.len() < Timeline::MAX_GET_VECTORED_KEYS as usize {
-                        if used_keys.contains(&selected_key)
-                            || selected_key >= start_key.add(KEY_DIMENSION_SIZE)
-                        {
-                            break;
-                        }
-
-                        keyspaces_at_lsn
-                            .entry(*selected_lsn)
-                            .or_default()
-                            .add_key(selected_key);
-                        used_keys.insert(selected_key);
-
-                        let pick_next = random.gen_range(0..=100) <= PICK_NEXT_CHANCE;
-                        if pick_next {
-                            selected_key = selected_key.next();
-                        } else {
-                            break;
-                        }
-                    }
-                }
-
-                VersionedKeySpaceQuery::scattered(
-                    keyspaces_at_lsn
-                        .into_iter()
-                        .map(|(lsn, acc)| (lsn, acc.to_keyspace()))
-                        .collect(),
-                )
-            };
-
-            // Run the query and validate the results
-
-            let results = tline
-                .get_vectored(query.clone(), IoConcurrency::Sequential, &ctx)
-                .await;
-
-            let blobs = match results {
-                Ok(ok) => ok,
-                Err(err) => {
-                    panic!("seed={seed} Error returned for query {query}: {err}");
-                }
-            };
-
-            for (key, key_res) in blobs.into_iter() {
-                match key_res {
-                    Ok(blob) => {
-                        let requested_at_lsn = query.map_key_to_lsn(&key);
-                        let expected = storage.get(key, requested_at_lsn);
-
-                        if blob != expected {
-                            tracing::error!(
-                                "seed={seed} Mismatch for {key}@{requested_at_lsn} from query: {query}"
-                            );
-                        }
-
-                        assert_eq!(blob, expected);
-                    }
-                    Err(err) => {
-                        let requested_at_lsn = query.map_key_to_lsn(&key);
-
-                        panic!(
-                            "seed={seed} Error returned for {key}@{requested_at_lsn} from query {query}: {err}"
-                        );
-                    }
-                }
-            }
-        }
-
-        Ok(())
-    }
-
     fn sort_layer_key(k1: &PersistentLayerKey, k2: &PersistentLayerKey) -> std::cmp::Ordering {
         (
             k1.is_delta,

pageserver/src/tenant/ephemeral_file.rs

@@ -28,6 +28,11 @@ pub struct EphemeralFile {
     _timeline_id: TimelineId,
     page_cache_file_id: page_cache::FileId,
     bytes_written: u64,
+    // Always Some except during Drop
+    inner: Option<Inner>,
+}
+
+struct Inner {
     buffered_writer: owned_buffers_io::write::BufferedWriter<IoBufferMut, VirtualFile>,
     /// Gate guard is held on as long as we need to do operations in the path (delete on drop)
     _gate_guard: utils::sync::gate::GateGuard,
@@ -44,9 +49,9 @@ impl EphemeralFile {
         cancel: &CancellationToken,
         ctx: &RequestContext,
     ) -> anyhow::Result<EphemeralFile> {
-        static NEXT_FILENAME: AtomicU64 = AtomicU64::new(1);
+        static NEXT_TEMP_DISAMBIGUATOR: AtomicU64 = AtomicU64::new(1);
         let filename_disambiguator =
-            NEXT_FILENAME.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
+            NEXT_TEMP_DISAMBIGUATOR.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
 
         let filename = conf
             .timeline_path(&tenant_shard_id, &timeline_id)
@@ -73,34 +78,68 @@ impl EphemeralFile {
             _timeline_id: timeline_id,
             page_cache_file_id,
             bytes_written: 0,
-            buffered_writer: owned_buffers_io::write::BufferedWriter::new(
-                file,
-                || IoBufferMut::with_capacity(TAIL_SZ),
-                gate.enter()?,
-                cancel.child_token(),
-                ctx,
-                info_span!(parent: None, "ephemeral_file_buffered_writer", tenant_id=%tenant_shard_id.tenant_id, shard_id=%tenant_shard_id.shard_slug(), timeline_id=%timeline_id, path = %filename),
-            ),
-            _gate_guard: gate.enter()?,
+            inner: Some(Inner {
+                buffered_writer: owned_buffers_io::write::BufferedWriter::new(
+                    file,
+                    || IoBufferMut::with_capacity(TAIL_SZ),
+                    gate.enter()?,
+                    cancel.child_token(),
+                    ctx,
+                    info_span!(parent: None, "ephemeral_file_buffered_writer", tenant_id=%tenant_shard_id.tenant_id, shard_id=%tenant_shard_id.shard_slug(), timeline_id=%timeline_id, path = %filename),
+                ),
+                _gate_guard: gate.enter()?,
+            }),
         })
     }
+
+    fn buffered_writer(
+        &self,
+    ) -> &owned_buffers_io::write::BufferedWriter<IoBufferMut, VirtualFile> {
+        &self
+            .inner
+            .as_ref()
+            .expect("we never take out except during drop")
+            .buffered_writer
+    }
+    fn buffered_writer_mut(
+        &mut self,
+    ) -> &mut owned_buffers_io::write::BufferedWriter<IoBufferMut, VirtualFile> {
+        &mut self
+            .inner
+            .as_mut()
+            .expect("we never take out except during drop")
+            .buffered_writer
+    }
 }
 
 impl Drop for EphemeralFile {
     fn drop(&mut self) {
-        // unlink the file
-        // we are clear to do this, because we have entered a gate
-        let path = self.buffered_writer.as_inner().path();
-        let res = std::fs::remove_file(path);
-        if let Err(e) = res {
-            if e.kind() != std::io::ErrorKind::NotFound {
-                // just never log the not found errors, we cannot do anything for them; on detach
-                // the tenant directory is already gone.
-                //
-                // not found files might also be related to https://github.com/neondatabase/neon/issues/2442
-                error!("could not remove ephemeral file '{path}': {e}");
+        let inner = self.inner.take().expect("we never take out except here");
+
+        tokio::spawn(async move {
+            let Inner {
+                buffered_writer,
+                _gate_guard,
+            } = inner;
+
+            // XXX kinda ugly that we have this Arc here, would like to call VirtualFile::remove()
+            let virtual_file: Arc<VirtualFile> = buffered_writer.into_inner_no_flush().await;
+            let path = virtual_file.path();
+            let res = std::fs::remove_file(path);
+            if let Err(e) = res {
+                if e.kind() != std::io::ErrorKind::NotFound {
+                    // TODO: can we retry?
+
+                    // just never log the not found errors, we cannot do anything for them; on detach
+                    // the tenant directory is already gone.
+                    //
+                    // not found files might also be related to https://github.com/neondatabase/neon/issues/2442
+                    error!("could not remove ephemeral file '{path}': {e}");
+                }
             }
-        }
+
+            drop(_gate_guard);
+        });
     }
 }
 
@@ -168,7 +207,7 @@ impl EphemeralFile {
 
         // Write the payload
         let (nwritten, control) = self
-            .buffered_writer
+            .buffered_writer_mut()
             .write_buffered_borrowed_controlled(srcbuf, ctx)
             .await
             .map_err(|e| match e {
@@ -193,9 +232,9 @@ impl super::storage_layer::inmemory_layer::vectored_dio_read::File for Ephemeral
         dst: tokio_epoll_uring::Slice<B>,
         ctx: &RequestContext,
     ) -> std::io::Result<(tokio_epoll_uring::Slice<B>, usize)> {
-        let submitted_offset = self.buffered_writer.bytes_submitted();
+        let submitted_offset = self.buffered_writer().bytes_submitted();
 
-        let mutable = match self.buffered_writer.inspect_mutable() {
+        let mutable = match self.buffered_writer().inspect_mutable() {
             Some(mutable) => &mutable[0..mutable.pending()],
             None => {
                 // Timeline::cancel and hence buffered writer flush was cancelled.
@@ -204,7 +243,7 @@ impl super::storage_layer::inmemory_layer::vectored_dio_read::File for Ephemeral
             }
         };
 
-        let maybe_flushed = self.buffered_writer.inspect_maybe_flushed();
+        let maybe_flushed = self.buffered_writer().inspect_maybe_flushed();
 
         let dst_cap = dst.bytes_total().into_u64();
         let end = {
@@ -262,7 +301,7 @@ impl super::storage_layer::inmemory_layer::vectored_dio_read::File for Ephemeral
         let mutable_range = Range(std::cmp::max(start, submitted_offset), end);
 
         let dst = if written_range.len() > 0 {
-            let file: &VirtualFile = self.buffered_writer.as_inner();
+            let file: &VirtualFile = self.buffered_writer().as_inner();
             let bounds = dst.bounds();
             let slice = file
                 .read_exact_at(dst.slice(0..written_range.len().into_usize()), start, ctx)
@@ -419,7 +458,7 @@ mod tests {
             .await
             .unwrap();
 
-        let mutable = file.buffered_writer.mutable();
+        let mutable = file.buffered_writer().mutable();
         let cap = mutable.capacity();
         let align = mutable.align();
 
@@ -456,13 +495,13 @@ mod tests {
             assert_eq!(&buf, &content[range]);
         }
 
-        let file_contents = std::fs::read(file.buffered_writer.as_inner().path()).unwrap();
+        let file_contents = std::fs::read(file.buffered_writer().as_inner().path()).unwrap();
         assert!(file_contents == content[0..cap * 2]);
 
-        let maybe_flushed_buffer_contents = file.buffered_writer.inspect_maybe_flushed().unwrap();
+        let maybe_flushed_buffer_contents = file.buffered_writer().inspect_maybe_flushed().unwrap();
         assert_eq!(&maybe_flushed_buffer_contents[..], &content[cap..cap * 2]);
 
-        let mutable_buffer_contents = file.buffered_writer.mutable();
+        let mutable_buffer_contents = file.buffered_writer().mutable();
         assert_eq!(mutable_buffer_contents, &content[cap * 2..write_nbytes]);
     }
 
@@ -477,7 +516,7 @@ mod tests {
             .unwrap();
 
         // mutable buffer and maybe_flushed buffer each has `cap` bytes.
-        let cap = file.buffered_writer.mutable().capacity();
+        let cap = file.buffered_writer().mutable().capacity();
 
         let content: Vec<u8> = rand::thread_rng()
             .sample_iter(rand::distributions::Standard)
@@ -489,18 +528,18 @@ mod tests {
         // assert the state is as this test expects it to be
         let load_io_buf_res = file.load_to_io_buf(&ctx).await.unwrap();
         assert_eq!(&load_io_buf_res[..], &content[0..cap * 2 + cap / 2]);
-        let md = file.buffered_writer.as_inner().path().metadata().unwrap();
+        let md = file.buffered_writer().as_inner().path().metadata().unwrap();
         assert_eq!(
             md.len(),
             2 * cap.into_u64(),
             "buffered writer requires one write to be flushed if we write 2.5x buffer capacity"
         );
         assert_eq!(
-            &file.buffered_writer.inspect_maybe_flushed().unwrap()[0..cap],
+            &file.buffered_writer().inspect_maybe_flushed().unwrap()[0..cap],
             &content[cap..cap * 2]
         );
         assert_eq!(
-            &file.buffered_writer.mutable()[0..cap / 2],
+            &file.buffered_writer().mutable()[0..cap / 2],
             &content[cap * 2..cap * 2 + cap / 2]
         );
     }
@@ -522,7 +561,7 @@ mod tests {
             .await
             .unwrap();
 
-        let mutable = file.buffered_writer.mutable();
+        let mutable = file.buffered_writer().mutable();
         let cap = mutable.capacity();
         let align = mutable.align();
         let content: Vec<u8> = rand::thread_rng()

pageserver/src/tenant/storage_layer.rs

@@ -715,34 +715,13 @@ pub(crate) enum LayerId {
 }
 
 /// Uniquely identify a layer visit by the layer
-/// and LSN range of the reads. Note that the end of the range is exclusive.
-///
-/// The layer itself is not enough since we may have different LSN lower
-/// bounds for delta layer reads. Scenarios where this can happen are:
-///
-/// 1. Layer overlaps: imagine an image layer inside and in-memory layer
-///    and a query that only partially hits the image layer. Part of the query
-///    needs to read the whole in-memory layer and the other part needs to read
-///    only up to the image layer. Hence, they'll have different LSN floor values
-///    for the read.
-///
-/// 2. Scattered reads: the read path supports starting at different LSNs. Imagine
-///    The start LSN for one range is inside a layer and the start LSN for another range
-///    Is above the layer (includes all of it). Both ranges need to read the layer all the
-///    Way to the end but starting at different points. Hence, they'll have different LSN
-///    Ceil values.
-///
-/// The implication is that we might visit the same layer multiple times
-/// in order to read different LSN ranges from it. In practice, this isn't very concerning
-/// because:
-/// 1. Layer overlaps are rare and generally not intended
-/// 2. Scattered reads will stabilise after the first few layers provided their starting LSNs
-///    are grouped tightly enough (likely the case).
+/// and LSN floor (or start LSN) of the reads.
+/// The layer itself is not enough since we may
+/// have different LSN lower bounds for delta layer reads.
 #[derive(Debug, PartialEq, Eq, Clone, Hash)]
 struct LayerToVisitId {
     layer_id: LayerId,
     lsn_floor: Lsn,
-    lsn_ceil: Lsn,
 }
 
 #[derive(Debug, PartialEq, Eq, Hash)]
@@ -826,7 +805,6 @@ impl LayerFringe {
         let layer_to_visit_id = LayerToVisitId {
             layer_id: layer.id(),
             lsn_floor: lsn_range.start,
-            lsn_ceil: lsn_range.end,
         };
 
         let entry = self.visit_reads.entry(layer_to_visit_id.clone());

pageserver/src/tenant/storage_layer/delta_layer.rs

@@ -34,6 +34,7 @@ use std::ops::Range;
 use std::os::unix::fs::FileExt;
 use std::str::FromStr;
 use std::sync::Arc;
+use std::sync::atomic::AtomicU64;
 
 use anyhow::{Context, Result, bail, ensure};
 use camino::{Utf8Path, Utf8PathBuf};
@@ -45,8 +46,6 @@ use pageserver_api::keyspace::KeySpace;
 use pageserver_api::models::ImageCompressionAlgorithm;
 use pageserver_api::shard::TenantShardId;
 use pageserver_api::value::Value;
-use rand::Rng;
-use rand::distributions::Alphanumeric;
 use serde::{Deserialize, Serialize};
 use tokio::sync::OnceCell;
 use tokio_epoll_uring::IoBuf;
@@ -288,19 +287,19 @@ impl DeltaLayer {
         key_start: Key,
         lsn_range: &Range<Lsn>,
     ) -> Utf8PathBuf {
-        let rand_string: String = rand::thread_rng()
-            .sample_iter(&Alphanumeric)
-            .take(8)
-            .map(char::from)
-            .collect();
+        // Never reuse a filename in the lifetime of a pageserver process so that we need
+        // not worry about laggard Drop impl's async unlink hitting an already reused filename.
+        static NEXT_TEMP_DISAMBIGUATOR: AtomicU64 = AtomicU64::new(1);
+        let filename_disambiguator =
+            NEXT_TEMP_DISAMBIGUATOR.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
 
         conf.timeline_path(tenant_shard_id, timeline_id)
             .join(format!(
-                "{}-XXX__{:016X}-{:016X}.{}.{}",
+                "{}-XXX__{:016X}-{:016X}.{:x}.{}",
                 key_start,
                 u64::from(lsn_range.start),
                 u64::from(lsn_range.end),
-                rand_string,
+                filename_disambiguator,
                 TEMP_FILE_SUFFIX,
             ))
     }
@@ -395,6 +394,8 @@ struct DeltaLayerWriterInner {
 
     // Number of key-lsns in the layer.
     num_keys: usize,
+
+    _gate_guard: utils::sync::gate::GateGuard,
 }
 
 impl DeltaLayerWriterInner {
@@ -439,6 +440,7 @@ impl DeltaLayerWriterInner {
             tree: tree_builder,
             blob_writer,
             num_keys: 0,
+            _gate_guard: gate.enter()?,
         })
     }
 
@@ -728,12 +730,22 @@ impl DeltaLayerWriter {
 
 impl Drop for DeltaLayerWriter {
     fn drop(&mut self) {
-        if let Some(inner) = self.inner.take() {
-            // We want to remove the virtual file here, so it's fine to not
-            // having completely flushed unwritten data.
-            let vfile = inner.blob_writer.into_inner_no_flush();
+        let Some(inner) = self.inner.take() else {
+            return;
+        };
+
+        tokio::spawn(async move {
+            let DeltaLayerWriterInner {
+                blob_writer,
+                _gate_guard,
+                ..
+            } = inner;
+
+            let vfile = blob_writer.into_inner_no_flush();
             vfile.remove();
-        }
+
+            drop(_gate_guard);
+        });
     }
 }
 
@@ -1609,8 +1621,8 @@ pub(crate) mod test {
     use bytes::Bytes;
     use itertools::MinMaxResult;
     use pageserver_api::value::Value;
-    use rand::RngCore;
     use rand::prelude::{SeedableRng, SliceRandom, StdRng};
+    use rand::{Rng, RngCore};
 
     use super::*;
     use crate::DEFAULT_PG_VERSION;

pageserver/src/tenant/storage_layer/image_layer.rs

@@ -32,6 +32,7 @@ use std::ops::Range;
 use std::os::unix::prelude::FileExt;
 use std::str::FromStr;
 use std::sync::Arc;
+use std::sync::atomic::AtomicU64;
 
 use anyhow::{Context, Result, bail, ensure};
 use bytes::Bytes;
@@ -43,8 +44,6 @@ use pageserver_api::key::{DBDIR_KEY, KEY_SIZE, Key};
 use pageserver_api::keyspace::KeySpace;
 use pageserver_api::shard::{ShardIdentity, TenantShardId};
 use pageserver_api::value::Value;
-use rand::Rng;
-use rand::distributions::Alphanumeric;
 use serde::{Deserialize, Serialize};
 use tokio::sync::OnceCell;
 use tokio_stream::StreamExt;
@@ -252,14 +251,17 @@ impl ImageLayer {
         tenant_shard_id: TenantShardId,
         fname: &ImageLayerName,
     ) -> Utf8PathBuf {
-        let rand_string: String = rand::thread_rng()
-            .sample_iter(&Alphanumeric)
-            .take(8)
-            .map(char::from)
-            .collect();
+        // Never reuse a filename in the lifetime of a pageserver process so that we need
+        // not worry about laggard Drop impl's async unlink hitting an already reused filename.
+        static NEXT_TEMP_DISAMBIGUATOR: AtomicU64 = AtomicU64::new(1);
+        let filename_disambiguator =
+            NEXT_TEMP_DISAMBIGUATOR.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
 
         conf.timeline_path(&tenant_shard_id, &timeline_id)
-            .join(format!("{fname}.{rand_string}.{TEMP_FILE_SUFFIX}"))
+            .join(format!(
+                "{fname}.{:x}.{TEMP_FILE_SUFFIX}",
+                filename_disambiguator
+            ))
     }
 
     ///
@@ -743,6 +745,8 @@ struct ImageLayerWriterInner {
 
     #[cfg(feature = "testing")]
     last_written_key: Key,
+
+    _gate_guard: utils::sync::gate::GateGuard,
 }
 
 impl ImageLayerWriterInner {
@@ -805,6 +809,7 @@ impl ImageLayerWriterInner {
             num_keys: 0,
             #[cfg(feature = "testing")]
             last_written_key: Key::MIN,
+            _gate_guard: gate.enter()?,
         };
 
         Ok(writer)
@@ -1066,9 +1071,22 @@ impl ImageLayerWriter {
 
 impl Drop for ImageLayerWriter {
     fn drop(&mut self) {
-        if let Some(inner) = self.inner.take() {
-            inner.blob_writer.into_inner().remove();
-        }
+        let Some(inner) = self.inner.take() else {
+            return;
+        };
+
+        tokio::spawn(async move {
+            let ImageLayerWriterInner {
+                blob_writer,
+                _gate_guard,
+                ..
+            } = inner;
+
+            let vfile = blob_writer.into_inner();
+            vfile.remove();
+
+            drop(_gate_guard);
+        });
     }
 }
 

pageserver/src/tenant/timeline.rs

@@ -585,7 +585,7 @@ pub(crate) enum PageReconstructError {
     WalRedo(anyhow::Error),
 
     #[error("{0}")]
-    MissingKey(Box<MissingKeyError>),
+    MissingKey(MissingKeyError),
 }
 
 impl From<anyhow::Error> for PageReconstructError {
@@ -690,23 +690,16 @@ impl std::fmt::Display for ReadPath {
 
 #[derive(thiserror::Error)]
 pub struct MissingKeyError {
-    keyspace: KeySpace,
+    key: Key,
     shard: ShardNumber,
-    query: Option<VersionedKeySpaceQuery>,
-    // This is largest request LSN from the get page request batch
-    original_hwm_lsn: Lsn,
+    cont_lsn: Lsn,
+    request_lsn: Lsn,
     ancestor_lsn: Option<Lsn>,
     /// Debug information about the read path if there's an error
     read_path: Option<ReadPath>,
     backtrace: Option<std::backtrace::Backtrace>,
 }
 
-impl MissingKeyError {
-    fn enrich(&mut self, query: VersionedKeySpaceQuery) {
-        self.query = Some(query);
-    }
-}
-
 impl std::fmt::Debug for MissingKeyError {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         write!(f, "{}", self)
@@ -717,18 +710,14 @@ impl std::fmt::Display for MissingKeyError {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         write!(
             f,
-            "could not find data for key {} (shard {:?}), original HWM LSN {}",
-            self.keyspace, self.shard, self.original_hwm_lsn
+            "could not find data for key {} (shard {:?}) at LSN {}, request LSN {}",
+            self.key, self.shard, self.cont_lsn, self.request_lsn
         )?;
 
         if let Some(ref ancestor_lsn) = self.ancestor_lsn {
             write!(f, ", ancestor {}", ancestor_lsn)?;
         }
 
-        if let Some(ref query) = self.query {
-            write!(f, ", query {}", query)?;
-        }
-
         if let Some(ref read_path) = self.read_path {
             write!(f, "\n{}", read_path)?;
         }
@@ -828,7 +817,7 @@ pub(crate) enum GetVectoredError {
     InvalidLsn(Lsn),
 
     #[error("requested key not found: {0}")]
-    MissingKey(Box<MissingKeyError>),
+    MissingKey(MissingKeyError),
 
     #[error("ancestry walk")]
     GetReadyAncestorError(#[source] GetReadyAncestorError),
@@ -939,7 +928,7 @@ impl std::fmt::Debug for Timeline {
     }
 }
 
-#[derive(thiserror::Error, Debug, Clone)]
+#[derive(thiserror::Error, Debug)]
 pub(crate) enum WaitLsnError {
     // Called on a timeline which is shutting down
     #[error("Shutdown")]
@@ -1139,12 +1128,14 @@ impl Timeline {
         // page_service.
         debug_assert!(!self.shard_identity.is_key_disposable(&key));
 
+        let keyspace = KeySpace {
+            ranges: vec![key..key.next()],
+        };
+
         let mut reconstruct_state = ValuesReconstructState::new(IoConcurrency::sequential());
 
-        let query = VersionedKeySpaceQuery::uniform(KeySpace::single(key..key.next()), lsn);
-
         let vectored_res = self
-            .get_vectored_impl(query, &mut reconstruct_state, ctx)
+            .get_vectored_impl(keyspace.clone(), lsn, &mut reconstruct_state, ctx)
             .await;
 
         let key_value = vectored_res?.pop_first();
@@ -1162,17 +1153,15 @@ impl Timeline {
                     value
                 }
             }
-            None => Err(PageReconstructError::MissingKey(Box::new(
-                MissingKeyError {
-                    keyspace: KeySpace::single(key..key.next()),
-                    shard: self.shard_identity.get_shard_number(&key),
-                    original_hwm_lsn: lsn,
-                    ancestor_lsn: None,
-                    backtrace: None,
-                    read_path: None,
-                    query: None,
-                },
-            ))),
+            None => Err(PageReconstructError::MissingKey(MissingKeyError {
+                key,
+                shard: self.shard_identity.get_shard_number(&key),
+                cont_lsn: Lsn(0),
+                request_lsn: lsn,
+                ancestor_lsn: None,
+                backtrace: None,
+                read_path: None,
+            })),
         }
     }
 
@@ -1185,18 +1174,21 @@ impl Timeline {
     /// which actually vectorizes the read path.
     pub(crate) async fn get_vectored(
         &self,
-        query: VersionedKeySpaceQuery,
+        keyspace: KeySpace,
+        lsn: Lsn,
         io_concurrency: super::storage_layer::IoConcurrency,
         ctx: &RequestContext,
     ) -> Result<BTreeMap<Key, Result<Bytes, PageReconstructError>>, GetVectoredError> {
-        let total_keyspace = query.total_keyspace();
+        if !lsn.is_valid() {
+            return Err(GetVectoredError::InvalidLsn(lsn));
+        }
 
-        let key_count = total_keyspace.total_raw_size().try_into().unwrap();
+        let key_count = keyspace.total_raw_size().try_into().unwrap();
         if key_count > Timeline::MAX_GET_VECTORED_KEYS {
             return Err(GetVectoredError::Oversized(key_count));
         }
 
-        for range in &total_keyspace.ranges {
+        for range in &keyspace.ranges {
             let mut key = range.start;
             while key != range.end {
                 assert!(!self.shard_identity.is_key_disposable(&key));
@@ -1205,8 +1197,9 @@ impl Timeline {
         }
 
         trace!(
-            "get vectored query {} from task kind {:?}",
-            query,
+            "get vectored request for {:?}@{} from task kind {:?}",
+            keyspace,
+            lsn,
             ctx.task_kind(),
         );
 
@@ -1215,7 +1208,12 @@ impl Timeline {
             .map(|metric| (metric, Instant::now()));
 
         let res = self
-            .get_vectored_impl(query, &mut ValuesReconstructState::new(io_concurrency), ctx)
+            .get_vectored_impl(
+                keyspace.clone(),
+                lsn,
+                &mut ValuesReconstructState::new(io_concurrency),
+                ctx,
+            )
             .await;
 
         if let Some((metric, start)) = start {
@@ -1266,10 +1264,13 @@ impl Timeline {
             .for_task_kind(ctx.task_kind())
             .map(ScanLatencyOngoingRecording::start_recording);
 
-        let query = VersionedKeySpaceQuery::uniform(keyspace, lsn);
-
         let vectored_res = self
-            .get_vectored_impl(query, &mut ValuesReconstructState::new(io_concurrency), ctx)
+            .get_vectored_impl(
+                keyspace.clone(),
+                lsn,
+                &mut ValuesReconstructState::new(io_concurrency),
+                ctx,
+            )
             .await;
 
         if let Some(recording) = start {
@@ -1281,19 +1282,16 @@ impl Timeline {
 
     pub(super) async fn get_vectored_impl(
         &self,
-        query: VersionedKeySpaceQuery,
+        keyspace: KeySpace,
+        lsn: Lsn,
         reconstruct_state: &mut ValuesReconstructState,
         ctx: &RequestContext,
     ) -> Result<BTreeMap<Key, Result<Bytes, PageReconstructError>>, GetVectoredError> {
         let read_path = if self.conf.enable_read_path_debugging || ctx.read_path_debug() {
-            Some(ReadPath::new(
-                query.total_keyspace(),
-                query.high_watermark_lsn()?,
-            ))
+            Some(ReadPath::new(keyspace.clone(), lsn))
         } else {
             None
         };
-
         reconstruct_state.read_path = read_path;
 
         let redo_attempt_type = if ctx.task_kind() == TaskKind::Compaction {
@@ -1313,7 +1311,7 @@ impl Timeline {
                 })
                 .attached_child();
 
-            self.get_vectored_reconstruct_data(query.clone(), reconstruct_state, &ctx)
+            self.get_vectored_reconstruct_data(keyspace.clone(), lsn, reconstruct_state, &ctx)
                 .maybe_perf_instrument(&ctx, |crnt_perf_span| crnt_perf_span.clone())
                 .await
         };
@@ -1326,13 +1324,6 @@ impl Timeline {
                 .map(|state| state.collect_pending_ios())
                 .collect::<FuturesUnordered<_>>();
             while collect_futs.next().await.is_some() {}
-
-            // Enrich the missing key error with the original query.
-            if let GetVectoredError::MissingKey(mut missing_err) = err {
-                missing_err.enrich(query.clone());
-                return Err(GetVectoredError::MissingKey(missing_err));
-            }
-
             return Err(err);
         };
 
@@ -1350,8 +1341,6 @@ impl Timeline {
 
         let futs = FuturesUnordered::new();
         for (key, state) in std::mem::take(&mut reconstruct_state.keys) {
-            let req_lsn_for_key = query.map_key_to_lsn(&key);
-
             futs.push({
                 let walredo_self = self.myself.upgrade().expect("&self method holds the arc");
                 let ctx = RequestContextBuilder::from(&ctx)
@@ -1398,7 +1387,7 @@ impl Timeline {
 
                     let walredo_deltas = converted.num_deltas();
                     let walredo_res = walredo_self
-                        .reconstruct_value(key, req_lsn_for_key, converted, redo_attempt_type)
+                        .reconstruct_value(key, lsn, converted, redo_attempt_type)
                         .maybe_perf_instrument(&ctx, |crnt_perf_span| {
                             info_span!(
                                 target: PERF_TRACE_TARGET,
@@ -1425,18 +1414,15 @@ impl Timeline {
         // to avoid infinite results.
         if !results.is_empty() {
             if layers_visited >= Self::LAYERS_VISITED_WARN_THRESHOLD {
-                let total_keyspace = query.total_keyspace();
-                let max_request_lsn = query.high_watermark_lsn().expect("Validated previously");
-
                 static LOG_PACER: Lazy<Mutex<RateLimit>> =
                     Lazy::new(|| Mutex::new(RateLimit::new(Duration::from_secs(60))));
                 LOG_PACER.lock().unwrap().call(|| {
-                    let num_keys = total_keyspace.total_raw_size();
+                    let num_keys = keyspace.total_raw_size();
                     let num_pages = results.len();
                     tracing::info!(
                       shard_id = %self.tenant_shard_id.shard_slug(),
-                      lsn = %max_request_lsn,
-                      "Vectored read for {total_keyspace} visited {layers_visited} layers. Returned {num_pages}/{num_keys} pages.",
+                      lsn = %lsn,
+                      "Vectored read for {keyspace} visited {layers_visited} layers. Returned {num_pages}/{num_keys} pages.",
                     );
                 });
             }
@@ -2737,10 +2723,6 @@ impl Timeline {
             .tenant_conf
             .gc_compaction_enabled
             .unwrap_or(self.conf.default_tenant_conf.gc_compaction_enabled);
-        let gc_compaction_verification = tenant_conf
-            .tenant_conf
-            .gc_compaction_verification
-            .unwrap_or(self.conf.default_tenant_conf.gc_compaction_verification);
         let gc_compaction_initial_threshold_kb = tenant_conf
             .tenant_conf
             .gc_compaction_initial_threshold_kb
@@ -2755,7 +2737,6 @@ impl Timeline {
             .unwrap_or(self.conf.default_tenant_conf.gc_compaction_ratio_percent);
         GcCompactionCombinedSettings {
             gc_compaction_enabled,
-            gc_compaction_verification,
             gc_compaction_initial_threshold_kb,
             gc_compaction_ratio_percent,
         }
@@ -3954,154 +3935,6 @@ impl Timeline {
     }
 }
 
-#[derive(Clone)]
-/// Type representing a query in the ([`Lsn`], [`Key`]) space.
-/// In other words, a set of segments in a 2D space.
-///
-/// This representation has the advatange of avoiding hash map
-/// allocations for uniform queries.
-pub(crate) enum VersionedKeySpaceQuery {
-    /// Variant for queries at a single [`Lsn`]
-    Uniform { keyspace: KeySpace, lsn: Lsn },
-    /// Variant for queries at multiple [`Lsn`]s
-    Scattered {
-        keyspaces_at_lsn: Vec<(Lsn, KeySpace)>,
-    },
-}
-
-impl VersionedKeySpaceQuery {
-    pub(crate) fn uniform(keyspace: KeySpace, lsn: Lsn) -> Self {
-        Self::Uniform { keyspace, lsn }
-    }
-
-    pub(crate) fn scattered(keyspaces_at_lsn: Vec<(Lsn, KeySpace)>) -> Self {
-        Self::Scattered { keyspaces_at_lsn }
-    }
-
-    /// Returns the most recent (largest) LSN included in the query.
-    /// If any of the LSNs included in the query are invalid, returns
-    /// an error instead.
-    fn high_watermark_lsn(&self) -> Result<Lsn, GetVectoredError> {
-        match self {
-            Self::Uniform { lsn, .. } => {
-                if !lsn.is_valid() {
-                    return Err(GetVectoredError::InvalidLsn(*lsn));
-                }
-
-                Ok(*lsn)
-            }
-            Self::Scattered { keyspaces_at_lsn } => {
-                let mut max_lsn = None;
-                for (lsn, _keyspace) in keyspaces_at_lsn.iter() {
-                    if !lsn.is_valid() {
-                        return Err(GetVectoredError::InvalidLsn(*lsn));
-                    }
-                    max_lsn = std::cmp::max(max_lsn, Some(lsn));
-                }
-
-                if let Some(computed) = max_lsn {
-                    Ok(*computed)
-                } else {
-                    Err(GetVectoredError::Other(anyhow!("empty input")))
-                }
-            }
-        }
-    }
-
-    /// Returns the total keyspace being queried: the result of projecting
-    /// everything in the key dimensions onto the key axis.
-    fn total_keyspace(&self) -> KeySpace {
-        match self {
-            Self::Uniform { keyspace, .. } => keyspace.clone(),
-            Self::Scattered { keyspaces_at_lsn } => keyspaces_at_lsn
-                .iter()
-                .map(|(_lsn, keyspace)| keyspace)
-                .fold(KeySpace::default(), |mut acc, v| {
-                    acc.merge(v);
-                    acc
-                }),
-        }
-    }
-
-    /// Returns LSN for a specific key.
-    ///
-    /// Invariant: requested key must be part of [`Self::total_keyspace`]
-    pub(super) fn map_key_to_lsn(&self, key: &Key) -> Lsn {
-        match self {
-            Self::Uniform { lsn, .. } => *lsn,
-            Self::Scattered { keyspaces_at_lsn } => {
-                keyspaces_at_lsn
-                    .iter()
-                    .find(|(_lsn, keyspace)| keyspace.contains(key))
-                    .expect("Returned key was requested")
-                    .0
-            }
-        }
-    }
-
-    /// Remove any parts of the query (segments) which overlap with the provided
-    /// key space (also segments).
-    fn remove_overlapping_with(&mut self, to_remove: &KeySpace) -> KeySpace {
-        match self {
-            Self::Uniform { keyspace, .. } => keyspace.remove_overlapping_with(to_remove),
-            Self::Scattered { keyspaces_at_lsn } => {
-                let mut removed_accum = KeySpaceRandomAccum::new();
-                keyspaces_at_lsn.iter_mut().for_each(|(_lsn, keyspace)| {
-                    let removed = keyspace.remove_overlapping_with(to_remove);
-                    removed_accum.add_keyspace(removed);
-                });
-
-                removed_accum.to_keyspace()
-            }
-        }
-    }
-
-    fn is_empty(&self) -> bool {
-        match self {
-            Self::Uniform { keyspace, .. } => keyspace.is_empty(),
-            Self::Scattered { keyspaces_at_lsn } => keyspaces_at_lsn
-                .iter()
-                .all(|(_lsn, keyspace)| keyspace.is_empty()),
-        }
-    }
-
-    /// "Lower" the query on the LSN dimension
-    fn lower(&mut self, to: Lsn) {
-        match self {
-            Self::Uniform { lsn, .. } => {
-                // If the originally requested LSN is smaller than the starting
-                // LSN of the ancestor we are descending into, we need to respect that.
-                // Hence the min.
-                *lsn = std::cmp::min(*lsn, to);
-            }
-            Self::Scattered { keyspaces_at_lsn } => {
-                keyspaces_at_lsn.iter_mut().for_each(|(lsn, _keyspace)| {
-                    *lsn = std::cmp::min(*lsn, to);
-                });
-            }
-        }
-    }
-}
-
-impl std::fmt::Display for VersionedKeySpaceQuery {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "[")?;
-
-        match self {
-            VersionedKeySpaceQuery::Uniform { keyspace, lsn } => {
-                write!(f, "{keyspace} @ {lsn}")?;
-            }
-            VersionedKeySpaceQuery::Scattered { keyspaces_at_lsn } => {
-                for (lsn, keyspace) in keyspaces_at_lsn.iter() {
-                    write!(f, "{keyspace} @ {lsn},")?;
-                }
-            }
-        }
-
-        write!(f, "]")
-    }
-}
-
 impl Timeline {
     #[allow(clippy::doc_lazy_continuation)]
     /// Get the data needed to reconstruct all keys in the provided keyspace
@@ -4116,15 +3949,16 @@ impl Timeline {
     /// 2.4. If the fringe is empty, go back to 1
     async fn get_vectored_reconstruct_data(
         &self,
-        mut query: VersionedKeySpaceQuery,
+        mut keyspace: KeySpace,
+        request_lsn: Lsn,
         reconstruct_state: &mut ValuesReconstructState,
         ctx: &RequestContext,
     ) -> Result<(), GetVectoredError> {
-        let original_hwm_lsn = query.high_watermark_lsn().unwrap();
-
         let mut timeline_owned: Arc<Timeline>;
         let mut timeline = self;
 
+        let mut cont_lsn = Lsn(request_lsn.0 + 1);
+
         let missing_keyspace = loop {
             if self.cancel.is_cancelled() {
                 return Err(GetVectoredError::Cancelled);
@@ -4141,14 +3975,15 @@ impl Timeline {
                             parent: crnt_perf_span,
                             "PLAN_IO_TIMELINE",
                             timeline = %timeline.timeline_id,
-                            high_watermark_lsn = %query.high_watermark_lsn().unwrap(),
+                            lsn = %cont_lsn,
                         )
                     })
                     .attached_child();
 
                 Self::get_vectored_reconstruct_data_timeline(
                     timeline,
-                    &query,
+                    keyspace.clone(),
+                    cont_lsn,
                     reconstruct_state,
                     &self.cancel,
                     &ctx,
@@ -4157,23 +3992,23 @@ impl Timeline {
                 .await?
             };
 
-            query.remove_overlapping_with(&completed);
+            keyspace.remove_overlapping_with(&completed);
 
             // Do not descend into the ancestor timeline for aux files.
             // We don't return a blanket [`GetVectoredError::MissingKey`] to avoid
             // stalling compaction.
-            query.remove_overlapping_with(&KeySpace {
+            keyspace.remove_overlapping_with(&KeySpace {
                 ranges: vec![NON_INHERITED_RANGE, Key::sparse_non_inherited_keyspace()],
             });
 
             // Keyspace is fully retrieved
-            if query.is_empty() {
+            if keyspace.is_empty() {
                 break None;
             }
 
             let Some(ancestor_timeline) = timeline.ancestor_timeline.as_ref() else {
                 // Not fully retrieved but no ancestor timeline.
-                break Some(query.total_keyspace());
+                break Some(keyspace);
             };
 
             // Now we see if there are keys covered by the image layer but does not exist in the
@@ -4184,7 +4019,7 @@ impl Timeline {
             // keys from `keyspace`, we expect there to be no overlap between it and the image covered key
             // space. If that's not the case, we had at least one key encounter a gap in the image layer
             // and stop the search as a result of that.
-            let mut removed = query.remove_overlapping_with(&image_covered_keyspace);
+            let mut removed = keyspace.remove_overlapping_with(&image_covered_keyspace);
             // Do not fire missing key error and end early for sparse keys. Note that we hava already removed
             // non-inherited keyspaces before, so we can safely do a full `SPARSE_RANGE` remove instead of
             // figuring out what is the inherited key range and do a fine-grained pruning.
@@ -4194,11 +4029,11 @@ impl Timeline {
             if !removed.is_empty() {
                 break Some(removed);
             }
+            // If we reached this point, `remove_overlapping_with` should not have made any change to the
+            // keyspace.
 
-            // Each key range in the original query is at some point in the LSN space.
-            // When descending into the ancestor, lower all ranges in the LSN space
-            // such that new changes on the parent timeline are not visible.
-            query.lower(timeline.ancestor_lsn);
+            // Take the min to avoid reconstructing a page with data newer than request Lsn.
+            cont_lsn = std::cmp::min(Lsn(request_lsn.0 + 1), Lsn(timeline.ancestor_lsn.0 + 1));
 
             let ctx = RequestContextBuilder::from(ctx)
                 .perf_span(|crnt_perf_span| {
@@ -4207,6 +4042,7 @@ impl Timeline {
                         parent: crnt_perf_span,
                         "GET_ANCESTOR",
                         timeline = %timeline.timeline_id,
+                        lsn = %cont_lsn,
                         ancestor = %ancestor_timeline.timeline_id,
                         ancestor_lsn = %timeline.ancestor_lsn
                     )
@@ -4236,47 +4072,22 @@ impl Timeline {
         };
 
         if let Some(missing_keyspace) = missing_keyspace {
-            return Err(GetVectoredError::MissingKey(Box::new(MissingKeyError {
-                keyspace: missing_keyspace, /* better if we can store the full keyspace */
-                shard: self.shard_identity.number,
-                original_hwm_lsn,
+            return Err(GetVectoredError::MissingKey(MissingKeyError {
+                key: missing_keyspace.start().unwrap(), /* better if we can store the full keyspace */
+                shard: self
+                    .shard_identity
+                    .get_shard_number(&missing_keyspace.start().unwrap()),
+                cont_lsn,
+                request_lsn,
                 ancestor_lsn: Some(timeline.ancestor_lsn),
                 backtrace: None,
                 read_path: std::mem::take(&mut reconstruct_state.read_path),
-                query: None,
-            })));
+            }));
         }
 
         Ok(())
     }
 
-    async fn get_vectored_init_fringe(
-        &self,
-        query: &VersionedKeySpaceQuery,
-    ) -> Result<LayerFringe, GetVectoredError> {
-        let mut fringe = LayerFringe::new();
-        let guard = self.layers.read().await;
-
-        match query {
-            VersionedKeySpaceQuery::Uniform { keyspace, lsn } => {
-                // LSNs requested by the compute or determined by the pageserver
-                // are inclusive. Queries to the layer map use exclusive LSNs.
-                // Hence, bump the value before the query - same in the other
-                // match arm.
-                let cont_lsn = Lsn(lsn.0 + 1);
-                guard.update_search_fringe(keyspace, cont_lsn, &mut fringe)?;
-            }
-            VersionedKeySpaceQuery::Scattered { keyspaces_at_lsn } => {
-                for (lsn, keyspace) in keyspaces_at_lsn.iter() {
-                    let cont_lsn_for_keyspace = Lsn(lsn.0 + 1);
-                    guard.update_search_fringe(keyspace, cont_lsn_for_keyspace, &mut fringe)?;
-                }
-            }
-        }
-
-        Ok(fringe)
-    }
-
     /// Collect the reconstruct data for a keyspace from the specified timeline.
     ///
     /// Maintain a fringe [`LayerFringe`] which tracks all the layers that intersect
@@ -4295,7 +4106,8 @@ impl Timeline {
     /// decides how to deal with these two keyspaces.
     async fn get_vectored_reconstruct_data_timeline(
         timeline: &Timeline,
-        query: &VersionedKeySpaceQuery,
+        keyspace: KeySpace,
+        mut cont_lsn: Lsn,
         reconstruct_state: &mut ValuesReconstructState,
         cancel: &CancellationToken,
         ctx: &RequestContext,
@@ -4311,7 +4123,14 @@ impl Timeline {
         let _guard = timeline.gc_compaction_layer_update_lock.read().await;
 
         // Initialize the fringe
-        let mut fringe = timeline.get_vectored_init_fringe(query).await?;
+        let mut fringe = {
+            let mut fringe = LayerFringe::new();
+
+            let guard = timeline.layers.read().await;
+            guard.update_search_fringe(&keyspace, cont_lsn, &mut fringe)?;
+
+            fringe
+        };
 
         let mut completed_keyspace = KeySpace::default();
         let mut image_covered_keyspace = KeySpaceRandomAccum::new();
@@ -4337,7 +4156,7 @@ impl Timeline {
                 .await?;
 
             let mut unmapped_keyspace = keyspace_to_read;
-            let cont_lsn = next_cont_lsn;
+            cont_lsn = next_cont_lsn;
 
             reconstruct_state.on_layer_visited(&layer_to_read);
 
@@ -5178,11 +4997,13 @@ impl Timeline {
                 if key_request_accum.raw_size() >= Timeline::MAX_GET_VECTORED_KEYS
                     || (last_key_in_range && key_request_accum.raw_size() > 0)
                 {
-                    let query =
-                        VersionedKeySpaceQuery::uniform(key_request_accum.consume_keyspace(), lsn);
-
                     let results = self
-                        .get_vectored(query, io_concurrency.clone(), ctx)
+                        .get_vectored(
+                            key_request_accum.consume_keyspace(),
+                            lsn,
+                            io_concurrency.clone(),
+                            ctx,
+                        )
                         .await?;
 
                     if self.cancel.is_cancelled() {
@@ -5271,11 +5092,7 @@ impl Timeline {
         // Directly use `get_vectored_impl` to skip the max_vectored_read_key limit check. Note that the keyspace should
         // not contain too many keys, otherwise this takes a lot of memory.
         let data = self
-            .get_vectored_impl(
-                VersionedKeySpaceQuery::uniform(partition.clone(), lsn),
-                &mut reconstruct_state,
-                ctx,
-            )
+            .get_vectored_impl(partition.clone(), lsn, &mut reconstruct_state, ctx)
             .await?;
         let (data, total_kb_retrieved, total_keys_retrieved) = {
             let mut new_data = BTreeMap::new();

pageserver/src/tenant/timeline/compaction.rs

@@ -80,7 +80,6 @@ impl std::fmt::Display for GcCompactionJobId {
 
 pub struct GcCompactionCombinedSettings {
     pub gc_compaction_enabled: bool,
-    pub gc_compaction_verification: bool,
     pub gc_compaction_initial_threshold_kb: u64,
     pub gc_compaction_ratio_percent: u64,
 }
@@ -226,7 +225,6 @@ impl GcCompactionQueue {
             gc_compaction_enabled,
             gc_compaction_initial_threshold_kb,
             gc_compaction_ratio_percent,
-            ..
         } = timeline.get_gc_compaction_settings();
         if !gc_compaction_enabled {
             return Ok(());
@@ -790,114 +788,6 @@ impl KeyHistoryRetention {
         }
         Ok(())
     }
-
-    /// Verify if every key in the retention is readable by replaying the logs.
-    async fn verify(
-        &self,
-        key: Key,
-        base_img_from_ancestor: &Option<(Key, Lsn, Bytes)>,
-        full_history: &[(Key, Lsn, Value)],
-        tline: &Arc<Timeline>,
-    ) -> anyhow::Result<()> {
-        // Usually the min_lsn should be the first record but we do a full iteration to be safe.
-        let Some(min_lsn) = full_history.iter().map(|(_, lsn, _)| *lsn).min() else {
-            // This should never happen b/c if we don't have any history of a key, we won't even do `generate_key_retention`.
-            return Ok(());
-        };
-        let Some(max_lsn) = full_history.iter().map(|(_, lsn, _)| *lsn).max() else {
-            // This should never happen b/c if we don't have any history of a key, we won't even do `generate_key_retention`.
-            return Ok(());
-        };
-        let mut base_img = base_img_from_ancestor
-            .as_ref()
-            .map(|(_, lsn, img)| (*lsn, img));
-        let mut history = Vec::new();
-
-        async fn collect_and_verify(
-            key: Key,
-            lsn: Lsn,
-            base_img: &Option<(Lsn, &Bytes)>,
-            history: &[(Lsn, &NeonWalRecord)],
-            tline: &Arc<Timeline>,
-        ) -> anyhow::Result<()> {
-            let mut records = history
-                .iter()
-                .map(|(lsn, val)| (*lsn, (*val).clone()))
-                .collect::<Vec<_>>();
-
-            // WAL redo requires records in the reverse LSN order
-            records.reverse();
-            let data = ValueReconstructState {
-                img: base_img.as_ref().map(|(lsn, img)| (*lsn, (*img).clone())),
-                records,
-            };
-
-            tline
-                .reconstruct_value(key, lsn, data, RedoAttemptType::GcCompaction)
-                .await
-                .with_context(|| format!("verification failed for key {} at lsn {}", key, lsn))?;
-
-            Ok(())
-        }
-
-        for (retain_lsn, KeyLogAtLsn(logs)) in &self.below_horizon {
-            for (lsn, val) in logs {
-                match val {
-                    Value::Image(img) => {
-                        base_img = Some((*lsn, img));
-                        history.clear();
-                    }
-                    Value::WalRecord(rec) if val.will_init() => {
-                        base_img = None;
-                        history.clear();
-                        history.push((*lsn, rec));
-                    }
-                    Value::WalRecord(rec) => {
-                        history.push((*lsn, rec));
-                    }
-                }
-            }
-            if *retain_lsn >= min_lsn {
-                // Only verify after the key appears in the full history for the first time.
-
-                if base_img.is_none() && history.is_empty() {
-                    anyhow::bail!(
-                        "verificatoin failed: key {} has no history at {}",
-                        key,
-                        retain_lsn
-                    );
-                };
-                // We don't modify history: in theory, we could replace the history with a single
-                // image as in `generate_key_retention` to make redos at later LSNs faster. But we
-                // want to verify everything as if they are read from the real layer map.
-                collect_and_verify(key, *retain_lsn, &base_img, &history, tline).await?;
-            }
-        }
-
-        for (lsn, val) in &self.above_horizon.0 {
-            match val {
-                Value::Image(img) => {
-                    // Above the GC horizon, we verify every time we see an image.
-                    collect_and_verify(key, *lsn, &base_img, &history, tline).await?;
-                    base_img = Some((*lsn, img));
-                    history.clear();
-                }
-                Value::WalRecord(rec) if val.will_init() => {
-                    // Above the GC horizon, we verify every time we see an init record.
-                    collect_and_verify(key, *lsn, &base_img, &history, tline).await?;
-                    base_img = None;
-                    history.clear();
-                    history.push((*lsn, rec));
-                }
-                Value::WalRecord(rec) => {
-                    history.push((*lsn, rec));
-                }
-            }
-        }
-        // Ensure the latest record is readable.
-        collect_and_verify(key, max_lsn, &base_img, &history, tline).await?;
-        Ok(())
-    }
 }
 
 #[derive(Debug, Serialize, Default)]
@@ -1229,17 +1119,7 @@ impl Timeline {
             // being potentially much longer.
             let rewrite_max = partition_count;
 
-            let outcome = self
-                .compact_shard_ancestors(
-                    rewrite_max,
-                    options.flags.contains(CompactFlags::YieldForL0),
-                    ctx,
-                )
-                .await?;
-            match outcome {
-                CompactionOutcome::Pending | CompactionOutcome::YieldForL0 => return Ok(outcome),
-                CompactionOutcome::Done | CompactionOutcome::Skipped => {}
-            }
+            self.compact_shard_ancestors(rewrite_max, ctx).await?;
         }
 
         Ok(CompactionOutcome::Done)
@@ -1256,12 +1136,11 @@ impl Timeline {
     async fn compact_shard_ancestors(
         self: &Arc<Self>,
         rewrite_max: usize,
-        yield_for_l0: bool,
         ctx: &RequestContext,
-    ) -> Result<CompactionOutcome, CompactionError> {
-        let mut outcome = CompactionOutcome::Done;
+    ) -> Result<(), CompactionError> {
         let mut drop_layers = Vec::new();
         let mut layers_to_rewrite: Vec<Layer> = Vec::new();
+        let mut rewrite_max_exceeded: bool = false;
 
         // We will use the Lsn cutoff of the last GC as a threshold for rewriting layers: if a
         // layer is behind this Lsn, it indicates that the layer is being retained beyond the
@@ -1354,8 +1233,8 @@ impl Timeline {
                 debug!(%layer, "Will rewrite layer on a future compaction, already rewrote {}",
                     layers_to_rewrite.len()
                 );
-                outcome = CompactionOutcome::Pending;
-                break;
+                rewrite_max_exceeded = true;
+                continue;
             }
 
             // Fall through: all our conditions for doing a rewrite passed.
@@ -1367,7 +1246,7 @@ impl Timeline {
 
         // Drop out early if there's nothing to do.
         if layers_to_rewrite.is_empty() && drop_layers.is_empty() {
-            return Ok(CompactionOutcome::Done);
+            return Ok(());
         }
 
         info!(
@@ -1437,20 +1316,6 @@ impl Timeline {
                 // the layer has no data for us with the ShardedRange check above, but
                 drop_layers.push(layer);
             }
-
-            // Yield for L0 compaction if necessary, but make sure we update the layer map below
-            // with the work we've already done.
-            if yield_for_l0
-                && self
-                    .l0_compaction_trigger
-                    .notified()
-                    .now_or_never()
-                    .is_some()
-            {
-                info!("shard ancestor compaction yielding for L0 compaction");
-                outcome = CompactionOutcome::YieldForL0;
-                break;
-            }
         }
 
         for layer in &drop_layers {
@@ -1474,36 +1339,27 @@ impl Timeline {
         // necessary for correctness, but it simplifies testing, and avoids proceeding with another
         // Timeline's compaction while this timeline's uploads may be generating lots of disk I/O
         // load.
-        if outcome != CompactionOutcome::YieldForL0 {
-            info!("shard ancestor compaction waiting for uploads");
-            tokio::select! {
-                result = self.remote_client.wait_completion() => match result {
-                    Ok(()) => {},
-                    Err(WaitCompletionError::NotInitialized(ni)) => return Err(CompactionError::from(ni)),
-                    Err(WaitCompletionError::UploadQueueShutDownOrStopped) => {
-                        return Err(CompactionError::ShuttingDown);
-                    }
-                },
-                // Don't wait if there's L0 compaction to do. We don't need to update the outcome
-                // here, because we've already done the actual work.
-                _ = self.l0_compaction_trigger.notified(), if yield_for_l0 => {},
+        info!("shard ancestor compaction waiting for uploads");
+        match self.remote_client.wait_completion().await {
+            Ok(()) => (),
+            Err(WaitCompletionError::NotInitialized(ni)) => return Err(CompactionError::from(ni)),
+            Err(WaitCompletionError::UploadQueueShutDownOrStopped) => {
+                return Err(CompactionError::ShuttingDown);
             }
         }
 
         info!(
             "shard ancestor compaction done in {:.3}s{}",
             started.elapsed().as_secs_f64(),
-            match outcome {
-                CompactionOutcome::Pending =>
-                    format!(", with pending work (rewrite_max={rewrite_max})"),
-                CompactionOutcome::YieldForL0 => String::from(", yielding for L0 compaction"),
-                CompactionOutcome::Skipped | CompactionOutcome::Done => String::new(),
+            match rewrite_max_exceeded {
+                true => format!(", more work pending due to rewrite_max={rewrite_max}"),
+                false => String::new(),
             }
         );
 
         fail::fail_point!("compact-shard-ancestors-persistent");
 
-        Ok(outcome)
+        Ok(())
     }
 
     /// Update the LayerVisibilityHint of layers covered by image layers, based on whether there is
@@ -2324,7 +2180,6 @@ impl Timeline {
     /// ```
     ///
     /// Note that `accumulated_values` must be sorted by LSN and should belong to a single key.
-    #[allow(clippy::too_many_arguments)]
     pub(crate) async fn generate_key_retention(
         self: &Arc<Timeline>,
         key: Key,
@@ -2333,7 +2188,6 @@ impl Timeline {
         retain_lsn_below_horizon: &[Lsn],
         delta_threshold_cnt: usize,
         base_img_from_ancestor: Option<(Key, Lsn, Bytes)>,
-        verification: bool,
     ) -> anyhow::Result<KeyHistoryRetention> {
         // Pre-checks for the invariants
 
@@ -2420,8 +2274,8 @@ impl Timeline {
             "should have at least below + above horizon batches"
         );
         let mut replay_history: Vec<(Key, Lsn, Value)> = Vec::new();
-        if let Some((key, lsn, ref img)) = base_img_from_ancestor {
-            replay_history.push((key, lsn, Value::Image(img.clone())));
+        if let Some((key, lsn, img)) = base_img_from_ancestor {
+            replay_history.push((key, lsn, Value::Image(img)));
         }
 
         /// Generate debug information for the replay history
@@ -2535,15 +2389,22 @@ impl Timeline {
             // Whether to reconstruct the image. In debug mode, we will generate an image
             // at every retain_lsn to ensure data is not corrupted, but we won't put the
             // image into the final layer.
-            let img_and_lsn = if produce_image {
+            let generate_image = produce_image || debug_mode;
+            if produce_image {
                 records_since_last_image = 0;
+            }
+            let img_and_lsn = if generate_image {
                 let replay_history_for_debug = if debug_mode {
                     Some(replay_history.clone())
                 } else {
                     None
                 };
                 let replay_history_for_debug_ref = replay_history_for_debug.as_deref();
-                let history = std::mem::take(&mut replay_history);
+                let history = if produce_image {
+                    std::mem::take(&mut replay_history)
+                } else {
+                    replay_history.clone()
+                };
                 let mut img = None;
                 let mut records = Vec::with_capacity(history.len());
                 if let (_, lsn, Value::Image(val)) = history.first().as_ref().unwrap() {
@@ -2578,7 +2439,6 @@ impl Timeline {
                         records.push((lsn, rec));
                     }
                 }
-                // WAL redo requires records in the reverse LSN order
                 records.reverse();
                 let state = ValueReconstructState { img, records };
                 // last batch does not generate image so i is always in range, unless we force generate
@@ -2611,16 +2471,10 @@ impl Timeline {
         assert_eq!(retention.len(), lsn_split_points.len() + 1);
         for (idx, logs) in retention.into_iter().enumerate() {
             if idx == lsn_split_points.len() {
-                let retention = KeyHistoryRetention {
+                return Ok(KeyHistoryRetention {
                     below_horizon: result,
                     above_horizon: KeyLogAtLsn(logs),
-                };
-                if verification {
-                    retention
-                        .verify(key, &base_img_from_ancestor, full_history, self)
-                        .await?;
-                }
-                return Ok(retention);
+                });
             } else {
                 result.push((lsn_split_points[idx], KeyLogAtLsn(logs)));
             }
@@ -3087,9 +2941,6 @@ impl Timeline {
             }
             (false, res)
         };
-
-        let verification = self.get_gc_compaction_settings().gc_compaction_verification;
-
         info!(
             "picked {} layers for compaction ({} layers need rewriting) with max_layer_lsn={} min_layer_lsn={} gc_cutoff={} lowest_retain_lsn={}, key_range={}..{}, has_data_below={}",
             job_desc.selected_layers.len(),
@@ -3414,7 +3265,6 @@ impl Timeline {
                             .await
                             .context("failed to get ancestor image")
                             .map_err(CompactionError::Other)?,
-                        verification,
                     )
                     .await
                     .context("failed to generate key retention")
@@ -3455,7 +3305,6 @@ impl Timeline {
                     .await
                     .context("failed to get ancestor image")
                     .map_err(CompactionError::Other)?,
-                verification,
             )
             .await
             .context("failed to generate key retention")

pageserver/src/tenant/timeline/detach_ancestor.rs

@@ -30,7 +30,6 @@ use crate::tenant::storage_layer::{
     AsLayerDesc as _, DeltaLayerWriter, ImageLayerWriter, IoConcurrency, Layer, ResidentLayer,
     ValuesReconstructState,
 };
-use crate::tenant::timeline::VersionedKeySpaceQuery;
 use crate::virtual_file::{MaybeFatalIo, VirtualFile};
 
 #[derive(Debug, thiserror::Error)]
@@ -213,9 +212,13 @@ async fn generate_tombstone_image_layer(
         }
     }
 
-    let query = VersionedKeySpaceQuery::uniform(KeySpace::single(key_range.clone()), image_lsn);
     let data = ancestor
-        .get_vectored_impl(query, &mut reconstruct_state, ctx)
+        .get_vectored_impl(
+            KeySpace::single(key_range.clone()),
+            image_lsn,
+            &mut reconstruct_state,
+            ctx,
+        )
         .await
         .context("failed to retrieve aux keys")
         .map_err(|e| Error::launder(e, Error::Prepare))?;

pageserver/src/virtual_file/owned_buffers_io/write.rs

@@ -134,6 +134,20 @@ where
         Ok((bytes_amount, writer))
     }
 
+    pub async fn into_inner_no_flush(self) -> Arc<W> {
+        let Self {
+            mutable: buf,
+            maybe_flushed: _,
+            writer,
+            mut flush_handle,
+            bytes_submitted: _,
+        } = self;
+        // If the flush task panicked, that's fine.
+        let _ = flush_handle.shutdown().await;
+        assert!(buf.is_some());
+        writer
+    }
+
     #[cfg(test)]
     pub(crate) fn mutable(&self) -> &B {
         self.mutable.as_ref().expect("must not use after an error")

pageserver/src/virtual_file/owned_buffers_io/write/flush.rs

@@ -20,6 +20,11 @@ pub struct FlushHandleInner<Buf, W> {
     /// A bi-directional channel that sends (buffer, offset) for writes,
     /// and receives recyled buffer.
     channel: duplex::mpsc::Duplex<FlushRequest<Buf>, FullSlice<Buf>>,
+    /// The flush task is sometimes sensitive to channel disconnection
+    /// (i.e. when we drop [`Self::channel`]), other times sensitive to
+    /// [`FlushBackgroundTask::cancel`], but never both.
+    /// So, also store this drop guard.
+    set_flush_task_cancelled: tokio_util::sync::DropGuard,
     /// Join handle for the background flush task.
     join_handle: tokio::task::JoinHandle<Result<Arc<W>, FlushTaskError>>,
 }
@@ -134,8 +139,10 @@ where
         back.try_send(buf.flush())
             .expect("we just created it with capacity 1");
 
+        let cancel = cancel.child_token();
+
         let join_handle = tokio::spawn(
-            FlushBackgroundTask::new(back, file, gate_guard, cancel, ctx)
+            FlushBackgroundTask::new(back, file, gate_guard, cancel.clone(), ctx)
                 .run()
                 .instrument(span),
         );
@@ -143,6 +150,7 @@ where
         FlushHandle {
             inner: Some(FlushHandleInner {
                 channel: front,
+                set_flush_task_cancelled: cancel.drop_guard(),
                 join_handle,
             }),
         }
@@ -189,6 +197,7 @@ where
             .take()
             .expect("must not use after we returned an error");
         drop(handle.channel.tx);
+        drop(handle.set_flush_task_cancelled);
         handle.join_handle.await.unwrap()
     }
 

pgxn/neon/walproposer.c

@@ -2118,6 +2118,9 @@ HandleSafekeeperResponse(WalProposer *wp, Safekeeper *fromsk)
 	 */
 	if (wp->config->syncSafekeepers)
 	{
+		int			n_synced;
+
+		n_synced = 0;
 		for (int i = 0; i < wp->n_safekeepers; i++)
 		{
 			Safekeeper *sk = &wp->safekeeper[i];
@@ -2126,6 +2129,8 @@ HandleSafekeeperResponse(WalProposer *wp, Safekeeper *fromsk)
 			/* alive safekeeper which is not synced yet; wait for it */
 			if (sk->state != SS_OFFLINE && !synced)
 				return;
+			if (synced)
+				n_synced++;
 		}
 
 		if (newCommitLsn >= wp->propTermStartLsn)

proxy/src/binary/proxy.rs

@@ -509,14 +509,7 @@ pub async fn run() -> anyhow::Result<()> {
             if let Some(mut redis_kv_client) = redis_kv_client {
                 maintenance_tasks.spawn(async move {
                     redis_kv_client.try_connect().await?;
-                    handle_cancel_messages(&mut redis_kv_client, rx_cancel).await?;
-
-                    drop(redis_kv_client);
-
-                    // `handle_cancel_messages` was terminated due to the tx_cancel
-                    // being dropped. this is not worthy of an error, and this task can only return `Err`,
-                    // so let's wait forever instead.
-                    std::future::pending().await
+                    handle_cancel_messages(&mut redis_kv_client, rx_cancel).await
                 });
             }
 

proxy/src/cancellation.rs

@@ -1,17 +1,16 @@
+use std::convert::Infallible;
 use std::net::{IpAddr, SocketAddr};
 use std::sync::Arc;
 
-use anyhow::{Context, anyhow};
 use ipnet::{IpNet, Ipv4Net, Ipv6Net};
 use postgres_client::CancelToken;
 use postgres_client::tls::MakeTlsConnect;
 use pq_proto::CancelKeyData;
-use redis::{FromRedisValue, Pipeline, Value, pipe};
 use serde::{Deserialize, Serialize};
 use thiserror::Error;
 use tokio::net::TcpStream;
 use tokio::sync::{mpsc, oneshot};
-use tracing::{debug, info, warn};
+use tracing::{debug, info};
 
 use crate::auth::backend::ComputeUserInfo;
 use crate::auth::{AuthError, check_peer_addr_is_in_list};
@@ -31,7 +30,6 @@ type IpSubnetKey = IpNet;
 
 const CANCEL_KEY_TTL: i64 = 1_209_600; // 2 weeks cancellation key expire time
 const REDIS_SEND_TIMEOUT: std::time::Duration = std::time::Duration::from_millis(10);
-const BATCH_SIZE: usize = 8;
 
 // Message types for sending through mpsc channel
 pub enum CancelKeyOp {
@@ -56,168 +54,78 @@ pub enum CancelKeyOp {
     },
 }
 
-impl CancelKeyOp {
-    fn register(self, pipe: &mut Pipeline) -> Option<CancelReplyOp> {
-        #[allow(clippy::used_underscore_binding)]
-        match self {
-            CancelKeyOp::StoreCancelKey {
-                key,
-                field,
-                value,
-                resp_tx,
-                _guard,
-                expire,
-            } => {
-                pipe.hset(&key, field, value);
-                pipe.expire(key, expire);
-                let resp_tx = resp_tx?;
-                Some(CancelReplyOp::StoreCancelKey { resp_tx, _guard })
-            }
-            CancelKeyOp::GetCancelData {
-                key,
-                resp_tx,
-                _guard,
-            } => {
-                pipe.hgetall(key);
-                Some(CancelReplyOp::GetCancelData { resp_tx, _guard })
-            }
-            CancelKeyOp::RemoveCancelKey {
-                key,
-                field,
-                resp_tx,
-                _guard,
-            } => {
-                pipe.hdel(key, field);
-                let resp_tx = resp_tx?;
-                Some(CancelReplyOp::RemoveCancelKey { resp_tx, _guard })
-            }
-        }
-    }
-}
-
-// Message types for sending through mpsc channel
-pub enum CancelReplyOp {
-    StoreCancelKey {
-        resp_tx: oneshot::Sender<anyhow::Result<()>>,
-        _guard: CancelChannelSizeGuard<'static>,
-    },
-    GetCancelData {
-        resp_tx: oneshot::Sender<anyhow::Result<Vec<(String, String)>>>,
-        _guard: CancelChannelSizeGuard<'static>,
-    },
-    RemoveCancelKey {
-        resp_tx: oneshot::Sender<anyhow::Result<()>>,
-        _guard: CancelChannelSizeGuard<'static>,
-    },
-}
-
-impl CancelReplyOp {
-    fn send_err(self, e: anyhow::Error) {
-        match self {
-            CancelReplyOp::StoreCancelKey { resp_tx, _guard } => {
-                resp_tx
-                    .send(Err(e))
-                    .inspect_err(|_| tracing::debug!("could not send reply"))
-                    .ok();
-            }
-            CancelReplyOp::GetCancelData { resp_tx, _guard } => {
-                resp_tx
-                    .send(Err(e))
-                    .inspect_err(|_| tracing::debug!("could not send reply"))
-                    .ok();
-            }
-            CancelReplyOp::RemoveCancelKey { resp_tx, _guard } => {
-                resp_tx
-                    .send(Err(e))
-                    .inspect_err(|_| tracing::debug!("could not send reply"))
-                    .ok();
-            }
-        }
-    }
-
-    fn send_value(self, v: redis::Value) {
-        match self {
-            CancelReplyOp::StoreCancelKey { resp_tx, _guard } => {
-                let send =
-                    FromRedisValue::from_owned_redis_value(v).context("could not parse value");
-                resp_tx
-                    .send(send)
-                    .inspect_err(|_| tracing::debug!("could not send reply"))
-                    .ok();
-            }
-            CancelReplyOp::GetCancelData { resp_tx, _guard } => {
-                let send =
-                    FromRedisValue::from_owned_redis_value(v).context("could not parse value");
-                resp_tx
-                    .send(send)
-                    .inspect_err(|_| tracing::debug!("could not send reply"))
-                    .ok();
-            }
-            CancelReplyOp::RemoveCancelKey { resp_tx, _guard } => {
-                let send =
-                    FromRedisValue::from_owned_redis_value(v).context("could not parse value");
-                resp_tx
-                    .send(send)
-                    .inspect_err(|_| tracing::debug!("could not send reply"))
-                    .ok();
-            }
-        }
-    }
-}
-
 // Running as a separate task to accept messages through the rx channel
+// In case of problems with RTT: switch to recv_many() + redis pipeline
 pub async fn handle_cancel_messages(
     client: &mut RedisKVClient,
     mut rx: mpsc::Receiver<CancelKeyOp>,
-) -> anyhow::Result<()> {
-    let mut batch = Vec::new();
-    let mut replies = vec![];
-
+) -> anyhow::Result<Infallible> {
     loop {
-        if rx.recv_many(&mut batch, BATCH_SIZE).await == 0 {
-            warn!("shutting down cancellation queue");
-            break Ok(());
-        }
-
-        let batch_size = batch.len();
-        debug!(batch_size, "running cancellation jobs");
-
-        let mut pipe = pipe();
-        for msg in batch.drain(..) {
-            if let Some(reply) = msg.register(&mut pipe) {
-                replies.push(reply);
-            } else {
-                pipe.ignore();
-            }
-        }
-
-        let responses = replies.len();
-
-        match client.query(pipe).await {
-            // for each reply, we expect that many values.
-            Ok(Value::Array(values)) if values.len() == responses => {
-                debug!(
-                    batch_size,
-                    responses, "successfully completed cancellation jobs",
-                );
-                for (value, reply) in std::iter::zip(values, replies.drain(..)) {
-                    reply.send_value(value);
+        if let Some(msg) = rx.recv().await {
+            match msg {
+                CancelKeyOp::StoreCancelKey {
+                    key,
+                    field,
+                    value,
+                    resp_tx,
+                    _guard,
+                    expire,
+                } => {
+                    let res = client.hset(&key, field, value).await;
+                    if let Some(resp_tx) = resp_tx {
+                        if res.is_ok() {
+                            resp_tx
+                                .send(client.expire(key, expire).await)
+                                .inspect_err(|e| {
+                                    tracing::debug!(
+                                        "failed to send StoreCancelKey response: {:?}",
+                                        e
+                                    );
+                                })
+                                .ok();
+                        } else {
+                            resp_tx
+                                .send(res)
+                                .inspect_err(|e| {
+                                    tracing::debug!(
+                                        "failed to send StoreCancelKey response: {:?}",
+                                        e
+                                    );
+                                })
+                                .ok();
+                        }
+                    } else if res.is_ok() {
+                        drop(client.expire(key, expire).await);
+                    } else {
+                        tracing::warn!("failed to store cancel key: {:?}", res);
+                    }
                 }
-            }
-            Ok(value) => {
-                debug!(?value, "unexpected redis return value");
-                for reply in replies.drain(..) {
-                    reply.send_err(anyhow!("incorrect response type from redis"));
+                CancelKeyOp::GetCancelData {
+                    key,
+                    resp_tx,
+                    _guard,
+                } => {
+                    drop(resp_tx.send(client.hget_all(key).await));
                 }
-            }
-            Err(err) => {
-                for reply in replies.drain(..) {
-                    reply.send_err(anyhow!("could not send cmd to redis: {err}"));
+                CancelKeyOp::RemoveCancelKey {
+                    key,
+                    field,
+                    resp_tx,
+                    _guard,
+                } => {
+                    if let Some(resp_tx) = resp_tx {
+                        resp_tx
+                            .send(client.hdel(key, field).await)
+                            .inspect_err(|e| {
+                                tracing::debug!("failed to send StoreCancelKey response: {:?}", e);
+                            })
+                            .ok();
+                    } else {
+                        drop(client.hdel(key, field).await);
+                    }
                 }
             }
         }
-
-        replies.clear();
     }
 }
 

proxy/src/redis/kv_ops.rs

@@ -1,5 +1,4 @@
-use redis::aio::ConnectionLike;
-use redis::{Cmd, FromRedisValue, Pipeline, RedisResult};
+use redis::{AsyncCommands, ToRedisArgs};
 
 use super::connection_with_credentials_provider::ConnectionWithCredentialsProvider;
 use crate::rate_limiter::{GlobalRateLimiter, RateBucketInfo};
@@ -9,23 +8,6 @@ pub struct RedisKVClient {
     limiter: GlobalRateLimiter,
 }
 
-#[allow(async_fn_in_trait)]
-pub trait Queryable {
-    async fn query<T: FromRedisValue>(&self, conn: &mut impl ConnectionLike) -> RedisResult<T>;
-}
-
-impl Queryable for Pipeline {
-    async fn query<T: FromRedisValue>(&self, conn: &mut impl ConnectionLike) -> RedisResult<T> {
-        self.query_async(conn).await
-    }
-}
-
-impl Queryable for Cmd {
-    async fn query<T: FromRedisValue>(&self, conn: &mut impl ConnectionLike) -> RedisResult<T> {
-        self.query_async(conn).await
-    }
-}
-
 impl RedisKVClient {
     pub fn new(client: ConnectionWithCredentialsProvider, info: &'static [RateBucketInfo]) -> Self {
         Self {
@@ -45,24 +27,158 @@ impl RedisKVClient {
         Ok(())
     }
 
-    pub(crate) async fn query<T: FromRedisValue>(
-        &mut self,
-        q: impl Queryable,
-    ) -> anyhow::Result<T> {
+    pub(crate) async fn hset<K, F, V>(&mut self, key: K, field: F, value: V) -> anyhow::Result<()>
+    where
+        K: ToRedisArgs + Send + Sync,
+        F: ToRedisArgs + Send + Sync,
+        V: ToRedisArgs + Send + Sync,
+    {
         if !self.limiter.check() {
-            tracing::info!("Rate limit exceeded. Skipping query");
+            tracing::info!("Rate limit exceeded. Skipping hset");
             return Err(anyhow::anyhow!("Rate limit exceeded"));
         }
 
-        match q.query(&mut self.client).await {
-            Ok(t) => return Ok(t),
+        match self.client.hset(&key, &field, &value).await {
+            Ok(()) => return Ok(()),
             Err(e) => {
-                tracing::error!("failed to run query: {e}");
+                tracing::error!("failed to set a key-value pair: {e}");
             }
         }
 
-        tracing::info!("Redis client is disconnected. Reconnecting...");
+        tracing::info!("Redis client is disconnected. Reconnectiong...");
         self.try_connect().await?;
-        Ok(q.query(&mut self.client).await?)
+        self.client
+            .hset(key, field, value)
+            .await
+            .map_err(anyhow::Error::new)
+    }
+
+    #[allow(dead_code)]
+    pub(crate) async fn hset_multiple<K, V>(
+        &mut self,
+        key: &str,
+        items: &[(K, V)],
+    ) -> anyhow::Result<()>
+    where
+        K: ToRedisArgs + Send + Sync,
+        V: ToRedisArgs + Send + Sync,
+    {
+        if !self.limiter.check() {
+            tracing::info!("Rate limit exceeded. Skipping hset_multiple");
+            return Err(anyhow::anyhow!("Rate limit exceeded"));
+        }
+
+        match self.client.hset_multiple(key, items).await {
+            Ok(()) => return Ok(()),
+            Err(e) => {
+                tracing::error!("failed to set a key-value pair: {e}");
+            }
+        }
+
+        tracing::info!("Redis client is disconnected. Reconnectiong...");
+        self.try_connect().await?;
+        self.client
+            .hset_multiple(key, items)
+            .await
+            .map_err(anyhow::Error::new)
+    }
+
+    #[allow(dead_code)]
+    pub(crate) async fn expire<K>(&mut self, key: K, seconds: i64) -> anyhow::Result<()>
+    where
+        K: ToRedisArgs + Send + Sync,
+    {
+        if !self.limiter.check() {
+            tracing::info!("Rate limit exceeded. Skipping expire");
+            return Err(anyhow::anyhow!("Rate limit exceeded"));
+        }
+
+        match self.client.expire(&key, seconds).await {
+            Ok(()) => return Ok(()),
+            Err(e) => {
+                tracing::error!("failed to set a key-value pair: {e}");
+            }
+        }
+
+        tracing::info!("Redis client is disconnected. Reconnectiong...");
+        self.try_connect().await?;
+        self.client
+            .expire(key, seconds)
+            .await
+            .map_err(anyhow::Error::new)
+    }
+
+    #[allow(dead_code)]
+    pub(crate) async fn hget<K, F, V>(&mut self, key: K, field: F) -> anyhow::Result<V>
+    where
+        K: ToRedisArgs + Send + Sync,
+        F: ToRedisArgs + Send + Sync,
+        V: redis::FromRedisValue,
+    {
+        if !self.limiter.check() {
+            tracing::info!("Rate limit exceeded. Skipping hget");
+            return Err(anyhow::anyhow!("Rate limit exceeded"));
+        }
+
+        match self.client.hget(&key, &field).await {
+            Ok(value) => return Ok(value),
+            Err(e) => {
+                tracing::error!("failed to get a value: {e}");
+            }
+        }
+
+        tracing::info!("Redis client is disconnected. Reconnectiong...");
+        self.try_connect().await?;
+        self.client
+            .hget(key, field)
+            .await
+            .map_err(anyhow::Error::new)
+    }
+
+    pub(crate) async fn hget_all<K, V>(&mut self, key: K) -> anyhow::Result<V>
+    where
+        K: ToRedisArgs + Send + Sync,
+        V: redis::FromRedisValue,
+    {
+        if !self.limiter.check() {
+            tracing::info!("Rate limit exceeded. Skipping hgetall");
+            return Err(anyhow::anyhow!("Rate limit exceeded"));
+        }
+
+        match self.client.hgetall(&key).await {
+            Ok(value) => return Ok(value),
+            Err(e) => {
+                tracing::error!("failed to get a value: {e}");
+            }
+        }
+
+        tracing::info!("Redis client is disconnected. Reconnectiong...");
+        self.try_connect().await?;
+        self.client.hgetall(key).await.map_err(anyhow::Error::new)
+    }
+
+    pub(crate) async fn hdel<K, F>(&mut self, key: K, field: F) -> anyhow::Result<()>
+    where
+        K: ToRedisArgs + Send + Sync,
+        F: ToRedisArgs + Send + Sync,
+    {
+        if !self.limiter.check() {
+            tracing::info!("Rate limit exceeded. Skipping hdel");
+            return Err(anyhow::anyhow!("Rate limit exceeded"));
+        }
+
+        match self.client.hdel(&key, &field).await {
+            Ok(()) => return Ok(()),
+            Err(e) => {
+                tracing::error!("failed to delete a key-value pair: {e}");
+            }
+        }
+
+        tracing::info!("Redis client is disconnected. Reconnectiong...");
+        self.try_connect().await?;
+        self.client
+            .hdel(key, field)
+            .await
+            .map_err(anyhow::Error::new)
     }
 }

storage_controller/src/compute_hook.rs

@@ -629,13 +629,15 @@ impl ComputeHook {
         };
 
         let result = if !self.config.use_local_compute_notifications {
-            let compute_hook_url =
-                self.config
-                    .control_plane_url
-                    .as_ref()
-                    .map(|control_plane_url| {
-                        format!("{}/notify-attach", control_plane_url.trim_end_matches('/'))
-                    });
+            let compute_hook_url = if let Some(control_plane_url) = &self.config.control_plane_url {
+                Some(if control_plane_url.ends_with('/') {
+                    format!("{control_plane_url}notify-attach")
+                } else {
+                    format!("{control_plane_url}/notify-attach")
+                })
+            } else {
+                self.config.compute_hook_url.clone()
+            };
 
             // We validate this at startup
             let notify_url = compute_hook_url.as_ref().unwrap();

storage_controller/src/main.rs

@@ -86,6 +86,10 @@ struct Cli {
     #[arg(long)]
     peer_jwt_token: Option<String>,
 
+    /// URL to control plane compute notification endpoint
+    #[arg(long)]
+    compute_hook_url: Option<String>,
+
     /// URL to control plane storage API prefix
     #[arg(long)]
     control_plane_url: Option<String>,
@@ -356,11 +360,13 @@ async fn async_main() -> anyhow::Result<()> {
                 "Insecure config!  One or more secrets is not set.  This is only permitted in `--dev` mode"
             );
         }
-        StrictMode::Strict if args.control_plane_url.is_none() => {
+        StrictMode::Strict
+            if args.compute_hook_url.is_none() && args.control_plane_url.is_none() =>
+        {
             // Production systems should always have a control plane URL set, to prevent falling
             // back to trying to use neon_local.
             anyhow::bail!(
-                "`--control-plane-url` is not set: this is only permitted in `--dev` mode"
+                "neither `--compute-hook-url` nor `--control-plane-url` are set: this is only permitted in `--dev` mode"
             );
         }
         StrictMode::Strict if args.use_local_compute_notifications => {
@@ -388,6 +394,7 @@ async fn async_main() -> anyhow::Result<()> {
         safekeeper_jwt_token: secrets.safekeeper_jwt_token,
         control_plane_jwt_token: secrets.control_plane_jwt_token,
         peer_jwt_token: secrets.peer_jwt_token,
+        compute_hook_url: args.compute_hook_url,
         control_plane_url: args.control_plane_url,
         max_offline_interval: args
             .max_offline_interval

storage_controller/src/service.rs

@@ -357,10 +357,18 @@ pub struct Config {
     // This JWT token will be used to authenticate with other storage controller instances
     pub peer_jwt_token: Option<String>,
 
+    /// Where the compute hook should send notifications of pageserver attachment locations
+    /// (this URL points to the control plane in prod). If this is None, the compute hook will
+    /// assume it is running in a test environment and try to update neon_local.
+    pub compute_hook_url: Option<String>,
+
     /// Prefix for storage API endpoints of the control plane. We use this prefix to compute
     /// URLs that we use to send pageserver and safekeeper attachment locations.
     /// If this is None, the compute hook will assume it is running in a test environment
     /// and try to invoke neon_local instead.
+    ///
+    /// For now, there is also `compute_hook_url` which allows configuration of the pageserver
+    /// specific endpoint, but it is in the process of being phased out.
     pub control_plane_url: Option<String>,
 
     /// Grace period within which a pageserver does not respond to heartbeats, but is still

test_runner/fixtures/metrics.py

@@ -194,7 +194,6 @@ PAGESERVER_PER_TENANT_METRICS: tuple[str, ...] = (
     counter("pageserver_wait_lsn_started_count"),
     counter("pageserver_wait_lsn_finished_count"),
     counter("pageserver_wait_ondemand_download_seconds_sum"),
-    counter("pageserver_page_service_batch_break_reason"),
     *histogram("pageserver_page_service_batch_size"),
     *histogram("pageserver_page_service_pagestream_batch_wait_time_seconds"),
     *PAGESERVER_PER_TENANT_REMOTE_TIMELINE_CLIENT_METRICS,

test_runner/fixtures/neon_fixtures.py

@@ -947,8 +947,6 @@ class NeonEnvBuilder:
                     continue
                 if SMALL_DB_FILE_NAME_REGEX.fullmatch(test_file.name):
                     continue
-                if FINAL_METRICS_FILE_NAME == test_file.name:
-                    continue
                 log.debug(f"Removing large database {test_file} file")
                 test_file.unlink()
             elif test_entry.is_dir():
@@ -1257,7 +1255,6 @@ class NeonEnv:
                 "mode": "pipelined",
                 "execution": "concurrent-futures",
                 "max_batch_size": 32,
-                "batching": "scattered-lsn",
             }
 
             get_vectored_concurrent_io = self.pageserver_get_vectored_concurrent_io
@@ -1459,12 +1456,6 @@ class NeonEnv:
                 except Exception as e:
                     metric_errors.append(e)
                     log.error(f"metric validation failed on {pageserver.id}: {e}")
-
-            try:
-                pageserver.snapshot_final_metrics()
-            except Exception as e:
-                log.error(f"metric snapshot failed on {pageserver.id}: {e}")
-
             try:
                 pageserver.stop(immediate=immediate)
             except RuntimeError:
@@ -2980,20 +2971,6 @@ class NeonPageserver(PgProtocol, LogUtils):
             value = self.http_client().get_metric_value(metric)
             assert value == 0, f"Nonzero {metric} == {value}"
 
-    def snapshot_final_metrics(self):
-        """
-        Take a snapshot of this pageserver's metrics and stash in its work directory.
-        """
-        if not self.running:
-            log.info(f"Skipping metrics snapshot on pageserver {self.id}, it is not running")
-            return
-
-        metrics = self.http_client().get_metrics_str()
-        metrics_snapshot_path = self.workdir / FINAL_METRICS_FILE_NAME
-
-        with open(metrics_snapshot_path, "w") as f:
-            f.write(metrics)
-
     def tenant_attach(
         self,
         tenant_id: TenantId,
@@ -4325,10 +4302,10 @@ class Endpoint(PgProtocol, LogUtils):
 
     def respec_deep(self, **kwargs: Any) -> None:
         """
-        Update the endpoint.json file taking into account nested keys.
-        Distinct method from respec() to do not break existing functionality.
-        NOTE: This method also updates the config.json file, not endpoint.json.
-        We need it because neon_local also writes to config.json, so intended
+        Update the spec.json file taking into account nested keys.
+        Distinct method from respec() to not break existing functionality.
+        NOTE: This method also updates the spec.json file, not endpoint.json.
+        We need it because neon_local also writes to spec.json, so intended
         use-case is i) start endpoint with some config, ii) respec_deep(),
         iii) call reconfigure() to apply the changes.
         """
@@ -4341,17 +4318,17 @@ class Endpoint(PgProtocol, LogUtils):
                     curr[k] = v
             return curr
 
-        config_path = os.path.join(self.endpoint_path(), "config.json")
+        config_path = os.path.join(self.endpoint_path(), "spec.json")
         with open(config_path) as f:
-            config: dict[str, Any] = json.load(f)
+            data_dict: dict[str, Any] = json.load(f)
 
-        log.debug("Current compute config: %s", json.dumps(config, indent=4))
+        log.debug("Current compute spec: %s", json.dumps(data_dict, indent=4))
 
-        update(config, kwargs)
+        update(data_dict, kwargs)
 
         with open(config_path, "w") as file:
-            log.debug("Updating compute config to: %s", json.dumps(config, indent=4))
-            json.dump(config, file, indent=4)
+            log.debug("Updating compute spec to: %s", json.dumps(data_dict, indent=4))
+            json.dump(data_dict, file, indent=4)
 
     def wait_for_migrations(self, wait_for: int = NUM_COMPUTE_MIGRATIONS) -> None:
         """
@@ -4368,7 +4345,7 @@ class Endpoint(PgProtocol, LogUtils):
             wait_until(check_migrations_done)
 
     # Mock the extension part of spec passed from control plane for local testing
-    # endpooint.rs adds content of this file as a part of the config.json
+    # endpooint.rs adds content of this file as a part of the spec.json
     def create_remote_extension_spec(self, spec: dict[str, Any]):
         """Create a remote extension spec file for the endpoint."""
         remote_extensions_spec_path = os.path.join(
@@ -5156,8 +5133,6 @@ SMALL_DB_FILE_NAME_REGEX: re.Pattern[str] = re.compile(
     r"config-v1|heatmap-v1|tenant-manifest|metadata|.+\.(?:toml|pid|json|sql|conf)"
 )
 
-FINAL_METRICS_FILE_NAME: str = "final_metrics.txt"
-
 
 SKIP_DIRS = frozenset(
     (

test_runner/performance/pageserver/test_page_service_batching.py

@@ -1,6 +1,7 @@
 import concurrent.futures
 import dataclasses
 import json
+import re
 import threading
 import time
 from dataclasses import dataclass
@@ -30,28 +31,20 @@ class PageServicePipeliningConfigSerial(PageServicePipeliningConfig):
 class PageServicePipeliningConfigPipelined(PageServicePipeliningConfig):
     max_batch_size: int
     execution: str
-    batching: str
     mode: str = "pipelined"
 
 
 EXECUTION = ["concurrent-futures"]
-BATCHING = ["uniform-lsn", "scattered-lsn"]
 
 NON_BATCHABLE: list[PageServicePipeliningConfig] = [PageServicePipeliningConfigSerial()]
 for max_batch_size in [1, 32]:
     for execution in EXECUTION:
-        for batching in BATCHING:
-            NON_BATCHABLE.append(
-                PageServicePipeliningConfigPipelined(max_batch_size, execution, batching)
-            )
+        NON_BATCHABLE.append(PageServicePipeliningConfigPipelined(max_batch_size, execution))
 
 BATCHABLE: list[PageServicePipeliningConfig] = []
 for max_batch_size in [32]:
     for execution in EXECUTION:
-        for batching in BATCHING:
-            BATCHABLE.append(
-                PageServicePipeliningConfigPipelined(max_batch_size, execution, batching)
-            )
+        BATCHABLE.append(PageServicePipeliningConfigPipelined(max_batch_size, execution))
 
 
 @pytest.mark.parametrize(
@@ -169,7 +162,6 @@ def test_throughput(
         time: float
         pageserver_batch_size_histo_sum: float
         pageserver_batch_size_histo_count: float
-        pageserver_batch_breaks_reason_count: dict[str, int]
         compute_getpage_count: float
         pageserver_cpu_seconds_total: float
 
@@ -183,10 +175,6 @@ def test_throughput(
                 compute_getpage_count=self.compute_getpage_count - other.compute_getpage_count,
                 pageserver_cpu_seconds_total=self.pageserver_cpu_seconds_total
                 - other.pageserver_cpu_seconds_total,
-                pageserver_batch_breaks_reason_count={
-                    reason: count - other.pageserver_batch_breaks_reason_count.get(reason, 0)
-                    for reason, count in self.pageserver_batch_breaks_reason_count.items()
-                },
             )
 
         def normalize(self, by) -> "Metrics":
@@ -196,10 +184,6 @@ def test_throughput(
                 pageserver_batch_size_histo_count=self.pageserver_batch_size_histo_count / by,
                 compute_getpage_count=self.compute_getpage_count / by,
                 pageserver_cpu_seconds_total=self.pageserver_cpu_seconds_total / by,
-                pageserver_batch_breaks_reason_count={
-                    reason: count / by
-                    for reason, count in self.pageserver_batch_breaks_reason_count.items()
-                },
             )
 
     def get_metrics() -> Metrics:
@@ -209,20 +193,6 @@ def test_throughput(
             )
             compute_getpage_count = cur.fetchall()[0][0]
             pageserver_metrics = ps_http.get_metrics()
-            for name, samples in pageserver_metrics.metrics.items():
-                for sample in samples:
-                    log.info(f"{name=} labels={sample.labels} {sample.value}")
-
-            raw_batch_break_reason_count = pageserver_metrics.query_all(
-                "pageserver_page_service_batch_break_reason_total",
-                filter={"timeline_id": str(env.initial_timeline)},
-            )
-
-            batch_break_reason_count = {
-                sample.labels["reason"]: int(sample.value)
-                for sample in raw_batch_break_reason_count
-            }
-
             return Metrics(
                 time=time.time(),
                 pageserver_batch_size_histo_sum=pageserver_metrics.query_one(
@@ -231,7 +201,6 @@ def test_throughput(
                 pageserver_batch_size_histo_count=pageserver_metrics.query_one(
                     "pageserver_page_service_batch_size_count"
                 ).value,
-                pageserver_batch_breaks_reason_count=batch_break_reason_count,
                 compute_getpage_count=compute_getpage_count,
                 pageserver_cpu_seconds_total=pageserver_metrics.query_one(
                     "libmetrics_process_cpu_seconds_highres"
@@ -286,6 +255,25 @@ def test_throughput(
 
     log.info("Results: %s", metrics)
 
+    since_last_start: list[str] = []
+    for line in env.pageserver.logfile.read_text().splitlines():
+        if "git:" in line:
+            since_last_start = []
+        since_last_start.append(line)
+
+    stopping_batching_because_re = re.compile(
+        r"stopping batching because (LSN changed|of batch size|timeline object mismatch|batch key changed|same page was requested at different LSNs|.*)"
+    )
+    reasons_for_stopping_batching = {}
+    for line in since_last_start:
+        match = stopping_batching_because_re.search(line)
+        if match:
+            if match.group(1) not in reasons_for_stopping_batching:
+                reasons_for_stopping_batching[match.group(1)] = 0
+            reasons_for_stopping_batching[match.group(1)] += 1
+
+    log.info("Reasons for stopping batching: %s", reasons_for_stopping_batching)
+
     #
     # Sanity-checks on the collected data
     #
@@ -299,16 +287,7 @@ def test_throughput(
     #
 
     for metric, value in dataclasses.asdict(metrics).items():
-        if metric == "pageserver_batch_breaks_reason_count":
-            assert isinstance(value, dict)
-            for reason, count in value.items():
-                zenbenchmark.record(
-                    f"counters.{metric}_{reason}", count, unit="", report=MetricReport.TEST_PARAM
-                )
-        else:
-            zenbenchmark.record(
-                f"counters.{metric}", value, unit="", report=MetricReport.TEST_PARAM
-            )
+        zenbenchmark.record(f"counters.{metric}", value, unit="", report=MetricReport.TEST_PARAM)
 
     zenbenchmark.record(
         "perfmetric.batching_factor",
@@ -321,10 +300,7 @@ def test_throughput(
 PRECISION_CONFIGS: list[PageServicePipeliningConfig] = [PageServicePipeliningConfigSerial()]
 for max_batch_size in [1, 32]:
     for execution in EXECUTION:
-        for batching in BATCHING:
-            PRECISION_CONFIGS.append(
-                PageServicePipeliningConfigPipelined(max_batch_size, execution, batching)
-            )
+        PRECISION_CONFIGS.append(PageServicePipeliningConfigPipelined(max_batch_size, execution))
 
 
 @pytest.mark.parametrize(

test_runner/performance/test_physical_replication.py

@@ -64,8 +64,8 @@ def test_ro_replica_lag(
 
     project = neon_api.create_project(pg_version)
     project_id = project["project"]["id"]
-    log.info("Project ID: %s", project_id)
-    log.info("Primary endpoint ID: %s", project["project"]["endpoints"][0]["id"])
+    log.info("Project ID: {}", project_id)
+    log.info("Primary endpoint ID: {}", project["project"]["endpoints"][0]["id"])
     neon_api.wait_for_operation_to_finish(project_id)
     error_occurred = False
     try:
@@ -81,7 +81,7 @@ def test_ro_replica_lag(
             endpoint_type="read_only",
             settings={"pg_settings": {"hot_standby_feedback": "on"}},
         )
-        log.info("Replica endpoint ID: %s", replica["endpoint"]["id"])
+        log.info("Replica endpoint ID: {}", replica["endpoint"]["id"])
         replica_env = master_env.copy()
         replica_env["PGHOST"] = replica["endpoint"]["host"]
         neon_api.wait_for_operation_to_finish(project_id)
@@ -197,8 +197,8 @@ def test_replication_start_stop(
 
     project = neon_api.create_project(pg_version)
     project_id = project["project"]["id"]
-    log.info("Project ID: %s", project_id)
-    log.info("Primary endpoint ID: %s", project["project"]["endpoints"][0]["id"])
+    log.info("Project ID: {}", project_id)
+    log.info("Primary endpoint ID: {}", project["project"]["endpoints"][0]["id"])
     neon_api.wait_for_operation_to_finish(project_id)
     try:
         branch_id = project["branch"]["id"]
@@ -215,7 +215,7 @@ def test_replication_start_stop(
                 endpoint_type="read_only",
                 settings={"pg_settings": {"hot_standby_feedback": "on"}},
             )
-            log.info("Replica %d endpoint ID: %s", i + 1, replica["endpoint"]["id"])
+            log.info("Replica {} endpoint ID: {}", i + 1, replica["endpoint"]["id"])
             replicas.append(replica)
             neon_api.wait_for_operation_to_finish(project_id)
 

test_runner/regress/test_attach_tenant_config.py

@@ -187,7 +187,6 @@ def test_fully_custom_config(positive_env: NeonEnv):
         },
         "rel_size_v2_enabled": False,  # test suite enables it by default as of https://github.com/neondatabase/neon/issues/11081, so, custom config means disabling it
         "gc_compaction_enabled": True,
-        "gc_compaction_verification": False,
         "gc_compaction_initial_threshold_kb": 1024000,
         "gc_compaction_ratio_percent": 200,
         "image_creation_preempt_threshold": 5,

test_runner/regress/test_compaction.py

@@ -162,8 +162,6 @@ def test_pageserver_compaction_preempt(
     conf = PREEMPT_COMPACTION_TENANT_CONF.copy()
     env = neon_env_builder.init_start(initial_tenant_conf=conf)
 
-    env.pageserver.allowed_errors.append(".*The timeline or pageserver is shutting down.*")
-
     tenant_id = env.initial_tenant
     timeline_id = env.initial_timeline
 

test_runner/regress/test_compute_catalog.py

@@ -90,12 +90,10 @@ def test_compute_catalog(neon_simple_env: NeonEnv):
     # and reconfigure the endpoint to create some test databases.
     endpoint.respec_deep(
         **{
-            "spec": {
-                "skip_pg_catalog_updates": False,
-                "cluster": {
-                    "roles": TEST_ROLE_NAMES,
-                    "databases": TEST_DB_NAMES,
-                },
+            "skip_pg_catalog_updates": False,
+            "cluster": {
+                "roles": TEST_ROLE_NAMES,
+                "databases": TEST_DB_NAMES,
             },
         }
     )
@@ -157,12 +155,10 @@ def test_compute_create_drop_dbs_and_roles(neon_simple_env: NeonEnv):
     # and reconfigure the endpoint to apply the changes.
     endpoint.respec_deep(
         **{
-            "spec": {
-                "skip_pg_catalog_updates": False,
-                "cluster": {
-                    "roles": TEST_ROLE_NAMES,
-                    "databases": TEST_DB_NAMES,
-                },
+            "skip_pg_catalog_updates": False,
+            "cluster": {
+                "roles": TEST_ROLE_NAMES,
+                "databases": TEST_DB_NAMES,
             },
         }
     )
@@ -200,14 +196,12 @@ def test_compute_create_drop_dbs_and_roles(neon_simple_env: NeonEnv):
 
     endpoint.respec_deep(
         **{
-            "spec": {
-                "skip_pg_catalog_updates": False,
-                "cluster": {
-                    "roles": [],
-                    "databases": [],
-                },
-                "delta_operations": delta_operations,
+            "skip_pg_catalog_updates": False,
+            "cluster": {
+                "roles": [],
+                "databases": [],
             },
+            "delta_operations": delta_operations,
         }
     )
     endpoint.reconfigure()
@@ -256,11 +250,9 @@ def test_dropdb_with_subscription(neon_simple_env: NeonEnv):
     # and reconfigure the endpoint to apply the changes.
     endpoint.respec_deep(
         **{
-            "spec": {
-                "skip_pg_catalog_updates": False,
-                "cluster": {
-                    "databases": TEST_DB_NAMES,
-                },
+            "skip_pg_catalog_updates": False,
+            "cluster": {
+                "databases": TEST_DB_NAMES,
             },
         }
     )
@@ -314,19 +306,17 @@ def test_dropdb_with_subscription(neon_simple_env: NeonEnv):
     # and reconfigure the endpoint to apply the changes.
     endpoint.respec_deep(
         **{
-            "spec": {
-                "skip_pg_catalog_updates": False,
-                "cluster": {
-                    "databases": TEST_DB_NAMES_NEW,
-                },
-                "delta_operations": [
-                    {"action": "delete_db", "name": SUB_DB_NAME},
-                    # also test the case when we try to delete a non-existent database
-                    # shouldn't happen in normal operation,
-                    # but can occur when failed operations are retried
-                    {"action": "delete_db", "name": "nonexistent_db"},
-                ],
+            "skip_pg_catalog_updates": False,
+            "cluster": {
+                "databases": TEST_DB_NAMES_NEW,
             },
+            "delta_operations": [
+                {"action": "delete_db", "name": SUB_DB_NAME},
+                # also test the case when we try to delete a non-existent database
+                # shouldn't happen in normal operation,
+                # but can occur when failed operations are retried
+                {"action": "delete_db", "name": "nonexistent_db"},
+            ],
         }
     )
 
@@ -364,27 +354,25 @@ def test_drop_role_with_table_privileges_from_neon_superuser(neon_simple_env: Ne
 
     endpoint.respec_deep(
         **{
-            "spec": {
-                "skip_pg_catalog_updates": False,
-                "cluster": {
-                    "roles": [
-                        {
-                            # We need to create role via compute_ctl, because in this case it will receive
-                            # additional grants equivalent to our real environment, so we can repro some
-                            # issues.
-                            "name": "neon",
-                            # Some autocomplete-suggested hash, no specific meaning.
-                            "encrypted_password": "SCRAM-SHA-256$4096:hBT22QjqpydQWqEulorfXA==$miBogcoj68JWYdsNB5PW1X6PjSLBEcNuctuhtGkb4PY=:hxk2gxkwxGo6P7GCtfpMlhA9zwHvPMsCz+NQf2HfvWk=",
-                            "options": [],
-                        },
-                    ],
-                    "databases": [
-                        {
-                            "name": TEST_DB_NAME,
-                            "owner": "neon",
-                        },
-                    ],
-                },
+            "skip_pg_catalog_updates": False,
+            "cluster": {
+                "roles": [
+                    {
+                        # We need to create role via compute_ctl, because in this case it will receive
+                        # additional grants equivalent to our real environment, so we can repro some
+                        # issues.
+                        "name": "neon",
+                        # Some autocomplete-suggested hash, no specific meaning.
+                        "encrypted_password": "SCRAM-SHA-256$4096:hBT22QjqpydQWqEulorfXA==$miBogcoj68JWYdsNB5PW1X6PjSLBEcNuctuhtGkb4PY=:hxk2gxkwxGo6P7GCtfpMlhA9zwHvPMsCz+NQf2HfvWk=",
+                        "options": [],
+                    },
+                ],
+                "databases": [
+                    {
+                        "name": TEST_DB_NAME,
+                        "owner": "neon",
+                    },
+                ],
             },
         }
     )
@@ -427,15 +415,13 @@ def test_drop_role_with_table_privileges_from_neon_superuser(neon_simple_env: Ne
     # Drop role via compute_ctl
     endpoint.respec_deep(
         **{
-            "spec": {
-                "skip_pg_catalog_updates": False,
-                "delta_operations": [
-                    {
-                        "action": "delete_role",
-                        "name": TEST_GRANTEE,
-                    },
-                ],
-            },
+            "skip_pg_catalog_updates": False,
+            "delta_operations": [
+                {
+                    "action": "delete_role",
+                    "name": TEST_GRANTEE,
+                },
+            ],
         }
     )
     endpoint.reconfigure()
@@ -458,15 +444,13 @@ def test_drop_role_with_table_privileges_from_neon_superuser(neon_simple_env: Ne
 
     endpoint.respec_deep(
         **{
-            "spec": {
-                "skip_pg_catalog_updates": False,
-                "delta_operations": [
-                    {
-                        "action": "delete_role",
-                        "name": "readonly2",
-                    },
-                ],
-            },
+            "skip_pg_catalog_updates": False,
+            "delta_operations": [
+                {
+                    "action": "delete_role",
+                    "name": "readonly2",
+                },
+            ],
         }
     )
     endpoint.reconfigure()
@@ -491,27 +475,25 @@ def test_drop_role_with_table_privileges_from_non_neon_superuser(neon_simple_env
     endpoint = env.endpoints.create_start("main")
     endpoint.respec_deep(
         **{
-            "spec": {
-                "skip_pg_catalog_updates": False,
-                "cluster": {
-                    "roles": [
-                        {
-                            # We need to create role via compute_ctl, because in this case it will receive
-                            # additional grants equivalent to our real environment, so we can repro some
-                            # issues.
-                            "name": TEST_GRANTOR,
-                            # Some autocomplete-suggested hash, no specific meaning.
-                            "encrypted_password": "SCRAM-SHA-256$4096:hBT22QjqpydQWqEulorfXA==$miBogcoj68JWYdsNB5PW1X6PjSLBEcNuctuhtGkb4PY=:hxk2gxkwxGo6P7GCtfpMlhA9zwHvPMsCz+NQf2HfvWk=",
-                            "options": [],
-                        },
-                    ],
-                    "databases": [
-                        {
-                            "name": TEST_DB_NAME,
-                            "owner": TEST_GRANTOR,
-                        },
-                    ],
-                },
+            "skip_pg_catalog_updates": False,
+            "cluster": {
+                "roles": [
+                    {
+                        # We need to create role via compute_ctl, because in this case it will receive
+                        # additional grants equivalent to our real environment, so we can repro some
+                        # issues.
+                        "name": TEST_GRANTOR,
+                        # Some autocomplete-suggested hash, no specific meaning.
+                        "encrypted_password": "SCRAM-SHA-256$4096:hBT22QjqpydQWqEulorfXA==$miBogcoj68JWYdsNB5PW1X6PjSLBEcNuctuhtGkb4PY=:hxk2gxkwxGo6P7GCtfpMlhA9zwHvPMsCz+NQf2HfvWk=",
+                        "options": [],
+                    },
+                ],
+                "databases": [
+                    {
+                        "name": TEST_DB_NAME,
+                        "owner": TEST_GRANTOR,
+                    },
+                ],
             },
         }
     )
@@ -525,15 +507,13 @@ def test_drop_role_with_table_privileges_from_non_neon_superuser(neon_simple_env
 
     endpoint.respec_deep(
         **{
-            "spec": {
-                "skip_pg_catalog_updates": False,
-                "delta_operations": [
-                    {
-                        "action": "delete_role",
-                        "name": TEST_GRANTEE,
-                    },
-                ],
-            },
+            "skip_pg_catalog_updates": False,
+            "delta_operations": [
+                {
+                    "action": "delete_role",
+                    "name": TEST_GRANTEE,
+                },
+            ],
         }
     )
     endpoint.reconfigure()

test_runner/regress/test_compute_reconfigure.py

@@ -31,17 +31,15 @@ def test_compute_reconfigure(neon_simple_env: NeonEnv):
 
     endpoint.respec_deep(
         **{
-            "spec": {
-                "skip_pg_catalog_updates": True,
-                "cluster": {
-                    "settings": [
-                        {
-                            "name": "log_line_prefix",
-                            "vartype": "string",
-                            "value": TEST_LOG_LINE_PREFIX,
-                        }
-                    ]
-                },
+            "skip_pg_catalog_updates": True,
+            "cluster": {
+                "settings": [
+                    {
+                        "name": "log_line_prefix",
+                        "vartype": "string",
+                        "value": TEST_LOG_LINE_PREFIX,
+                    }
+                ]
             },
         }
     )

test_runner/regress/test_page_service_batching_regressions.py

@@ -16,7 +16,6 @@ def test_slow_flush(neon_env_builder: NeonEnvBuilder, neon_binpath: Path, kind:
             "mode": "pipelined",
             "max_batch_size": 32,
             "execution": "concurrent-futures",
-            "batching": "uniform-lsn",
         }
 
     neon_env_builder.pageserver_config_override = patch_pageserver_toml

test_runner/regress/test_pg_regress.py

@@ -239,8 +239,6 @@ def test_isolation(
             "neon.regress_test_mode = true",
             # Stack size should be increased for tests to pass with asan.
             "max_stack_depth = 4MB",
-            # Neon extensiosn starts 2 BGW so decreasing number of parallel workers which can affect deadlock-parallel test if it hits max_worker_processes.
-            "max_worker_processes = 16",
         ],
     )
     endpoint.safe_psql(f"CREATE DATABASE {DBNAME}")

test_runner/regress/test_subscriber_branching.py

@@ -251,7 +251,7 @@ def test_multiple_subscription_branching(neon_simple_env: NeonEnv):
     NUMBER_OF_DBS = 5
 
     # Create and start endpoint so that neon_local put all the generated
-    # stuff into the config.json file.
+    # stuff into the spec.json file.
     endpoint = env.endpoints.create_start(
         "main",
         config_lines=[
@@ -280,15 +280,13 @@ def test_multiple_subscription_branching(neon_simple_env: NeonEnv):
             }
         )
 
-    # Update the config.json file to create the databases
+    # Update the spec.json file to create the databases
     # and reconfigure the endpoint to apply the changes.
     endpoint.respec_deep(
         **{
-            "spec": {
-                "skip_pg_catalog_updates": False,
-                "cluster": {
-                    "databases": TEST_DB_NAMES,
-                },
+            "skip_pg_catalog_updates": False,
+            "cluster": {
+                "databases": TEST_DB_NAMES,
             },
         }
     )

test_runner/regress/test_tenants.py

@@ -390,7 +390,6 @@ def test_create_churn_during_restart(neon_env_builder: NeonEnvBuilder):
     # Tenant creation requests which arrive out of order will generate complaints about
     # generation nubmers out of order.
     env.pageserver.allowed_errors.append(".*Generation .+ is less than existing .+")
-    env.pageserver.allowed_errors.append(".*due to stale generation.+")
 
     # Timeline::flush_and_shutdown cannot tell if it is hitting a failure because of
     # an incomplete attach, or some other problem.  In the field this should be rare,