Merge branch 'layer-stats' of github.com:neondatabase/neon into layer-stats

Co-authored-by: Joonas Koivunen <joonas@neon.tech>

.gitignore

@@ -1,6 +1,5 @@
 /pg_install
 /target
-/alek_ext/target
 /tmp_check
 /tmp_check_cli
 __pycache__/

Cargo.lock

@@ -37,10 +37,6 @@ dependencies = [
  "memchr",
 ]
 
-[[package]]
-name = "alek_ext"
-version = "0.1.0"
-
 [[package]]
 name = "android_system_properties"
 version = "0.1.5"
@@ -928,14 +924,12 @@ dependencies = [
  "opentelemetry",
  "postgres",
  "regex",
- "remote_storage",
  "reqwest",
  "serde",
  "serde_json",
  "tar",
  "tokio",
  "tokio-postgres",
- "toml_edit",
  "tracing",
  "tracing-opentelemetry",
  "tracing-subscriber",
@@ -2536,6 +2530,7 @@ dependencies = [
  "bytes",
  "clap 4.3.0",
  "git-version",
+ "itertools",
  "pageserver",
  "postgres_ffi",
  "svg_fmt",
@@ -2880,6 +2875,7 @@ dependencies = [
  "serde",
  "thiserror",
  "utils",
+ "wal_craft",
  "workspace_hack",
 ]
 
@@ -4899,9 +4895,7 @@ dependencies = [
  "once_cell",
  "postgres",
  "postgres_ffi",
- "regex",
  "tempfile",
- "utils",
  "workspace_hack",
 ]
 

Cargo.toml

@@ -1,6 +1,5 @@
 [workspace]
 members = [
-    "alek_ext",
     "compute_tools",
     "control_plane",
     "pageserver",
@@ -10,20 +9,7 @@ members = [
     "storage_broker",
     "workspace_hack",
     "trace",
-    "libs/compute_api",
-    "libs/pageserver_api",
-    "libs/postgres_ffi",
-    "libs/safekeeper_api",
-    "libs/utils",
-    "libs/consumption_metrics",
-    "libs/postgres_backend",
-    "libs/pq_proto",
-    "libs/tenant_size_model",
-    "libs/metrics",
-    "libs/postgres_connection",
-    "libs/remote_storage",
-    "libs/tracing-utils",
-    "libs/postgres_ffi/wal_craft",
+    "libs/*",
 ]
 
 [workspace.package]

Dockerfile.compute-node

@@ -517,22 +517,6 @@ RUN wget https://github.com/kelvich/pg_tiktoken/archive/801f84f08c6881c8aa30f405
     cargo pgx install --release && \
     echo "trusted = true" >> /usr/local/pgsql/share/extension/pg_tiktoken.control
 
-#########################################################################################
-#
-# Layer "pg-pgx-ulid-build"
-# Compile "pgx_ulid" extension
-#
-#########################################################################################
-
-FROM rust-extensions-build AS pg-pgx-ulid-build
-
-RUN wget https://github.com/pksunkara/pgx_ulid/archive/refs/tags/v0.1.0.tar.gz -O pgx_ulid.tar.gz && \
-    echo "908b7358e6f846e87db508ae5349fb56a88ee6305519074b12f3d5b0ff09f791 pgx_ulid.tar.gz" | sha256sum --check && \
-    mkdir pgx_ulid-src && cd pgx_ulid-src && tar xvzf ../pgx_ulid.tar.gz --strip-components=1 -C . && \
-    sed -i 's/pgx        = "=0.7.3"/pgx = { version = "0.7.3", features = [ "unsafe-postgres" ] }/g' Cargo.toml && \
-    cargo pgx install --release && \
-    echo "trusted = true" >> /usr/local/pgsql/share/extension/ulid.control
-
 #########################################################################################
 #
 # Layer "neon-pg-ext-build"
@@ -563,7 +547,6 @@ COPY --from=timescaledb-pg-build /usr/local/pgsql/ /usr/local/pgsql/
 COPY --from=pg-hint-plan-pg-build /usr/local/pgsql/ /usr/local/pgsql/
 COPY --from=kq-imcx-pg-build /usr/local/pgsql/ /usr/local/pgsql/
 COPY --from=pg-cron-pg-build /usr/local/pgsql/ /usr/local/pgsql/
-COPY --from=pg-pgx-ulid-build /usr/local/pgsql/ /usr/local/pgsql/
 COPY pgxn/ pgxn/
 
 RUN make -j $(getconf _NPROCESSORS_ONLN) \
@@ -573,10 +556,6 @@ RUN make -j $(getconf _NPROCESSORS_ONLN) \
     make -j $(getconf _NPROCESSORS_ONLN) \
         PG_CONFIG=/usr/local/pgsql/bin/pg_config \
         -C pgxn/neon_utils \
-        -s install && \
-    make -j $(getconf _NPROCESSORS_ONLN) \
-        PG_CONFIG=/usr/local/pgsql/bin/pg_config \
-        -C pgxn/hnsw \
         -s install
 
 #########################################################################################

Makefile

@@ -138,11 +138,6 @@ neon-pg-ext-%: postgres-%
 	$(MAKE) PG_CONFIG=$(POSTGRES_INSTALL_DIR)/$*/bin/pg_config CFLAGS='$(PG_CFLAGS) $(COPT)' \
 		-C $(POSTGRES_INSTALL_DIR)/build/neon-utils-$* \
 		-f $(ROOT_PROJECT_DIR)/pgxn/neon_utils/Makefile install
-	+@echo "Compiling hnsw $*"
-	mkdir -p $(POSTGRES_INSTALL_DIR)/build/hnsw-$*
-	$(MAKE) PG_CONFIG=$(POSTGRES_INSTALL_DIR)/$*/bin/pg_config CFLAGS='$(PG_CFLAGS) $(COPT)' \
-		-C $(POSTGRES_INSTALL_DIR)/build/hnsw-$* \
-		-f $(ROOT_PROJECT_DIR)/pgxn/hnsw/Makefile install
 
 .PHONY: neon-pg-ext-clean-%
 neon-pg-ext-clean-%:
@@ -158,9 +153,6 @@ neon-pg-ext-clean-%:
 	$(MAKE) PG_CONFIG=$(POSTGRES_INSTALL_DIR)/$*/bin/pg_config \
 	-C $(POSTGRES_INSTALL_DIR)/build/neon-utils-$* \
 	-f $(ROOT_PROJECT_DIR)/pgxn/neon_utils/Makefile clean
-	$(MAKE) PG_CONFIG=$(POSTGRES_INSTALL_DIR)/$*/bin/pg_config \
-	-C $(POSTGRES_INSTALL_DIR)/build/hnsw-$* \
-	-f $(ROOT_PROJECT_DIR)/pgxn/hnsw/Makefile clean
 
 .PHONY: neon-pg-ext
 neon-pg-ext: \

README.md

@@ -28,19 +28,18 @@ See developer documentation in [SUMMARY.md](/docs/SUMMARY.md) for more informati
 * On Ubuntu or Debian, this set of packages should be sufficient to build the code:
 ```bash
 apt install build-essential libtool libreadline-dev zlib1g-dev flex bison libseccomp-dev \
-libssl-dev clang pkg-config libpq-dev cmake postgresql-client protobuf-compiler \
-libcurl4-openssl-dev
+libssl-dev clang pkg-config libpq-dev cmake postgresql-client protobuf-compiler
 ```
 * On Fedora, these packages are needed:
 ```bash
 dnf install flex bison readline-devel zlib-devel openssl-devel \
   libseccomp-devel perl clang cmake postgresql postgresql-contrib protobuf-compiler \
-  protobuf-devel libcurl-devel
+  protobuf-devel
 ```
 * On Arch based systems, these packages are needed:
 ```bash
 pacman -S base-devel readline zlib libseccomp openssl clang \
-postgresql-libs cmake postgresql protobuf curl
+postgresql-libs cmake postgresql protobuf
 ```
 
 Building Neon requires 3.15+ version of `protoc` (protobuf-compiler). If your distribution provides an older version, you can install a newer version from [here](https://github.com/protocolbuffers/protobuf/releases).

alek_ext/Cargo.toml

@@ -1,21 +0,0 @@
-[package]
-name = "alek_ext"
-version = "0.1.0"
-edition = "2021"
-
-# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
-
-[dependencies]
-anyhow = "1.0.71"
-aws-config = { version = "0.55", default-features = false, features=["rustls"] }
-aws-sdk-s3 = "0.27"
-aws-smithy-http = "0.55"
-aws-credential-types = "0.55"
-aws-types = "0.55"
-remote_storage = { version = "0.1", path = "../libs/remote_storage/" }
-tokio = "1.28.2"
-toml_edit = "0.19.10"
-tracing = "0.1.37"
-tracing-subscriber = "0.3.17"
-
-[workspace]

alek_ext/fuzzystrmatch.control

@@ -1,6 +0,0 @@
-# fuzzystrmatch extension
-comment = 'determine similarities and distance between strings'
-default_version = '1.2'
-module_pathname = '$libdir/fuzzystrmatch'
-relocatable = true
-trusted = true

alek_ext/pg_cron.control

@@ -1,5 +0,0 @@
-comment = 'Job scheduler for PostgreSQL'
-default_version = '1.5'
-module_pathname = '$libdir/pg_cron'
-relocatable = false
-schema = pg_catalog

alek_ext/src/awsmwe_v1.rs

@@ -1,33 +0,0 @@
-/*
- * This is a MWE of using the aws-sdk-s3 to download a file from an S3 bucket 
- * */
-
-use aws_sdk_s3::{self, config::Region, Error};
-use aws_config::{self, meta::region::RegionProviderChain};
-
-
-#[tokio::main]
-async fn main() -> Result<(), Error> {
-    let region_provider = RegionProviderChain::first_try(Region::new("eu-central-1"))
-        .or_default_provider()
-        .or_else(Region::new("eu-central-1"));
-
-    let shared_config = aws_config::from_env().region(region_provider).load().await;
-    let client = aws_sdk_s3::Client::new(&shared_config);
-
-    let bucket_name = "neon-dev-extensions";
-    let object_key = "fuzzystrmatch.control";
-    let response = client
-        .get_object()
-        .bucket(bucket_name)
-        .key(object_key)
-        .send()
-        .await?;
-
-    let stuff = response.body;
-    let data = stuff.collect().await.expect("error reading data").to_vec();
-    println!("data: {:?}", std::str::from_utf8(&data));
-
-    Ok(())
-}
-

alek_ext/src/download_with_remote_api_v2.rs

@@ -1,52 +0,0 @@
-/* This is a MWE of using our RemoteStorage API to call the aws stuff and download a file
- * 
- */
-
-use remote_storage::*;
-use std::path::Path;
-use std::fs::File;
-use std::io::{BufWriter, Write};
-use toml_edit;
-use anyhow;
-use tokio::io::AsyncReadExt;                                  
-
-#[tokio::main]
-async fn main() -> anyhow::Result<()> {
-    let from_path = "fuzzystrmatch.control";
-    let remote_from_path = RemotePath::new(Path::new(from_path))?;
-    println!("{:?}", remote_from_path.clone());
-
-    // read configurations from `pageserver.toml`
-    let cfg_file_path = Path::new("./../.neon/pageserver.toml");
-    let cfg_file_contents = std::fs::read_to_string(cfg_file_path).unwrap();
-    let toml = cfg_file_contents
-        .parse::<toml_edit::Document>()
-        .expect("Error parsing toml");
-    let remote_storage_data = toml.get("remote_storage")
-        .expect("field should be present");
-    let remote_storage_config = RemoteStorageConfig::from_toml(remote_storage_data)
-        .expect("error parsing toml")
-        .expect("error parsing toml");
-
-    // query S3 bucket
-    let remote_storage = GenericRemoteStorage::from_config(&remote_storage_config)?;
-    let from_path = "fuzzystrmatch.control";
-    let remote_from_path = RemotePath::new(Path::new(from_path))?;
-        
-    println!("{:?}", remote_from_path.clone());
-    // if let GenericRemoteStorage::AwsS3(mybucket) = remote_storage {
-    //     println!("{:?}",mybucket.relative_path_to_s3_object(&remote_from_path));
-    // }
-    let mut data = remote_storage.download(&remote_from_path).await.expect("data yay");
-    let mut write_data_buffer = Vec::new(); 
-    data.download_stream.read_to_end(&mut write_data_buffer).await?;
-    let f = File::create("alek.out").expect("problem creating file");
-    let mut f = BufWriter::new(f);
-    f.write_all(&mut write_data_buffer).expect("error writing data");
-
-    // let stuff = response.body;
-    // let data = stuff.collect().await.expect("error reading data").to_vec();
-    // println!("data: {:?}", std::str::from_utf8(&data));
-
-    Ok(())
-}

alek_ext/src/localfiledownload_v0.rs

@@ -1,53 +0,0 @@
-/*
- * This is a MWE of "downloading" a local file from a fake local bucket
- * */
-
-use remote_storage::*;
-use std::path::Path;
-use std::fs::File;
-use std::io::{BufWriter, Write};
-use toml_edit;
-use anyhow;
-use tokio::io::AsyncReadExt;                                  
-
-async fn download_file() -> anyhow::Result<()> {
-    // read configurations from `pageserver.toml`
-    let cfg_file_path = Path::new("./../.neon/pageserver.toml");
-    let cfg_file_contents = std::fs::read_to_string(cfg_file_path).unwrap();
-    let toml = cfg_file_contents
-        .parse::<toml_edit::Document>()
-        .expect("Error parsing toml");
-    let remote_storage_data = toml.get("remote_storage")
-        .expect("field should be present");
-    let remote_storage_config = RemoteStorageConfig::from_toml(remote_storage_data)
-        .expect("error parsing toml")
-        .expect("error parsing toml");
-
-    // query S3 bucket
-    let remote_storage = GenericRemoteStorage::from_config(&remote_storage_config)?;
-    let from_path = "neon-dev-extensions/fuzzystrmatch.control";
-    let remote_from_path = RemotePath::new(Path::new(from_path))?;
-        
-    println!("im fine");
-    println!("{:?}",remote_storage_config);
-
-    let mut data = remote_storage.download(&remote_from_path).await.expect("data yay");
-    let mut write_data_buffer = Vec::new(); 
-
-    data.download_stream.read_to_end(&mut write_data_buffer).await?;
-
-    // write `data` to a file locally
-    let f = File::create("alek.out").expect("problem creating file");
-    let mut f = BufWriter::new(f);
-    f.write_all(&mut write_data_buffer).expect("error writing data");
-
-    Ok(())
-}
-
-#[tokio::main]
-async fn main() {
-    match download_file().await {
-        Err(_)=>println!("Err"),
-        _ => println!("SUCEECESS")
-    }
-}

alek_ext/src/main.rs

@@ -1,53 +0,0 @@
-/* 
- * This is a MWE of using the RemoteStorage API to list and download files from aws
-*/
-macro_rules! alek { ($expression:expr) => { println!("{:?}", $expression); }; }
-
-use remote_storage::*;
-use std::path::Path;
-use std::fs::File;
-use std::io::{BufWriter, Write};
-use toml_edit;
-use anyhow::{self, Context};
-use tokio::io::AsyncReadExt;
-use tracing::*;
-use tracing_subscriber;
-
-#[tokio::main]
-async fn main() -> anyhow::Result<()> {
-    let subscriber = tracing_subscriber::FmtSubscriber::new();
-    tracing::subscriber::set_global_default(subscriber)?;
-    // TODO: read configs from a different place!
-    let cfg_file_path = Path::new("./../.neon/pageserver.toml");
-    let cfg_file_contents = std::fs::read_to_string(cfg_file_path)
-    .with_context(|| format!( "Failed to read pageserver config at '{}'", cfg_file_path.display()))?;
-    let toml = cfg_file_contents
-        .parse::<toml_edit::Document>()
-        .with_context(|| format!( "Failed to parse '{}' as pageserver config", cfg_file_path.display()))?;
-    let remote_storage_data = toml.get("remote_storage")
-        .context("field should be present")?;
-    let remote_storage_config = RemoteStorageConfig::from_toml(remote_storage_data)?
-        .context("error configuring remote storage")?;
-    let remote_storage = GenericRemoteStorage::from_config(&remote_storage_config)?;
-
-    let folder = RemotePath::new(Path::new("public_extensions"))?;
-    // lists all the files in the public_extensions folder
-    let from_paths = remote_storage.list_files(Some(&folder)).await?;
-    alek!(from_paths);
-    for remote_from_path in from_paths {
-        if remote_from_path.extension() == Some("control") {
-            let file_name = remote_from_path.object_name().expect("it must exist");
-            info!("{:?}", file_name);
-            alek!(&remote_from_path);
-            // download the file
-            let mut download = remote_storage.download(&remote_from_path).await?;
-            // write the file to a local location
-            let mut write_data_buffer = Vec::new(); 
-            download.download_stream.read_to_end(&mut write_data_buffer).await?;
-            let mut output_file = BufWriter::new(File::create(file_name)?);
-            output_file.write_all(&mut write_data_buffer)?;
-        }
-    }
-
-    Ok(())
-}

alek_ext/src/old_multiple_files.rs

@@ -1,65 +0,0 @@
-/* 
-**WIP**
- * This is a MWE of using our RemoteStorage API to call the aws stuff and download multiple files
-*/
-
-#![allow(unused_imports)]
-use remote_storage::*;
-use std::path::Path;
-use std::fs::File;
-use std::io::{BufWriter, Write};
-use toml_edit;
-use anyhow::{self, Context};
-use tokio::io::AsyncReadExt;                                  
-
-#[tokio::main]
-async fn main() -> anyhow::Result<()> {
-    /* me trying to hack RemotePath into submission */
-
-    let cfg_file_path = Path::new("./../.neon/pageserver.toml");
-    let cfg_file_contents = std::fs::read_to_string(cfg_file_path)
-        .expect("couldn't find pageserver.toml; make sure you are in neon/alek_ext");
-    let toml = cfg_file_contents
-        .parse::<toml_edit::Document>()
-        .expect("Error parsing toml");
-    let remote_storage_data = toml.get("remote_storage")
-        .expect("field should be present");
-    let remote_storage_config = RemoteStorageConfig::from_toml(remote_storage_data)
-        .expect("error parsing toml")
-        .expect("error parsing toml");
-    let remote_storage = GenericRemoteStorage::from_config(&remote_storage_config)?;
-
-    if let GenericRemoteStorage::AwsS3(mybucket) = remote_storage {
-        let resp = mybucket
-        .client
-        .list_objects_v2()
-        .bucket("neon-dev-extensions")
-        .set_prefix(Some("public_extensions".to_string()))
-        .delimiter("/".to_string())
-        .send().await?;
-
-        let z = resp.common_prefixes.unwrap();
-        for yy in z {
-            println!("plzplz: {:?}",yy);
-        }
-        let mut i = 0;
-        for remote_from_path in from_paths {
-            i += 1;
-            println!("{:?}", &remote_from_path);
-            if remote_from_path.extension() == Some("control") {
-                let mut data = remote_storage.download(&remote_from_path).await?;
-                // write `data` to a file locally
-                // TODO: I think that the way I'm doing this is not optimal;
-                // It should be possible to write the data directly to a file
-                // rather than first writing it to a vector...
-                let mut write_data_buffer = Vec::new(); 
-                data.download_stream.read_to_end(&mut write_data_buffer).await?;
-                let f = File::create("alek{i}.out").expect("problem creating file");
-                let mut f = BufWriter::new(f);
-                f.write_all(&mut write_data_buffer).expect("error writing data");
-            }
-        }
-    }
-
-    Ok(())
-}

compute_tools/Cargo.toml

@@ -30,5 +30,3 @@ url.workspace = true
 compute_api.workspace = true
 utils.workspace = true
 workspace_hack.workspace = true
-remote_storage = { version = "0.1", path = "../libs/remote_storage/" }
-toml_edit.workspace = true

compute_tools/src/bin/compute_ctl.rs

@@ -53,24 +53,12 @@ use compute_tools::logger::*;
 use compute_tools::monitor::launch_monitor;
 use compute_tools::params::*;
 use compute_tools::spec::*;
-use compute_tools::extensions::*;
 
-#[tokio::main]
-async fn main() -> Result<()> {
+fn main() -> Result<()> {
     init_tracing_and_logging(DEFAULT_LOG_LEVEL)?;
 
     let matches = cli().get_matches();
-    let config = get_s3_config(&matches)
-        .expect("Hopefully get_s3_config works");
-    download_extension(&config, ExtensionType::Shared)
-        .await
-        .expect("Assume downloads can't error.");
-    // let mut file = File::create("alek.txt")?;
-    // file.write_all(b"success?")?;
 
-    let http_port = *matches
-        .get_one::<u16>("http-port")
-        .expect("http-port is required");
     let pgdata = matches
         .get_one::<String>("pgdata")
         .expect("PGDATA path is required");
@@ -190,8 +178,7 @@ async fn main() -> Result<()> {
 
     // Launch http service first, so we were able to serve control-plane
     // requests, while configuration is still in progress.
-    let _http_handle =
-        launch_http_server(http_port, &compute).expect("cannot launch http endpoint thread");
+    let _http_handle = launch_http_server(&compute).expect("cannot launch http endpoint thread");
 
     if !spec_set {
         // No spec provided, hang waiting for it.
@@ -211,9 +198,6 @@ async fn main() -> Result<()> {
     // We got all we need, update the state.
     let mut state = compute.state.lock().unwrap();
 
-    // Now we have the spec, and also the tenant id, so we can download the user's personal extensions
-    // download_extension(&config, ExtensionType::Tenant(FIXME tenant_id.into()));
-
     // Record for how long we slept waiting for the spec.
     state.metrics.wait_for_spec_ms = Utc::now()
         .signed_duration_since(state.start_time)
@@ -233,9 +217,6 @@ async fn main() -> Result<()> {
     let _configurator_handle =
         launch_configurator(&compute).expect("cannot launch configurator thread");
 
-    // Now we are ready to download library extensions
-    // download_extension(&config, ExtensionType::Library(FIXME library_name.into()));
-    
     // Start Postgres
     let mut delay_exit = false;
     let mut exit_code = None;
@@ -305,14 +286,6 @@ fn cli() -> clap::Command {
     let version = option_env!("CARGO_PKG_VERSION").unwrap_or("unknown");
     clap::Command::new("compute_ctl")
         .version(version)
-        .arg(
-            Arg::new("http-port")
-                .long("http-port")
-                .value_name("HTTP_PORT")
-                .default_value("3080")
-                .value_parser(clap::value_parser!(u16))
-                .required(false),
-        )
         .arg(
             Arg::new("connstr")
                 .short('C')

compute_tools/src/compute.rs

@@ -1,3 +1,19 @@
+//
+// XXX: This starts to be scarry similar to the `PostgresNode` from `control_plane`,
+// but there are several things that makes `PostgresNode` usage inconvenient in the
+// cloud:
+// - it inherits from `LocalEnv`, which contains **all-all** the information about
+//   a complete service running
+// - it uses `PageServerNode` with information about http endpoint, which we do not
+//   need in the cloud again
+// - many tiny pieces like, for example, we do not use `pg_ctl` in the cloud
+//
+// Thus, to use `PostgresNode` in the cloud, we need to 'mock' a bunch of required
+// attributes (not required for the cloud). Yet, it is still tempting to unify these
+// `PostgresNode` and `ComputeNode` and use one in both places.
+//
+// TODO: stabilize `ComputeNode` and think about using it in the `control_plane`.
+//
 use std::fs;
 use std::os::unix::fs::PermissionsExt;
 use std::path::Path;
@@ -90,38 +106,26 @@ pub struct ParsedSpec {
 impl TryFrom<ComputeSpec> for ParsedSpec {
     type Error = String;
     fn try_from(spec: ComputeSpec) -> Result<Self, String> {
-        // Extract the options from the spec file that are needed to connect to
-        // the storage system.
-        //
-        // For backwards-compatibility, the top-level fields in the spec file
-        // may be empty. In that case, we need to dig them from the GUCs in the
-        // cluster.settings field.
         let pageserver_connstr = spec
-            .pageserver_connstring
-            .clone()
-            .or_else(|| spec.cluster.settings.find("neon.pageserver_connstring"))
+            .cluster
+            .settings
+            .find("neon.pageserver_connstring")
             .ok_or("pageserver connstr should be provided")?;
         let storage_auth_token = spec.storage_auth_token.clone();
-        let tenant_id: TenantId = if let Some(tenant_id) = spec.tenant_id {
-            tenant_id
-        } else {
-            spec.cluster
-                .settings
-                .find("neon.tenant_id")
-                .ok_or("tenant id should be provided")
-                .map(|s| TenantId::from_str(&s))?
-                .or(Err("invalid tenant id"))?
-        };
-        let timeline_id: TimelineId = if let Some(timeline_id) = spec.timeline_id {
-            timeline_id
-        } else {
-            spec.cluster
-                .settings
-                .find("neon.timeline_id")
-                .ok_or("timeline id should be provided")
-                .map(|s| TimelineId::from_str(&s))?
-                .or(Err("invalid timeline id"))?
-        };
+        let tenant_id: TenantId = spec
+            .cluster
+            .settings
+            .find("neon.tenant_id")
+            .ok_or("tenant id should be provided")
+            .map(|s| TenantId::from_str(&s))?
+            .or(Err("invalid tenant id"))?;
+        let timeline_id: TimelineId = spec
+            .cluster
+            .settings
+            .find("neon.timeline_id")
+            .ok_or("timeline id should be provided")
+            .map(|s| TimelineId::from_str(&s))?
+            .or(Err("invalid timeline id"))?;
 
         Ok(ParsedSpec {
             spec,
@@ -291,8 +295,8 @@ impl ComputeNode {
         update_pg_hba(pgdata_path)?;
 
         match spec.mode {
-            ComputeMode::Primary => {}
-            ComputeMode::Replica | ComputeMode::Static(..) => {
+            ComputeMode::Primary | ComputeMode::Static(..) => {}
+            ComputeMode::Replica => {
                 add_standby_signal(pgdata_path)?;
             }
         }
@@ -372,7 +376,7 @@ impl ComputeNode {
 
         info!(
             "finished configuration of compute for project {}",
-            spec.cluster.cluster_id.as_deref().unwrap_or("None")
+            spec.cluster.cluster_id
         );
 
         Ok(())
@@ -430,7 +434,7 @@ impl ComputeNode {
         let spec = compute_state.pspec.as_ref().expect("spec must be set");
         info!(
             "starting compute for project {}, operation {}, tenant {}, timeline {}",
-            spec.spec.cluster.cluster_id.as_deref().unwrap_or("None"),
+            spec.spec.cluster.cluster_id,
             spec.spec.operation_uuid.as_deref().unwrap_or("None"),
             spec.tenant_id,
             spec.timeline_id,

compute_tools/src/config.rs

@@ -5,7 +5,6 @@ use std::path::Path;
 
 use anyhow::Result;
 
-use crate::pg_helpers::escape_conf_value;
 use crate::pg_helpers::PgOptionsSerialize;
 use compute_api::spec::{ComputeMode, ComputeSpec};
 
@@ -37,44 +36,10 @@ pub fn write_postgres_conf(path: &Path, spec: &ComputeSpec) -> Result<()> {
     // File::create() destroys the file content if it exists.
     let mut file = File::create(path)?;
 
-    // Write the postgresql.conf content from the spec file as is.
-    if let Some(conf) = &spec.cluster.postgresql_conf {
-        writeln!(file, "{}", conf)?;
-    }
+    writeln!(file, "# Managed by compute_ctl: begin")?;
 
     write!(file, "{}", &spec.cluster.settings.as_pg_settings())?;
 
-    // Add options for connecting to storage
-    writeln!(file, "# Neon storage settings")?;
-    if let Some(s) = &spec.pageserver_connstring {
-        writeln!(
-            file,
-            "neon.pageserver_connstring='{}'",
-            escape_conf_value(s)
-        )?;
-    }
-    if !spec.safekeeper_connstrings.is_empty() {
-        writeln!(
-            file,
-            "neon.safekeepers='{}'",
-            escape_conf_value(&spec.safekeeper_connstrings.join(","))
-        )?;
-    }
-    if let Some(s) = &spec.tenant_id {
-        writeln!(
-            file,
-            "neon.tenant_id='{}'",
-            escape_conf_value(&s.to_string())
-        )?;
-    }
-    if let Some(s) = &spec.timeline_id {
-        writeln!(
-            file,
-            "neon.timeline_id='{}'",
-            escape_conf_value(&s.to_string())
-        )?;
-    }
-
     match spec.mode {
         ComputeMode::Primary => {}
         ComputeMode::Static(lsn) => {
@@ -88,12 +53,7 @@ pub fn write_postgres_conf(path: &Path, spec: &ComputeSpec) -> Result<()> {
         }
     }
 
-    // If there are any extra options in the 'settings' field, append those
-    if spec.cluster.settings.is_some() {
-        writeln!(file, "# Managed by compute_ctl: begin")?;
-        write!(file, "{}", spec.cluster.settings.as_pg_settings())?;
-        writeln!(file, "# Managed by compute_ctl: end")?;
-    }
+    writeln!(file, "# Managed by compute_ctl: end")?;
 
     Ok(())
 }

compute_tools/src/extensions.rs

@@ -1,96 +0,0 @@
-// This is some code for downloading postgres extensions from AWS s3
-use std::path::Path;
-use std::fs::File;
-use std::io::Write;
-use clap::{ArgMatches};
-use toml_edit;
-use remote_storage::*;
-
-fn get_pg_config(argument: &str) -> String {
-    // FIXME: this function panics if it runs into any issues
-    let config_output = std::process::Command::new("pg_config")
-        .arg(argument)
-        .output()
-        .expect("pg_config should be installed");
-    assert!(config_output.status.success());
-    let stdout = std::str::from_utf8(&config_output.stdout).unwrap();
-    stdout.trim().to_string()
-}
-
-fn download_helper(remote_storage: &GenericRemoteStorage, remote_from_path: &RemotePath, to_path: &str) -> anyhow::Result<()> {
-    let file_name = remote_from_path.object_name().expect("it must exist");
-    info!("Downloading {:?}",file_name);
-    let mut download = remote_storage.download(&remote_from_path).await?;
-    let mut write_data_buffer = Vec::new(); 
-    download.download_stream.read_to_end(&mut write_data_buffer).await?;
-    let mut output_file = BufWriter::new(File::create(file_name)?);
-    output_file.write_all(&mut write_data_buffer)?;
-    Ok(())
-}
-
-pub enum ExtensionType {
-    Shared,
-    Tenant(String),
-    Library(String)
-}
-
-pub async fn download_extension(config: &RemoteStorageConfig, ext_type: ExtensionType) -> anyhow::Result<()>{
-    let sharedir = get_pg_config("--sharedir");
-    let sharedir = format!("{}/extension", sharedir);
-    let libdir = get_pg_config("--libdir");
-    let remote_storage = GenericRemoteStorage::from_config(config)?;
-
-    match ext_type {
-        ExtensionType::Shared => {
-            // 1. Download control files from s3-bucket/public/*.control to SHAREDIR/extension
-            // We can do this step even before we have spec,
-            // because public extensions are common for all projects.
-            let folder = RemotePath::new(Path::new("public_extensions"))?;
-            let from_paths = remote_storage.list_files(Some(&folder)).await?;
-            for remote_from_path in from_paths {
-                if remote_from_path.extension() == Some("control") {
-                    // FIXME: CAUTION: if you run this, it will actually write stuff to my postgress directory
-                    // but atm that stuff that it is going to write is not good. 
-                    // don't run atm without changing path
-                    download_helper(&remote_storage, &remote_from_path, &sharedir)?;
-                }
-            }
-        }
-        ExtensionType::Tenant(tenant_id) => {
-            // 2. After we have spec, before project start
-            // Download control files from s3-bucket/[tenant-id]/*.control to SHAREDIR/extension
-            let folder = RemotePath::new(Path::new(format!("{tenant_id}")))?;
-            let from_paths = remote_storage.list_files(Some(&folder)).await?;
-            for remote_from_path in from_paths {
-                if remote_from_path.extension() == Some("control") {
-                    download_helper(&remote_storage, &remote_from_path, &sharedir)?;
-                }
-            }
-        }
-        ExtensionType::Library(library_name) => {
-            // 3. After we have spec, before postgres start
-            // Download preload_shared_libraries from s3-bucket/public/[library-name].control into LIBDIR/
-            let from_path = format!("neon-dev-extensions/public/{library_name}.control");
-            let remote_from_path = RemotePath::new(Path::new(&from_path))?;
-            download_helper(&remote_storage, &remote_from_path, &libdir)?;
-        }
-    }
-    Ok(())
-}
-
-pub fn get_s3_config(arg_matches: &ArgMatches) -> anyhow::Result<RemoteStorageConfig> {
-    // TODO: Right now we are using the same config parameters as pageserver; but should we have our own configs?
-    // TODO: Should we read the s3_config from CLI arguments?
-    let cfg_file_path = Path::new("./../.neon/pageserver.toml");
-    let cfg_file_contents = std::fs::read_to_string(cfg_file_path)
-    .with_context(|| format!( "Failed to read pageserver config at '{}'", cfg_file_path.display()))?;
-    let toml = cfg_file_contents
-        .parse::<toml_edit::Document>()
-        .with_context(|| format!( "Failed to parse '{}' as pageserver config", cfg_file_path.display()))?;
-    let remote_storage_data = toml.get("remote_storage")
-        .context("field should be present")?;
-    let remote_storage_config = RemoteStorageConfig::from_toml(remote_storage_data)?
-        .context("error configuring remote storage")?;
-    Ok(remote_storage_config)
-}
-

compute_tools/src/http/api.rs

@@ -220,8 +220,8 @@ fn render_json_error(e: &str, status: StatusCode) -> Response<Body> {
 
 // Main Hyper HTTP server function that runs it and blocks waiting on it forever.
 #[tokio::main]
-async fn serve(port: u16, state: Arc<ComputeNode>) {
-    let addr = SocketAddr::from(([0, 0, 0, 0], port));
+async fn serve(state: Arc<ComputeNode>) {
+    let addr = SocketAddr::from(([0, 0, 0, 0], 3080));
 
     let make_service = make_service_fn(move |_conn| {
         let state = state.clone();
@@ -256,10 +256,10 @@ async fn serve(port: u16, state: Arc<ComputeNode>) {
 }
 
 /// Launch a separate Hyper HTTP API server thread and return its `JoinHandle`.
-pub fn launch_http_server(port: u16, state: &Arc<ComputeNode>) -> Result<thread::JoinHandle<()>> {
+pub fn launch_http_server(state: &Arc<ComputeNode>) -> Result<thread::JoinHandle<()>> {
     let state = Arc::clone(state);
 
     Ok(thread::Builder::new()
         .name("http-endpoint".into())
-        .spawn(move || serve(port, state))?)
+        .spawn(move || serve(state))?)
 }

compute_tools/src/lib.rs

@@ -13,4 +13,3 @@ pub mod monitor;
 pub mod params;
 pub mod pg_helpers;
 pub mod spec;
-pub mod extensions;

compute_tools/src/pg_helpers.rs

@@ -23,7 +23,7 @@ fn escape_literal(s: &str) -> String {
 
 /// Escape a string so that it can be used in postgresql.conf.
 /// Same as escape_literal, currently.
-pub fn escape_conf_value(s: &str) -> String {
+fn escape_conf_value(s: &str) -> String {
     s.replace('\'', "''").replace('\\', "\\\\")
 }
 

control_plane/src/bin/neon_local.rs

@@ -476,11 +476,10 @@ fn handle_timeline(timeline_match: &ArgMatches, env: &mut local_env::LocalEnv) -
 
             println!("Creating endpoint for imported timeline ...");
             cplane.new_endpoint(
-                name,
                 tenant_id,
+                name,
                 timeline_id,
                 None,
-                None,
                 pg_version,
                 ComputeMode::Primary,
             )?;
@@ -592,7 +591,7 @@ fn handle_endpoint(ep_match: &ArgMatches, env: &local_env::LocalEnv) -> Result<(
 
                 table.add_row([
                     endpoint_id.as_str(),
-                    &endpoint.pg_address.to_string(),
+                    &endpoint.address.to_string(),
                     &endpoint.timeline_id.to_string(),
                     branch_name,
                     lsn_str.as_str(),
@@ -621,8 +620,8 @@ fn handle_endpoint(ep_match: &ArgMatches, env: &local_env::LocalEnv) -> Result<(
                 .get_branch_timeline_id(branch_name, tenant_id)
                 .ok_or_else(|| anyhow!("Found no timeline id for branch name '{branch_name}'"))?;
 
-            let pg_port: Option<u16> = sub_args.get_one::<u16>("pg-port").copied();
-            let http_port: Option<u16> = sub_args.get_one::<u16>("http-port").copied();
+            let port: Option<u16> = sub_args.get_one::<u16>("port").copied();
+
             let pg_version = sub_args
                 .get_one::<u32>("pg-version")
                 .copied()
@@ -640,38 +639,14 @@ fn handle_endpoint(ep_match: &ArgMatches, env: &local_env::LocalEnv) -> Result<(
                 (Some(_), true) => anyhow::bail!("cannot specify both lsn and hot-standby"),
             };
 
-            cplane.new_endpoint(
-                &endpoint_id,
-                tenant_id,
-                timeline_id,
-                pg_port,
-                http_port,
-                pg_version,
-                mode,
-            )?;
+            cplane.new_endpoint(tenant_id, &endpoint_id, timeline_id, port, pg_version, mode)?;
         }
         "start" => {
-            let pg_port: Option<u16> = sub_args.get_one::<u16>("pg-port").copied();
-            let http_port: Option<u16> = sub_args.get_one::<u16>("http-port").copied();
+            let port: Option<u16> = sub_args.get_one::<u16>("port").copied();
             let endpoint_id = sub_args
                 .get_one::<String>("endpoint_id")
                 .ok_or_else(|| anyhow!("No endpoint ID was provided to start"))?;
 
-            // If --safekeepers argument is given, use only the listed safekeeper nodes.
-            let safekeepers =
-                if let Some(safekeepers_str) = sub_args.get_one::<String>("safekeepers") {
-                    let mut safekeepers: Vec<NodeId> = Vec::new();
-                    for sk_id in safekeepers_str.split(',').map(str::trim) {
-                        let sk_id = NodeId(u64::from_str(sk_id).map_err(|_| {
-                            anyhow!("invalid node ID \"{sk_id}\" in --safekeepers list")
-                        })?);
-                        safekeepers.push(sk_id);
-                    }
-                    safekeepers
-                } else {
-                    env.safekeepers.iter().map(|sk| sk.id).collect()
-                };
-
             let endpoint = cplane.endpoints.get(endpoint_id.as_str());
 
             let auth_token = if matches!(env.pageserver.pg_auth_type, AuthType::NeonJWT) {
@@ -698,7 +673,7 @@ fn handle_endpoint(ep_match: &ArgMatches, env: &local_env::LocalEnv) -> Result<(
                     _ => {}
                 }
                 println!("Starting existing endpoint {endpoint_id}...");
-                endpoint.start(&auth_token, safekeepers)?;
+                endpoint.start(&auth_token)?;
             } else {
                 let branch_name = sub_args
                     .get_one::<String>("branch-name")
@@ -734,15 +709,14 @@ fn handle_endpoint(ep_match: &ArgMatches, env: &local_env::LocalEnv) -> Result<(
                 println!("Starting new endpoint {endpoint_id} (PostgreSQL v{pg_version}) on timeline {timeline_id} ...");
 
                 let ep = cplane.new_endpoint(
-                    endpoint_id,
                     tenant_id,
+                    endpoint_id,
                     timeline_id,
-                    pg_port,
-                    http_port,
+                    port,
                     pg_version,
                     mode,
                 )?;
-                ep.start(&auth_token, safekeepers)?;
+                ep.start(&auth_token)?;
             }
         }
         "stop" => {
@@ -970,22 +944,11 @@ fn cli() -> Command {
         .value_parser(value_parser!(u32))
         .default_value(DEFAULT_PG_VERSION);
 
-    let pg_port_arg = Arg::new("pg-port")
-        .long("pg-port")
+    let port_arg = Arg::new("port")
+        .long("port")
         .required(false)
         .value_parser(value_parser!(u16))
-        .value_name("pg-port");
-
-    let http_port_arg = Arg::new("http-port")
-        .long("http-port")
-        .required(false)
-        .value_parser(value_parser!(u16))
-        .value_name("http-port");
-
-    let safekeepers_arg = Arg::new("safekeepers")
-        .long("safekeepers")
-        .required(false)
-        .value_name("safekeepers");
+        .value_name("port");
 
     let stop_mode_arg = Arg::new("stop-mode")
         .short('m')
@@ -1130,8 +1093,7 @@ fn cli() -> Command {
                     .arg(branch_name_arg.clone())
                     .arg(tenant_id_arg.clone())
                     .arg(lsn_arg.clone())
-                    .arg(pg_port_arg.clone())
-                    .arg(http_port_arg.clone())
+                    .arg(port_arg.clone())
                     .arg(
                         Arg::new("config-only")
                             .help("Don't do basebackup, create endpoint directory with only config files")
@@ -1147,11 +1109,9 @@ fn cli() -> Command {
                     .arg(branch_name_arg)
                     .arg(timeline_id_arg)
                     .arg(lsn_arg)
-                    .arg(pg_port_arg)
-                    .arg(http_port_arg)
+                    .arg(port_arg)
                     .arg(pg_version_arg)
                     .arg(hot_standby_arg)
-                    .arg(safekeepers_arg)
                 )
                 .subcommand(
                     Command::new("stop")

control_plane/src/broker.rs

@@ -1,9 +1,3 @@
-//! Code to manage the storage broker
-//!
-//! In the local test environment, the data for each safekeeper is stored in
-//!
-//!   .neon/safekeepers/<safekeeper id>
-//!
 use anyhow::Context;
 
 use std::path::PathBuf;

control_plane/src/endpoint.rs

@@ -1,71 +1,40 @@
-//! Code to manage compute endpoints
-//!
-//! In the local test environment, the data for each endpoint is stored in
-//!
-//!   .neon/endpoints/<endpoint id>
-//!
-//! Some basic information about the endpoint, like the tenant and timeline IDs,
-//! are stored in the `endpoint.json` file. The `endpoint.json` file is created
-//! when the endpoint is created, and doesn't change afterwards.
-//!
-//! The endpoint is managed by the `compute_ctl` binary. When an endpoint is
-//! started, we launch `compute_ctl` It synchronizes the safekeepers, downloads
-//! the basebackup from the pageserver to initialize the the data directory, and
-//! finally launches the PostgreSQL process. It watches the PostgreSQL process
-//! until it exits.
-//!
-//! When an endpoint is created, a `postgresql.conf` file is also created in
-//! the endpoint's directory. The file can be modified before starting PostgreSQL.
-//! However, the `postgresql.conf` file in the endpoint directory is not used directly
-//! by PostgreSQL. It is passed to `compute_ctl`, and `compute_ctl` writes another
-//! copy of it in the data directory.
-//!
-//! Directory contents:
-//!
-//! ```ignore
-//! .neon/endpoints/main/
-//!     compute.log               - log output of `compute_ctl` and `postgres`
-//!     endpoint.json             - serialized `EndpointConf` struct
-//!     postgresql.conf           - postgresql settings
-//!     spec.json                 - passed to `compute_ctl`
-//!     pgdata/
-//!         postgresql.conf       - copy of postgresql.conf created by `compute_ctl`
-//!         zenith.signal
-//!         <other PostgreSQL files>
-//! ```
-//!
 use std::collections::BTreeMap;
+use std::fs::{self, File};
+use std::io::Write;
 use std::net::SocketAddr;
 use std::net::TcpStream;
+use std::os::unix::fs::PermissionsExt;
 use std::path::PathBuf;
-use std::process::Command;
+use std::process::{Command, Stdio};
+use std::str::FromStr;
 use std::sync::Arc;
 use std::time::Duration;
 
-use anyhow::{anyhow, bail, Context, Result};
+use anyhow::{Context, Result};
 use serde::{Deserialize, Serialize};
 use serde_with::{serde_as, DisplayFromStr};
-use utils::id::{NodeId, TenantId, TimelineId};
+use utils::{
+    id::{TenantId, TimelineId},
+    lsn::Lsn,
+};
 
 use crate::local_env::LocalEnv;
 use crate::pageserver::PageServerNode;
 use crate::postgresql_conf::PostgresConf;
 
-use compute_api::responses::{ComputeState, ComputeStatus};
-use compute_api::spec::{Cluster, ComputeMode, ComputeSpec};
+use compute_api::spec::ComputeMode;
 
 // contents of a endpoint.json file
 #[serde_as]
 #[derive(Serialize, Deserialize, PartialEq, Eq, Clone, Debug)]
 pub struct EndpointConf {
-    endpoint_id: String,
+    name: String,
     #[serde_as(as = "DisplayFromStr")]
     tenant_id: TenantId,
     #[serde_as(as = "DisplayFromStr")]
     timeline_id: TimelineId,
     mode: ComputeMode,
-    pg_port: u16,
-    http_port: u16,
+    port: u16,
     pg_version: u32,
 }
 
@@ -88,11 +57,11 @@ impl ComputeControlPlane {
         let pageserver = Arc::new(PageServerNode::from_env(&env));
 
         let mut endpoints = BTreeMap::default();
-        for endpoint_dir in std::fs::read_dir(env.endpoints_path())
+        for endpoint_dir in fs::read_dir(env.endpoints_path())
             .with_context(|| format!("failed to list {}", env.endpoints_path().display()))?
         {
             let ep = Endpoint::from_dir_entry(endpoint_dir?, &env, &pageserver)?;
-            endpoints.insert(ep.endpoint_id.clone(), Arc::new(ep));
+            endpoints.insert(ep.name.clone(), Arc::new(ep));
         }
 
         Ok(ComputeControlPlane {
@@ -107,28 +76,25 @@ impl ComputeControlPlane {
         1 + self
             .endpoints
             .values()
-            .map(|ep| std::cmp::max(ep.pg_address.port(), ep.http_address.port()))
+            .map(|ep| ep.address.port())
             .max()
             .unwrap_or(self.base_port)
     }
 
-    #[allow(clippy::too_many_arguments)]
     pub fn new_endpoint(
         &mut self,
-        endpoint_id: &str,
         tenant_id: TenantId,
+        name: &str,
         timeline_id: TimelineId,
-        pg_port: Option<u16>,
-        http_port: Option<u16>,
+        port: Option<u16>,
         pg_version: u32,
         mode: ComputeMode,
     ) -> Result<Arc<Endpoint>> {
-        let pg_port = pg_port.unwrap_or_else(|| self.get_port());
-        let http_port = http_port.unwrap_or_else(|| self.get_port() + 1);
+        let port = port.unwrap_or_else(|| self.get_port());
+
         let ep = Arc::new(Endpoint {
-            endpoint_id: endpoint_id.to_owned(),
-            pg_address: SocketAddr::new("127.0.0.1".parse().unwrap(), pg_port),
-            http_address: SocketAddr::new("127.0.0.1".parse().unwrap(), http_port),
+            name: name.to_owned(),
+            address: SocketAddr::new("127.0.0.1".parse().unwrap(), port),
             env: self.env.clone(),
             pageserver: Arc::clone(&self.pageserver),
             timeline_id,
@@ -136,27 +102,21 @@ impl ComputeControlPlane {
             tenant_id,
             pg_version,
         });
-
-        ep.create_endpoint_dir()?;
+        ep.create_pgdata()?;
         std::fs::write(
             ep.endpoint_path().join("endpoint.json"),
             serde_json::to_string_pretty(&EndpointConf {
-                endpoint_id: endpoint_id.to_string(),
+                name: name.to_string(),
                 tenant_id,
                 timeline_id,
                 mode,
-                http_port,
-                pg_port,
+                port,
                 pg_version,
             })?,
         )?;
-        std::fs::write(
-            ep.endpoint_path().join("postgresql.conf"),
-            ep.setup_pg_conf()?.to_string(),
-        )?;
+        ep.setup_pg_conf()?;
 
-        self.endpoints
-            .insert(ep.endpoint_id.clone(), Arc::clone(&ep));
+        self.endpoints.insert(ep.name.clone(), Arc::clone(&ep));
 
         Ok(ep)
     }
@@ -167,15 +127,13 @@ impl ComputeControlPlane {
 #[derive(Debug)]
 pub struct Endpoint {
     /// used as the directory name
-    endpoint_id: String,
+    name: String,
     pub tenant_id: TenantId,
     pub timeline_id: TimelineId,
     pub mode: ComputeMode,
 
-    // port and address of the Postgres server and `compute_ctl`'s HTTP API
-    pub pg_address: SocketAddr,
-    pub http_address: SocketAddr,
-
+    // port and address of the Postgres server
+    pub address: SocketAddr,
     // postgres major version in the format: 14, 15, etc.
     pg_version: u32,
 
@@ -200,16 +158,16 @@ impl Endpoint {
 
         // parse data directory name
         let fname = entry.file_name();
-        let endpoint_id = fname.to_str().unwrap().to_string();
+        let name = fname.to_str().unwrap().to_string();
 
         // Read the endpoint.json file
         let conf: EndpointConf =
             serde_json::from_slice(&std::fs::read(entry.path().join("endpoint.json"))?)?;
 
+        // ok now
         Ok(Endpoint {
-            pg_address: SocketAddr::new("127.0.0.1".parse().unwrap(), conf.pg_port),
-            http_address: SocketAddr::new("127.0.0.1".parse().unwrap(), conf.http_port),
-            endpoint_id,
+            address: SocketAddr::new("127.0.0.1".parse().unwrap(), conf.port),
+            name,
             env: env.clone(),
             pageserver: Arc::clone(pageserver),
             timeline_id: conf.timeline_id,
@@ -219,17 +177,104 @@ impl Endpoint {
         })
     }
 
-    fn create_endpoint_dir(&self) -> Result<()> {
-        std::fs::create_dir_all(self.endpoint_path()).with_context(|| {
-            format!(
-                "could not create endpoint directory {}",
-                self.endpoint_path().display()
+    fn sync_safekeepers(&self, auth_token: &Option<String>, pg_version: u32) -> Result<Lsn> {
+        let pg_path = self.env.pg_bin_dir(pg_version)?.join("postgres");
+        let mut cmd = Command::new(pg_path);
+
+        cmd.arg("--sync-safekeepers")
+            .env_clear()
+            .env(
+                "LD_LIBRARY_PATH",
+                self.env.pg_lib_dir(pg_version)?.to_str().unwrap(),
             )
-        })
+            .env(
+                "DYLD_LIBRARY_PATH",
+                self.env.pg_lib_dir(pg_version)?.to_str().unwrap(),
+            )
+            .env("PGDATA", self.pgdata().to_str().unwrap())
+            .stdout(Stdio::piped())
+            // Comment this to avoid capturing stderr (useful if command hangs)
+            .stderr(Stdio::piped());
+
+        if let Some(token) = auth_token {
+            cmd.env("NEON_AUTH_TOKEN", token);
+        }
+
+        let sync_handle = cmd
+            .spawn()
+            .expect("postgres --sync-safekeepers failed to start");
+
+        let sync_output = sync_handle
+            .wait_with_output()
+            .expect("postgres --sync-safekeepers failed");
+        if !sync_output.status.success() {
+            anyhow::bail!(
+                "sync-safekeepers failed: '{}'",
+                String::from_utf8_lossy(&sync_output.stderr)
+            );
+        }
+
+        let lsn = Lsn::from_str(std::str::from_utf8(&sync_output.stdout)?.trim())?;
+        println!("Safekeepers synced on {}", lsn);
+        Ok(lsn)
     }
 
-    // Generate postgresql.conf with default configuration
-    fn setup_pg_conf(&self) -> Result<PostgresConf> {
+    /// Get basebackup from the pageserver as a tar archive and extract it
+    /// to the `self.pgdata()` directory.
+    fn do_basebackup(&self, lsn: Option<Lsn>) -> Result<()> {
+        println!(
+            "Extracting base backup to create postgres instance: path={} port={}",
+            self.pgdata().display(),
+            self.address.port()
+        );
+
+        let sql = if let Some(lsn) = lsn {
+            format!("basebackup {} {} {}", self.tenant_id, self.timeline_id, lsn)
+        } else {
+            format!("basebackup {} {}", self.tenant_id, self.timeline_id)
+        };
+
+        let mut client = self
+            .pageserver
+            .page_server_psql_client()
+            .context("connecting to page server failed")?;
+
+        let copyreader = client
+            .copy_out(sql.as_str())
+            .context("page server 'basebackup' command failed")?;
+
+        // Read the archive directly from the `CopyOutReader`
+        //
+        // Set `ignore_zeros` so that unpack() reads all the Copy data and
+        // doesn't stop at the end-of-archive marker. Otherwise, if the server
+        // sends an Error after finishing the tarball, we will not notice it.
+        let mut ar = tar::Archive::new(copyreader);
+        ar.set_ignore_zeros(true);
+        ar.unpack(&self.pgdata())
+            .context("extracting base backup failed")?;
+
+        Ok(())
+    }
+
+    fn create_pgdata(&self) -> Result<()> {
+        fs::create_dir_all(self.pgdata()).with_context(|| {
+            format!(
+                "could not create data directory {}",
+                self.pgdata().display()
+            )
+        })?;
+        fs::set_permissions(self.pgdata().as_path(), fs::Permissions::from_mode(0o700))
+            .with_context(|| {
+                format!(
+                    "could not set permissions in data directory {}",
+                    self.pgdata().display()
+                )
+            })
+    }
+
+    // Write postgresql.conf with default configuration
+    // and PG_VERSION file to the data directory of a new endpoint.
+    fn setup_pg_conf(&self) -> Result<()> {
         let mut conf = PostgresConf::new();
         conf.append("max_wal_senders", "10");
         conf.append("wal_log_hints", "off");
@@ -242,14 +287,25 @@ impl Endpoint {
         // wal_sender_timeout is the maximum time to wait for WAL replication.
         // It also defines how often the walreciever will send a feedback message to the wal sender.
         conf.append("wal_sender_timeout", "5s");
-        conf.append("listen_addresses", &self.pg_address.ip().to_string());
-        conf.append("port", &self.pg_address.port().to_string());
+        conf.append("listen_addresses", &self.address.ip().to_string());
+        conf.append("port", &self.address.port().to_string());
         conf.append("wal_keep_size", "0");
         // walproposer panics when basebackup is invalid, it is pointless to restart in this case.
         conf.append("restart_after_crash", "off");
 
-        // Load the 'neon' extension
+        // Configure the Neon Postgres extension to fetch pages from pageserver
+        let pageserver_connstr = {
+            let config = &self.pageserver.pg_connection_config;
+            let (host, port) = (config.host(), config.port());
+
+            // NOTE: avoid spaces in connection string, because it is less error prone if we forward it somewhere.
+            format!("postgresql://no_user@{host}:{port}")
+        };
         conf.append("shared_preload_libraries", "neon");
+        conf.append_line("");
+        conf.append("neon.pageserver_connstring", &pageserver_connstr);
+        conf.append("neon.tenant_id", &self.tenant_id.to_string());
+        conf.append("neon.timeline_id", &self.timeline_id.to_string());
 
         conf.append_line("");
         // Replication-related configurations, such as WAL sending
@@ -334,11 +390,46 @@ impl Endpoint {
             }
         }
 
-        Ok(conf)
+        let mut file = File::create(self.pgdata().join("postgresql.conf"))?;
+        file.write_all(conf.to_string().as_bytes())?;
+
+        let mut file = File::create(self.pgdata().join("PG_VERSION"))?;
+        file.write_all(self.pg_version.to_string().as_bytes())?;
+
+        Ok(())
+    }
+
+    fn load_basebackup(&self, auth_token: &Option<String>) -> Result<()> {
+        let backup_lsn = match &self.mode {
+            ComputeMode::Primary => {
+                if !self.env.safekeepers.is_empty() {
+                    // LSN 0 means that it is bootstrap and we need to download just
+                    // latest data from the pageserver. That is a bit clumsy but whole bootstrap
+                    // procedure evolves quite actively right now, so let's think about it again
+                    // when things would be more stable (TODO).
+                    let lsn = self.sync_safekeepers(auth_token, self.pg_version)?;
+                    if lsn == Lsn(0) {
+                        None
+                    } else {
+                        Some(lsn)
+                    }
+                } else {
+                    None
+                }
+            }
+            ComputeMode::Static(lsn) => Some(*lsn),
+            ComputeMode::Replica => {
+                None // Take the latest snapshot available to start with
+            }
+        };
+
+        self.do_basebackup(backup_lsn)?;
+
+        Ok(())
     }
 
     pub fn endpoint_path(&self) -> PathBuf {
-        self.env.endpoints_path().join(&self.endpoint_id)
+        self.env.endpoints_path().join(&self.name)
     }
 
     pub fn pgdata(&self) -> PathBuf {
@@ -348,7 +439,7 @@ impl Endpoint {
     pub fn status(&self) -> &str {
         let timeout = Duration::from_millis(300);
         let has_pidfile = self.pgdata().join("postmaster.pid").exists();
-        let can_connect = TcpStream::connect_timeout(&self.pg_address, timeout).is_ok();
+        let can_connect = TcpStream::connect_timeout(&self.address, timeout).is_ok();
 
         match (has_pidfile, can_connect) {
             (true, true) => "running",
@@ -366,6 +457,8 @@ impl Endpoint {
                 &[
                     "-D",
                     self.pgdata().to_str().unwrap(),
+                    "-l",
+                    self.pgdata().join("pg.log").to_str().unwrap(),
                     "-w", //wait till pg_ctl actually does what was asked
                 ],
                 args,
@@ -401,183 +494,36 @@ impl Endpoint {
         Ok(())
     }
 
-    pub fn start(&self, auth_token: &Option<String>, safekeepers: Vec<NodeId>) -> Result<()> {
+    pub fn start(&self, auth_token: &Option<String>) -> Result<()> {
         if self.status() == "running" {
             anyhow::bail!("The endpoint is already running");
         }
 
-        // Slurp the endpoints/<endpoint id>/postgresql.conf file into
-        // memory. We will include it in the spec file that we pass to
-        // `compute_ctl`, and `compute_ctl` will write it to the postgresql.conf
-        // in the data directory.
-        let postgresql_conf_path = self.endpoint_path().join("postgresql.conf");
-        let postgresql_conf = match std::fs::read(&postgresql_conf_path) {
-            Ok(content) => String::from_utf8(content)?,
-            Err(e) if e.kind() == std::io::ErrorKind::NotFound => "".to_string(),
-            Err(e) => {
-                return Err(anyhow::Error::new(e).context(format!(
-                    "failed to read config file in {}",
-                    postgresql_conf_path.to_str().unwrap()
-                )))
-            }
-        };
-
-        // We always start the compute node from scratch, so if the Postgres
-        // data dir exists from a previous launch, remove it first.
-        if self.pgdata().exists() {
-            std::fs::remove_dir_all(self.pgdata())?;
-        }
-
-        let pageserver_connstring = {
-            let config = &self.pageserver.pg_connection_config;
-            let (host, port) = (config.host(), config.port());
-
-            // NOTE: avoid spaces in connection string, because it is less error prone if we forward it somewhere.
-            format!("postgresql://no_user@{host}:{port}")
-        };
-        let mut safekeeper_connstrings = Vec::new();
-        if self.mode == ComputeMode::Primary {
-            for sk_id in safekeepers {
-                let sk = self
-                    .env
-                    .safekeepers
-                    .iter()
-                    .find(|node| node.id == sk_id)
-                    .ok_or_else(|| anyhow!("safekeeper {sk_id} does not exist"))?;
-                safekeeper_connstrings.push(format!("127.0.0.1:{}", sk.pg_port));
-            }
-        }
-
-        // Create spec file
-        let spec = ComputeSpec {
-            format_version: 1.0,
-            operation_uuid: None,
-            cluster: Cluster {
-                cluster_id: None, // project ID: not used
-                name: None,       // project name: not used
-                state: None,
-                roles: vec![],
-                databases: vec![],
-                settings: None,
-                postgresql_conf: Some(postgresql_conf),
-            },
-            delta_operations: None,
-            tenant_id: Some(self.tenant_id),
-            timeline_id: Some(self.timeline_id),
-            mode: self.mode,
-            pageserver_connstring: Some(pageserver_connstring),
-            safekeeper_connstrings,
-            storage_auth_token: auth_token.clone(),
-        };
-        let spec_path = self.endpoint_path().join("spec.json");
-        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()
-            .create(true)
-            .append(true)
-            .open(self.endpoint_path().join("compute.log"))?;
-
-        // Launch compute_ctl
-        println!("Starting postgres node at '{}'", self.connstr());
-        let mut cmd = Command::new(self.env.neon_distrib_dir.join("compute_ctl"));
-        cmd.args(["--http-port", &self.http_address.port().to_string()])
-            .args(["--pgdata", self.pgdata().to_str().unwrap()])
-            .args(["--connstr", &self.connstr()])
-            .args([
-                "--spec-path",
-                self.endpoint_path().join("spec.json").to_str().unwrap(),
-            ])
-            .args([
-                "--pgbin",
-                self.env
-                    .pg_bin_dir(self.pg_version)?
-                    .join("postgres")
-                    .to_str()
-                    .unwrap(),
-            ])
-            .stdin(std::process::Stdio::null())
-            .stderr(logfile.try_clone()?)
-            .stdout(logfile);
-        let _child = cmd.spawn()?;
-
-        // Wait for it to start
-        let mut attempt = 0;
-        const ATTEMPT_INTERVAL: Duration = Duration::from_millis(100);
-        const MAX_ATTEMPTS: u32 = 10 * 30; // Wait up to 30 s
-        loop {
-            attempt += 1;
-            match self.get_status() {
-                Ok(state) => {
-                    match state.status {
-                        ComputeStatus::Init => {
-                            if attempt == MAX_ATTEMPTS {
-                                bail!("compute startup timed out; still in Init state");
-                            }
-                            // keep retrying
-                        }
-                        ComputeStatus::Running => {
-                            // All good!
-                            break;
-                        }
-                        ComputeStatus::Failed => {
-                            bail!(
-                                "compute startup failed: {}",
-                                state
-                                    .error
-                                    .as_deref()
-                                    .unwrap_or("<no error from compute_ctl>")
-                            );
-                        }
-                        ComputeStatus::Empty
-                        | ComputeStatus::ConfigurationPending
-                        | ComputeStatus::Configuration => {
-                            bail!("unexpected compute status: {:?}", state.status)
-                        }
-                    }
-                }
-                Err(e) => {
-                    if attempt == MAX_ATTEMPTS {
-                        return Err(e).context(
-                            "timed out waiting to connect to compute_ctl HTTP; last error: {e}",
-                        );
-                    }
-                }
-            }
-            std::thread::sleep(ATTEMPT_INTERVAL);
-        }
-
-        Ok(())
-    }
-
-    // Call the /status HTTP API
-    pub fn get_status(&self) -> Result<ComputeState> {
-        let client = reqwest::blocking::Client::new();
-
-        let response = client
-            .request(
-                reqwest::Method::GET,
-                format!(
-                    "http://{}:{}/status",
-                    self.http_address.ip(),
-                    self.http_address.port()
-                ),
+        // 1. We always start Postgres from scratch, so
+        // if old dir exists, preserve 'postgresql.conf' and drop the directory
+        let postgresql_conf_path = self.pgdata().join("postgresql.conf");
+        let postgresql_conf = fs::read(&postgresql_conf_path).with_context(|| {
+            format!(
+                "failed to read config file in {}",
+                postgresql_conf_path.to_str().unwrap()
             )
-            .send()?;
+        })?;
+        fs::remove_dir_all(self.pgdata())?;
+        self.create_pgdata()?;
 
-        // Interpret the response
-        let status = response.status();
-        if !(status.is_client_error() || status.is_server_error()) {
-            Ok(response.json()?)
-        } else {
-            // reqwest does not export its error construction utility functions, so let's craft the message ourselves
-            let url = response.url().to_owned();
-            let msg = match response.text() {
-                Ok(err_body) => format!("Error: {}", err_body),
-                Err(_) => format!("Http error ({}) at {}.", status.as_u16(), url),
-            };
-            Err(anyhow::anyhow!(msg))
+        // 2. Bring back config files
+        fs::write(&postgresql_conf_path, postgresql_conf)?;
+
+        // 3. Load basebackup
+        self.load_basebackup(auth_token)?;
+
+        if self.mode != ComputeMode::Primary {
+            File::create(self.pgdata().join("standby.signal"))?;
         }
+
+        // 4. Finally start postgres
+        println!("Starting postgres at '{}'", self.connstr());
+        self.pg_ctl(&["start"], auth_token)
     }
 
     pub fn stop(&self, destroy: bool) -> Result<()> {
@@ -594,7 +540,7 @@ impl Endpoint {
                 "Destroying postgres data directory '{}'",
                 self.pgdata().to_str().unwrap()
             );
-            std::fs::remove_dir_all(self.endpoint_path())?;
+            fs::remove_dir_all(self.endpoint_path())?;
         } else {
             self.pg_ctl(&["stop"], &None)?;
         }
@@ -603,10 +549,10 @@ impl Endpoint {
 
     pub fn connstr(&self) -> String {
         format!(
-            "postgresql://{}@{}:{}/{}",
+            "host={} port={} user={} dbname={}",
+            self.address.ip(),
+            self.address.port(),
             "cloud_admin",
-            self.pg_address.ip(),
-            self.pg_address.port(),
             "postgres"
         )
     }

control_plane/src/local_env.rs

@@ -37,7 +37,7 @@ pub const DEFAULT_PG_VERSION: u32 = 15;
 #[derive(Serialize, Deserialize, PartialEq, Eq, Clone, Debug)]
 pub struct LocalEnv {
     // Base directory for all the nodes (the pageserver, safekeepers and
-    // compute endpoints).
+    // compute nodes).
     //
     // This is not stored in the config file. Rather, this is the path where the
     // config file itself is. It is read from the NEON_REPO_DIR env variable or

control_plane/src/pageserver.rs

@@ -1,9 +1,3 @@
-//! Code to manage pageservers
-//!
-//! In the local test environment, the pageserver stores its data directly in
-//!
-//!   .neon/
-//!
 use std::borrow::Cow;
 use std::collections::HashMap;
 use std::fs::File;

control_plane/src/safekeeper.rs

@@ -1,9 +1,3 @@
-//! Code to manage safekeepers
-//!
-//! In the local test environment, the data for each safekeeper is stored in
-//!
-//!   .neon/safekeepers/<safekeeper id>
-//!
 use std::io::Write;
 use std::path::PathBuf;
 use std::process::Child;

fakes3/public_extensions/fuzzystrmatch.control

@@ -1,5 +0,0 @@
-this 
-Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.
-
-lol
-

fakes3/public_extensions/pg_cron.control

@@ -1,7 +0,0 @@
-this 
-ax mod p  < p/2
-
-its gonna be big! 
-coming soon at 4pm
-lol
-

fakes3/tenant123/test.control

@@ -1,3 +0,0 @@
-
-Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.
-

libs/compute_api/src/responses.rs

@@ -5,13 +5,13 @@ use serde::{Deserialize, Serialize, Serializer};
 
 use crate::spec::ComputeSpec;
 
-#[derive(Serialize, Debug, Deserialize)]
+#[derive(Serialize, Debug)]
 pub struct GenericAPIError {
     pub error: String,
 }
 
 /// Response of the /status API
-#[derive(Serialize, Debug, Deserialize)]
+#[derive(Serialize, Debug)]
 #[serde(rename_all = "snake_case")]
 pub struct ComputeStatusResponse {
     pub start_time: DateTime<Utc>,
@@ -23,7 +23,7 @@ pub struct ComputeStatusResponse {
     pub error: Option<String>,
 }
 
-#[derive(Deserialize, Serialize)]
+#[derive(Serialize)]
 #[serde(rename_all = "snake_case")]
 pub struct ComputeState {
     pub status: ComputeStatus,
@@ -33,7 +33,7 @@ pub struct ComputeState {
     pub error: Option<String>,
 }
 
-#[derive(Serialize, Clone, Copy, Debug, Deserialize, PartialEq, Eq)]
+#[derive(Serialize, Clone, Copy, Debug, PartialEq, Eq)]
 #[serde(rename_all = "snake_case")]
 pub enum ComputeStatus {
     // Spec wasn't provided at start, waiting for it to be

libs/compute_api/src/spec.rs

@@ -5,7 +5,6 @@
 //! and connect it to the storage nodes.
 use serde::{Deserialize, Serialize};
 use serde_with::{serde_as, DisplayFromStr};
-use utils::id::{TenantId, TimelineId};
 use utils::lsn::Lsn;
 
 /// String type alias representing Postgres identifier and
@@ -15,7 +14,7 @@ pub type PgIdent = String;
 /// Cluster spec or configuration represented as an optional number of
 /// delta operations + final cluster state description.
 #[serde_as]
-#[derive(Clone, Debug, Default, Deserialize, Serialize)]
+#[derive(Clone, Debug, Default, Deserialize)]
 pub struct ComputeSpec {
     pub format_version: f32,
 
@@ -27,32 +26,9 @@ pub struct ComputeSpec {
     pub cluster: Cluster,
     pub delta_operations: Option<Vec<DeltaOp>>,
 
-    // Information needed to connect to the storage layer.
-    //
-    // `tenant_id`, `timeline_id` and `pageserver_connstring` are always needed.
-    //
-    // Depending on `mode`, this can be a primary read-write node, a read-only
-    // replica, or a read-only node pinned at an older LSN.
-    // `safekeeper_connstrings` must be set for a primary.
-    //
-    // For backwards compatibility, the control plane may leave out all of
-    // these, and instead set the "neon.tenant_id", "neon.timeline_id",
-    // etc. GUCs in cluster.settings. TODO: Once the control plane has been
-    // updated to fill these fields, we can make these non optional.
-    #[serde_as(as = "Option<DisplayFromStr>")]
-    pub tenant_id: Option<TenantId>,
-    #[serde_as(as = "Option<DisplayFromStr>")]
-    pub timeline_id: Option<TimelineId>,
-    #[serde_as(as = "Option<DisplayFromStr>")]
-    pub pageserver_connstring: Option<String>,
-    #[serde(default)]
-    pub safekeeper_connstrings: Vec<String>,
-
     #[serde(default)]
     pub mode: ComputeMode,
 
-    /// If set, 'storage_auth_token' is used as the password to authenticate to
-    /// the pageserver and safekeepers.
     pub storage_auth_token: Option<String>,
 }
 
@@ -71,19 +47,13 @@ pub enum ComputeMode {
     Replica,
 }
 
-#[derive(Clone, Debug, Default, Deserialize, Serialize)]
+#[derive(Clone, Debug, Default, Deserialize)]
 pub struct Cluster {
-    pub cluster_id: Option<String>,
-    pub name: Option<String>,
+    pub cluster_id: String,
+    pub name: String,
     pub state: Option<String>,
     pub roles: Vec<Role>,
     pub databases: Vec<Database>,
-
-    /// Desired contents of 'postgresql.conf' file. (The 'compute_ctl'
-    /// tool may add additional settings to the final file.)
-    pub postgresql_conf: Option<String>,
-
-    /// Additional settings that will be appended to the 'postgresql.conf' file.
     pub settings: GenericOptions,
 }
 
@@ -93,7 +63,7 @@ pub struct Cluster {
 /// - DROP ROLE
 /// - ALTER ROLE name RENAME TO new_name
 /// - ALTER DATABASE name RENAME TO new_name
-#[derive(Clone, Debug, Deserialize, Serialize)]
+#[derive(Clone, Debug, Deserialize)]
 pub struct DeltaOp {
     pub action: String,
     pub name: PgIdent,
@@ -102,7 +72,7 @@ pub struct DeltaOp {
 
 /// Rust representation of Postgres role info with only those fields
 /// that matter for us.
-#[derive(Clone, Debug, Deserialize, Serialize)]
+#[derive(Clone, Debug, Deserialize)]
 pub struct Role {
     pub name: PgIdent,
     pub encrypted_password: Option<String>,
@@ -111,7 +81,7 @@ pub struct Role {
 
 /// Rust representation of Postgres database info with only those fields
 /// that matter for us.
-#[derive(Clone, Debug, Deserialize, Serialize)]
+#[derive(Clone, Debug, Deserialize)]
 pub struct Database {
     pub name: PgIdent,
     pub owner: PgIdent,
@@ -121,7 +91,7 @@ pub struct Database {
 /// Common type representing both SQL statement params with or without value,
 /// like `LOGIN` or `OWNER username` in the `CREATE/ALTER ROLE`, and config
 /// options like `wal_level = logical`.
-#[derive(Clone, Debug, Deserialize, Serialize)]
+#[derive(Clone, Debug, Deserialize)]
 pub struct GenericOption {
     pub name: String,
     pub value: Option<String>,

libs/pageserver_api/src/models.rs

@@ -110,11 +110,12 @@ impl TenantState {
             Self::Active => Attached,
             // If the (initial or resumed) attach procedure fails, the tenant becomes Broken.
             // However, it also becomes Broken if the regular load fails.
-            // From Console's perspective there's no practical difference
-            // because attachment_status is polled by console only during attach operation execution.
-            Self::Broken { reason, .. } => Failed {
-                reason: reason.to_owned(),
-            },
+            // We would need a separate TenantState variant to distinguish these cases.
+            // However, there's no practical difference from Console's perspective.
+            // It will run a Postgres-level health check as soon as it observes Attached.
+            // That will fail on Broken tenants.
+            // Console can then rollback the attach, or, wait for operator to fix the Broken tenant.
+            Self::Broken { .. } => Attached,
             // Why is Stopping a Maybe case? Because, during pageserver shutdown,
             // we set the Stopping state irrespective of whether the tenant
             // has finished attaching or not.
@@ -311,11 +312,10 @@ impl std::ops::Deref for TenantAttachConfig {
 
 /// See [`TenantState::attachment_status`] and the OpenAPI docs for context.
 #[derive(Serialize, Deserialize, Clone)]
-#[serde(tag = "slug", content = "data", rename_all = "snake_case")]
+#[serde(rename_all = "snake_case")]
 pub enum TenantAttachmentStatus {
     Maybe,
     Attached,
-    Failed { reason: String },
 }
 
 #[serde_as]
@@ -809,9 +809,7 @@ mod tests {
                 "slug": "Active",
             },
             "current_physical_size": 42,
-            "attachment_status": {
-                "slug":"attached",
-            }
+            "attachment_status": "attached",
         });
 
         let original_broken = TenantInfo {
@@ -833,9 +831,7 @@ mod tests {
                 }
             },
             "current_physical_size": 42,
-            "attachment_status": {
-                "slug":"attached",
-            }
+            "attachment_status": "attached",
         });
 
         assert_eq!(

libs/postgres_ffi/Cargo.toml

@@ -24,6 +24,7 @@ workspace_hack.workspace = true
 [dev-dependencies]
 env_logger.workspace = true
 postgres.workspace = true
+wal_craft = { path = "wal_craft" }
 
 [build-dependencies]
 anyhow.workspace = true

libs/postgres_ffi/src/lib.rs

@@ -33,7 +33,6 @@ macro_rules! postgres_ffi {
             }
             pub mod controlfile_utils;
             pub mod nonrelfile_utils;
-            pub mod wal_craft_test_export;
             pub mod waldecoder_handler;
             pub mod xlog_utils;
 
@@ -46,15 +45,8 @@ macro_rules! postgres_ffi {
     };
 }
 
-#[macro_export]
-macro_rules! for_all_postgres_versions {
-    ($macro:tt) => {
-        $macro!(v14);
-        $macro!(v15);
-    };
-}
-
-for_all_postgres_versions! { postgres_ffi }
+postgres_ffi!(v14);
+postgres_ffi!(v15);
 
 pub mod pg_constants;
 pub mod relfile_utils;

libs/postgres_ffi/src/wal_craft_test_export.rs

@@ -1,6 +0,0 @@
-//! This module is for WAL craft to test with postgres_ffi. Should not import any thing in normal usage.
-
-pub use super::PG_MAJORVERSION;
-pub use super::xlog_utils::*;
-pub use super::bindings::*;
-pub use crate::WAL_SEGMENT_SIZE;

libs/postgres_ffi/src/xlog_utils.rs

@@ -481,4 +481,220 @@ pub fn encode_logical_message(prefix: &str, message: &str) -> Vec<u8> {
     wal
 }
 
-// If you need to craft WAL and write tests for this module, put it at wal_craft crate.
+#[cfg(test)]
+mod tests {
+    use super::super::PG_MAJORVERSION;
+    use super::*;
+    use regex::Regex;
+    use std::cmp::min;
+    use std::fs;
+    use std::{env, str::FromStr};
+    use utils::const_assert;
+
+    fn init_logging() {
+        let _ = env_logger::Builder::from_env(env_logger::Env::default().default_filter_or(
+            format!("wal_craft=info,postgres_ffi::{PG_MAJORVERSION}::xlog_utils=trace"),
+        ))
+        .is_test(true)
+        .try_init();
+    }
+
+    fn test_end_of_wal<C: wal_craft::Crafter>(test_name: &str) {
+        use wal_craft::*;
+
+        let pg_version = PG_MAJORVERSION[1..3].parse::<u32>().unwrap();
+
+        // Craft some WAL
+        let top_path = PathBuf::from(env!("CARGO_MANIFEST_DIR"))
+            .join("..")
+            .join("..");
+        let cfg = Conf {
+            pg_version,
+            pg_distrib_dir: top_path.join("pg_install"),
+            datadir: top_path.join(format!("test_output/{}-{PG_MAJORVERSION}", test_name)),
+        };
+        if cfg.datadir.exists() {
+            fs::remove_dir_all(&cfg.datadir).unwrap();
+        }
+        cfg.initdb().unwrap();
+        let srv = cfg.start_server().unwrap();
+        let (intermediate_lsns, expected_end_of_wal_partial) =
+            C::craft(&mut srv.connect_with_timeout().unwrap()).unwrap();
+        let intermediate_lsns: Vec<Lsn> = intermediate_lsns
+            .iter()
+            .map(|&lsn| u64::from(lsn).into())
+            .collect();
+        let expected_end_of_wal: Lsn = u64::from(expected_end_of_wal_partial).into();
+        srv.kill();
+
+        // Check find_end_of_wal on the initial WAL
+        let last_segment = cfg
+            .wal_dir()
+            .read_dir()
+            .unwrap()
+            .map(|f| f.unwrap().file_name().into_string().unwrap())
+            .filter(|fname| IsXLogFileName(fname))
+            .max()
+            .unwrap();
+        check_pg_waldump_end_of_wal(&cfg, &last_segment, expected_end_of_wal);
+        for start_lsn in intermediate_lsns
+            .iter()
+            .chain(std::iter::once(&expected_end_of_wal))
+        {
+            // Erase all WAL before `start_lsn` to ensure it's not used by `find_end_of_wal`.
+            // We assume that `start_lsn` is non-decreasing.
+            info!(
+                "Checking with start_lsn={}, erasing WAL before it",
+                start_lsn
+            );
+            for file in fs::read_dir(cfg.wal_dir()).unwrap().flatten() {
+                let fname = file.file_name().into_string().unwrap();
+                if !IsXLogFileName(&fname) {
+                    continue;
+                }
+                let (segno, _) = XLogFromFileName(&fname, WAL_SEGMENT_SIZE);
+                let seg_start_lsn = XLogSegNoOffsetToRecPtr(segno, 0, WAL_SEGMENT_SIZE);
+                if seg_start_lsn > u64::from(*start_lsn) {
+                    continue;
+                }
+                let mut f = File::options().write(true).open(file.path()).unwrap();
+                const ZEROS: [u8; WAL_SEGMENT_SIZE] = [0u8; WAL_SEGMENT_SIZE];
+                f.write_all(
+                    &ZEROS[0..min(
+                        WAL_SEGMENT_SIZE,
+                        (u64::from(*start_lsn) - seg_start_lsn) as usize,
+                    )],
+                )
+                .unwrap();
+            }
+            check_end_of_wal(&cfg, &last_segment, *start_lsn, expected_end_of_wal);
+        }
+    }
+
+    fn check_pg_waldump_end_of_wal(
+        cfg: &wal_craft::Conf,
+        last_segment: &str,
+        expected_end_of_wal: Lsn,
+    ) {
+        // Get the actual end of WAL by pg_waldump
+        let waldump_output = cfg
+            .pg_waldump("000000010000000000000001", last_segment)
+            .unwrap()
+            .stderr;
+        let waldump_output = std::str::from_utf8(&waldump_output).unwrap();
+        let caps = match Regex::new(r"invalid record length at (.+):")
+            .unwrap()
+            .captures(waldump_output)
+        {
+            Some(caps) => caps,
+            None => {
+                error!("Unable to parse pg_waldump's stderr:\n{}", waldump_output);
+                panic!();
+            }
+        };
+        let waldump_wal_end = Lsn::from_str(caps.get(1).unwrap().as_str()).unwrap();
+        info!(
+            "waldump erred on {}, expected wal end at {}",
+            waldump_wal_end, expected_end_of_wal
+        );
+        assert_eq!(waldump_wal_end, expected_end_of_wal);
+    }
+
+    fn check_end_of_wal(
+        cfg: &wal_craft::Conf,
+        last_segment: &str,
+        start_lsn: Lsn,
+        expected_end_of_wal: Lsn,
+    ) {
+        // Check end_of_wal on non-partial WAL segment (we treat it as fully populated)
+        // let wal_end = find_end_of_wal(&cfg.wal_dir(), WAL_SEGMENT_SIZE, start_lsn).unwrap();
+        // info!(
+        //     "find_end_of_wal returned wal_end={} with non-partial WAL segment",
+        //     wal_end
+        // );
+        // assert_eq!(wal_end, expected_end_of_wal_non_partial);
+
+        // Rename file to partial to actually find last valid lsn, then rename it back.
+        fs::rename(
+            cfg.wal_dir().join(last_segment),
+            cfg.wal_dir().join(format!("{}.partial", last_segment)),
+        )
+        .unwrap();
+        let wal_end = find_end_of_wal(&cfg.wal_dir(), WAL_SEGMENT_SIZE, start_lsn).unwrap();
+        info!(
+            "find_end_of_wal returned wal_end={} with partial WAL segment",
+            wal_end
+        );
+        assert_eq!(wal_end, expected_end_of_wal);
+        fs::rename(
+            cfg.wal_dir().join(format!("{}.partial", last_segment)),
+            cfg.wal_dir().join(last_segment),
+        )
+        .unwrap();
+    }
+
+    const_assert!(WAL_SEGMENT_SIZE == 16 * 1024 * 1024);
+
+    #[test]
+    pub fn test_find_end_of_wal_simple() {
+        init_logging();
+        test_end_of_wal::<wal_craft::Simple>("test_find_end_of_wal_simple");
+    }
+
+    #[test]
+    pub fn test_find_end_of_wal_crossing_segment_followed_by_small_one() {
+        init_logging();
+        test_end_of_wal::<wal_craft::WalRecordCrossingSegmentFollowedBySmallOne>(
+            "test_find_end_of_wal_crossing_segment_followed_by_small_one",
+        );
+    }
+
+    #[test]
+    pub fn test_find_end_of_wal_last_crossing_segment() {
+        init_logging();
+        test_end_of_wal::<wal_craft::LastWalRecordCrossingSegment>(
+            "test_find_end_of_wal_last_crossing_segment",
+        );
+    }
+
+    /// Check the math in update_next_xid
+    ///
+    /// NOTE: These checks are sensitive to the value of XID_CHECKPOINT_INTERVAL,
+    /// currently 1024.
+    #[test]
+    pub fn test_update_next_xid() {
+        let checkpoint_buf = [0u8; std::mem::size_of::<CheckPoint>()];
+        let mut checkpoint = CheckPoint::decode(&checkpoint_buf).unwrap();
+
+        checkpoint.nextXid = FullTransactionId { value: 10 };
+        assert_eq!(checkpoint.nextXid.value, 10);
+
+        // The input XID gets rounded up to the next XID_CHECKPOINT_INTERVAL
+        // boundary
+        checkpoint.update_next_xid(100);
+        assert_eq!(checkpoint.nextXid.value, 1024);
+
+        // No change
+        checkpoint.update_next_xid(500);
+        assert_eq!(checkpoint.nextXid.value, 1024);
+        checkpoint.update_next_xid(1023);
+        assert_eq!(checkpoint.nextXid.value, 1024);
+
+        // The function returns the *next* XID, given the highest XID seen so
+        // far. So when we pass 1024, the nextXid gets bumped up to the next
+        // XID_CHECKPOINT_INTERVAL boundary.
+        checkpoint.update_next_xid(1024);
+        assert_eq!(checkpoint.nextXid.value, 2048);
+    }
+
+    #[test]
+    pub fn test_encode_logical_message() {
+        let expected = [
+            64, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 21, 0, 0, 170, 34, 166, 227, 255,
+            38, 0, 0, 0, 0, 0, 0, 0, 0, 7, 0, 0, 0, 0, 0, 0, 0, 7, 0, 0, 0, 0, 0, 0, 0, 112, 114,
+            101, 102, 105, 120, 0, 109, 101, 115, 115, 97, 103, 101,
+        ];
+        let actual = encode_logical_message("prefix", "message");
+        assert_eq!(expected, actual[..]);
+    }
+}

libs/postgres_ffi/wal_craft/Cargo.toml

@@ -15,7 +15,3 @@ postgres_ffi.workspace = true
 tempfile.workspace = true
 
 workspace_hack.workspace = true
-
-[dev-dependencies]
-regex.workspace = true
-utils.workspace = true

libs/postgres_ffi/wal_craft/src/lib.rs

@@ -10,20 +10,6 @@ use std::process::Command;
 use std::time::{Duration, Instant};
 use tempfile::{tempdir, TempDir};
 
-macro_rules! xlog_utils_test {
-    ($version:ident) => {
-        #[path = "."]
-        mod $version {
-            pub use postgres_ffi::$version::wal_craft_test_export::*;
-            #[allow(clippy::duplicate_mod)]
-            #[cfg(test)]
-            mod xlog_utils_test;
-        }
-    };
-}
-
-postgres_ffi::for_all_postgres_versions! { xlog_utils_test }
-
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct Conf {
     pub pg_version: u32,

libs/postgres_ffi/wal_craft/src/xlog_utils_test.rs

@@ -1,219 +0,0 @@
-//! Tests for postgres_ffi xlog_utils module. Put it here to break cyclic dependency.
-
-use super::*;
-use crate::{error, info};
-use regex::Regex;
-use std::cmp::min;
-use std::fs::{self, File};
-use std::io::Write;
-use std::{env, str::FromStr};
-use utils::const_assert;
-use utils::lsn::Lsn;
-
-fn init_logging() {
-    let _ = env_logger::Builder::from_env(env_logger::Env::default().default_filter_or(
-        format!("crate=info,postgres_ffi::{PG_MAJORVERSION}::xlog_utils=trace"),
-    ))
-    .is_test(true)
-    .try_init();
-}
-
-fn test_end_of_wal<C: crate::Crafter>(test_name: &str) {
-    use crate::*;
-
-    let pg_version = PG_MAJORVERSION[1..3].parse::<u32>().unwrap();
-
-    // Craft some WAL
-    let top_path = PathBuf::from(env!("CARGO_MANIFEST_DIR"))
-        .join("..")
-        .join("..")
-        .join("..");
-    let cfg = Conf {
-        pg_version,
-        pg_distrib_dir: top_path.join("pg_install"),
-        datadir: top_path.join(format!("test_output/{}-{PG_MAJORVERSION}", test_name)),
-    };
-    if cfg.datadir.exists() {
-        fs::remove_dir_all(&cfg.datadir).unwrap();
-    }
-    cfg.initdb().unwrap();
-    let srv = cfg.start_server().unwrap();
-    let (intermediate_lsns, expected_end_of_wal_partial) =
-        C::craft(&mut srv.connect_with_timeout().unwrap()).unwrap();
-    let intermediate_lsns: Vec<Lsn> = intermediate_lsns
-        .iter()
-        .map(|&lsn| u64::from(lsn).into())
-        .collect();
-    let expected_end_of_wal: Lsn = u64::from(expected_end_of_wal_partial).into();
-    srv.kill();
-
-    // Check find_end_of_wal on the initial WAL
-    let last_segment = cfg
-        .wal_dir()
-        .read_dir()
-        .unwrap()
-        .map(|f| f.unwrap().file_name().into_string().unwrap())
-        .filter(|fname| IsXLogFileName(fname))
-        .max()
-        .unwrap();
-    check_pg_waldump_end_of_wal(&cfg, &last_segment, expected_end_of_wal);
-    for start_lsn in intermediate_lsns
-        .iter()
-        .chain(std::iter::once(&expected_end_of_wal))
-    {
-        // Erase all WAL before `start_lsn` to ensure it's not used by `find_end_of_wal`.
-        // We assume that `start_lsn` is non-decreasing.
-        info!(
-            "Checking with start_lsn={}, erasing WAL before it",
-            start_lsn
-        );
-        for file in fs::read_dir(cfg.wal_dir()).unwrap().flatten() {
-            let fname = file.file_name().into_string().unwrap();
-            if !IsXLogFileName(&fname) {
-                continue;
-            }
-            let (segno, _) = XLogFromFileName(&fname, WAL_SEGMENT_SIZE);
-            let seg_start_lsn = XLogSegNoOffsetToRecPtr(segno, 0, WAL_SEGMENT_SIZE);
-            if seg_start_lsn > u64::from(*start_lsn) {
-                continue;
-            }
-            let mut f = File::options().write(true).open(file.path()).unwrap();
-            const ZEROS: [u8; WAL_SEGMENT_SIZE] = [0u8; WAL_SEGMENT_SIZE];
-            f.write_all(
-                &ZEROS[0..min(
-                    WAL_SEGMENT_SIZE,
-                    (u64::from(*start_lsn) - seg_start_lsn) as usize,
-                )],
-            )
-            .unwrap();
-        }
-        check_end_of_wal(&cfg, &last_segment, *start_lsn, expected_end_of_wal);
-    }
-}
-
-fn check_pg_waldump_end_of_wal(
-    cfg: &crate::Conf,
-    last_segment: &str,
-    expected_end_of_wal: Lsn,
-) {
-    // Get the actual end of WAL by pg_waldump
-    let waldump_output = cfg
-        .pg_waldump("000000010000000000000001", last_segment)
-        .unwrap()
-        .stderr;
-    let waldump_output = std::str::from_utf8(&waldump_output).unwrap();
-    let caps = match Regex::new(r"invalid record length at (.+):")
-        .unwrap()
-        .captures(waldump_output)
-    {
-        Some(caps) => caps,
-        None => {
-            error!("Unable to parse pg_waldump's stderr:\n{}", waldump_output);
-            panic!();
-        }
-    };
-    let waldump_wal_end = Lsn::from_str(caps.get(1).unwrap().as_str()).unwrap();
-    info!(
-        "waldump erred on {}, expected wal end at {}",
-        waldump_wal_end, expected_end_of_wal
-    );
-    assert_eq!(waldump_wal_end, expected_end_of_wal);
-}
-
-fn check_end_of_wal(
-    cfg: &crate::Conf,
-    last_segment: &str,
-    start_lsn: Lsn,
-    expected_end_of_wal: Lsn,
-) {
-    // Check end_of_wal on non-partial WAL segment (we treat it as fully populated)
-    // let wal_end = find_end_of_wal(&cfg.wal_dir(), WAL_SEGMENT_SIZE, start_lsn).unwrap();
-    // info!(
-    //     "find_end_of_wal returned wal_end={} with non-partial WAL segment",
-    //     wal_end
-    // );
-    // assert_eq!(wal_end, expected_end_of_wal_non_partial);
-
-    // Rename file to partial to actually find last valid lsn, then rename it back.
-    fs::rename(
-        cfg.wal_dir().join(last_segment),
-        cfg.wal_dir().join(format!("{}.partial", last_segment)),
-    )
-    .unwrap();
-    let wal_end = find_end_of_wal(&cfg.wal_dir(), WAL_SEGMENT_SIZE, start_lsn).unwrap();
-    info!(
-        "find_end_of_wal returned wal_end={} with partial WAL segment",
-        wal_end
-    );
-    assert_eq!(wal_end, expected_end_of_wal);
-    fs::rename(
-        cfg.wal_dir().join(format!("{}.partial", last_segment)),
-        cfg.wal_dir().join(last_segment),
-    )
-    .unwrap();
-}
-
-const_assert!(WAL_SEGMENT_SIZE == 16 * 1024 * 1024);
-
-#[test]
-pub fn test_find_end_of_wal_simple() {
-    init_logging();
-    test_end_of_wal::<crate::Simple>("test_find_end_of_wal_simple");
-}
-
-#[test]
-pub fn test_find_end_of_wal_crossing_segment_followed_by_small_one() {
-    init_logging();
-    test_end_of_wal::<crate::WalRecordCrossingSegmentFollowedBySmallOne>(
-        "test_find_end_of_wal_crossing_segment_followed_by_small_one",
-    );
-}
-
-#[test]
-pub fn test_find_end_of_wal_last_crossing_segment() {
-    init_logging();
-    test_end_of_wal::<crate::LastWalRecordCrossingSegment>(
-        "test_find_end_of_wal_last_crossing_segment",
-    );
-}
-
-/// Check the math in update_next_xid
-///
-/// NOTE: These checks are sensitive to the value of XID_CHECKPOINT_INTERVAL,
-/// currently 1024.
-#[test]
-pub fn test_update_next_xid() {
-    let checkpoint_buf = [0u8; std::mem::size_of::<CheckPoint>()];
-    let mut checkpoint = CheckPoint::decode(&checkpoint_buf).unwrap();
-
-    checkpoint.nextXid = FullTransactionId { value: 10 };
-    assert_eq!(checkpoint.nextXid.value, 10);
-
-    // The input XID gets rounded up to the next XID_CHECKPOINT_INTERVAL
-    // boundary
-    checkpoint.update_next_xid(100);
-    assert_eq!(checkpoint.nextXid.value, 1024);
-
-    // No change
-    checkpoint.update_next_xid(500);
-    assert_eq!(checkpoint.nextXid.value, 1024);
-    checkpoint.update_next_xid(1023);
-    assert_eq!(checkpoint.nextXid.value, 1024);
-
-    // The function returns the *next* XID, given the highest XID seen so
-    // far. So when we pass 1024, the nextXid gets bumped up to the next
-    // XID_CHECKPOINT_INTERVAL boundary.
-    checkpoint.update_next_xid(1024);
-    assert_eq!(checkpoint.nextXid.value, 2048);
-}
-
-#[test]
-pub fn test_encode_logical_message() {
-    let expected = [
-        64, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 21, 0, 0, 170, 34, 166, 227, 255,
-        38, 0, 0, 0, 0, 0, 0, 0, 0, 7, 0, 0, 0, 0, 0, 0, 0, 7, 0, 0, 0, 0, 0, 0, 0, 112, 114,
-        101, 102, 105, 120, 0, 109, 101, 115, 115, 97, 103, 101,
-    ];
-    let actual = encode_logical_message("prefix", "message");
-    assert_eq!(expected, actual[..]);
-}

libs/remote_storage/src/lib.rs

@@ -70,10 +70,6 @@ impl RemotePath {
     pub fn join(&self, segment: &Path) -> Self {
         Self(self.0.join(segment))
     }
-
-    pub fn extension(&self) -> Option<&str> {
-        self.0.extension()?.to_str()
-    }
 }
 
 /// Storage (potentially remote) API to manage its state.
@@ -90,12 +86,6 @@ pub trait RemoteStorage: Send + Sync + 'static {
         prefix: Option<&RemotePath>,
     ) -> Result<Vec<RemotePath>, DownloadError>;
 
-    /// Lists all files in a subdirectories
-    async fn list_files(
-        &self,
-        prefix: Option<&RemotePath>,
-    ) -> anyhow::Result<Vec<RemotePath>>;
-
     /// Streams the local file contents into remote into the remote storage entry.
     async fn upload(
         &self,
@@ -171,23 +161,6 @@ pub enum GenericRemoteStorage {
 }
 
 impl GenericRemoteStorage {
-    // A function for listing all the files in a "directory"
-    // Example:
-    // list_files("foo/bar") = ["foo/bar/a.txt", "foo/bar/b.txt"]
-    pub async fn list_files(
-        &self,
-        folder: Option<&RemotePath>
-    ) -> anyhow::Result<Vec<RemotePath>>{
-        match self {
-            Self::LocalFs(s) => s.list_files(folder).await,
-            Self::AwsS3(s) => s.list_files(folder).await,
-            Self::Unreliable(s) => s.list_files(folder).await,
-        }
-    }
-
-    // lists common *prefixes*, if any of files
-    // Example:
-    // list_prefixes("foo123","foo567","bar123","bar432") = ["foo", "bar"]
     pub async fn list_prefixes(
         &self,
         prefix: Option<&RemotePath>,

libs/remote_storage/src/local_fs.rs

@@ -116,34 +116,6 @@ impl RemoteStorage for LocalFs {
             .collect())
     }
 
-    async fn list_files(
-        &self, 
-        folder: Option<&RemotePath>
-    ) -> anyhow::Result<Vec<RemotePath>> {
-        /* Note: if you want, you can return a DownloadError instead of an anyhow::Error
-        as follows: replace all ?'s with:
-            .map_err(|e| DownloadError::Other(anyhow::Error::from(e)))?;
-        */
-        let full_path = match folder.clone() {
-            Some(folder) => folder.with_base(&self.storage_root),
-            None => self.storage_root.clone(),
-        };
-        let mut entries = fs::read_dir(full_path).await?;
-        let mut files = vec![];
-        while let Some(entry) = entries.next_entry().await? {
-            let file_name: PathBuf = entry.file_name().into();
-            let file_type = entry.file_type().await?;
-            if file_type.is_file() {
-                let mut file_remote_path = RemotePath::new(&file_name)?;
-                if let Some(folder) = folder {
-                    file_remote_path = folder.join(&file_name);
-                } 
-                files.push(file_remote_path);
-            }
-        }
-        Ok(files)
-    }
-
     async fn upload(
         &self,
         data: impl io::AsyncRead + Unpin + Send + Sync + 'static,

libs/remote_storage/src/s3_bucket.rs

@@ -332,51 +332,6 @@ impl RemoteStorage for S3Bucket {
         Ok(document_keys)
     }
 
-    async fn list_files(
-        &self,
-        folder: Option<&RemotePath>
-    ) -> anyhow::Result<Vec<RemotePath>>{
-        let folder_name = folder.map(|x| 
-            String::from(x.object_name().expect("invalid folder name"))
-        );
-        let mut continuation_token = None;
-        let mut all_files = vec![];
-        loop {
-            let _guard = self
-                .concurrency_limiter
-                .acquire()
-                .await
-                .context("Concurrency limiter semaphore got closed during S3 list_files")?;
-            metrics::inc_list_objects();
-
-            let response = self
-                .client
-                .list_objects_v2()
-                .bucket(self.bucket_name.clone())
-                .set_prefix(folder_name.clone())
-                .set_continuation_token(continuation_token)
-                .set_max_keys(self.max_keys_per_list_response)
-                .send()
-                .await
-                .map_err(|e| {
-                    metrics::inc_list_objects_fail();
-                    e
-                })
-                .context("Failed to list files in S3 bucket")?;
-        
-            for object in response.contents().unwrap_or_default() {
-                let object_path = object.key().unwrap();
-                let remote_path = self.s3_object_to_relative_path(object_path);
-                all_files.push(remote_path);
-            }
-            match response.next_continuation_token {
-                Some(new_token) => continuation_token = Some(new_token),
-                None => break,
-            }
-        }
-        Ok(all_files)
-    }
-
     async fn upload(
         &self,
         from: impl io::AsyncRead + Unpin + Send + Sync + 'static,

libs/remote_storage/src/simulate_failures.rs

@@ -82,14 +82,6 @@ impl RemoteStorage for UnreliableWrapper {
         self.inner.list_prefixes(prefix).await
     }
 
-    async fn list_files(
-        &self,
-        folder: Option<&RemotePath>
-    ) -> anyhow::Result<Vec<RemotePath>>{
-        self.attempt(RemoteOp::ListPrefixes(folder.cloned()))?;
-        self.inner.list_files(folder).await
-    }
-
     async fn upload(
         &self,
         data: impl tokio::io::AsyncRead + Unpin + Send + Sync + 'static,

pageserver/benches/bench_layer_map.rs

@@ -33,7 +33,7 @@ fn build_layer_map(filename_dump: PathBuf) -> LayerMap<LayerDescriptor> {
         min_lsn = min(min_lsn, lsn_range.start);
         max_lsn = max(max_lsn, Lsn(lsn_range.end.0 - 1));
 
-        updates.insert_historic(layer.get_persistent_layer_desc(), Arc::new(layer));
+        updates.insert_historic(Arc::new(layer));
     }
 
     println!("min: {min_lsn}, max: {max_lsn}");
@@ -215,7 +215,7 @@ fn bench_sequential(c: &mut Criterion) {
             is_incremental: false,
             short_id: format!("Layer {}", i),
         };
-        updates.insert_historic(layer.get_persistent_layer_desc(), Arc::new(layer));
+        updates.insert_historic(Arc::new(layer));
     }
     updates.flush();
     println!("Finished layer map init in {:?}", now.elapsed());

pageserver/ctl/Cargo.toml

@@ -16,3 +16,4 @@ postgres_ffi.workspace = true
 utils.workspace = true
 svg_fmt.workspace = true
 workspace_hack.workspace = true
+itertools.workspace = true

pageserver/ctl/src/layers.rs

@@ -41,9 +41,18 @@ pub(crate) enum LayerCmd {
         /// The id from list-layer command
         id: usize,
     },
+    /// Output layer statistics
+    GetStats {
+        path: PathBuf,
+        tenant: String,
+        timeline: String,
+        /// The id from list-layer command
+        id: usize,
+    },
 }
 
-fn read_delta_file(path: impl AsRef<Path>) -> Result<()> {
+// Return (key, value.len) for all keys, sorted by key.
+fn read_delta_file(path: impl AsRef<Path>) -> Result<Vec<(Key, usize)>> {
     use pageserver::tenant::blob_io::BlobCursor;
     use pageserver::tenant::block_io::BlockReader;
 
@@ -70,11 +79,48 @@ fn read_delta_file(path: impl AsRef<Path>) -> Result<()> {
         },
     )?;
     let mut cursor = BlockCursor::new(&file);
+
+    let mut result = vec![];
     for (k, v) in all {
         let value = cursor.read_blob(v.pos())?;
-        println!("key:{} value_len:{}", k, value.len());
+        result.push((k, value.len()));
     }
     // TODO(chi): special handling for last key?
+    Ok(result)
+}
+
+// We divide the entire i128 keyspace into pre-assigned fixed segments,
+// 8MB each. Group keys by segment, and report segment size for each.
+//
+// 8MB is chosen as the segment size because we're unlikely to make
+// s3 partial downloads smaller than 8MB (due to cost). So summarizing
+// layer metadata in 8MB segments could be enough to generate good test
+// data for write amplification tests.
+//
+// Note that the segments are fixed, and don't depend on what keyspace
+// is actually in use.
+fn read_delta_segments(path: impl AsRef<Path>) -> Result<Vec<(i128, usize)>> {
+    fn key_to_segment(key: &Key) -> i128 {
+        // A page is 8KB. So 1024 pages are 8MB.
+        key.to_i128() >> 10
+    }
+
+    use itertools::Itertools;
+    let delta_metadata = read_delta_file(path)?;
+    let group_iter = delta_metadata.iter().group_by(|(k, _)| key_to_segment(k));
+    let group_sizes = group_iter.into_iter().map(|(segment, lengths_group)| {
+        let sum: usize = lengths_group.map(|(_k, len)| len).sum();
+        (segment, sum)
+    });
+    Ok(group_sizes.collect())
+}
+
+fn summarize_delta_file(path: impl AsRef<Path>) -> Result<()> {
+    // TODO write in some compressed binary format
+    for (segment, size) in read_delta_segments(path)? {
+        println!("segment:{} size:{}", segment, size);
+    }
+
     Ok(())
 }
 
@@ -153,7 +199,38 @@ pub(crate) fn main(cmd: &LayerCmd) -> Result<()> {
                         );
 
                         if layer_file.is_delta {
-                            read_delta_file(layer.path())?;
+                            for (k, len) in read_delta_file(layer.path())? {
+                                println!("key:{} value_len:{}", k, len);
+                            }
+                        } else {
+                            anyhow::bail!("not supported yet :(");
+                        }
+
+                        break;
+                    }
+                    idx += 1;
+                }
+            }
+        }
+        LayerCmd::GetStats {
+            path,
+            tenant,
+            timeline,
+            id,
+        } => {
+            let timeline_path = path
+                .join("tenants")
+                .join(tenant)
+                .join("timelines")
+                .join(timeline);
+            let mut idx = 0;
+            for layer in fs::read_dir(timeline_path)? {
+                let layer = layer?;
+                if let Some(layer_file) = parse_filename(&layer.file_name().into_string().unwrap())
+                {
+                    if *id == idx {
+                        if layer_file.is_delta {
+                            summarize_delta_file(layer.path())?;
                         } else {
                             anyhow::bail!("not supported yet :(");
                         }

pageserver/src/bin/pageserver.rs

@@ -335,116 +335,31 @@ fn start_pageserver(
     // Set up remote storage client
     let remote_storage = create_remote_storage_client(conf)?;
 
-    // Startup staging or optimizing:
-    //
-    // We want to minimize downtime for `page_service` connections, and trying not to overload
-    // BACKGROUND_RUNTIME by doing initial compactions and initial logical sizes at the same time.
-    //
-    // init_done_rx will notify when all initial load operations have completed.
-    //
-    // background_jobs_can_start (same name used to hold off background jobs from starting at
-    // consumer side) will be dropped once we can start the background jobs. Currently it is behind
-    // completing all initial logical size calculations (init_logical_size_done_rx) and a timeout
-    // (background_task_maximum_delay).
+    // All tenant load operations carry this while they are ongoing; it will be dropped once those
+    // operations finish either successfully or in some other manner. However, the initial load
+    // will be then done, and we can start the global background tasks.
     let (init_done_tx, init_done_rx) = utils::completion::channel();
 
-    let (init_logical_size_done_tx, init_logical_size_done_rx) = utils::completion::channel();
-
-    let (background_jobs_can_start, background_jobs_barrier) = utils::completion::channel();
-
-    let order = pageserver::InitializationOrder {
-        initial_tenant_load: Some(init_done_tx),
-        initial_logical_size_can_start: init_done_rx.clone(),
-        initial_logical_size_attempt: init_logical_size_done_tx,
-        background_jobs_can_start: background_jobs_barrier.clone(),
-    };
-
     // Scan the local 'tenants/' directory and start loading the tenants
     let init_started_at = std::time::Instant::now();
-    let shutdown_pageserver = tokio_util::sync::CancellationToken::new();
-
     BACKGROUND_RUNTIME.block_on(mgr::init_tenant_mgr(
         conf,
         broker_client.clone(),
         remote_storage.clone(),
-        order,
+        (init_done_tx, init_done_rx.clone()),
     ))?;
 
     BACKGROUND_RUNTIME.spawn({
-        let init_done_rx = init_done_rx;
-        let shutdown_pageserver = shutdown_pageserver.clone();
-        let drive_init = async move {
-            // NOTE: unlike many futures in pageserver, this one is cancellation-safe
-            let guard = scopeguard::guard_on_success((), |_| tracing::info!("Cancelled before initial load completed"));
-
+        let init_done_rx = init_done_rx.clone();
+        async move {
             init_done_rx.wait().await;
-            // initial logical sizes can now start, as they were waiting on init_done_rx.
 
-            scopeguard::ScopeGuard::into_inner(guard);
-
-            let init_done = std::time::Instant::now();
-            let elapsed = init_done - init_started_at;
+            let elapsed = init_started_at.elapsed();
 
             tracing::info!(
                 elapsed_millis = elapsed.as_millis(),
-                "Initial load completed"
+                "Initial load completed."
             );
-
-            let mut init_sizes_done = std::pin::pin!(init_logical_size_done_rx.wait());
-
-            let timeout = conf.background_task_maximum_delay;
-
-            let guard = scopeguard::guard_on_success((), |_| tracing::info!("Cancelled before initial logical sizes completed"));
-
-            let init_sizes_done = tokio::select! {
-                _ = &mut init_sizes_done => {
-                    let now = std::time::Instant::now();
-                    tracing::info!(
-                        from_init_done_millis = (now - init_done).as_millis(),
-                        from_init_millis = (now - init_started_at).as_millis(),
-                        "Initial logical sizes completed"
-                    );
-                    None
-                }
-                _ = tokio::time::sleep(timeout) => {
-                    tracing::info!(
-                        timeout_millis = timeout.as_millis(),
-                        "Initial logical size timeout elapsed; starting background jobs"
-                    );
-                    Some(init_sizes_done)
-                }
-            };
-
-            scopeguard::ScopeGuard::into_inner(guard);
-
-            // allow background jobs to start
-            drop(background_jobs_can_start);
-
-            if let Some(init_sizes_done) = init_sizes_done {
-                // ending up here is not a bug; at the latest logical sizes will be queried by
-                // consumption metrics.
-                let guard = scopeguard::guard_on_success((), |_| tracing::info!("Cancelled before initial logical sizes completed"));
-                init_sizes_done.await;
-
-                scopeguard::ScopeGuard::into_inner(guard);
-
-                let now = std::time::Instant::now();
-                tracing::info!(
-                    from_init_done_millis = (now - init_done).as_millis(),
-                    from_init_millis = (now - init_started_at).as_millis(),
-                    "Initial logical sizes completed after timeout (background jobs already started)"
-                );
-
-            }
-        };
-
-        async move {
-            let mut drive_init = std::pin::pin!(drive_init);
-            // just race these tasks
-            tokio::select! {
-                _ = shutdown_pageserver.cancelled() => {},
-                _ = &mut drive_init => {},
-            }
         }
     });
 
@@ -459,7 +374,7 @@ fn start_pageserver(
             conf,
             remote_storage.clone(),
             disk_usage_eviction_state.clone(),
-            background_jobs_barrier.clone(),
+            init_done_rx.clone(),
         )?;
     }
 
@@ -497,7 +412,7 @@ fn start_pageserver(
         );
 
         if let Some(metric_collection_endpoint) = &conf.metric_collection_endpoint {
-            let background_jobs_barrier = background_jobs_barrier;
+            let init_done_rx = init_done_rx;
             let metrics_ctx = RequestContext::todo_child(
                 TaskKind::MetricsCollection,
                 // This task itself shouldn't download anything.
@@ -513,17 +428,12 @@ fn start_pageserver(
                 "consumption metrics collection",
                 true,
                 async move {
-                    // first wait until background jobs are cleared to launch.
+                    // first wait for initial load to complete before first iteration.
                     //
                     // this is because we only process active tenants and timelines, and the
                     // Timeline::get_current_logical_size will spawn the logical size calculation,
                     // which will not be rate-limited.
-                    let cancel = task_mgr::shutdown_token();
-
-                    tokio::select! {
-                        _ = cancel.cancelled() => { return Ok(()); },
-                        _ = background_jobs_barrier.wait() => {}
-                    };
+                    init_done_rx.wait().await;
 
                     pageserver::consumption_metrics::collect_metrics(
                         metric_collection_endpoint,
@@ -573,8 +483,6 @@ fn start_pageserver(
         );
     }
 
-    let mut shutdown_pageserver = Some(shutdown_pageserver.drop_guard());
-
     // All started up! Now just sit and wait for shutdown signal.
     ShutdownSignals::handle(|signal| match signal {
         Signal::Quit => {
@@ -590,11 +498,6 @@ fn start_pageserver(
                 "Got {}. Terminating gracefully in fast shutdown mode",
                 signal.name()
             );
-
-            // This cancels the `shutdown_pageserver` cancellation tree.
-            // Right now that tree doesn't reach very far, and `task_mgr` is used instead.
-            // The plan is to change that over time.
-            shutdown_pageserver.take();
             BACKGROUND_RUNTIME.block_on(pageserver::shutdown_pageserver(0));
             unreachable!()
         }

pageserver/src/config.rs

@@ -63,7 +63,6 @@ pub mod defaults {
     pub const DEFAULT_CACHED_METRIC_COLLECTION_INTERVAL: &str = "1 hour";
     pub const DEFAULT_METRIC_COLLECTION_ENDPOINT: Option<reqwest::Url> = None;
     pub const DEFAULT_SYNTHETIC_SIZE_CALCULATION_INTERVAL: &str = "10 min";
-    pub const DEFAULT_BACKGROUND_TASK_MAXIMUM_DELAY: &str = "10s";
 
     ///
     /// Default built-in configuration file.
@@ -92,9 +91,8 @@ pub mod defaults {
 #cached_metric_collection_interval = '{DEFAULT_CACHED_METRIC_COLLECTION_INTERVAL}'
 #synthetic_size_calculation_interval = '{DEFAULT_SYNTHETIC_SIZE_CALCULATION_INTERVAL}'
 
-#disk_usage_based_eviction = {{ max_usage_pct = .., min_avail_bytes = .., period = "10s"}}
 
-#background_task_maximum_delay = '{DEFAULT_BACKGROUND_TASK_MAXIMUM_DELAY}'
+#disk_usage_based_eviction = {{ max_usage_pct = .., min_avail_bytes = .., period = "10s"}}
 
 # [tenant_config]
 #checkpoint_distance = {DEFAULT_CHECKPOINT_DISTANCE} # in bytes
@@ -189,15 +187,6 @@ pub struct PageServerConf {
     pub test_remote_failures: u64,
 
     pub ondemand_download_behavior_treat_error_as_warn: bool,
-
-    /// How long will background tasks be delayed at most after initial load of tenants.
-    ///
-    /// Our largest initialization completions are in the range of 100-200s, so perhaps 10s works
-    /// as we now isolate initial loading, initial logical size calculation and background tasks.
-    /// Smaller nodes will have background tasks "not running" for this long unless every timeline
-    /// has it's initial logical size calculated. Not running background tasks for some seconds is
-    /// not terrible.
-    pub background_task_maximum_delay: Duration,
 }
 
 /// We do not want to store this in a PageServerConf because the latter may be logged
@@ -270,8 +259,6 @@ struct PageServerConfigBuilder {
     test_remote_failures: BuilderValue<u64>,
 
     ondemand_download_behavior_treat_error_as_warn: BuilderValue<bool>,
-
-    background_task_maximum_delay: BuilderValue<Duration>,
 }
 
 impl Default for PageServerConfigBuilder {
@@ -329,11 +316,6 @@ impl Default for PageServerConfigBuilder {
             test_remote_failures: Set(0),
 
             ondemand_download_behavior_treat_error_as_warn: Set(false),
-
-            background_task_maximum_delay: Set(humantime::parse_duration(
-                DEFAULT_BACKGROUND_TASK_MAXIMUM_DELAY,
-            )
-            .unwrap()),
         }
     }
 }
@@ -458,10 +440,6 @@ impl PageServerConfigBuilder {
             BuilderValue::Set(ondemand_download_behavior_treat_error_as_warn);
     }
 
-    pub fn background_task_maximum_delay(&mut self, delay: Duration) {
-        self.background_task_maximum_delay = BuilderValue::Set(delay);
-    }
-
     pub fn build(self) -> anyhow::Result<PageServerConf> {
         let concurrent_tenant_size_logical_size_queries = self
             .concurrent_tenant_size_logical_size_queries
@@ -544,9 +522,6 @@ impl PageServerConfigBuilder {
                 .ok_or(anyhow!(
                     "missing ondemand_download_behavior_treat_error_as_warn"
                 ))?,
-            background_task_maximum_delay: self
-                .background_task_maximum_delay
-                .ok_or(anyhow!("missing background_task_maximum_delay"))?,
         })
     }
 }
@@ -735,7 +710,6 @@ impl PageServerConf {
                     )
                 },
                 "ondemand_download_behavior_treat_error_as_warn" => builder.ondemand_download_behavior_treat_error_as_warn(parse_toml_bool(key, item)?),
-                "background_task_maximum_delay" => builder.background_task_maximum_delay(parse_toml_duration(key, item)?),
                 _ => bail!("unrecognized pageserver option '{key}'"),
             }
         }
@@ -903,7 +877,6 @@ impl PageServerConf {
             disk_usage_based_eviction: None,
             test_remote_failures: 0,
             ondemand_download_behavior_treat_error_as_warn: false,
-            background_task_maximum_delay: Duration::ZERO,
         }
     }
 }
@@ -1063,7 +1036,6 @@ metric_collection_endpoint = 'http://localhost:80/metrics'
 synthetic_size_calculation_interval = '333 s'
 
 log_format = 'json'
-background_task_maximum_delay = '334 s'
 
 "#;
 
@@ -1122,9 +1094,6 @@ background_task_maximum_delay = '334 s'
                 disk_usage_based_eviction: None,
                 test_remote_failures: 0,
                 ondemand_download_behavior_treat_error_as_warn: false,
-                background_task_maximum_delay: humantime::parse_duration(
-                    defaults::DEFAULT_BACKGROUND_TASK_MAXIMUM_DELAY
-                )?,
             },
             "Correct defaults should be used when no config values are provided"
         );
@@ -1179,7 +1148,6 @@ background_task_maximum_delay = '334 s'
                 disk_usage_based_eviction: None,
                 test_remote_failures: 0,
                 ondemand_download_behavior_treat_error_as_warn: false,
-                background_task_maximum_delay: Duration::from_secs(334),
             },
             "Should be able to parse all basic config values correctly"
         );

pageserver/src/disk_usage_eviction_task.rs

@@ -83,7 +83,7 @@ pub fn launch_disk_usage_global_eviction_task(
     conf: &'static PageServerConf,
     storage: GenericRemoteStorage,
     state: Arc<State>,
-    background_jobs_barrier: completion::Barrier,
+    init_done: completion::Barrier,
 ) -> anyhow::Result<()> {
     let Some(task_config) = &conf.disk_usage_based_eviction else {
         info!("disk usage based eviction task not configured");
@@ -100,16 +100,17 @@ pub fn launch_disk_usage_global_eviction_task(
         "disk usage based eviction",
         false,
         async move {
-            let cancel = task_mgr::shutdown_token();
-
             // wait until initial load is complete, because we cannot evict from loading tenants.
-            tokio::select! {
-                _ = cancel.cancelled() => { return Ok(()); },
-                _ = background_jobs_barrier.wait() => { }
-            };
+            init_done.wait().await;
 
-            disk_usage_eviction_task(&state, task_config, storage, &conf.tenants_path(), cancel)
-                .await;
+            disk_usage_eviction_task(
+                &state,
+                task_config,
+                storage,
+                &conf.tenants_path(),
+                task_mgr::shutdown_token(),
+            )
+            .await;
             info!("disk usage based eviction task finishing");
             Ok(())
         },

pageserver/src/http/openapi_spec.yml

@@ -928,28 +928,12 @@ components:
               writing to the tenant's S3 state, so, DO NOT ATTACH the
               tenant to any other pageserver, or we risk split-brain.
             - `attached` means that the attach operation has completed,
-              successfully
-            - `failed` means that attach has failed. For reason check corresponding `reason` failed.
-              `failed` is the terminal state, retrying attach call wont resolve the issue.
-              For example this can be caused by s3 being unreachable. The retry may be implemented
-              with call to detach, though it would be better to not automate it and inspec failed state
-              manually before proceeding with a retry.
+              maybe successfully, maybe not. Perform a health check at
+              the Postgres level to determine healthiness of the tenant.
 
             See the tenant `/attach` endpoint for more information.
-          type: object
-          required:
-            - slug
-            - data
-          properties:
-            slug:
-              type: string
-              enum: [ "maybe", "attached", "failed" ]
-            data:
-            - type: object
-              properties:
-                reason:
-                  type: string
-
+          type: string
+          enum: [ "maybe", "attached" ]
     TenantCreateRequest:
       allOf:
         - $ref: '#/components/schemas/TenantConfig'

pageserver/src/lib.rs

@@ -58,6 +58,12 @@ pub async fn shutdown_pageserver(exit_code: i32) {
     // the checkpoint and GC tasks.
     tenant::mgr::shutdown_all_tenants().await;
 
+    // Stop syncing with remote storage.
+    //
+    // FIXME: Does this wait for the sync tasks to finish syncing what's queued up?
+    // Should it?
+    task_mgr::shutdown_tasks(Some(TaskKind::RemoteUploadTask), None, None).await;
+
     // Shut down the HTTP endpoint last, so that you can still check the server's
     // status while it's shutting down.
     // FIXME: We should probably stop accepting commands like attach/detach earlier.
@@ -132,29 +138,6 @@ pub fn is_uninit_mark(path: &Path) -> bool {
     }
 }
 
-/// During pageserver startup, we need to order operations not to exhaust tokio worker threads by
-/// blocking.
-///
-/// The instances of this value exist only during startup, otherwise `None` is provided, meaning no
-/// delaying is needed.
-#[derive(Clone)]
-pub struct InitializationOrder {
-    /// Each initial tenant load task carries this until completion.
-    pub initial_tenant_load: Option<utils::completion::Completion>,
-
-    /// Barrier for when we can start initial logical size calculations.
-    pub initial_logical_size_can_start: utils::completion::Barrier,
-
-    /// Each timeline owns a clone of this to be consumed on the initial logical size calculation
-    /// attempt. It is important to drop this once the attempt has completed.
-    pub initial_logical_size_attempt: utils::completion::Completion,
-
-    /// Barrier for when we can start any background jobs.
-    ///
-    /// This can be broken up later on, but right now there is just one class of a background job.
-    pub background_jobs_can_start: utils::completion::Barrier,
-}
-
 #[cfg(test)]
 mod backoff_defaults_tests {
     use super::*;

pageserver/src/task_mgr.rs

@@ -476,35 +476,18 @@ pub async fn shutdown_tasks(
                 && (timeline_id.is_none() || task_mut.timeline_id == timeline_id)
             {
                 task.cancel.cancel();
-                victim_tasks.push((
-                    Arc::clone(task),
-                    task.kind,
-                    task_mut.tenant_id,
-                    task_mut.timeline_id,
-                ));
+                victim_tasks.push(Arc::clone(task));
             }
         }
     }
 
-    let log_all = kind.is_none() && tenant_id.is_none() && timeline_id.is_none();
-
-    for (task, task_kind, tenant_id, timeline_id) in victim_tasks {
+    for task in victim_tasks {
         let join_handle = {
             let mut task_mut = task.mutable.lock().unwrap();
             task_mut.join_handle.take()
         };
         if let Some(mut join_handle) = join_handle {
-            if log_all {
-                if tenant_id.is_none() {
-                    // there are quite few of these
-                    info!(name = task.name, kind = ?task_kind, "stopping global task");
-                } else {
-                    // warn to catch these in tests; there shouldn't be any
-                    warn!(name = task.name, tenant_id = ?tenant_id, timeline_id = ?timeline_id, kind = ?task_kind, "stopping left-over");
-                }
-            }
             let completed = tokio::select! {
-                biased;
                 _ = &mut join_handle => { true },
                 _ = tokio::time::sleep(std::time::Duration::from_secs(1)) => {
                     // allow some time to elapse before logging to cut down the number of log

pageserver/src/tenant.rs

@@ -65,7 +65,6 @@ use crate::tenant::remote_timeline_client::PersistIndexPartWithDeletedFlagError;
 use crate::tenant::storage_layer::DeltaLayer;
 use crate::tenant::storage_layer::ImageLayer;
 use crate::tenant::storage_layer::Layer;
-use crate::InitializationOrder;
 
 use crate::virtual_file::VirtualFile;
 use crate::walredo::PostgresRedoManager;
@@ -268,7 +267,7 @@ impl UninitializedTimeline<'_> {
         // updated it for the layers that we created during the import.
         let mut timelines = self.owning_tenant.timelines.lock().unwrap();
         let tl = self.initialize_with_lock(ctx, &mut timelines, false)?;
-        tl.activate(broker_client, None, ctx);
+        tl.activate(broker_client, ctx);
         Ok(tl)
     }
 
@@ -487,10 +486,6 @@ impl std::fmt::Display for WaitToBecomeActiveError {
     }
 }
 
-pub(crate) enum ShutdownError {
-    AlreadyStopping,
-}
-
 impl Tenant {
     /// Yet another helper for timeline initialization.
     /// Contains the common part of `load_local_timeline` and `load_remote_timeline`.
@@ -511,7 +506,6 @@ impl Tenant {
         local_metadata: Option<TimelineMetadata>,
         ancestor: Option<Arc<Timeline>>,
         first_save: bool,
-        init_order: Option<&InitializationOrder>,
         ctx: &RequestContext,
     ) -> anyhow::Result<()> {
         let tenant_id = self.tenant_id;
@@ -537,7 +531,6 @@ impl Tenant {
                 up_to_date_metadata,
                 ancestor.clone(),
                 remote_client,
-                init_order,
             )?;
 
             let timeline = UninitializedTimeline {
@@ -563,7 +556,6 @@ impl Tenant {
                             up_to_date_metadata,
                             ancestor.clone(),
                             None,
-                            None,
                         )
                         .with_context(|| {
                             format!("creating broken timeline data for {tenant_id}/{timeline_id}")
@@ -862,7 +854,6 @@ impl Tenant {
             local_metadata,
             ancestor,
             true,
-            None,
             ctx,
         )
         .await
@@ -888,6 +879,7 @@ impl Tenant {
         ))
     }
 
+    ///
     /// Load a tenant that's available on local disk
     ///
     /// This is used at pageserver startup, to rebuild the in-memory
@@ -897,13 +889,14 @@ impl Tenant {
     ///
     /// If the loading fails for some reason, the Tenant will go into Broken
     /// state.
+    ///
     #[instrument(skip_all, fields(tenant_id=%tenant_id))]
     pub fn spawn_load(
         conf: &'static PageServerConf,
         tenant_id: TenantId,
         broker_client: storage_broker::BrokerClientChannel,
         remote_storage: Option<GenericRemoteStorage>,
-        init_order: Option<InitializationOrder>,
+        init_done: Option<(completion::Completion, completion::Barrier)>,
         ctx: &RequestContext,
     ) -> Arc<Tenant> {
         debug_assert_current_span_has_tenant_id();
@@ -939,17 +932,17 @@ impl Tenant {
             "initial tenant load",
             false,
             async move {
-                let mut init_order = init_order;
-
-                // take the completion because initial tenant loading will complete when all of
-                // these tasks complete.
-                let _completion = init_order.as_mut().and_then(|x| x.initial_tenant_load.take());
-
-                match tenant_clone.load(init_order.as_ref(), &ctx).await {
+                // keep the sender alive as long as we have the initial load ongoing; it will be
+                // None for loads spawned after init_tenant_mgr.
+                let (_tx, rx) = if let Some((tx, rx)) = init_done {
+                    (Some(tx), Some(rx))
+                } else {
+                    (None, None)
+                };
+                match tenant_clone.load(&ctx).await {
                     Ok(()) => {
                         debug!("load finished, activating");
-                        let background_jobs_can_start = init_order.as_ref().map(|x| &x.background_jobs_can_start);
-                        tenant_clone.activate(broker_client, background_jobs_can_start, &ctx);
+                        tenant_clone.activate(broker_client, rx.as_ref(), &ctx);
                     }
                     Err(err) => {
                         error!("load failed, setting tenant state to Broken: {err:?}");
@@ -976,11 +969,7 @@ impl Tenant {
     /// files on disk. Used at pageserver startup.
     ///
     /// No background tasks are started as part of this routine.
-    async fn load(
-        self: &Arc<Tenant>,
-        init_order: Option<&InitializationOrder>,
-        ctx: &RequestContext,
-    ) -> anyhow::Result<()> {
+    async fn load(self: &Arc<Tenant>, ctx: &RequestContext) -> anyhow::Result<()> {
         debug_assert_current_span_has_tenant_id();
 
         debug!("loading tenant task");
@@ -1100,7 +1089,7 @@ impl Tenant {
         //    1. "Timeline has no ancestor and no layer files"
 
         for (timeline_id, local_metadata) in sorted_timelines {
-            self.load_local_timeline(timeline_id, local_metadata, init_order, ctx)
+            self.load_local_timeline(timeline_id, local_metadata, ctx)
                 .await
                 .with_context(|| format!("load local timeline {timeline_id}"))?;
         }
@@ -1118,7 +1107,6 @@ impl Tenant {
         &self,
         timeline_id: TimelineId,
         local_metadata: TimelineMetadata,
-        init_order: Option<&InitializationOrder>,
         ctx: &RequestContext,
     ) -> anyhow::Result<()> {
         debug_assert_current_span_has_tenant_id();
@@ -1188,7 +1176,6 @@ impl Tenant {
             Some(local_metadata),
             ancestor,
             false,
-            init_order,
             ctx,
         )
         .await
@@ -1371,7 +1358,7 @@ impl Tenant {
             }
         };
 
-        loaded_timeline.activate(broker_client, None, ctx);
+        loaded_timeline.activate(broker_client, ctx);
 
         if let Some(remote_client) = loaded_timeline.remote_client.as_ref() {
             // Wait for the upload of the 'index_part.json` file to finish, so that when we return
@@ -1407,7 +1394,6 @@ impl Tenant {
         pitr: Duration,
         ctx: &RequestContext,
     ) -> anyhow::Result<GcResult> {
-        // there is a global allowed_error for this
         anyhow::ensure!(
             self.is_active(),
             "Cannot run GC iteration on inactive tenant"
@@ -1451,63 +1437,28 @@ impl Tenant {
         Ok(())
     }
 
-    /// Flush all in-memory data to disk and remote storage, if any.
+    /// Flush all in-memory data to disk.
     ///
     /// Used at graceful shutdown.
-    async fn freeze_and_flush_on_shutdown(&self) {
-        let mut js = tokio::task::JoinSet::new();
-
-        // execute on each timeline on the JoinSet, join after.
-        let per_timeline = |timeline_id: TimelineId, timeline: Arc<Timeline>| {
-            async move {
-                debug_assert_current_span_has_tenant_and_timeline_id();
-
-                match timeline.freeze_and_flush().await {
-                    Ok(()) => {}
-                    Err(e) => {
-                        warn!("failed to freeze and flush: {e:#}");
-                        return;
-                    }
-                }
-
-                let res = if let Some(client) = timeline.remote_client.as_ref() {
-                    // if we did not wait for completion here, it might be our shutdown process
-                    // didn't wait for remote uploads to complete at all, as new tasks can forever
-                    // be spawned.
-                    //
-                    // what is problematic is the shutting down of RemoteTimelineClient, because
-                    // obviously it does not make sense to stop while we wait for it, but what
-                    // about corner cases like s3 suddenly hanging up?
-                    client.wait_completion().await
-                } else {
-                    Ok(())
-                };
-
-                if let Err(e) = res {
-                    warn!("failed to await for frozen and flushed uploads: {e:#}");
-                }
-            }
-            .instrument(tracing::info_span!("freeze_and_flush_on_shutdown", %timeline_id))
-        };
-
-        {
+    ///
+    pub async fn freeze_and_flush(&self) -> anyhow::Result<()> {
+        // Scan through the hashmap and collect a list of all the timelines,
+        // while holding the lock. Then drop the lock and actually perform the
+        // flushing. We don't want to block everything else while the
+        // flushing is performed.
+        let timelines_to_flush = {
             let timelines = self.timelines.lock().unwrap();
             timelines
                 .iter()
-                .map(|(id, tl)| (*id, Arc::clone(tl)))
-                .for_each(|(timeline_id, timeline)| {
-                    js.spawn(per_timeline(timeline_id, timeline));
-                })
+                .map(|(_id, timeline)| Arc::clone(timeline))
+                .collect::<Vec<_>>()
         };
 
-        while let Some(res) = js.join_next().await {
-            match res {
-                Ok(()) => {}
-                Err(je) if je.is_cancelled() => unreachable!("no cancelling used"),
-                Err(je) if je.is_panic() => { /* logged already */ }
-                Err(je) => warn!("unexpected JoinError: {je:?}"),
-            }
+        for timeline in &timelines_to_flush {
+            timeline.freeze_and_flush().await?;
         }
+
+        Ok(())
     }
 
     /// Shuts down a timeline's tasks, removes its in-memory structures, and deletes its
@@ -1731,13 +1682,10 @@ impl Tenant {
     }
 
     /// Changes tenant status to active, unless shutdown was already requested.
-    ///
-    /// `background_jobs_can_start` is an optional barrier set to a value during pageserver startup
-    /// to delay background jobs. Background jobs can be started right away when None is given.
     fn activate(
         self: &Arc<Self>,
         broker_client: BrokerClientChannel,
-        background_jobs_can_start: Option<&completion::Barrier>,
+        init_done: Option<&completion::Barrier>,
         ctx: &RequestContext,
     ) {
         debug_assert_current_span_has_tenant_id();
@@ -1770,12 +1718,12 @@ impl Tenant {
 
             // Spawn gc and compaction loops. The loops will shut themselves
             // down when they notice that the tenant is inactive.
-            tasks::start_background_loops(self, background_jobs_can_start);
+            tasks::start_background_loops(self, init_done);
 
             let mut activated_timelines = 0;
 
             for timeline in not_broken_timelines {
-                timeline.activate(broker_client.clone(), background_jobs_can_start, ctx);
+                timeline.activate(broker_client.clone(), ctx);
                 activated_timelines += 1;
             }
 
@@ -1803,70 +1751,12 @@ impl Tenant {
         }
     }
 
-    /// Shutdown the tenant and join all of the spawned tasks.
-    ///
-    /// The method caters for all use-cases:
-    /// - pageserver shutdown (freeze_and_flush == true)
-    /// - detach + ignore (freeze_and_flush == false)
-    ///
-    /// This will attempt to shutdown even if tenant is broken.
-    pub(crate) async fn shutdown(&self, freeze_and_flush: bool) -> Result<(), ShutdownError> {
-        debug_assert_current_span_has_tenant_id();
-        // Set tenant (and its timlines) to Stoppping state.
-        //
-        // Since we can only transition into Stopping state after activation is complete,
-        // run it in a JoinSet so all tenants have a chance to stop before we get SIGKILLed.
-        //
-        // Transitioning tenants to Stopping state has a couple of non-obvious side effects:
-        // 1. Lock out any new requests to the tenants.
-        // 2. Signal cancellation to WAL receivers (we wait on it below).
-        // 3. Signal cancellation for other tenant background loops.
-        // 4. ???
-        //
-        // The waiting for the cancellation is not done uniformly.
-        // We certainly wait for WAL receivers to shut down.
-        // That is necessary so that no new data comes in before the freeze_and_flush.
-        // But the tenant background loops are joined-on in our caller.
-        // It's mesed up.
-        // we just ignore the failure to stop
-        match self.set_stopping().await {
-            Ok(()) => {}
-            Err(SetStoppingError::Broken) => {
-                // assume that this is acceptable
-            }
-            Err(SetStoppingError::AlreadyStopping) => return Err(ShutdownError::AlreadyStopping),
-        };
-
-        if freeze_and_flush {
-            // walreceiver has already began to shutdown with TenantState::Stopping, but we need to
-            // await for them to stop.
-            task_mgr::shutdown_tasks(
-                Some(TaskKind::WalReceiverManager),
-                Some(self.tenant_id),
-                None,
-            )
-            .await;
-
-            // this will wait for uploads to complete; in the past, it was done outside tenant
-            // shutdown in pageserver::shutdown_pageserver.
-            self.freeze_and_flush_on_shutdown().await;
-        }
-
-        // shutdown all tenant and timeline tasks: gc, compaction, page service
-        // No new tasks will be started for this tenant because it's in `Stopping` state.
-        //
-        // this will additionally shutdown and await all timeline tasks.
-        task_mgr::shutdown_tasks(None, Some(self.tenant_id), None).await;
-
-        Ok(())
-    }
-
     /// Change tenant status to Stopping, to mark that it is being shut down.
     ///
     /// This function waits for the tenant to become active if it isn't already, before transitioning it into Stopping state.
     ///
     /// This function is not cancel-safe!
-    async fn set_stopping(&self) -> Result<(), SetStoppingError> {
+    pub async fn set_stopping(&self) -> Result<(), SetStoppingError> {
         let mut rx = self.state.subscribe();
 
         // cannot stop before we're done activating, so wait out until we're done activating
@@ -2166,7 +2056,6 @@ impl Tenant {
         new_metadata: &TimelineMetadata,
         ancestor: Option<Arc<Timeline>>,
         remote_client: Option<RemoteTimelineClient>,
-        init_order: Option<&InitializationOrder>,
     ) -> anyhow::Result<Arc<Timeline>> {
         if let Some(ancestor_timeline_id) = new_metadata.ancestor_timeline() {
             anyhow::ensure!(
@@ -2175,9 +2064,6 @@ impl Tenant {
             )
         }
 
-        let initial_logical_size_can_start = init_order.map(|x| &x.initial_logical_size_can_start);
-        let initial_logical_size_attempt = init_order.map(|x| &x.initial_logical_size_attempt);
-
         let pg_version = new_metadata.pg_version();
         Ok(Timeline::new(
             self.conf,
@@ -2189,8 +2075,6 @@ impl Tenant {
             Arc::clone(&self.walredo_mgr),
             remote_client,
             pg_version,
-            initial_logical_size_can_start.cloned(),
-            initial_logical_size_attempt.cloned(),
         ))
     }
 
@@ -2866,7 +2750,7 @@ impl Tenant {
         remote_client: Option<RemoteTimelineClient>,
     ) -> anyhow::Result<Arc<Timeline>> {
         let timeline_data = self
-            .create_timeline_data(new_timeline_id, new_metadata, ancestor, remote_client, None)
+            .create_timeline_data(new_timeline_id, new_metadata, ancestor, remote_client)
             .context("Failed to create timeline data structure")?;
         crashsafe::create_dir_all(timeline_path).context("Failed to create timeline directory")?;
 
@@ -3434,7 +3318,7 @@ pub mod harness {
                 timelines_to_load.insert(timeline_id, timeline_metadata);
             }
             tenant
-                .load(None, ctx)
+                .load(ctx)
                 .instrument(info_span!("try_load", tenant_id=%self.tenant_id))
                 .await?;
             tenant.state.send_replace(TenantState::Active);

pageserver/src/tenant/layer_map.rs

@@ -51,9 +51,7 @@ use crate::keyspace::KeyPartitioning;
 use crate::repository::Key;
 use crate::tenant::storage_layer::InMemoryLayer;
 use crate::tenant::storage_layer::Layer;
-use anyhow::Context;
 use anyhow::Result;
-use std::collections::HashMap;
 use std::collections::VecDeque;
 use std::ops::Range;
 use std::sync::Arc;
@@ -63,8 +61,6 @@ use historic_layer_coverage::BufferedHistoricLayerCoverage;
 pub use historic_layer_coverage::Replacement;
 
 use super::storage_layer::range_eq;
-use super::storage_layer::PersistentLayerDesc;
-use super::storage_layer::PersistentLayerKey;
 
 ///
 /// LayerMap tracks what layers exist on a timeline.
@@ -90,16 +86,11 @@ pub struct LayerMap<L: ?Sized> {
     pub frozen_layers: VecDeque<Arc<InMemoryLayer>>,
 
     /// Index of the historic layers optimized for search
-    historic: BufferedHistoricLayerCoverage<Arc<PersistentLayerDesc>>,
+    historic: BufferedHistoricLayerCoverage<Arc<L>>,
 
     /// L0 layers have key range Key::MIN..Key::MAX, and locating them using R-Tree search is very inefficient.
     /// So L0 layers are held in l0_delta_layers vector, in addition to the R-tree.
-    l0_delta_layers: Vec<Arc<PersistentLayerDesc>>,
-
-    /// Mapping from persistent layer key to the actual layer object. Currently, it stores delta, image, and
-    /// remote layers. In future refactors, this will be eventually moved out of LayerMap into Timeline, and
-    /// RemoteLayer will be removed.
-    mapping: HashMap<PersistentLayerKey, Arc<L>>,
+    l0_delta_layers: Vec<Arc<L>>,
 }
 
 impl<L: ?Sized> Default for LayerMap<L> {
@@ -110,7 +101,6 @@ impl<L: ?Sized> Default for LayerMap<L> {
             frozen_layers: VecDeque::default(),
             l0_delta_layers: Vec::default(),
             historic: BufferedHistoricLayerCoverage::default(),
-            mapping: HashMap::default(),
         }
     }
 }
@@ -135,9 +125,8 @@ where
     ///
     /// Insert an on-disk layer.
     ///
-    // TODO remove the `layer` argument when `mapping` is refactored out of `LayerMap`
-    pub fn insert_historic(&mut self, layer_desc: PersistentLayerDesc, layer: Arc<L>) {
-        self.layer_map.insert_historic_noflush(layer_desc, layer)
+    pub fn insert_historic(&mut self, layer: Arc<L>) {
+        self.layer_map.insert_historic_noflush(layer)
     }
 
     ///
@@ -145,8 +134,8 @@ where
     ///
     /// This should be called when the corresponding file on disk has been deleted.
     ///
-    pub fn remove_historic(&mut self, layer_desc: PersistentLayerDesc, layer: Arc<L>) {
-        self.layer_map.remove_historic_noflush(layer_desc, layer)
+    pub fn remove_historic(&mut self, layer: Arc<L>) {
+        self.layer_map.remove_historic_noflush(layer)
     }
 
     /// Replaces existing layer iff it is the `expected`.
@@ -161,15 +150,12 @@ where
     ///      that we can replace values only by updating a hashmap.
     pub fn replace_historic(
         &mut self,
-        expected_desc: PersistentLayerDesc,
         expected: &Arc<L>,
-        new_desc: PersistentLayerDesc,
         new: Arc<L>,
     ) -> anyhow::Result<Replacement<Arc<L>>> {
         fail::fail_point!("layermap-replace-notfound", |_| Ok(Replacement::NotFound));
 
-        self.layer_map
-            .replace_historic_noflush(expected_desc, expected, new_desc, new)
+        self.layer_map.replace_historic_noflush(expected, new)
     }
 
     // We will flush on drop anyway, but this method makes it
@@ -244,7 +230,6 @@ where
             (None, None) => None,
             (None, Some(image)) => {
                 let lsn_floor = image.get_lsn_range().start;
-                let image = self.get_layer_from_mapping(&image.key()).clone();
                 Some(SearchResult {
                     layer: image,
                     lsn_floor,
@@ -252,7 +237,6 @@ where
             }
             (Some(delta), None) => {
                 let lsn_floor = delta.get_lsn_range().start;
-                let delta = self.get_layer_from_mapping(&delta.key()).clone();
                 Some(SearchResult {
                     layer: delta,
                     lsn_floor,
@@ -263,7 +247,6 @@ where
                 let image_is_newer = image.get_lsn_range().end >= delta.get_lsn_range().end;
                 let image_exact_match = img_lsn + 1 == end_lsn;
                 if image_is_newer || image_exact_match {
-                    let image = self.get_layer_from_mapping(&image.key()).clone();
                     Some(SearchResult {
                         layer: image,
                         lsn_floor: img_lsn,
@@ -271,7 +254,6 @@ where
                 } else {
                     let lsn_floor =
                         std::cmp::max(delta.get_lsn_range().start, image.get_lsn_range().start + 1);
-                    let delta = self.get_layer_from_mapping(&delta.key()).clone();
                     Some(SearchResult {
                         layer: delta,
                         lsn_floor,
@@ -291,33 +273,16 @@ where
     ///
     /// Helper function for BatchedUpdates::insert_historic
     ///
-    /// TODO(chi): remove L generic so that we do not need to pass layer object.
-    pub(self) fn insert_historic_noflush(
-        &mut self,
-        layer_desc: PersistentLayerDesc,
-        layer: Arc<L>,
-    ) {
-        self.mapping.insert(layer_desc.key(), layer.clone());
-
+    pub(self) fn insert_historic_noflush(&mut self, layer: Arc<L>) {
         // TODO: See #3869, resulting #4088, attempted fix and repro #4094
-
-        if Self::is_l0(&layer) {
-            self.l0_delta_layers.push(layer_desc.clone().into());
-        }
-
         self.historic.insert(
             historic_layer_coverage::LayerKey::from(&*layer),
-            layer_desc.into(),
+            Arc::clone(&layer),
         );
-    }
 
-    fn get_layer_from_mapping(&self, key: &PersistentLayerKey) -> &Arc<L> {
-        let layer = self
-            .mapping
-            .get(key)
-            .with_context(|| format!("{key:?}"))
-            .expect("inconsistent layer mapping");
-        layer
+        if Self::is_l0(&layer) {
+            self.l0_delta_layers.push(layer);
+        }
     }
 
     ///
@@ -325,16 +290,14 @@ where
     ///
     /// Helper function for BatchedUpdates::remove_historic
     ///
-    pub fn remove_historic_noflush(&mut self, layer_desc: PersistentLayerDesc, layer: Arc<L>) {
+    pub fn remove_historic_noflush(&mut self, layer: Arc<L>) {
         self.historic
             .remove(historic_layer_coverage::LayerKey::from(&*layer));
+
         if Self::is_l0(&layer) {
             let len_before = self.l0_delta_layers.len();
-            let mut l0_delta_layers = std::mem::take(&mut self.l0_delta_layers);
-            l0_delta_layers.retain(|other| {
-                !Self::compare_arced_layers(self.get_layer_from_mapping(&other.key()), &layer)
-            });
-            self.l0_delta_layers = l0_delta_layers;
+            self.l0_delta_layers
+                .retain(|other| !Self::compare_arced_layers(other, &layer));
             // this assertion is related to use of Arc::ptr_eq in Self::compare_arced_layers,
             // there's a chance that the comparison fails at runtime due to it comparing (pointer,
             // vtable) pairs.
@@ -344,14 +307,11 @@ where
                 "failed to locate removed historic layer from l0_delta_layers"
             );
         }
-        self.mapping.remove(&layer_desc.key());
     }
 
     pub(self) fn replace_historic_noflush(
         &mut self,
-        expected_desc: PersistentLayerDesc,
         expected: &Arc<L>,
-        new_desc: PersistentLayerDesc,
         new: Arc<L>,
     ) -> anyhow::Result<Replacement<Arc<L>>> {
         let key = historic_layer_coverage::LayerKey::from(&**expected);
@@ -372,9 +332,10 @@ where
 
         let l0_index = if expected_l0 {
             // find the index in case replace worked, we need to replace that as well
-            let pos = self.l0_delta_layers.iter().position(|slot| {
-                Self::compare_arced_layers(self.get_layer_from_mapping(&slot.key()), expected)
-            });
+            let pos = self
+                .l0_delta_layers
+                .iter()
+                .position(|slot| Self::compare_arced_layers(slot, expected));
 
             if pos.is_none() {
                 return Ok(Replacement::NotFound);
@@ -384,28 +345,16 @@ where
             None
         };
 
-        let new_desc = Arc::new(new_desc);
-        let replaced = self.historic.replace(&key, new_desc.clone(), |existing| {
-            **existing == expected_desc
+        let replaced = self.historic.replace(&key, new.clone(), |existing| {
+            Self::compare_arced_layers(existing, expected)
         });
 
         if let Replacement::Replaced { .. } = &replaced {
-            self.mapping.remove(&expected_desc.key());
-            self.mapping.insert(new_desc.key(), new);
             if let Some(index) = l0_index {
-                self.l0_delta_layers[index] = new_desc;
+                self.l0_delta_layers[index] = new;
             }
         }
 
-        let replaced = match replaced {
-            Replacement::Replaced { in_buffered } => Replacement::Replaced { in_buffered },
-            Replacement::NotFound => Replacement::NotFound,
-            Replacement::RemovalBuffered => Replacement::RemovalBuffered,
-            Replacement::Unexpected(x) => {
-                Replacement::Unexpected(self.get_layer_from_mapping(&x.key()).clone())
-            }
-        };
-
         Ok(replaced)
     }
 
@@ -434,7 +383,7 @@ where
         let start = key.start.to_i128();
         let end = key.end.to_i128();
 
-        let layer_covers = |layer: Option<Arc<PersistentLayerDesc>>| match layer {
+        let layer_covers = |layer: Option<Arc<L>>| match layer {
             Some(layer) => layer.get_lsn_range().start >= lsn.start,
             None => false,
         };
@@ -455,9 +404,7 @@ where
     }
 
     pub fn iter_historic_layers(&self) -> impl '_ + Iterator<Item = Arc<L>> {
-        self.historic
-            .iter()
-            .map(|x| self.get_layer_from_mapping(&x.key()).clone())
+        self.historic.iter()
     }
 
     ///
@@ -489,24 +436,14 @@ where
         // Loop through the change events and push intervals
         for (change_key, change_val) in version.image_coverage.range(start..end) {
             let kr = Key::from_i128(current_key)..Key::from_i128(change_key);
-            coverage.push((
-                kr,
-                current_val
-                    .take()
-                    .map(|l| self.get_layer_from_mapping(&l.key()).clone()),
-            ));
+            coverage.push((kr, current_val.take()));
             current_key = change_key;
             current_val = change_val.clone();
         }
 
         // Add the final interval
         let kr = Key::from_i128(current_key)..Key::from_i128(end);
-        coverage.push((
-            kr,
-            current_val
-                .take()
-                .map(|l| self.get_layer_from_mapping(&l.key()).clone()),
-        ));
+        coverage.push((kr, current_val.take()));
 
         Ok(coverage)
     }
@@ -595,9 +532,7 @@ where
                     let kr = Key::from_i128(current_key)..Key::from_i128(change_key);
                     let lr = lsn.start..val.get_lsn_range().start;
                     if !kr.is_empty() {
-                        let base_count =
-                            Self::is_reimage_worthy(self.get_layer_from_mapping(&val.key()), key)
-                                as usize;
+                        let base_count = Self::is_reimage_worthy(&val, key) as usize;
                         let new_limit = limit.map(|l| l - base_count);
                         let max_stacked_deltas_underneath =
                             self.count_deltas(&kr, &lr, new_limit)?;
@@ -620,9 +555,7 @@ where
                 let lr = lsn.start..val.get_lsn_range().start;
 
                 if !kr.is_empty() {
-                    let base_count =
-                        Self::is_reimage_worthy(self.get_layer_from_mapping(&val.key()), key)
-                            as usize;
+                    let base_count = Self::is_reimage_worthy(&val, key) as usize;
                     let new_limit = limit.map(|l| l - base_count);
                     let max_stacked_deltas_underneath = self.count_deltas(&kr, &lr, new_limit)?;
                     max_stacked_deltas = std::cmp::max(
@@ -773,11 +706,7 @@ where
 
     /// Return all L0 delta layers
     pub fn get_level0_deltas(&self) -> Result<Vec<Arc<L>>> {
-        Ok(self
-            .l0_delta_layers
-            .iter()
-            .map(|x| self.get_layer_from_mapping(&x.key()).clone())
-            .collect())
+        Ok(self.l0_delta_layers.clone())
     }
 
     /// debugging function to print out the contents of the layer map
@@ -880,17 +809,12 @@ mod tests {
             let layer = LayerDescriptor::from(layer);
 
             // same skeletan construction; see scenario below
-            let not_found = Arc::new(layer.clone());
-            let new_version = Arc::new(layer);
+            let not_found: Arc<dyn Layer> = Arc::new(layer.clone());
+            let new_version: Arc<dyn Layer> = Arc::new(layer);
 
             let mut map = LayerMap::default();
 
-            let res = map.batch_update().replace_historic(
-                not_found.get_persistent_layer_desc(),
-                &not_found,
-                new_version.get_persistent_layer_desc(),
-                new_version,
-            );
+            let res = map.batch_update().replace_historic(&not_found, new_version);
 
             assert!(matches!(res, Ok(Replacement::NotFound)), "{res:?}");
         }
@@ -899,8 +823,8 @@ mod tests {
             let name = LayerFileName::from_str(layer_name).unwrap();
             let skeleton = LayerDescriptor::from(name);
 
-            let remote = Arc::new(skeleton.clone());
-            let downloaded = Arc::new(skeleton);
+            let remote: Arc<dyn Layer> = Arc::new(skeleton.clone());
+            let downloaded: Arc<dyn Layer> = Arc::new(skeleton);
 
             let mut map = LayerMap::default();
 
@@ -910,18 +834,12 @@ mod tests {
 
             let expected_in_counts = (1, usize::from(expected_l0));
 
-            map.batch_update()
-                .insert_historic(remote.get_persistent_layer_desc(), remote.clone());
+            map.batch_update().insert_historic(remote.clone());
             assert_eq!(count_layer_in(&map, &remote), expected_in_counts);
 
             let replaced = map
                 .batch_update()
-                .replace_historic(
-                    remote.get_persistent_layer_desc(),
-                    &remote,
-                    downloaded.get_persistent_layer_desc(),
-                    downloaded.clone(),
-                )
+                .replace_historic(&remote, downloaded.clone())
                 .expect("name derived attributes are the same");
             assert!(
                 matches!(replaced, Replacement::Replaced { .. }),
@@ -929,12 +847,11 @@ mod tests {
             );
             assert_eq!(count_layer_in(&map, &downloaded), expected_in_counts);
 
-            map.batch_update()
-                .remove_historic(downloaded.get_persistent_layer_desc(), downloaded.clone());
+            map.batch_update().remove_historic(downloaded.clone());
             assert_eq!(count_layer_in(&map, &downloaded), (0, 0));
         }
 
-        fn count_layer_in<L: Layer + ?Sized>(map: &LayerMap<L>, layer: &Arc<L>) -> (usize, usize) {
+        fn count_layer_in(map: &LayerMap<dyn Layer>, layer: &Arc<dyn Layer>) -> (usize, usize) {
             let historic = map
                 .iter_historic_layers()
                 .filter(|x| LayerMap::compare_arced_layers(x, layer))

pageserver/src/tenant/mgr.rs

@@ -20,9 +20,12 @@ use crate::config::PageServerConf;
 use crate::context::{DownloadBehavior, RequestContext};
 use crate::task_mgr::{self, TaskKind};
 use crate::tenant::config::TenantConfOpt;
-use crate::tenant::{create_tenant_files, CreateTenantFilesMode, Tenant, TenantState};
-use crate::{InitializationOrder, IGNORED_TENANT_FILE_NAME};
+use crate::tenant::{
+    create_tenant_files, CreateTenantFilesMode, SetStoppingError, Tenant, TenantState,
+};
+use crate::IGNORED_TENANT_FILE_NAME;
 
+use utils::completion;
 use utils::fs_ext::PathExt;
 use utils::id::{TenantId, TimelineId};
 
@@ -64,7 +67,7 @@ pub async fn init_tenant_mgr(
     conf: &'static PageServerConf,
     broker_client: storage_broker::BrokerClientChannel,
     remote_storage: Option<GenericRemoteStorage>,
-    init_order: InitializationOrder,
+    init_done: (completion::Completion, completion::Barrier),
 ) -> anyhow::Result<()> {
     // Scan local filesystem for attached tenants
     let tenants_dir = conf.tenants_path();
@@ -121,7 +124,7 @@ pub async fn init_tenant_mgr(
                         &tenant_dir_path,
                         broker_client.clone(),
                         remote_storage.clone(),
-                        Some(init_order.clone()),
+                        Some(init_done.clone()),
                         &ctx,
                     ) {
                         Ok(tenant) => {
@@ -157,7 +160,7 @@ pub fn schedule_local_tenant_processing(
     tenant_path: &Path,
     broker_client: storage_broker::BrokerClientChannel,
     remote_storage: Option<GenericRemoteStorage>,
-    init_order: Option<InitializationOrder>,
+    init_done: Option<(completion::Completion, completion::Barrier)>,
     ctx: &RequestContext,
 ) -> anyhow::Result<Arc<Tenant>> {
     anyhow::ensure!(
@@ -216,7 +219,7 @@ pub fn schedule_local_tenant_processing(
             tenant_id,
             broker_client,
             remote_storage,
-            init_order,
+            init_done,
             ctx,
         )
     };
@@ -250,28 +253,46 @@ pub async fn shutdown_all_tenants() {
                 tenants_clone
             }
             TenantsMap::ShuttingDown(_) => {
-                // TODO: it is possible that detach and shutdown happen at the same time. as a
-                // result, during shutdown we do not wait for detach.
                 error!("already shutting down, this function isn't supposed to be called more than once");
                 return;
             }
         }
     };
 
+    // Set tenant (and its timlines) to Stoppping state.
+    //
+    // Since we can only transition into Stopping state after activation is complete,
+    // run it in a JoinSet so all tenants have a chance to stop before we get SIGKILLed.
+    //
+    // Transitioning tenants to Stopping state has a couple of non-obvious side effects:
+    // 1. Lock out any new requests to the tenants.
+    // 2. Signal cancellation to WAL receivers (we wait on it below).
+    // 3. Signal cancellation for other tenant background loops.
+    // 4. ???
+    //
+    // The waiting for the cancellation is not done uniformly.
+    // We certainly wait for WAL receivers to shut down.
+    // That is necessary so that no new data comes in before the freeze_and_flush.
+    // But the tenant background loops are joined-on in our caller.
+    // It's mesed up.
     let mut join_set = JoinSet::new();
+    let mut tenants_to_freeze_and_flush = Vec::with_capacity(tenants_to_shut_down.len());
     for (tenant_id, tenant) in tenants_to_shut_down {
         join_set.spawn(
             async move {
-                let freeze_and_flush = true;
-
-                match tenant.shutdown(freeze_and_flush).await {
+                match tenant.set_stopping().await {
                     Ok(()) => debug!("tenant successfully stopped"),
-                    Err(super::ShutdownError::AlreadyStopping) => {
-                        warn!("tenant was already shutting down")
+                    Err(SetStoppingError::Broken) => {
+                        info!("tenant is broken, so stopping failed, freeze_and_flush is likely going to make noise as well");
+                    },
+                    Err(SetStoppingError::AlreadyStopping) => {
+                        // our task_mgr::shutdown_tasks are going to coalesce on that just fine
                     }
                 }
+
+                tenant
             }
-            .instrument(info_span!("shutdown", %tenant_id)),
+            .instrument(info_span!("set_stopping", %tenant_id)),
         );
     }
 
@@ -279,7 +300,6 @@ pub async fn shutdown_all_tenants() {
 
     while let Some(res) = join_set.join_next().await {
         match res {
-            Ok(()) => {}
             Err(join_error) if join_error.is_cancelled() => {
                 unreachable!("we are not cancelling any of the futures");
             }
@@ -290,11 +310,50 @@ pub async fn shutdown_all_tenants() {
             Err(join_error) => {
                 warn!("unknown kind of JoinError: {join_error}");
             }
+            Ok(tenant) => tenants_to_freeze_and_flush.push(tenant),
         }
     }
 
     if panicked > 0 {
-        warn!(panicked, "observed panicks while shutting down tenants");
+        warn!(panicked, "observed panicks while stopping tenants");
+    }
+
+    // Shut down all existing walreceiver connections and stop accepting the new ones.
+    task_mgr::shutdown_tasks(Some(TaskKind::WalReceiverManager), None, None).await;
+
+    // Ok, no background tasks running anymore. Flush any remaining data in
+    // memory to disk.
+    //
+    // We assume that any incoming connections that might request pages from
+    // the tenant have already been terminated by the caller, so there
+    // should be no more activity in any of the repositories.
+    //
+    // On error, log it but continue with the shutdown for other tenants.
+
+    let mut join_set = tokio::task::JoinSet::new();
+
+    for tenant in tenants_to_freeze_and_flush {
+        let tenant_id = tenant.tenant_id();
+
+        join_set.spawn(
+            async move {
+                if let Err(err) = tenant.freeze_and_flush().await {
+                    warn!("Could not checkpoint tenant during shutdown: {err:?}");
+                }
+            }
+            .instrument(info_span!("freeze_and_flush", %tenant_id)),
+        );
+    }
+
+    while let Some(next) = join_set.join_next().await {
+        match next {
+            Ok(()) => {}
+            Err(join_error) if join_error.is_cancelled() => {
+                unreachable!("no cancelling")
+            }
+            Err(join_error) if join_error.is_panic() => { /* reported already */ }
+            Err(join_error) => warn!("unknown kind of JoinError: {join_error}"),
+        }
     }
 }
 
@@ -610,26 +669,35 @@ where
     // The exclusive lock here ensures we don't miss the tenant state updates before trying another removal.
     // tenant-wde cleanup operations may take some time (removing the entire tenant directory), we want to
     // avoid holding the lock for the entire process.
-    let tenant = {
-        TENANTS
-            .write()
-            .await
-            .get(&tenant_id)
-            .cloned()
-            .ok_or(TenantStateError::NotFound(tenant_id))?
-    };
-
-    let freeze_and_flush = false;
-
-    // shutdown is sure to transition tenant to stopping, and wait for all tasks to complete, so
-    // that we can continue safely to cleanup.
-    match tenant.shutdown(freeze_and_flush).await {
-        Ok(()) => {}
-        Err(super::ShutdownError::AlreadyStopping) => {
-            return Err(TenantStateError::IsStopping(tenant_id))
+    {
+        let tenants_accessor = TENANTS.write().await;
+        match tenants_accessor.get(&tenant_id) {
+            Some(tenant) => {
+                let tenant = Arc::clone(tenant);
+                // don't hold TENANTS lock while set_stopping waits for activation to finish
+                drop(tenants_accessor);
+                match tenant.set_stopping().await {
+                    Ok(()) => {
+                        // we won, continue stopping procedure
+                    }
+                    Err(SetStoppingError::Broken) => {
+                        // continue the procedure, let's hope the closure can deal with broken tenants
+                    }
+                    Err(SetStoppingError::AlreadyStopping) => {
+                        // the tenant is already stopping or broken, don't do anything
+                        return Err(TenantStateError::IsStopping(tenant_id));
+                    }
+                }
+            }
+            None => return Err(TenantStateError::NotFound(tenant_id)),
         }
     }
 
+    // shutdown all tenant and timeline tasks: gc, compaction, page service)
+    // No new tasks will be started for this tenant because it's in `Stopping` state.
+    // Hence, once we're done here, the `tenant_cleanup` callback can mutate tenant on-disk state freely.
+    task_mgr::shutdown_tasks(None, Some(tenant_id), None).await;
+
     match tenant_cleanup
         .await
         .with_context(|| format!("Failed to run cleanup for tenant {tenant_id}"))

pageserver/src/tenant/storage_layer.rs

@@ -38,7 +38,7 @@ pub use delta_layer::{DeltaLayer, DeltaLayerWriter};
 pub use filename::{DeltaFileName, ImageFileName, LayerFileName};
 pub use image_layer::{ImageLayer, ImageLayerWriter};
 pub use inmemory_layer::InMemoryLayer;
-pub use layer_desc::{PersistentLayerDesc, PersistentLayerKey};
+pub use layer_desc::PersistentLayerDesc;
 pub use remote_layer::RemoteLayer;
 
 use super::layer_map::BatchedUpdates;
@@ -454,9 +454,7 @@ pub trait PersistentLayer: Layer {
     ///
     /// Should not change over the lifetime of the layer object because
     /// current_physical_size is computed as the som of this value.
-    fn file_size(&self) -> u64 {
-        self.layer_desc().file_size
-    }
+    fn file_size(&self) -> u64;
 
     fn info(&self, reset: LayerAccessStatsReset) -> HistoricLayerInfo;
 
@@ -485,20 +483,6 @@ pub struct LayerDescriptor {
     pub short_id: String,
 }
 
-impl LayerDescriptor {
-    /// `LayerDescriptor` is only used for testing purpose so it does not matter whether it is image / delta,
-    /// and the tenant / timeline id does not matter.
-    pub fn get_persistent_layer_desc(&self) -> PersistentLayerDesc {
-        PersistentLayerDesc::new_delta(
-            TenantId::from_array([0; 16]),
-            TimelineId::from_array([0; 16]),
-            self.key.clone(),
-            self.lsn.clone(),
-            233,
-        )
-    }
-}
-
 impl Layer for LayerDescriptor {
     fn get_key_range(&self) -> Range<Key> {
         self.key.clone()

pageserver/src/tenant/storage_layer/delta_layer.rs

@@ -182,6 +182,8 @@ pub struct DeltaLayer {
 
     pub desc: PersistentLayerDesc,
 
+    pub file_size: u64,
+
     access_stats: LayerAccessStats,
 
     inner: RwLock<DeltaLayerInner>,
@@ -194,7 +196,7 @@ impl std::fmt::Debug for DeltaLayer {
         f.debug_struct("DeltaLayer")
             .field("key_range", &RangeDisplayDebug(&self.desc.key_range))
             .field("lsn_range", &self.desc.lsn_range)
-            .field("file_size", &self.desc.file_size)
+            .field("file_size", &self.file_size)
             .field("inner", &self.inner)
             .finish()
     }
@@ -437,6 +439,10 @@ impl PersistentLayer for DeltaLayer {
         Ok(())
     }
 
+    fn file_size(&self) -> u64 {
+        self.file_size
+    }
+
     fn info(&self, reset: LayerAccessStatsReset) -> HistoricLayerInfo {
         let layer_file_name = self.filename().file_name();
         let lsn_range = self.get_lsn_range();
@@ -445,7 +451,7 @@ impl PersistentLayer for DeltaLayer {
 
         HistoricLayerInfo::Delta {
             layer_file_name,
-            layer_file_size: self.desc.file_size,
+            layer_file_size: self.file_size,
             lsn_start: lsn_range.start,
             lsn_end: lsn_range.end,
             remote: false,
@@ -596,8 +602,8 @@ impl DeltaLayer {
                 timeline_id,
                 filename.key_range.clone(),
                 filename.lsn_range.clone(),
-                file_size,
             ),
+            file_size,
             access_stats,
             inner: RwLock::new(DeltaLayerInner {
                 loaded: false,
@@ -628,8 +634,8 @@ impl DeltaLayer {
                 summary.timeline_id,
                 summary.key_range,
                 summary.lsn_range,
-                metadata.len(),
             ),
+            file_size: metadata.len(),
             access_stats: LayerAccessStats::empty_will_record_residence_event_later(),
             inner: RwLock::new(DeltaLayerInner {
                 loaded: false,
@@ -797,8 +803,8 @@ impl DeltaLayerWriterInner {
                 self.timeline_id,
                 self.key_start..key_end,
                 self.lsn_range.clone(),
-                metadata.len(),
             ),
+            file_size: metadata.len(),
             access_stats: LayerAccessStats::empty_will_record_residence_event_later(),
             inner: RwLock::new(DeltaLayerInner {
                 loaded: false,

pageserver/src/tenant/storage_layer/image_layer.rs

@@ -109,6 +109,8 @@ pub struct ImageLayer {
     // This entry contains an image of all pages as of this LSN, should be the same as desc.lsn
     pub lsn: Lsn,
 
+    pub file_size: u64,
+
     access_stats: LayerAccessStats,
 
     inner: RwLock<ImageLayerInner>,
@@ -120,7 +122,7 @@ impl std::fmt::Debug for ImageLayer {
 
         f.debug_struct("ImageLayer")
             .field("key_range", &RangeDisplayDebug(&self.desc.key_range))
-            .field("file_size", &self.desc.file_size)
+            .field("file_size", &self.file_size)
             .field("lsn", &self.lsn)
             .field("inner", &self.inner)
             .finish()
@@ -256,13 +258,17 @@ impl PersistentLayer for ImageLayer {
         Ok(())
     }
 
+    fn file_size(&self) -> u64 {
+        self.file_size
+    }
+
     fn info(&self, reset: LayerAccessStatsReset) -> HistoricLayerInfo {
         let layer_file_name = self.filename().file_name();
         let lsn_range = self.get_lsn_range();
 
         HistoricLayerInfo::Image {
             layer_file_name,
-            layer_file_size: self.desc.file_size,
+            layer_file_size: self.file_size,
             lsn_start: lsn_range.start,
             remote: false,
             access_stats: self.access_stats.as_api_model(reset),
@@ -405,9 +411,9 @@ impl ImageLayer {
                 filename.key_range.clone(),
                 filename.lsn,
                 false,
-                file_size,
             ), // Now we assume image layer ALWAYS covers the full range. This may change in the future.
             lsn: filename.lsn,
+            file_size,
             access_stats,
             inner: RwLock::new(ImageLayerInner {
                 loaded: false,
@@ -437,9 +443,9 @@ impl ImageLayer {
                 summary.key_range,
                 summary.lsn,
                 false,
-                metadata.len(),
             ), // Now we assume image layer ALWAYS covers the full range. This may change in the future.
             lsn: summary.lsn,
+            file_size: metadata.len(),
             access_stats: LayerAccessStats::empty_will_record_residence_event_later(),
             inner: RwLock::new(ImageLayerInner {
                 file: None,
@@ -572,6 +578,14 @@ impl ImageLayerWriterInner {
             file.write_all(buf.as_ref())?;
         }
 
+        let desc = PersistentLayerDesc::new_img(
+            self.tenant_id,
+            self.timeline_id,
+            self.key_range.clone(),
+            self.lsn,
+            self.is_incremental, // for now, image layer ALWAYS covers the full range
+        );
+
         // Fill in the summary on blk 0
         let summary = Summary {
             magic: IMAGE_FILE_MAGIC,
@@ -590,15 +604,6 @@ impl ImageLayerWriterInner {
             .metadata()
             .context("get metadata to determine file size")?;
 
-        let desc = PersistentLayerDesc::new_img(
-            self.tenant_id,
-            self.timeline_id,
-            self.key_range.clone(),
-            self.lsn,
-            self.is_incremental, // for now, image layer ALWAYS covers the full range
-            metadata.len(),
-        );
-
         // Note: Because we open the file in write-only mode, we cannot
         // reuse the same VirtualFile for reading later. That's why we don't
         // set inner.file here. The first read will have to re-open it.
@@ -606,6 +611,7 @@ impl ImageLayerWriterInner {
             path_or_conf: PathOrConf::Conf(self.conf),
             desc,
             lsn: self.lsn,
+            file_size: metadata.len(),
             access_stats: LayerAccessStats::empty_will_record_residence_event_later(),
             inner: RwLock::new(ImageLayerInner {
                 loaded: false,

pageserver/src/tenant/storage_layer/layer_desc.rs

@@ -1,11 +1,10 @@
-use anyhow::Result;
 use std::ops::Range;
 use utils::{
     id::{TenantId, TimelineId},
     lsn::Lsn,
 };
 
-use crate::{context::RequestContext, repository::Key};
+use crate::repository::Key;
 
 use super::{DeltaFileName, ImageFileName, LayerFileName};
 
@@ -25,27 +24,9 @@ pub struct PersistentLayerDesc {
     /// always be equal to `is_delta`. If we land the partial image layer PR someday, image layer could also be
     /// incremental.
     pub is_incremental: bool,
-    /// File size
-    pub file_size: u64,
-}
-
-/// A unique identifier of a persistent layer within the context of one timeline.
-#[derive(Debug, PartialEq, Eq, Clone, Hash)]
-pub struct PersistentLayerKey {
-    pub key_range: Range<Key>,
-    pub lsn_range: Range<Lsn>,
-    pub is_delta: bool,
 }
 
 impl PersistentLayerDesc {
-    pub fn key(&self) -> PersistentLayerKey {
-        PersistentLayerKey {
-            key_range: self.key_range.clone(),
-            lsn_range: self.lsn_range.clone(),
-            is_delta: self.is_delta,
-        }
-    }
-
     pub fn short_id(&self) -> String {
         self.filename().file_name()
     }
@@ -56,7 +37,6 @@ impl PersistentLayerDesc {
         key_range: Range<Key>,
         lsn: Lsn,
         is_incremental: bool,
-        file_size: u64,
     ) -> Self {
         Self {
             tenant_id,
@@ -65,7 +45,6 @@ impl PersistentLayerDesc {
             lsn_range: Self::image_layer_lsn_range(lsn),
             is_delta: false,
             is_incremental,
-            file_size,
         }
     }
 
@@ -74,7 +53,6 @@ impl PersistentLayerDesc {
         timeline_id: TimelineId,
         key_range: Range<Key>,
         lsn_range: Range<Lsn>,
-        file_size: u64,
     ) -> Self {
         Self {
             tenant_id,
@@ -83,7 +61,6 @@ impl PersistentLayerDesc {
             lsn_range,
             is_delta: true,
             is_incremental: true,
-            file_size,
         }
     }
 
@@ -129,48 +106,4 @@ impl PersistentLayerDesc {
             self.image_file_name().into()
         }
     }
-
-    // TODO: remove this in the future once we refactor timeline APIs.
-
-    pub fn get_lsn_range(&self) -> Range<Lsn> {
-        self.lsn_range.clone()
-    }
-
-    pub fn get_key_range(&self) -> Range<Key> {
-        self.key_range.clone()
-    }
-
-    pub fn get_timeline_id(&self) -> TimelineId {
-        self.timeline_id
-    }
-
-    pub fn get_tenant_id(&self) -> TenantId {
-        self.tenant_id
-    }
-
-    pub fn is_incremental(&self) -> bool {
-        self.is_incremental
-    }
-
-    pub fn is_delta(&self) -> bool {
-        self.is_delta
-    }
-
-    pub fn dump(&self, _verbose: bool, _ctx: &RequestContext) -> Result<()> {
-        println!(
-            "----- layer for ten {} tli {} keys {}-{} lsn {}-{} ----",
-            self.tenant_id,
-            self.timeline_id,
-            self.key_range.start,
-            self.key_range.end,
-            self.lsn_range.start,
-            self.lsn_range.end
-        );
-
-        Ok(())
-    }
-
-    pub fn file_size(&self) -> u64 {
-        self.file_size
-    }
 }

pageserver/src/tenant/storage_layer/remote_layer.rs

@@ -142,6 +142,10 @@ impl PersistentLayer for RemoteLayer {
         true
     }
 
+    fn file_size(&self) -> u64 {
+        self.layer_metadata.file_size()
+    }
+
     fn info(&self, reset: LayerAccessStatsReset) -> HistoricLayerInfo {
         let layer_file_name = self.filename().file_name();
         let lsn_range = self.get_lsn_range();
@@ -186,7 +190,6 @@ impl RemoteLayer {
                 fname.key_range.clone(),
                 fname.lsn,
                 false,
-                layer_metadata.file_size(),
             ),
             layer_metadata: layer_metadata.clone(),
             ongoing_download: Arc::new(tokio::sync::Semaphore::new(1)),
@@ -208,7 +211,6 @@ impl RemoteLayer {
                 timelineid,
                 fname.key_range.clone(),
                 fname.lsn_range.clone(),
-                layer_metadata.file_size(),
             ),
             layer_metadata: layer_metadata.clone(),
             ongoing_download: Arc::new(tokio::sync::Semaphore::new(1)),

pageserver/src/tenant/tasks.rs

@@ -14,11 +14,7 @@ use tokio_util::sync::CancellationToken;
 use tracing::*;
 use utils::completion;
 
-/// Start per tenant background loops: compaction and gc.
-pub fn start_background_loops(
-    tenant: &Arc<Tenant>,
-    background_jobs_can_start: Option<&completion::Barrier>,
-) {
+pub fn start_background_loops(tenant: &Arc<Tenant>, init_done: Option<&completion::Barrier>) {
     let tenant_id = tenant.tenant_id;
     task_mgr::spawn(
         BACKGROUND_RUNTIME.handle(),
@@ -29,14 +25,10 @@ pub fn start_background_loops(
         false,
         {
             let tenant = Arc::clone(tenant);
-            let background_jobs_can_start = background_jobs_can_start.cloned();
+            let init_done = init_done.cloned();
             async move {
-                let cancel = task_mgr::shutdown_token();
-                tokio::select! {
-                    _ = cancel.cancelled() => { return Ok(()) },
-                    _ = completion::Barrier::maybe_wait(background_jobs_can_start) => {}
-                };
-                compaction_loop(tenant, cancel)
+                completion::Barrier::maybe_wait(init_done).await;
+                compaction_loop(tenant)
                     .instrument(info_span!("compaction_loop", tenant_id = %tenant_id))
                     .await;
                 Ok(())
@@ -52,14 +44,10 @@ pub fn start_background_loops(
         false,
         {
             let tenant = Arc::clone(tenant);
-            let background_jobs_can_start = background_jobs_can_start.cloned();
+            let init_done = init_done.cloned();
             async move {
-                let cancel = task_mgr::shutdown_token();
-                tokio::select! {
-                    _ = cancel.cancelled() => { return Ok(()) },
-                    _ = completion::Barrier::maybe_wait(background_jobs_can_start) => {}
-                };
-                gc_loop(tenant, cancel)
+                completion::Barrier::maybe_wait(init_done).await;
+                gc_loop(tenant)
                     .instrument(info_span!("gc_loop", tenant_id = %tenant_id))
                     .await;
                 Ok(())
@@ -71,11 +59,12 @@ pub fn start_background_loops(
 ///
 /// Compaction task's main loop
 ///
-async fn compaction_loop(tenant: Arc<Tenant>, cancel: CancellationToken) {
+async fn compaction_loop(tenant: Arc<Tenant>) {
     let wait_duration = Duration::from_secs(2);
     info!("starting");
     TENANT_TASK_EVENTS.with_label_values(&["start"]).inc();
     async {
+        let cancel = task_mgr::shutdown_token();
         let ctx = RequestContext::todo_child(TaskKind::Compaction, DownloadBehavior::Download);
         let mut first = true;
         loop {
@@ -140,11 +129,12 @@ async fn compaction_loop(tenant: Arc<Tenant>, cancel: CancellationToken) {
 ///
 /// GC task's main loop
 ///
-async fn gc_loop(tenant: Arc<Tenant>, cancel: CancellationToken) {
+async fn gc_loop(tenant: Arc<Tenant>) {
     let wait_duration = Duration::from_secs(2);
     info!("starting");
     TENANT_TASK_EVENTS.with_label_values(&["start"]).inc();
     async {
+        let cancel = task_mgr::shutdown_token();
         // GC might require downloading, to find the cutoff LSN that corresponds to the
         // cutoff specified as time.
         let ctx =

pageserver/src/tenant/timeline.rs

@@ -57,7 +57,6 @@ use pageserver_api::reltag::RelTag;
 use postgres_connection::PgConnectionConfig;
 use postgres_ffi::to_pg_timestamp;
 use utils::{
-    completion,
     id::{TenantId, TimelineId},
     lsn::{AtomicLsn, Lsn, RecordLsn},
     seqwait::SeqWait,
@@ -242,13 +241,6 @@ pub struct Timeline {
     pub delete_lock: tokio::sync::Mutex<bool>,
 
     eviction_task_timeline_state: tokio::sync::Mutex<EvictionTaskTimelineState>,
-
-    /// Barrier to wait before doing initial logical size calculation. Used only during startup.
-    initial_logical_size_can_start: Option<completion::Barrier>,
-
-    /// Completion shared between all timelines loaded during startup; used to delay heavier
-    /// background tasks until some logical sizes have been calculated.
-    initial_logical_size_attempt: Mutex<Option<completion::Completion>>,
 }
 
 /// Internal structure to hold all data needed for logical size calculation.
@@ -936,15 +928,10 @@ impl Timeline {
         Ok(())
     }
 
-    pub fn activate(
-        self: &Arc<Self>,
-        broker_client: BrokerClientChannel,
-        background_jobs_can_start: Option<&completion::Barrier>,
-        ctx: &RequestContext,
-    ) {
+    pub fn activate(self: &Arc<Self>, broker_client: BrokerClientChannel, ctx: &RequestContext) {
         self.launch_wal_receiver(ctx, broker_client);
         self.set_state(TimelineState::Active);
-        self.launch_eviction_task(background_jobs_can_start);
+        self.launch_eviction_task();
     }
 
     pub fn set_state(&self, new_state: TimelineState) {
@@ -962,14 +949,6 @@ impl Timeline {
                 error!("Not activating a Stopping timeline");
             }
             (_, new_state) => {
-                if matches!(new_state, TimelineState::Stopping | TimelineState::Broken) {
-                    // drop the copmletion guard, if any; it might be holding off the completion
-                    // forever needlessly
-                    self.initial_logical_size_attempt
-                        .lock()
-                        .unwrap_or_else(|e| e.into_inner())
-                        .take();
-                }
                 self.state.send_replace(new_state);
             }
         }
@@ -1211,12 +1190,7 @@ impl Timeline {
             ),
         });
 
-        let replaced = match batch_updates.replace_historic(
-            local_layer.layer_desc().clone(),
-            local_layer,
-            new_remote_layer.layer_desc().clone(),
-            new_remote_layer,
-        )? {
+        let replaced = match batch_updates.replace_historic(local_layer, new_remote_layer)? {
             Replacement::Replaced { .. } => {
                 if let Err(e) = local_layer.delete_resident_layer_file() {
                     error!("failed to remove layer file on evict after replacement: {e:#?}");
@@ -1365,8 +1339,6 @@ impl Timeline {
         walredo_mgr: Arc<dyn WalRedoManager + Send + Sync>,
         remote_client: Option<RemoteTimelineClient>,
         pg_version: u32,
-        initial_logical_size_can_start: Option<completion::Barrier>,
-        initial_logical_size_attempt: Option<completion::Completion>,
     ) -> Arc<Self> {
         let disk_consistent_lsn = metadata.disk_consistent_lsn();
         let (state, _) = watch::channel(TimelineState::Loading);
@@ -1461,9 +1433,6 @@ impl Timeline {
                     EvictionTaskTimelineState::default(),
                 ),
                 delete_lock: tokio::sync::Mutex::new(false),
-
-                initial_logical_size_can_start,
-                initial_logical_size_attempt: Mutex::new(initial_logical_size_attempt),
             };
             result.repartition_threshold = result.get_checkpoint_distance() / 10;
             result
@@ -1612,7 +1581,7 @@ impl Timeline {
 
                 trace!("found layer {}", layer.path().display());
                 total_physical_size += file_size;
-                updates.insert_historic(layer.layer_desc().clone(), Arc::new(layer));
+                updates.insert_historic(Arc::new(layer));
                 num_layers += 1;
             } else if let Some(deltafilename) = DeltaFileName::parse_str(&fname) {
                 // Create a DeltaLayer struct for each delta file.
@@ -1644,7 +1613,7 @@ impl Timeline {
 
                 trace!("found layer {}", layer.path().display());
                 total_physical_size += file_size;
-                updates.insert_historic(layer.layer_desc().clone(), Arc::new(layer));
+                updates.insert_historic(Arc::new(layer));
                 num_layers += 1;
             } else if fname == METADATA_FILE_NAME || fname.ends_with(".old") {
                 // ignore these
@@ -1743,7 +1712,7 @@ impl Timeline {
                         anyhow::bail!("could not rename file {local_layer_path:?}: {err:?}");
                     } else {
                         self.metrics.resident_physical_size_gauge.sub(local_size);
-                        updates.remove_historic(local_layer.layer_desc().clone(), local_layer);
+                        updates.remove_historic(local_layer);
                         // fall-through to adding the remote layer
                     }
                 } else {
@@ -1782,7 +1751,7 @@ impl Timeline {
                     );
                     let remote_layer = Arc::new(remote_layer);
 
-                    updates.insert_historic(remote_layer.layer_desc().clone(), remote_layer);
+                    updates.insert_historic(remote_layer);
                 }
                 LayerFileName::Delta(deltafilename) => {
                     // Create a RemoteLayer for the delta file.
@@ -1809,7 +1778,7 @@ impl Timeline {
                         ),
                     );
                     let remote_layer = Arc::new(remote_layer);
-                    updates.insert_historic(remote_layer.layer_desc().clone(), remote_layer);
+                    updates.insert_historic(remote_layer);
                 }
             }
         }
@@ -1952,27 +1921,7 @@ impl Timeline {
             false,
             // NB: don't log errors here, task_mgr will do that.
             async move {
-
-                let cancel = task_mgr::shutdown_token();
-
-                // in case we were created during pageserver initialization, wait for
-                // initialization to complete before proceeding. startup time init runs on the same
-                // runtime.
-                tokio::select! {
-                    _ = cancel.cancelled() => { return Ok(()); },
-                    _ = completion::Barrier::maybe_wait(self_clone.initial_logical_size_can_start.clone()) => {}
-                };
-
-                // hold off background tasks from starting until all timelines get to try at least
-                // once initial logical size calculation; though retry will rarely be useful.
-                // holding off is done because heavier tasks execute blockingly on the same
-                // runtime.
-                //
-                // dropping this at every outcome is probably better than trying to cling on to it,
-                // delay will be terminated by a timeout regardless.
-                let _completion = { self_clone.initial_logical_size_attempt.lock().expect("unexpected initial_logical_size_attempt poisoned").take() };
-
-                // no extra cancellation here, because nothing really waits for this to complete compared
+                // no cancellation here, because nothing really waits for this to complete compared
                 // to spawn_ondemand_logical_size_calculation.
                 let cancel = CancellationToken::new();
 
@@ -2257,7 +2206,7 @@ impl Timeline {
         //      won't be needed for page reconstruction for this timeline,
         //      and mark what we can't delete yet as deleted from the layer
         //      map index without actually rebuilding the index.
-        updates.remove_historic(layer.layer_desc().clone(), layer);
+        updates.remove_historic(layer);
 
         Ok(())
     }
@@ -2967,7 +2916,7 @@ impl Timeline {
             LayerResidenceStatus::Resident,
             LayerResidenceEventReason::LayerCreate,
         );
-        batch_updates.insert_historic(l.layer_desc().clone(), l);
+        batch_updates.insert_historic(l);
         batch_updates.flush();
 
         // update the timeline's physical size
@@ -3215,7 +3164,7 @@ impl Timeline {
                 LayerResidenceStatus::Resident,
                 LayerResidenceEventReason::LayerCreate,
             );
-            updates.insert_historic(l.layer_desc().clone(), l);
+            updates.insert_historic(l);
         }
         updates.flush();
         drop(layers);
@@ -3662,7 +3611,7 @@ impl Timeline {
                 LayerResidenceStatus::Resident,
                 LayerResidenceEventReason::LayerCreate,
             );
-            updates.insert_historic(x.layer_desc().clone(), x);
+            updates.insert_historic(x);
         }
 
         // Now that we have reshuffled the data to set of new delta layers, we can
@@ -3794,7 +3743,6 @@ impl Timeline {
         // Is the timeline being deleted?
         let state = *self.state.borrow();
         if state == TimelineState::Stopping {
-            // there's a global allowed_error for this
             anyhow::bail!("timeline is Stopping");
         }
 
@@ -4197,7 +4145,7 @@ impl Timeline {
                     {
                         use crate::tenant::layer_map::Replacement;
                         let l: Arc<dyn PersistentLayer> = remote_layer.clone();
-                        let failure = match updates.replace_historic(l.layer_desc().clone(), &l, new_layer.layer_desc().clone(), new_layer) {
+                        let failure = match updates.replace_historic(&l, new_layer) {
                             Ok(Replacement::Replaced { .. }) => false,
                             Ok(Replacement::NotFound) => {
                                 // TODO: the downloaded file should probably be removed, otherwise

pageserver/src/tenant/timeline/eviction_task.rs

@@ -34,8 +34,6 @@ use crate::{
     },
 };
 
-use utils::completion;
-
 use super::Timeline;
 
 #[derive(Default)]
@@ -49,12 +47,8 @@ pub struct EvictionTaskTenantState {
 }
 
 impl Timeline {
-    pub(super) fn launch_eviction_task(
-        self: &Arc<Self>,
-        background_tasks_can_start: Option<&completion::Barrier>,
-    ) {
+    pub(super) fn launch_eviction_task(self: &Arc<Self>) {
         let self_clone = Arc::clone(self);
-        let background_tasks_can_start = background_tasks_can_start.cloned();
         task_mgr::spawn(
             BACKGROUND_RUNTIME.handle(),
             TaskKind::Eviction,
@@ -63,13 +57,7 @@ impl Timeline {
             &format!("layer eviction for {}/{}", self.tenant_id, self.timeline_id),
             false,
             async move {
-                let cancel = task_mgr::shutdown_token();
-                tokio::select! {
-                    _ = cancel.cancelled() => { return Ok(()); }
-                    _ = completion::Barrier::maybe_wait(background_tasks_can_start) => {}
-                };
-
-                self_clone.eviction_task(cancel).await;
+                self_clone.eviction_task(task_mgr::shutdown_token()).await;
                 info!("eviction task finishing");
                 Ok(())
             },

pageserver/src/tenant/timeline/walreceiver.rs

@@ -25,7 +25,6 @@ mod walreceiver_connection;
 
 use crate::context::{DownloadBehavior, RequestContext};
 use crate::task_mgr::{self, TaskKind, WALRECEIVER_RUNTIME};
-use crate::tenant::debug_assert_current_span_has_tenant_and_timeline_id;
 use crate::tenant::timeline::walreceiver::connection_manager::{
     connection_manager_loop_step, ConnectionManagerState,
 };
@@ -86,8 +85,7 @@ impl WalReceiver {
             &format!("walreceiver for timeline {tenant_id}/{timeline_id}"),
             false,
             async move {
-                debug_assert_current_span_has_tenant_and_timeline_id();
-                debug!("WAL receiver manager started, connecting to broker");
+                info!("WAL receiver manager started, connecting to broker");
                 let mut connection_manager_state = ConnectionManagerState::new(
                     timeline,
                     conf,
@@ -95,7 +93,7 @@ impl WalReceiver {
                 loop {
                     select! {
                         _ = task_mgr::shutdown_watcher() => {
-                            trace!("WAL receiver shutdown requested, shutting down");
+                            info!("WAL receiver shutdown requested, shutting down");
                             break;
                         },
                         loop_step_result = connection_manager_loop_step(
@@ -106,7 +104,7 @@ impl WalReceiver {
                         ) => match loop_step_result {
                             ControlFlow::Continue(()) => continue,
                             ControlFlow::Break(()) => {
-                                trace!("Connection manager loop ended, shutting down");
+                                info!("Connection manager loop ended, shutting down");
                                 break;
                             }
                         },
@@ -117,7 +115,7 @@ impl WalReceiver {
                 *loop_status.write().unwrap() = None;
                 Ok(())
             }
-            .instrument(info_span!(parent: None, "wal_connection_manager", tenant_id = %tenant_id, timeline_id = %timeline_id))
+            .instrument(info_span!(parent: None, "wal_connection_manager", tenant = %tenant_id, timeline = %timeline_id))
         );
 
         Self {
@@ -200,19 +198,29 @@ impl<E: Clone> TaskHandle<E> {
                 TaskEvent::End(match self.join_handle.as_mut() {
                     Some(jh) => {
                         if !jh.is_finished() {
-                            // See: https://github.com/neondatabase/neon/issues/2885
-                            trace!("sender is dropped while join handle is still alive");
+                            // Barring any implementation errors in this module, we can
+                            // only arrive here while the task that executes the future
+                            // passed to `Self::spawn()` is still execution. Cf the comment
+                            // in Self::spawn().
+                            //
+                            // This was logging at warning level in earlier versions, presumably
+                            // to leave some breadcrumbs in case we had an implementation
+                            // error that would would make us get stuck in `jh.await`.
+                            //
+                            // There hasn't been such a bug so far.
+                            // But in a busy system, e.g., during pageserver restart,
+                            // we arrive here often enough that the warning-level logs
+                            // became a distraction.
+                            // So, tone them down to info-level.
+                            //
+                            // XXX: rewrite this module to eliminate the race condition.
+                            info!("sender is dropped while join handle is still alive");
                         }
 
-                        let res = match jh.await {
-                            Ok(res) => res,
-                            Err(je) if je.is_cancelled() => unreachable!("not used"),
-                            Err(je) if je.is_panic() => {
-                                // already logged
-                                Ok(())
-                            }
-                            Err(je) => Err(anyhow::Error::new(je).context("join walreceiver task")),
-                        };
+                        let res = jh
+                            .await
+                            .map_err(|e| anyhow::anyhow!("Failed to join task: {e}"))
+                            .and_then(|x| x);
 
                         // For cancellation-safety, drop join_handle only after successful .await.
                         self.join_handle = None;
@@ -235,12 +243,12 @@ impl<E: Clone> TaskHandle<E> {
             match jh.await {
                 Ok(Ok(())) => debug!("Shutdown success"),
                 Ok(Err(e)) => error!("Shutdown task error: {e:?}"),
-                Err(je) if je.is_cancelled() => unreachable!("not used"),
-                Err(je) if je.is_panic() => {
-                    // already logged
-                }
-                Err(je) => {
-                    error!("Shutdown task join error: {je}")
+                Err(join_error) => {
+                    if join_error.is_cancelled() {
+                        error!("Shutdown task was cancelled");
+                    } else {
+                        error!("Shutdown task join error: {join_error}")
+                    }
                 }
             }
         }

pageserver/src/tenant/timeline/walreceiver/connection_manager.rs

@@ -18,7 +18,7 @@ use crate::metrics::{
     WALRECEIVER_CANDIDATES_REMOVED, WALRECEIVER_SWITCHES,
 };
 use crate::task_mgr::TaskKind;
-use crate::tenant::{debug_assert_current_span_has_tenant_and_timeline_id, Timeline};
+use crate::tenant::Timeline;
 use anyhow::Context;
 use chrono::{NaiveDateTime, Utc};
 use pageserver_api::models::TimelineState;
@@ -55,11 +55,8 @@ pub(super) async fn connection_manager_loop_step(
         .await
     {
         Ok(()) => {}
-        Err(new_state) => {
-            debug!(
-                ?new_state,
-                "state changed, stopping wal connection manager loop"
-            );
+        Err(_) => {
+            info!("Timeline dropped state updates sender before becoming active, stopping wal connection manager loop");
             return ControlFlow::Break(());
         }
     }
@@ -82,7 +79,7 @@ pub(super) async fn connection_manager_loop_step(
     // with other streams on this client (other connection managers). When
     // object goes out of scope, stream finishes in drop() automatically.
     let mut broker_subscription = subscribe_for_timeline_updates(broker_client, id).await;
-    debug!("Subscribed for broker timeline updates");
+    info!("Subscribed for broker timeline updates");
 
     loop {
         let time_until_next_retry = connection_manager_state.time_until_next_retry();
@@ -154,12 +151,12 @@ pub(super) async fn connection_manager_loop_step(
                                 // we're already active as walreceiver, no need to reactivate
                                 TimelineState::Active => continue,
                                 TimelineState::Broken | TimelineState::Stopping => {
-                                    debug!("timeline entered terminal state {new_state:?}, stopping wal connection manager loop");
+                                    info!("timeline entered terminal state {new_state:?}, stopping wal connection manager loop");
                                     return ControlFlow::Break(());
                                 }
                                 TimelineState::Loading => {
                                     warn!("timeline transitioned back to Loading state, that should not happen");
-                                    return ControlFlow::Continue(());
+                                    return ControlFlow::Continue(new_state);
                                 }
                             }
                         }
@@ -167,11 +164,12 @@ pub(super) async fn connection_manager_loop_step(
                     }
                 }
             } => match new_event {
-                ControlFlow::Continue(()) => {
+                ControlFlow::Continue(new_state) => {
+                    info!("observed timeline state change, new state is {new_state:?}");
                     return ControlFlow::Continue(());
                 }
                 ControlFlow::Break(()) => {
-                    debug!("Timeline is no longer active, stopping wal connection manager loop");
+                    info!("Timeline dropped state updates sender, stopping wal connection manager loop");
                     return ControlFlow::Break(());
                 }
             },
@@ -392,6 +390,7 @@ impl ConnectionManagerState {
 
         self.drop_old_connection(true).await;
 
+        let id = self.id;
         let node_id = new_sk.safekeeper_id;
         let connect_timeout = self.conf.wal_connect_timeout;
         let timeline = Arc::clone(&self.timeline);
@@ -399,13 +398,9 @@ impl ConnectionManagerState {
             TaskKind::WalReceiverConnectionHandler,
             DownloadBehavior::Download,
         );
-
-        let span = info_span!("connection", %node_id);
         let connection_handle = TaskHandle::spawn(move |events_sender, cancellation| {
             async move {
-                debug_assert_current_span_has_tenant_and_timeline_id();
-
-                let res = super::walreceiver_connection::handle_walreceiver_connection(
+                super::walreceiver_connection::handle_walreceiver_connection(
                     timeline,
                     new_sk.wal_source_connconf,
                     events_sender,
@@ -414,23 +409,12 @@ impl ConnectionManagerState {
                     ctx,
                     node_id,
                 )
-                .await;
-
-                match res {
-                    Ok(()) => Ok(()),
-                    Err(e) => {
-                        use super::walreceiver_connection::ExpectedError;
-                        if e.is_expected() {
-                            info!("walreceiver connection handling ended: {e:#}");
-                            Ok(())
-                        } else {
-                            // give out an error to have task_mgr give it a really verbose logging
-                            Err(e).context("walreceiver connection handling failure")
-                        }
-                    }
-                }
+                .await
+                .context("walreceiver connection handling failure")
             }
-            .instrument(span)
+            .instrument(
+                info_span!("walreceiver_connection", tenant_id = %id.tenant_id, timeline_id = %id.timeline_id, %node_id),
+            )
         });
 
         let now = Utc::now().naive_utc();

pageserver/src/tenant/timeline/walreceiver/walreceiver_connection.rs

@@ -21,16 +21,16 @@ use postgres_types::PgLsn;
 use tokio::{select, sync::watch, time};
 use tokio_postgres::{replication::ReplicationStream, Client};
 use tokio_util::sync::CancellationToken;
-use tracing::{debug, error, info, trace, warn, Instrument};
+use tracing::{debug, error, info, trace, warn};
 
 use super::TaskStateUpdate;
+use crate::metrics::LIVE_CONNECTIONS_COUNT;
+use crate::{context::RequestContext, metrics::WALRECEIVER_STARTED_CONNECTIONS};
 use crate::{
-    context::RequestContext,
-    metrics::{LIVE_CONNECTIONS_COUNT, WALRECEIVER_STARTED_CONNECTIONS},
     task_mgr,
     task_mgr::TaskKind,
     task_mgr::WALRECEIVER_RUNTIME,
-    tenant::{debug_assert_current_span_has_tenant_and_timeline_id, Timeline, WalReceiverInfo},
+    tenant::{Timeline, WalReceiverInfo},
     walingest::WalIngest,
     walrecord::DecodedWALRecord,
 };
@@ -81,8 +81,13 @@ pub(super) async fn handle_walreceiver_connection(
         config.application_name("pageserver");
         config.replication_mode(tokio_postgres::config::ReplicationMode::Physical);
         match time::timeout(connect_timeout, config.connect(postgres::NoTls)).await {
-            Ok(client_and_conn) => client_and_conn?,
-            Err(_elapsed) => {
+            Ok(Ok(client_and_conn)) => client_and_conn,
+            Ok(Err(conn_err)) => {
+                let expected_error = ignore_expected_errors(conn_err)?;
+                info!("DB connection stream finished: {expected_error}");
+                return Ok(());
+            }
+            Err(_) => {
                 // Timing out to connect to a safekeeper node could happen long time, due to
                 // many reasons that pageserver cannot control.
                 // Do not produce an error, but make it visible, that timeouts happen by logging the `event.
@@ -92,7 +97,7 @@ pub(super) async fn handle_walreceiver_connection(
         }
     };
 
-    debug!("connected!");
+    info!("connected!");
     let mut connection_status = WalConnectionStatus {
         is_connected: true,
         has_processed_wal: false,
@@ -122,25 +127,20 @@ pub(super) async fn handle_walreceiver_connection(
         "walreceiver connection",
         false,
         async move {
-            debug_assert_current_span_has_tenant_and_timeline_id();
-
             select! {
                 connection_result = connection => match connection_result {
-                    Ok(()) => debug!("Walreceiver db connection closed"),
+                    Ok(()) => info!("Walreceiver db connection closed"),
                     Err(connection_error) => {
-                        if connection_error.is_expected() {
-                            // silence, because most likely we've already exited the outer call
-                            // with a similar error.
-                        } else {
-                            warn!("Connection aborted: {connection_error:#}")
+                        if let Err(e) = ignore_expected_errors(connection_error) {
+                            warn!("Connection aborted: {e:#}")
                         }
                     }
                 },
-                _ = connection_cancellation.cancelled() => debug!("Connection cancelled"),
+                // Future: replace connection_cancellation with connection_ctx cancellation
+                _ = connection_cancellation.cancelled() => info!("Connection cancelled"),
             }
             Ok(())
-        }
-        .instrument(tracing::info_span!("poller")),
+        },
     );
 
     // Immediately increment the gauge, then create a job to decrement it on task exit.
@@ -203,13 +203,20 @@ pub(super) async fn handle_walreceiver_connection(
     while let Some(replication_message) = {
         select! {
             _ = cancellation.cancelled() => {
-                debug!("walreceiver interrupted");
+                info!("walreceiver interrupted");
                 None
             }
             replication_message = physical_stream.next() => replication_message,
         }
     } {
-        let replication_message = replication_message?;
+        let replication_message = match replication_message {
+            Ok(message) => message,
+            Err(replication_error) => {
+                let expected_error = ignore_expected_errors(replication_error)?;
+                info!("Replication stream finished: {expected_error}");
+                return Ok(());
+            }
+        };
 
         let now = Utc::now().naive_utc();
         let last_rec_lsn_before_msg = last_rec_lsn;
@@ -254,6 +261,8 @@ pub(super) async fn handle_walreceiver_connection(
                     let mut decoded = DecodedWALRecord::default();
                     let mut modification = timeline.begin_modification(endlsn);
                     while let Some((lsn, recdata)) = waldecoder.poll_decode()? {
+                        // let _enter = info_span!("processing record", lsn = %lsn).entered();
+
                         // It is important to deal with the aligned records as lsn in getPage@LSN is
                         // aligned and can be several bytes bigger. Without this alignment we are
                         // at risk of hitting a deadlock.
@@ -412,50 +421,31 @@ async fn identify_system(client: &mut Client) -> anyhow::Result<IdentifySystem>
     }
 }
 
-/// Trait for avoid reporting walreceiver specific expected or "normal" or "ok" errors.
-pub(super) trait ExpectedError {
-    /// Test if this error is an ok error.
-    ///
-    /// We don't want to report connectivity problems as real errors towards connection manager because
-    /// 1. they happen frequently enough to make server logs hard to read and
-    /// 2. the connection manager can retry other safekeeper.
-    ///
-    /// If this function returns `true`, it's such an error.
-    /// The caller should log it at info level and then report to connection manager that we're done handling this connection.
-    /// Connection manager will then handle reconnections.
-    ///
-    /// If this function returns an `false` the error should be propagated and the connection manager
-    /// will log the error at ERROR level.
-    fn is_expected(&self) -> bool;
-}
-
-impl ExpectedError for postgres::Error {
-    fn is_expected(&self) -> bool {
-        self.is_closed()
-            || self
-                .source()
-                .and_then(|source| source.downcast_ref::<std::io::Error>())
-                .map(is_expected_io_error)
-                .unwrap_or(false)
-            || self
-                .as_db_error()
-                .filter(|db_error| {
-                    db_error.code() == &SqlState::SUCCESSFUL_COMPLETION
-                        && db_error.message().contains("ending streaming")
-                })
-                .is_some()
-    }
-}
-
-impl ExpectedError for anyhow::Error {
-    fn is_expected(&self) -> bool {
-        let head = self.downcast_ref::<postgres::Error>();
-
-        let tail = self
-            .chain()
-            .filter_map(|e| e.downcast_ref::<postgres::Error>());
-
-        // check if self or any of the chained/sourced errors are expected
-        head.into_iter().chain(tail).any(|e| e.is_expected())
+/// We don't want to report connectivity problems as real errors towards connection manager because
+/// 1. they happen frequently enough to make server logs hard to read and
+/// 2. the connection manager can retry other safekeeper.
+///
+/// If this function returns `Ok(pg_error)`, it's such an error.
+/// The caller should log it at info level and then report to connection manager that we're done handling this connection.
+/// Connection manager will then handle reconnections.
+///
+/// If this function returns an `Err()`, the caller can bubble it up using `?`.
+/// The connection manager will log the error at ERROR level.
+fn ignore_expected_errors(pg_error: postgres::Error) -> anyhow::Result<postgres::Error> {
+    if pg_error.is_closed()
+        || pg_error
+            .source()
+            .and_then(|source| source.downcast_ref::<std::io::Error>())
+            .map(is_expected_io_error)
+            .unwrap_or(false)
+    {
+        return Ok(pg_error);
+    } else if let Some(db_error) = pg_error.as_db_error() {
+        if db_error.code() == &SqlState::SUCCESSFUL_COMPLETION
+            && db_error.message().contains("ending streaming")
+        {
+            return Ok(pg_error);
+        }
     }
+    Err(pg_error).context("connection error")
 }

pgxn/hnsw/Makefile

@@ -1,26 +0,0 @@
-EXTENSION = hnsw
-EXTVERSION = 0.1.0
-
-MODULE_big = hnsw
-DATA = $(wildcard *--*.sql)
-OBJS = hnsw.o hnswalg.o
-
-TESTS = $(wildcard test/sql/*.sql)
-REGRESS = $(patsubst test/sql/%.sql,%,$(TESTS))
-REGRESS_OPTS = --inputdir=test --load-extension=hnsw
-
-# For auto-vectorization:
-# - GCC (needs -ftree-vectorize OR -O3) - https://gcc.gnu.org/projects/tree-ssa/vectorization.html
-PG_CFLAGS += -O3
-PG_CXXFLAGS +=  -O3 -std=c++11
-PG_LDFLAGS += -lstdc++
-
-all: $(EXTENSION)--$(EXTVERSION).sql
-
-PG_CONFIG ?= pg_config
-PGXS := $(shell $(PG_CONFIG) --pgxs)
-include $(PGXS)
-
-dist:
-	mkdir -p dist
-	git archive --format zip --prefix=$(EXTENSION)-$(EXTVERSION)/ --output dist/$(EXTENSION)-$(EXTVERSION).zip master

pgxn/hnsw/README.md

@@ -1,25 +0,0 @@
-# Revisiting the Inverted Indices for Billion-Scale Approximate Nearest Neighbors
-
-This ANN extension of Postgres is based
-on [ivf-hnsw](https://github.com/dbaranchuk/ivf-hnsw.git) implementation of [HNSW](https://www.pinecone.io/learn/hnsw),
-the code for the current state-of-the-art billion-scale nearest neighbor search system presented in the paper:
-
-[Revisiting the Inverted Indices for Billion-Scale Approximate Nearest Neighbors](http://openaccess.thecvf.com/content_ECCV_2018/html/Dmitry_Baranchuk_Revisiting_the_Inverted_ECCV_2018_paper.html),
-<br>
-Dmitry Baranchuk, Artem Babenko, Yury Malkov
-
-# Postgres extension
-
-HNSW index is hold in memory (built on demand) and it's maxial size is limited
-by `maxelements` index parameter. Another required parameter is nubmer of dimensions (if it is not specified in column type).
-Optional parameter `ef` specifies number of neighbors which are considered during index construction and search (corresponds `efConstruction` and `efSearch` parameters
-described in the article).
-
-# Example of usage:
-
-```
-create extension hnsw;
-create table embeddings(id integer primary key, payload real[]);
-create index on embeddings using hnsw(payload) with (maxelements=1000000, dims=100, m=32);
-select id from embeddings order by payload <-> array[1.0, 2.0,...] limit 100;
-```

pgxn/hnsw/hnsw--0.1.0.sql

@@ -1,29 +0,0 @@
--- complain if script is sourced in psql, rather than via CREATE EXTENSION
-\echo Use "CREATE EXTENSION hnsw" to load this file. \quit
-
--- functions
-
-CREATE FUNCTION l2_distance(real[], real[]) RETURNS real
-	AS 'MODULE_PATHNAME' LANGUAGE C IMMUTABLE STRICT PARALLEL SAFE;
-
--- operators
-
-CREATE OPERATOR <-> (
-	LEFTARG = real[], RIGHTARG = real[], PROCEDURE = l2_distance,
-	COMMUTATOR = '<->'
-);
-
--- access method
-
-CREATE FUNCTION hnsw_handler(internal) RETURNS index_am_handler
-	AS 'MODULE_PATHNAME' LANGUAGE C;
-
-CREATE ACCESS METHOD hnsw TYPE INDEX HANDLER hnsw_handler;
-
-COMMENT ON ACCESS METHOD hnsw IS 'hnsw index access method';
-
--- opclasses
-
-CREATE OPERATOR CLASS knn_ops
-	DEFAULT FOR TYPE real[] USING hnsw AS
-	OPERATOR 1 <-> (real[], real[]) FOR ORDER BY float_ops;

pgxn/hnsw/hnsw.c

@@ -1,551 +0,0 @@
-#include "postgres.h"
-
-#include "access/amapi.h"
-#include "access/generic_xlog.h"
-#include "access/relation.h"
-#include "access/reloptions.h"
-#include "access/tableam.h"
-#include "catalog/index.h"
-#include "commands/vacuum.h"
-#include "nodes/execnodes.h"
-#include "storage/bufmgr.h"
-#include "utils/guc.h"
-#include "utils/selfuncs.h"
-
-#include <math.h>
-#include <float.h>
-
-#include "hnsw.h"
-
-PG_MODULE_MAGIC;
-
-typedef struct {
-	int32 vl_len_;		/* varlena header (do not touch directly!) */
-	int dims;
-	int maxelements;
-	int efConstruction;
-	int efSearch;
-	int M;
-} HnswOptions;
-
-static relopt_kind hnsw_relopt_kind;
-
-typedef struct {
-	HierarchicalNSW* hnsw;
-	size_t curr;
-	size_t n_results;
-	ItemPointer results;
-} HnswScanOpaqueData;
-
-typedef HnswScanOpaqueData* HnswScanOpaque;
-
-typedef struct {
-	Oid relid;
-	uint32 status;
-	HierarchicalNSW* hnsw;
-} HnswHashEntry;
-
-
-#define SH_PREFIX			 hnsw_index
-#define SH_ELEMENT_TYPE		 HnswHashEntry
-#define SH_KEY_TYPE			 Oid
-#define SH_KEY				 relid
-#define SH_STORE_HASH
-#define SH_GET_HASH(tb, a)	 ((a)->relid)
-#define SH_HASH_KEY(tb, key) (key)
-#define SH_EQUAL(tb, a, b)	((a) == (b))
-#define SH_SCOPE			static inline
-#define SH_DEFINE
-#define SH_DECLARE
-#include "lib/simplehash.h"
-
-#define INDEX_HASH_SIZE     11
-
-#define DEFAULT_EF_SEARCH   64
-
-PGDLLEXPORT void _PG_init(void);
-
-static hnsw_index_hash *hnsw_indexes;
-
-/*
- * Initialize index options and variables
- */
-void
-_PG_init(void)
-{
-	hnsw_relopt_kind = add_reloption_kind();
-	add_int_reloption(hnsw_relopt_kind, "dims", "Number of dimensions",
-					  0, 0, INT_MAX, AccessExclusiveLock);
-	add_int_reloption(hnsw_relopt_kind, "maxelements", "Maximal number of elements",
-					  0, 0, INT_MAX, AccessExclusiveLock);
-	add_int_reloption(hnsw_relopt_kind, "m", "Number of neighbors of each vertex",
-					  100, 0, INT_MAX, AccessExclusiveLock);
-	add_int_reloption(hnsw_relopt_kind, "efconstruction", "Number of inspected neighbors during index construction",
-					  16, 1, INT_MAX, AccessExclusiveLock);
-	add_int_reloption(hnsw_relopt_kind, "efsearch", "Number of inspected neighbors during index search",
-					  64, 1, INT_MAX, AccessExclusiveLock);
-	hnsw_indexes = hnsw_index_create(TopMemoryContext, INDEX_HASH_SIZE, NULL);
-}
-
-
-static void
-hnsw_build_callback(Relation index, ItemPointer tid, Datum *values,
-					bool *isnull, bool tupleIsAlive, void *state)
-{
-	HierarchicalNSW* hnsw = (HierarchicalNSW*) state;
-	ArrayType* array;
-	int n_items;
-	label_t label = 0;
-
-	/* Skip nulls */
-	if (isnull[0])
-		return;
-
-	array = DatumGetArrayTypeP(values[0]);
-	n_items = ArrayGetNItems(ARR_NDIM(array), ARR_DIMS(array));
-	if (n_items != hnsw_dimensions(hnsw))
-	{
-		elog(ERROR, "Wrong number of dimensions: %d instead of %d expected",
-			 n_items, hnsw_dimensions(hnsw));
-	}
-
-	memcpy(&label, tid, sizeof(*tid));
-	hnsw_add_point(hnsw, (coord_t*)ARR_DATA_PTR(array), label);
-}
-
-static void
-hnsw_populate(HierarchicalNSW* hnsw, Relation indexRel, Relation heapRel)
-{
-	IndexInfo* indexInfo = BuildIndexInfo(indexRel);
-	Assert(indexInfo->ii_NumIndexAttrs == 1);
-	table_index_build_scan(heapRel, indexRel, indexInfo,
-						   true, true, hnsw_build_callback, (void *) hnsw, NULL);
-}
-
-static HierarchicalNSW*
-hnsw_get_index(Relation indexRel, Relation heapRel)
-{
-	HierarchicalNSW* hnsw;
-	Oid indexoid = RelationGetRelid(indexRel);
-	HnswHashEntry* entry = hnsw_index_lookup(hnsw_indexes, indexoid);
-	if (entry == NULL)
-	{
-		size_t dims, maxelements;
-		size_t M;
-		size_t maxM;
-		size_t size_links_level0;
-		size_t size_data_per_element;
-		size_t data_size;
-		dsm_handle handle = indexoid << 1; /* make it even */
-		void* impl_private = NULL;
-		void* mapped_address = NULL;
-		Size  mapped_size = 0;
-		Size  shmem_size;
-		bool exists = true;
-		bool found;
-		HnswOptions *opts = (HnswOptions *) indexRel->rd_options;
-		if (opts == NULL || opts->maxelements == 0 || opts->dims == 0) {
-			elog(ERROR, "HNSW index requires 'maxelements' and 'dims' to be specified");
-		}
-		dims = opts->dims;
-		maxelements = opts->maxelements;
-		M = opts->M;
-		maxM = M * 2;
-		data_size = dims * sizeof(coord_t);
-		size_links_level0 = (maxM + 1) * sizeof(idx_t);
-		size_data_per_element = size_links_level0 + data_size + sizeof(label_t);
-		shmem_size =  hnsw_sizeof() + maxelements * size_data_per_element;
-
-		/* first try to attach to existed index */
-		if (!dsm_impl_op(DSM_OP_ATTACH, handle, 0, &impl_private,
-						 &mapped_address, &mapped_size, DEBUG1))
-		{
-			/* index doesn't exists: try to create it */
-			if (!dsm_impl_op(DSM_OP_CREATE, handle, shmem_size, &impl_private,
-							 &mapped_address, &mapped_size, DEBUG1))
-			{
-				/* We can do it under shared lock, so some other backend may
-				 * try to initialize index. If create is failed because index already
-				 * created by somebody else, then try to attach to it once again
-				 */
-				if (!dsm_impl_op(DSM_OP_ATTACH, handle, 0, &impl_private,
-								 &mapped_address, &mapped_size, ERROR))
-				{
-					return NULL;
-				}
-			}
-			else
-			{
-				exists = false;
-			}
-		}
-		Assert(mapped_size == shmem_size);
-		hnsw = (HierarchicalNSW*)mapped_address;
-
-		if (!exists)
-		{
-			hnsw_init(hnsw, dims, maxelements, M, maxM, opts->efConstruction);
-			hnsw_populate(hnsw, indexRel, heapRel);
-		}
-		entry = hnsw_index_insert(hnsw_indexes, indexoid, &found);
-		Assert(!found);
-		entry->hnsw = hnsw;
-	}
-	else
-	{
-		hnsw = entry->hnsw;
-	}
-	return hnsw;
-}
-
-/*
- * Start or restart an index scan
- */
-static IndexScanDesc
-hnsw_beginscan(Relation index, int nkeys, int norderbys)
-{
-	IndexScanDesc scan = RelationGetIndexScan(index, nkeys, norderbys);
-	HnswScanOpaque so = (HnswScanOpaque) palloc(sizeof(HnswScanOpaqueData));
-	Relation heap = relation_open(index->rd_index->indrelid, NoLock);
-	so->hnsw = hnsw_get_index(index, heap);
-	relation_close(heap, NoLock);
-	so->curr = 0;
-	so->n_results = 0;
-	so->results = NULL;
-	scan->opaque = so;
-	return scan;
-}
-
-/*
- * Start or restart an index scan
- */
-static void
-hnsw_rescan(IndexScanDesc scan, ScanKey keys, int nkeys, ScanKey orderbys, int norderbys)
-{
-	HnswScanOpaque so = (HnswScanOpaque) scan->opaque;
-	if (so->results)
-	{
-		pfree(so->results);
-		so->results = NULL;
-	}
-	so->curr = 0;
-	if (orderbys && scan->numberOfOrderBys > 0)
-		memmove(scan->orderByData, orderbys, scan->numberOfOrderBys * sizeof(ScanKeyData));
-}
-
-/*
- * Fetch the next tuple in the given scan
- */
-static bool
-hnsw_gettuple(IndexScanDesc scan, ScanDirection dir)
-{
-	HnswScanOpaque so = (HnswScanOpaque) scan->opaque;
-
-	/*
-	 * Index can be used to scan backward, but Postgres doesn't support
-	 * backward scan on operators
-	 */
-	Assert(ScanDirectionIsForward(dir));
-
-	if (so->curr == 0)
-	{
-		Datum		value;
-		ArrayType*	array;
-		int         n_items;
-		size_t      n_results;
-		label_t*    results;
-		HnswOptions *opts = (HnswOptions *) scan->indexRelation->rd_options;
-		size_t      efSearch = opts ? opts->efSearch : DEFAULT_EF_SEARCH;
-
-		/* Safety check */
-		if (scan->orderByData == NULL)
-			elog(ERROR, "cannot scan HNSW index without order");
-
-		/* No items will match if null */
-		if (scan->orderByData->sk_flags & SK_ISNULL)
-			return false;
-
-		value = scan->orderByData->sk_argument;
-		array = DatumGetArrayTypeP(value);
-		n_items = ArrayGetNItems(ARR_NDIM(array), ARR_DIMS(array));
-		if (n_items != hnsw_dimensions(so->hnsw))
-		{
-			elog(ERROR, "Wrong number of dimensions: %d instead of %d expected",
-				 n_items, hnsw_dimensions(so->hnsw));
-		}
-
-		if (!hnsw_search(so->hnsw, (coord_t*)ARR_DATA_PTR(array), efSearch, &n_results, &results))
-			elog(ERROR, "HNSW index search failed");
-		so->results = (ItemPointer)palloc(n_results*sizeof(ItemPointerData));
-		so->n_results = n_results;
-		for (size_t i = 0; i < n_results; i++)
-		{
-			memcpy(&so->results[i], &results[i], sizeof(so->results[i]));
-		}
-		free(results);
-	}
-	if (so->curr >= so->n_results)
-	{
-		return false;
-	}
-	else
-	{
-		scan->xs_heaptid = so->results[so->curr++];
-		scan->xs_recheckorderby = false;
-		return true;
-	}
-}
-
-/*
- * End a scan and release resources
- */
-static void
-hnsw_endscan(IndexScanDesc scan)
-{
-	HnswScanOpaque so = (HnswScanOpaque) scan->opaque;
-	if (so->results)
-		pfree(so->results);
-	pfree(so);
-	scan->opaque = NULL;
-}
-
-
-/*
- * Estimate the cost of an index scan
- */
-static void
-hnsw_costestimate(PlannerInfo *root, IndexPath *path, double loop_count,
-				 Cost *indexStartupCost, Cost *indexTotalCost,
-				 Selectivity *indexSelectivity, double *indexCorrelation
-				 ,double *indexPages
-)
-{
-	GenericCosts costs;
-
-	/* Never use index without order */
-	if (path->indexorderbys == NULL)
-	{
-		*indexStartupCost = DBL_MAX;
-		*indexTotalCost = DBL_MAX;
-		*indexSelectivity = 0;
-		*indexCorrelation = 0;
-		*indexPages = 0;
-		return;
-	}
-
-	MemSet(&costs, 0, sizeof(costs));
-
-	genericcostestimate(root, path, loop_count, &costs);
-
-	/* Startup cost and total cost are same */
-	*indexStartupCost = costs.indexTotalCost;
-	*indexTotalCost = costs.indexTotalCost;
-	*indexSelectivity = costs.indexSelectivity;
-	*indexCorrelation = costs.indexCorrelation;
-	*indexPages = costs.numIndexPages;
-}
-
-/*
- * Parse and validate the reloptions
- */
-static bytea *
-hnsw_options(Datum reloptions, bool validate)
-{
-	static const relopt_parse_elt tab[] = {
-		{"dims", RELOPT_TYPE_INT, offsetof(HnswOptions, dims)},
-		{"maxelements", RELOPT_TYPE_INT, offsetof(HnswOptions, maxelements)},
-		{"efconstruction", RELOPT_TYPE_INT, offsetof(HnswOptions, efConstruction)},
-		{"efsearch", RELOPT_TYPE_INT, offsetof(HnswOptions, efSearch)},
-		{"m", RELOPT_TYPE_INT, offsetof(HnswOptions, M)}
-	};
-
-	return (bytea *) build_reloptions(reloptions, validate,
-									  hnsw_relopt_kind,
-									  sizeof(HnswOptions),
-									  tab, lengthof(tab));
-}
-
-/*
- * Validate catalog entries for the specified operator class
- */
-static bool
-hnsw_validate(Oid opclassoid)
-{
-	return true;
-}
-
-/*
- * Build the index for a logged table
- */
-static IndexBuildResult *
-hnsw_build(Relation heap, Relation index, IndexInfo *indexInfo)
-{
-	HierarchicalNSW* hnsw = hnsw_get_index(index, heap);
-	IndexBuildResult* result = (IndexBuildResult *) palloc(sizeof(IndexBuildResult));
-	result->heap_tuples = result->index_tuples = hnsw_count(hnsw);
-
-	return result;
-}
-
-/*
- * Insert a tuple into the index
- */
-static bool
-hnsw_insert(Relation index, Datum *values, bool *isnull, ItemPointer heap_tid,
-			  Relation heap, IndexUniqueCheck checkUnique,
-			  bool indexUnchanged,
-			  IndexInfo *indexInfo)
-{
-	HierarchicalNSW* hnsw = hnsw_get_index(index, heap);
-	Datum value;
-	ArrayType* array;
-	int n_items;
-	label_t label = 0;
-
-	/* Skip nulls */
-	if (isnull[0])
-		return false;
-
-	/* Detoast value */
-	value = PointerGetDatum(PG_DETOAST_DATUM(values[0]));
-	array = DatumGetArrayTypeP(value);
-	n_items = ArrayGetNItems(ARR_NDIM(array), ARR_DIMS(array));
-	if (n_items != hnsw_dimensions(hnsw))
-	{
-		elog(ERROR, "Wrong number of dimensions: %d instead of %d expected",
-			 n_items, hnsw_dimensions(hnsw));
-	}
-	memcpy(&label, heap_tid, sizeof(*heap_tid));
-	if (!hnsw_add_point(hnsw, (coord_t*)ARR_DATA_PTR(array), label))
-		elog(ERROR, "HNSW index insert failed");
-	return true;
-}
-
-/*
- * Build the index for an unlogged table
- */
-static void
-hnsw_buildempty(Relation index)
-{
-	/* index will be constructed on dema nd when accessed */
-}
-
-/*
- * Clean up after a VACUUM operation
- */
-static IndexBulkDeleteResult *
-hnsw_vacuumcleanup(IndexVacuumInfo *info, IndexBulkDeleteResult *stats)
-{
-	Relation	rel = info->index;
-
-	if (stats == NULL)
-		return NULL;
-
-	stats->num_pages = RelationGetNumberOfBlocks(rel);
-
-	return stats;
-}
-
-/*
- * Bulk delete tuples from the index
- */
-static IndexBulkDeleteResult *
-hnsw_bulkdelete(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
-				IndexBulkDeleteCallback callback, void *callback_state)
-{
-	if (stats == NULL)
-		stats = (IndexBulkDeleteResult *) palloc0(sizeof(IndexBulkDeleteResult));
-	return stats;
-}
-
-/*
- * Define index handler
- *
- * See https://www.postgresql.org/docs/current/index-api.html
- */
-PGDLLEXPORT PG_FUNCTION_INFO_V1(hnsw_handler);
-Datum
-hnsw_handler(PG_FUNCTION_ARGS)
-{
-	IndexAmRoutine *amroutine = makeNode(IndexAmRoutine);
-
-	amroutine->amstrategies = 0;
-	amroutine->amsupport = 0;
-	amroutine->amoptsprocnum = 0;
-	amroutine->amcanorder = false;
-	amroutine->amcanorderbyop = true;
-	amroutine->amcanbackward = false;	/* can change direction mid-scan */
-	amroutine->amcanunique = false;
-	amroutine->amcanmulticol = false;
-	amroutine->amoptionalkey = true;
-	amroutine->amsearcharray = false;
-	amroutine->amsearchnulls = false;
-	amroutine->amstorage = false;
-	amroutine->amclusterable = false;
-	amroutine->ampredlocks = false;
-	amroutine->amcanparallel = false;
-	amroutine->amcaninclude = false;
-	amroutine->amusemaintenanceworkmem = false; /* not used during VACUUM */
-	amroutine->amparallelvacuumoptions = VACUUM_OPTION_PARALLEL_BULKDEL;
-	amroutine->amkeytype = InvalidOid;
-
-	/* Interface functions */
-	amroutine->ambuild = hnsw_build;
-	amroutine->ambuildempty = hnsw_buildempty;
-	amroutine->aminsert = hnsw_insert;
-	amroutine->ambulkdelete = hnsw_bulkdelete;
-	amroutine->amvacuumcleanup = hnsw_vacuumcleanup;
-	amroutine->amcanreturn = NULL;	/* tuple not included in heapsort */
-	amroutine->amcostestimate = hnsw_costestimate;
-	amroutine->amoptions = hnsw_options;
-	amroutine->amproperty = NULL;	/* TODO AMPROP_DISTANCE_ORDERABLE */
-	amroutine->ambuildphasename = NULL;
-	amroutine->amvalidate = hnsw_validate;
-	amroutine->amadjustmembers = NULL;
-	amroutine->ambeginscan = hnsw_beginscan;
-	amroutine->amrescan = hnsw_rescan;
-	amroutine->amgettuple = hnsw_gettuple;
-	amroutine->amgetbitmap = NULL;
-	amroutine->amendscan = hnsw_endscan;
-	amroutine->ammarkpos = NULL;
-	amroutine->amrestrpos = NULL;
-
-	/* Interface functions to support parallel index scans */
-	amroutine->amestimateparallelscan = NULL;
-	amroutine->aminitparallelscan = NULL;
-	amroutine->amparallelrescan = NULL;
-
-	PG_RETURN_POINTER(amroutine);
-}
-
-/*
- * Get the L2 distance between vectors
- */
-PGDLLEXPORT PG_FUNCTION_INFO_V1(l2_distance);
-Datum
-l2_distance(PG_FUNCTION_ARGS)
-{
-	ArrayType  *a = PG_GETARG_ARRAYTYPE_P(0);
-	ArrayType  *b = PG_GETARG_ARRAYTYPE_P(1);
-	int         a_dim = ArrayGetNItems(ARR_NDIM(a), ARR_DIMS(a));
-	int         b_dim = ArrayGetNItems(ARR_NDIM(b), ARR_DIMS(b));
-	dist_t 		distance = 0.0;
-	dist_t		diff;
-	coord_t	   *ax = (coord_t*)ARR_DATA_PTR(a);
-	coord_t	   *bx = (coord_t*)ARR_DATA_PTR(b);
-
-	if (a_dim != b_dim)
-	{
-		ereport(ERROR,
-				(errcode(ERRCODE_DATA_EXCEPTION),
-				 errmsg("different array dimensions %d and %d", a_dim, b_dim)));
-	}
-
-	for (int i = 0; i < a_dim; i++)
-	{
-		diff = ax[i] - bx[i];
-		distance += diff * diff;
-	}
-
-	PG_RETURN_FLOAT4((dist_t)sqrt(distance));
-}

pgxn/hnsw/hnsw.control

@@ -1,5 +0,0 @@
-comment = 'hNsw index'
-default_version = '0.1.0'
-module_pathname = '$libdir/hnsw'
-relocatable = true
-trusted = true

pgxn/hnsw/hnsw.h

@@ -1,15 +0,0 @@
-#pragma once
-
-typedef float    coord_t;
-typedef float    dist_t;
-typedef uint32_t idx_t;
-typedef uint64_t label_t;
-
-typedef struct HierarchicalNSW HierarchicalNSW;
-
-bool hnsw_search(HierarchicalNSW* hnsw, const coord_t *point, size_t efSearch, size_t* n_results, label_t** results);
-bool hnsw_add_point(HierarchicalNSW* hnsw, const coord_t *point, label_t label);
-void hnsw_init(HierarchicalNSW* hnsw, size_t dim, size_t maxelements, size_t M, size_t maxM, size_t efConstruction);
-int  hnsw_dimensions(HierarchicalNSW* hnsw);
-size_t hnsw_count(HierarchicalNSW* hnsw);
-size_t hnsw_sizeof(void);

pgxn/hnsw/hnswalg.cpp

@@ -1,379 +0,0 @@
-#include "hnswalg.h"
-
-#if defined(__GNUC__)
-#define PORTABLE_ALIGN32 __attribute__((aligned(32)))
-#define PREFETCH(addr,hint) __builtin_prefetch(addr, 0, hint)
-#else
-#define PORTABLE_ALIGN32 __declspec(align(32))
-#define PREFETCH(addr,hint)
-#endif
-
-HierarchicalNSW::HierarchicalNSW(size_t dim_, size_t maxelements_, size_t M_, size_t maxM_, size_t efConstruction_)
-{
-    dim = dim_;
-    data_size = dim * sizeof(coord_t);
-
-    efConstruction = efConstruction_;
-
-    maxelements = maxelements_;
-    M = M_;
-    maxM = maxM_;
-    size_links_level0 = (maxM + 1) * sizeof(idx_t);
-    size_data_per_element = size_links_level0 + data_size  + sizeof(label_t);
-    offset_data = size_links_level0;
-	offset_label = offset_data + data_size;
-
-    enterpoint_node = 0;
-    cur_element_count = 0;
-#ifdef __x86_64__
-    use_avx2 = __builtin_cpu_supports("avx2");
-#endif
-}
-
-std::priority_queue<std::pair<dist_t, idx_t>> HierarchicalNSW::searchBaseLayer(const coord_t *point, size_t ef)
-{
-	std::vector<uint32_t> visited;
-	visited.resize((cur_element_count + 31) >> 5);
-
-    std::priority_queue<std::pair<dist_t, idx_t >> topResults;
-    std::priority_queue<std::pair<dist_t, idx_t >> candidateSet;
-
-    dist_t dist = fstdistfunc(point, getDataByInternalId(enterpoint_node));
-
-    topResults.emplace(dist, enterpoint_node);
-    candidateSet.emplace(-dist, enterpoint_node);
-    visited[enterpoint_node >> 5] = 1 << (enterpoint_node & 31);
-    dist_t lowerBound = dist;
-
-    while (!candidateSet.empty())
-    {
-        std::pair<dist_t, idx_t> curr_el_pair = candidateSet.top();
-        if (-curr_el_pair.first > lowerBound)
-            break;
-
-        candidateSet.pop();
-        idx_t curNodeNum = curr_el_pair.second;
-
-        idx_t* data = get_linklist0(curNodeNum);
-        size_t size = *data++;
-
-        PREFETCH(getDataByInternalId(*data), 0);
-
-        for (size_t j = 0; j < size; ++j) {
-            size_t tnum = *(data + j);
-
-            PREFETCH(getDataByInternalId(*(data + j + 1)), 0);
-
-            if (!(visited[tnum >> 5] & (1 << (tnum & 31)))) {
-				visited[tnum >> 5] |= 1 << (tnum & 31);
-
-                dist = fstdistfunc(point, getDataByInternalId(tnum));
-
-                if (topResults.top().first > dist || topResults.size() < ef) {
-                    candidateSet.emplace(-dist, tnum);
-
-                    PREFETCH(get_linklist0(candidateSet.top().second), 0);
-                    topResults.emplace(dist, tnum);
-
-                    if (topResults.size() > ef)
-                        topResults.pop();
-
-                    lowerBound = topResults.top().first;
-                }
-            }
-        }
-    }
-    return topResults;
-}
-
-
-void HierarchicalNSW::getNeighborsByHeuristic(std::priority_queue<std::pair<dist_t, idx_t>> &topResults, size_t NN)
-{
-    if (topResults.size() < NN)
-        return;
-
-    std::priority_queue<std::pair<dist_t, idx_t>> resultSet;
-    std::vector<std::pair<dist_t, idx_t>> returnlist;
-
-    while (topResults.size() > 0) {
-        resultSet.emplace(-topResults.top().first, topResults.top().second);
-        topResults.pop();
-    }
-
-    while (resultSet.size()) {
-        if (returnlist.size() >= NN)
-            break;
-        std::pair<dist_t, idx_t> curen = resultSet.top();
-        dist_t dist_to_query = -curen.first;
-        resultSet.pop();
-        bool good = true;
-        for (std::pair<dist_t, idx_t> curen2 : returnlist) {
-            dist_t curdist = fstdistfunc(getDataByInternalId(curen2.second),
-                                         getDataByInternalId(curen.second));
-            if (curdist < dist_to_query) {
-                good = false;
-                break;
-            }
-        }
-        if (good) returnlist.push_back(curen);
-    }
-    for (std::pair<dist_t, idx_t> elem : returnlist)
-        topResults.emplace(-elem.first, elem.second);
-}
-
-void HierarchicalNSW::mutuallyConnectNewElement(const coord_t *point, idx_t cur_c,
-                               std::priority_queue<std::pair<dist_t, idx_t>> topResults)
-{
-    getNeighborsByHeuristic(topResults, M);
-
-    std::vector<idx_t> res;
-    res.reserve(M);
-    while (topResults.size() > 0) {
-        res.push_back(topResults.top().second);
-        topResults.pop();
-    }
-    {
-        idx_t* data = get_linklist0(cur_c);
-        if (*data)
-            throw std::runtime_error("Should be blank");
-
-        *data++ = res.size();
-
-        for (size_t idx = 0; idx < res.size(); idx++) {
-            if (data[idx])
-                throw std::runtime_error("Should be blank");
-            data[idx] = res[idx];
-        }
-    }
-    for (size_t idx = 0; idx < res.size(); idx++) {
-        if (res[idx] == cur_c)
-            throw std::runtime_error("Connection to the same element");
-
-        size_t resMmax = maxM;
-        idx_t *ll_other = get_linklist0(res[idx]);
-        idx_t sz_link_list_other = *ll_other;
-
-        if (sz_link_list_other > resMmax || sz_link_list_other < 0)
-            throw std::runtime_error("Bad sz_link_list_other");
-
-        if (sz_link_list_other < resMmax) {
-            idx_t *data = ll_other + 1;
-            data[sz_link_list_other] = cur_c;
-            *ll_other = sz_link_list_other + 1;
-        } else {
-            // finding the "weakest" element to replace it with the new one
-            idx_t *data = ll_other + 1;
-            dist_t d_max = fstdistfunc(getDataByInternalId(cur_c), getDataByInternalId(res[idx]));
-            // Heuristic:
-            std::priority_queue<std::pair<dist_t, idx_t>> candidates;
-            candidates.emplace(d_max, cur_c);
-
-            for (size_t j = 0; j < sz_link_list_other; j++)
-                candidates.emplace(fstdistfunc(getDataByInternalId(data[j]), getDataByInternalId(res[idx])), data[j]);
-
-            getNeighborsByHeuristic(candidates, resMmax);
-
-            size_t indx = 0;
-            while (!candidates.empty()) {
-                data[indx] = candidates.top().second;
-                candidates.pop();
-                indx++;
-            }
-            *ll_other = indx;
-        }
-    }
-}
-
-void HierarchicalNSW::addPoint(const coord_t *point, label_t label)
-{
-    if (cur_element_count >= maxelements) {
-        throw std::runtime_error("The number of elements exceeds the specified limit");
-    }
-    idx_t cur_c = cur_element_count++;
-    memset((char *) get_linklist0(cur_c), 0, size_data_per_element);
-    memcpy(getDataByInternalId(cur_c), point, data_size);
-    memcpy(getExternalLabel(cur_c), &label, sizeof label);
-
-    // Do nothing for the first element
-    if (cur_c != 0) {
-        std::priority_queue <std::pair<dist_t, idx_t>> topResults = searchBaseLayer(point, efConstruction);
-        mutuallyConnectNewElement(point, cur_c, topResults);
-    }
-};
-
-std::priority_queue<std::pair<dist_t, label_t>> HierarchicalNSW::searchKnn(const coord_t *query, size_t k)
-{
-	std::priority_queue<std::pair<dist_t, label_t>> topResults;
-	auto topCandidates = searchBaseLayer(query, k);
-    while (topCandidates.size() > k) {
-        topCandidates.pop();
-	}
-	while (!topCandidates.empty()) {
-		std::pair<dist_t, idx_t> rez = topCandidates.top();
-		label_t label;
-		memcpy(&label, getExternalLabel(rez.second), sizeof(label));
-		topResults.push(std::pair<dist_t, label_t>(rez.first, label));
-		topCandidates.pop();
-	}
-
-    return topResults;
-};
-
-dist_t fstdistfunc_scalar(const coord_t *x, const coord_t *y, size_t n)
-{
-    dist_t 	distance = 0.0;
-
-    for (size_t i = 0; i < n; i++)
-    {
-        dist_t diff = x[i] - y[i];
-        distance += diff * diff;
-    }
-    return distance;
-
-}
-
-#ifdef __x86_64__
-#include <immintrin.h>
-
-__attribute__((target("avx2")))
-dist_t fstdistfunc_avx2(const coord_t *x, const coord_t *y, size_t n)
-{
-    const size_t TmpResSz = sizeof(__m256) / sizeof(float);
-    float PORTABLE_ALIGN32 TmpRes[TmpResSz];
-    size_t qty16 = n / 16;
-    const float *pEnd1 = x + (qty16 * 16);
-    __m256 diff, v1, v2;
-    __m256 sum = _mm256_set1_ps(0);
-
-    while (x < pEnd1) {
-        v1 = _mm256_loadu_ps(x);
-        x += 8;
-        v2 = _mm256_loadu_ps(y);
-        y += 8;
-        diff = _mm256_sub_ps(v1, v2);
-        sum = _mm256_add_ps(sum, _mm256_mul_ps(diff, diff));
-
-        v1 = _mm256_loadu_ps(x);
-        x += 8;
-        v2 = _mm256_loadu_ps(y);
-        y += 8;
-        diff = _mm256_sub_ps(v1, v2);
-        sum = _mm256_add_ps(sum, _mm256_mul_ps(diff, diff));
-    }
-    _mm256_store_ps(TmpRes, sum);
-    float res = TmpRes[0] + TmpRes[1] + TmpRes[2] + TmpRes[3] + TmpRes[4] + TmpRes[5] + TmpRes[6] + TmpRes[7];
-    return (res);
-}
-
-dist_t fstdistfunc_sse(const coord_t *x, const coord_t *y, size_t n)
-{
-    const size_t TmpResSz = sizeof(__m128) / sizeof(float);
-    float PORTABLE_ALIGN32 TmpRes[TmpResSz];
-    size_t qty16 = n / 16;
-    const float *pEnd1 = x + (qty16 * 16);
-
-    __m128 diff, v1, v2;
-    __m128 sum = _mm_set1_ps(0);
-
-    while (x < pEnd1) {
-        v1 = _mm_loadu_ps(x);
-        x += 4;
-        v2 = _mm_loadu_ps(y);
-        y += 4;
-        diff = _mm_sub_ps(v1, v2);
-        sum = _mm_add_ps(sum, _mm_mul_ps(diff, diff));
-
-        v1 = _mm_loadu_ps(x);
-        x += 4;
-        v2 = _mm_loadu_ps(y);
-        y += 4;
-        diff = _mm_sub_ps(v1, v2);
-        sum = _mm_add_ps(sum, _mm_mul_ps(diff, diff));
-
-        v1 = _mm_loadu_ps(x);
-        x += 4;
-        v2 = _mm_loadu_ps(y);
-        y += 4;
-        diff = _mm_sub_ps(v1, v2);
-        sum = _mm_add_ps(sum, _mm_mul_ps(diff, diff));
-
-        v1 = _mm_loadu_ps(x);
-        x += 4;
-        v2 = _mm_loadu_ps(y);
-        y += 4;
-        diff = _mm_sub_ps(v1, v2);
-        sum = _mm_add_ps(sum, _mm_mul_ps(diff, diff));
-    }
-    _mm_store_ps(TmpRes, sum);
-    float res = TmpRes[0] + TmpRes[1] + TmpRes[2] + TmpRes[3];
-    return res;
-}
-#endif
-
-dist_t HierarchicalNSW::fstdistfunc(const coord_t *x, const coord_t *y)
-{
-#ifndef __x86_64__
-    return fstdistfunc_scalar(x, y, dim);
-#else
-    if(use_avx2)
-        return fstdistfunc_avx2(x, y, dim);
-
-    return fstdistfunc_sse(x, y, dim);
-#endif
-}
-
-bool hnsw_search(HierarchicalNSW* hnsw, const coord_t *point, size_t efSearch, size_t* n_results, label_t** results)
-{
-	try
-	{
-		auto result = hnsw->searchKnn(point, efSearch);
-		size_t nResults = result.size();
-		*results = (label_t*)malloc(nResults*sizeof(label_t));
-		for (size_t i = nResults; i-- != 0;)
-		{
-			(*results)[i] = result.top().second;
-			result.pop();
-		}
-		*n_results = nResults;
-		return true;
-	}
-	catch (std::exception& x)
-	{
-		return false;
-	}
-}
-
-bool hnsw_add_point(HierarchicalNSW* hnsw, const coord_t *point, label_t label)
-{
-	try
-	{
-		hnsw->addPoint(point, label);
-		return true;
-	}
-	catch (std::exception& x)
-	{
-		fprintf(stderr, "Catch %s\n", x.what());
-		return false;
-	}
-}
-
-void hnsw_init(HierarchicalNSW* hnsw, size_t dims, size_t maxelements, size_t M, size_t maxM, size_t efConstruction)
-{
-	new ((void*)hnsw) HierarchicalNSW(dims, maxelements, M, maxM, efConstruction);
-}
-
-
-int hnsw_dimensions(HierarchicalNSW* hnsw)
-{
-	return (int)hnsw->dim;
-}
-
-size_t hnsw_count(HierarchicalNSW* hnsw)
-{
-	return hnsw->cur_element_count;
-}
-
-size_t hnsw_sizeof(void)
-{
-	return sizeof(HierarchicalNSW);
-}

pgxn/hnsw/hnswalg.h

@@ -1,69 +0,0 @@
-#pragma once
-
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <stdint.h>
-#include <unordered_map>
-#include <unordered_set>
-#include <map>
-#include <cmath>
-#include <queue>
-#include <stdexcept>
-
-extern "C" {
-#include "hnsw.h"
-}
-
-struct HierarchicalNSW
-{
-	size_t maxelements;
-	size_t cur_element_count;
-
-	idx_t  enterpoint_node;
-
-	size_t dim;
-	size_t data_size;
-	size_t offset_data;
-	size_t offset_label;
-	size_t size_data_per_element;
-	size_t M;
-	size_t maxM;
-	size_t size_links_level0;
-	size_t efConstruction;
-
-#ifdef __x86_64__
-	bool	use_avx2;
-#endif
-
-	char   data_level0_memory[0]; // varying size
-
-  public:
-	HierarchicalNSW(size_t dim, size_t maxelements, size_t M, size_t maxM, size_t efConstruction);
-	~HierarchicalNSW();
-
-
-	inline coord_t *getDataByInternalId(idx_t internal_id) const {
-		return (coord_t *)&data_level0_memory[internal_id * size_data_per_element + offset_data];
-	}
-
-	inline idx_t *get_linklist0(idx_t internal_id) const {
-		return (idx_t*)&data_level0_memory[internal_id * size_data_per_element];
-	}
-
-	inline label_t *getExternalLabel(idx_t internal_id) const {
-		return (label_t *)&data_level0_memory[internal_id * size_data_per_element + offset_label];
-	}
-
-	std::priority_queue<std::pair<dist_t, idx_t>> searchBaseLayer(const coord_t *x, size_t ef);
-
-	void getNeighborsByHeuristic(std::priority_queue<std::pair<dist_t, idx_t>> &topResults, size_t NN);
-
-	void mutuallyConnectNewElement(const coord_t *x, idx_t id, std::priority_queue<std::pair<dist_t, idx_t>> topResults);
-
-	void addPoint(const coord_t *point, label_t label);
-
-	std::priority_queue<std::pair<dist_t, label_t>> searchKnn(const coord_t *query_data, size_t k);
-
-	dist_t fstdistfunc(const coord_t *x, const coord_t *y);
-};

pgxn/hnsw/test/expected/knn.out

@@ -1,28 +0,0 @@
-SET enable_seqscan = off;
-CREATE TABLE t (val real[]);
-INSERT INTO t (val) VALUES ('{0,0,0}'), ('{1,2,3}'), ('{1,1,1}'), (NULL);
-CREATE INDEX ON t USING hnsw (val) WITH (maxelements = 10, dims=3, m=3);
-INSERT INTO t (val) VALUES (array[1,2,4]);
-explain SELECT * FROM t ORDER BY val <-> array[3,3,3];
-                             QUERY PLAN                             
---------------------------------------------------------------------
- Index Scan using t_val_idx on t  (cost=4.02..8.06 rows=3 width=36)
-   Order By: (val <-> '{3,3,3}'::real[])
-(2 rows)
-
-SELECT * FROM t ORDER BY val <-> array[3,3,3];
-   val   
----------
- {1,2,3}
- {1,2,4}
- {1,1,1}
- {0,0,0}
-(4 rows)
-
-SELECT COUNT(*) FROM t;
- count 
--------
-     5
-(1 row)
-
-DROP TABLE t;

pgxn/hnsw/test/sql/knn.sql

@@ -1,13 +0,0 @@
-SET enable_seqscan = off;
-
-CREATE TABLE t (val real[]);
-INSERT INTO t (val) VALUES ('{0,0,0}'), ('{1,2,3}'), ('{1,1,1}'), (NULL);
-CREATE INDEX ON t USING hnsw (val) WITH (maxelements = 10, dims=3, m=3);
-
-INSERT INTO t (val) VALUES (array[1,2,4]);
-
-explain SELECT * FROM t ORDER BY val <-> array[3,3,3];
-SELECT * FROM t ORDER BY val <-> array[3,3,3];
-SELECT COUNT(*) FROM t;
-
-DROP TABLE t;

pgxn/neon/walproposer.c

@@ -254,20 +254,20 @@ nwp_register_gucs(void)
 
 	DefineCustomIntVariable(
 							"neon.safekeeper_reconnect_timeout",
-							"Walproposer reconnects to offline safekeepers once in this interval.",
+							"Timeout for reconnecting to offline wal acceptor.",
 							NULL,
 							&wal_acceptor_reconnect_timeout,
-							5000, 0, INT_MAX,	/* default, min, max */
+							1000, 0, INT_MAX,	/* default, min, max */
 							PGC_SIGHUP, /* context */
 							GUC_UNIT_MS,	/* flags */
 							NULL, NULL, NULL);
 
 	DefineCustomIntVariable(
 							"neon.safekeeper_connect_timeout",
-							"Connection or connection attempt to safekeeper is terminated if no message is received (or connection attempt doesn't finish) within this period.",
+							"Timeout for connection establishement and it's maintenance against safekeeper",
 							NULL,
 							&wal_acceptor_connection_timeout,
-							10000, 0, INT_MAX,
+							5000, 0, INT_MAX,
 							PGC_SIGHUP,
 							GUC_UNIT_MS,
 							NULL, NULL, NULL);
@@ -441,7 +441,7 @@ WalProposerPoll(void)
 				if (TimestampDifferenceExceeds(sk->latestMsgReceivedAt, now,
 											   wal_acceptor_connection_timeout))
 				{
-					elog(WARNING, "terminating connection to safekeeper '%s:%s' in '%s' state: no messages received during the last %dms or connection attempt took longer than that",
+					elog(WARNING, "failed to connect to node '%s:%s' in '%s' state: exceeded connection timeout %dms",
 						 sk->host, sk->port, FormatSafekeeperState(sk->state), wal_acceptor_connection_timeout);
 					ShutdownConnection(sk);
 				}
@@ -1035,16 +1035,9 @@ RecvAcceptorGreeting(Safekeeper *sk)
 	if (!AsyncReadMessage(sk, (AcceptorProposerMessage *) & sk->greetResponse))
 		return;
 
-	elog(LOG, "received AcceptorGreeting from safekeeper %s:%s", sk->host, sk->port);
-
 	/* Protocol is all good, move to voting. */
 	sk->state = SS_VOTING;
 
-	/* 
-	 * Note: it would be better to track the counter on per safekeeper basis,
-	 * but at worst walproposer would restart with 'term rejected', so leave as
-	 * is for now.
-	 */
 	++n_connected;
 	if (n_connected <= quorum)
 	{

proxy/src/console.rs

@@ -1,5 +1,5 @@
-//! Various stuff for dealing with the Neon Console.
-//! Later we might move some API wrappers here.
+///! Various stuff for dealing with the Neon Console.
+///! Later we might move some API wrappers here.
 
 /// Payloads used in the console's APIs.
 pub mod messages;

proxy/src/proxy/tests.rs

@@ -1,4 +1,4 @@
-//! A group of high-level tests for connection establishing logic and auth.
+///! A group of high-level tests for connection establishing logic and auth.
 use super::*;
 use crate::{auth, sasl, scram};
 use async_trait::async_trait;

safekeeper/src/control_file.rs

@@ -7,7 +7,6 @@ use std::fs::{self, File, OpenOptions};
 use std::io::{Read, Write};
 use std::ops::Deref;
 use std::path::{Path, PathBuf};
-use std::time::Instant;
 
 use crate::control_file_upgrade::upgrade_control_file;
 use crate::metrics::PERSIST_CONTROL_FILE_SECONDS;
@@ -29,9 +28,6 @@ pub const CHECKSUM_SIZE: usize = std::mem::size_of::<u32>();
 pub trait Storage: Deref<Target = SafeKeeperState> {
     /// Persist safekeeper state on disk and update internal state.
     fn persist(&mut self, s: &SafeKeeperState) -> Result<()>;
-
-    /// Timestamp of last persist.
-    fn last_persist_at(&self) -> Instant;
 }
 
 #[derive(Debug)]
@@ -42,8 +38,6 @@ pub struct FileStorage {
 
     /// Last state persisted to disk.
     state: SafeKeeperState,
-    /// Not preserved across restarts.
-    last_persist_at: Instant,
 }
 
 impl FileStorage {
@@ -57,7 +51,6 @@ impl FileStorage {
             timeline_dir,
             conf: conf.clone(),
             state,
-            last_persist_at: Instant::now(),
         })
     }
 
@@ -73,7 +66,6 @@ impl FileStorage {
             timeline_dir,
             conf: conf.clone(),
             state,
-            last_persist_at: Instant::now(),
         };
 
         Ok(store)
@@ -224,10 +216,6 @@ impl Storage for FileStorage {
         self.state = s.clone();
         Ok(())
     }
-
-    fn last_persist_at(&self) -> Instant {
-        self.last_persist_at
-    }
 }
 
 #[cfg(test)]

safekeeper/src/remove_wal.rs

@@ -17,9 +17,6 @@ pub fn thread_main(conf: SafeKeeperConf) {
             let ttid = tli.ttid;
             let _enter =
                 info_span!("", tenant = %ttid.tenant_id, timeline = %ttid.timeline_id).entered();
-            if let Err(e) = tli.maybe_pesist_control_file() {
-                warn!("failed to persist control file: {e}");
-            }
             if let Err(e) = tli.remove_old_wal(conf.wal_backup_enabled) {
                 warn!("failed to remove WAL: {}", e);
             }

safekeeper/src/safekeeper.rs

@@ -10,7 +10,6 @@ use std::cmp::max;
 use std::cmp::min;
 use std::fmt;
 use std::io::Read;
-use std::time::Duration;
 use storage_broker::proto::SafekeeperTimelineInfo;
 
 use tracing::*;
@@ -635,8 +634,7 @@ where
         }
 
         // system_id will be updated on mismatch
-        // sync-safekeepers doesn't know sysid and sends 0, ignore it
-        if self.state.server.system_id != msg.system_id && msg.system_id != 0 {
+        if self.state.server.system_id != msg.system_id {
             if self.state.server.system_id != 0 {
                 warn!(
                     "unexpected system ID arrived, got {}, expected {}",
@@ -838,26 +836,6 @@ where
         self.state.persist(&state)
     }
 
-    /// Persist control file if there is something to save and enough time
-    /// passed after the last save.
-    pub fn maybe_persist_control_file(&mut self, inmem_remote_consistent_lsn: Lsn) -> Result<()> {
-        const CF_SAVE_INTERVAL: Duration = Duration::from_secs(300);
-        if self.state.last_persist_at().elapsed() < CF_SAVE_INTERVAL {
-            return Ok(());
-        }
-        let need_persist = self.inmem.commit_lsn > self.state.commit_lsn
-            || self.inmem.backup_lsn > self.state.backup_lsn
-            || self.inmem.peer_horizon_lsn > self.state.peer_horizon_lsn
-            || inmem_remote_consistent_lsn > self.state.remote_consistent_lsn;
-        if need_persist {
-            let mut state = self.state.clone();
-            state.remote_consistent_lsn = inmem_remote_consistent_lsn;
-            self.persist_control_file(state)?;
-            trace!("saved control file: {CF_SAVE_INTERVAL:?} passed");
-        }
-        Ok(())
-    }
-
     /// Handle request to append WAL.
     #[allow(clippy::comparison_chain)]
     fn handle_append_request(
@@ -970,8 +948,9 @@ where
 
         if sync_control_file {
             let mut state = self.state.clone();
-            // Note: we could make remote_consistent_lsn update in cf common by
-            // storing Arc to walsenders in Safekeeper.
+            // Note: we do not persist remote_consistent_lsn in other paths of
+            // persisting cf -- that is not much needed currently. We could do
+            // that by storing Arc to walsenders in Safekeeper.
             state.remote_consistent_lsn = new_remote_consistent_lsn;
             self.persist_control_file(state)?;
         }
@@ -1001,7 +980,7 @@ mod tests {
 
     use super::*;
     use crate::wal_storage::Storage;
-    use std::{ops::Deref, time::Instant};
+    use std::ops::Deref;
 
     // fake storage for tests
     struct InMemoryState {
@@ -1013,10 +992,6 @@ mod tests {
             self.persisted_state = s.clone();
             Ok(())
         }
-
-        fn last_persist_at(&self) -> Instant {
-            Instant::now()
-        }
     }
 
     impl Deref for InMemoryState {

safekeeper/src/timeline.rs

@@ -234,6 +234,7 @@ impl SharedState {
             flush_lsn: self.sk.wal_store.flush_lsn().0,
             // note: this value is not flushed to control file yet and can be lost
             commit_lsn: self.sk.inmem.commit_lsn.0,
+            // TODO: rework feedbacks to avoid max here
             remote_consistent_lsn: remote_consistent_lsn.0,
             peer_horizon_lsn: self.sk.inmem.peer_horizon_lsn.0,
             safekeeper_connstr: conf.listen_pg_addr.clone(),
@@ -672,17 +673,6 @@ impl Timeline {
         Ok(())
     }
 
-    /// Persist control file if there is something to save and enough time
-    /// passed after the last save. This helps to keep remote_consistent_lsn up
-    /// to date so that storage nodes restart doesn't cause many pageserver ->
-    /// safekeeper reconnections.
-    pub fn maybe_pesist_control_file(&self) -> Result<()> {
-        let remote_consistent_lsn = self.walsenders.get_remote_consistent_lsn();
-        self.write_shared_state()
-            .sk
-            .maybe_persist_control_file(remote_consistent_lsn)
-    }
-
     /// Returns full timeline info, required for the metrics. If the timeline is
     /// not active, returns None instead.
     pub fn info_for_metrics(&self) -> Option<FullTimelineInfo> {

safekeeper/src/wal_storage.rs

@@ -379,12 +379,6 @@ impl Storage for PhysicalStorage {
             );
         }
 
-        // Quick exit if nothing to do to avoid writing up to 16 MiB of zeros on
-        // disk (this happens on each connect).
-        if end_pos == self.write_lsn {
-            return Ok(());
-        }
-
         // Close previously opened file, if any
         if let Some(mut unflushed_file) = self.file.take() {
             self.fdatasync_file(&mut unflushed_file)?;

test_runner/fixtures/neon_fixtures.py

@@ -629,7 +629,7 @@ class NeonEnvBuilder:
         assert self.env is not None, "environment is not already initialized, call init() first"
         self.env.start()
 
-    def init_start(self, initial_tenant_conf: Optional[Dict[str, str]] = None) -> NeonEnv:
+    def init_start(self) -> NeonEnv:
         env = self.init_configs()
         self.start()
 
@@ -638,9 +638,7 @@ class NeonEnvBuilder:
         log.info(
             f"Services started, creating initial tenant {env.initial_tenant} and its initial timeline"
         )
-        initial_tenant, initial_timeline = env.neon_cli.create_tenant(
-            tenant_id=env.initial_tenant, conf=initial_tenant_conf
-        )
+        initial_tenant, initial_timeline = env.neon_cli.create_tenant(tenant_id=env.initial_tenant)
         env.initial_timeline = initial_timeline
         log.info(f"Initial timeline {initial_tenant}/{initial_timeline} created successfully")
 
@@ -1446,12 +1444,11 @@ class NeonCli(AbstractNeonCli):
     def endpoint_create(
         self,
         branch_name: str,
-        pg_port: int,
-        http_port: int,
         endpoint_id: Optional[str] = None,
         tenant_id: Optional[TenantId] = None,
         hot_standby: bool = False,
         lsn: Optional[Lsn] = None,
+        port: Optional[int] = None,
     ) -> "subprocess.CompletedProcess[str]":
         args = [
             "endpoint",
@@ -1465,10 +1462,8 @@ class NeonCli(AbstractNeonCli):
         ]
         if lsn is not None:
             args.extend(["--lsn", str(lsn)])
-        if pg_port is not None:
-            args.extend(["--pg-port", str(pg_port)])
-        if http_port is not None:
-            args.extend(["--http-port", str(http_port)])
+        if port is not None:
+            args.extend(["--port", str(port)])
         if endpoint_id is not None:
             args.append(endpoint_id)
         if hot_standby:
@@ -1481,11 +1476,9 @@ class NeonCli(AbstractNeonCli):
     def endpoint_start(
         self,
         endpoint_id: str,
-        pg_port: int,
-        http_port: int,
-        safekeepers: Optional[List[int]] = None,
         tenant_id: Optional[TenantId] = None,
         lsn: Optional[Lsn] = None,
+        port: Optional[int] = None,
     ) -> "subprocess.CompletedProcess[str]":
         args = [
             "endpoint",
@@ -1497,10 +1490,8 @@ class NeonCli(AbstractNeonCli):
         ]
         if lsn is not None:
             args.append(f"--lsn={lsn}")
-        args.extend(["--pg-port", str(pg_port)])
-        args.extend(["--http-port", str(http_port)])
-        if safekeepers is not None:
-            args.extend(["--safekeepers", (",".join(map(str, safekeepers)))])
+        if port is not None:
+            args.append(f"--port={port}")
         if endpoint_id is not None:
             args.append(endpoint_id)
 
@@ -1592,11 +1583,13 @@ class NeonPageserver(PgProtocol):
             ".*serving compute connection task.*exited with error: Postgres connection error.*",
             ".*serving compute connection task.*exited with error: Connection reset by peer.*",
             ".*serving compute connection task.*exited with error: Postgres query error.*",
-            ".*Connection aborted: error communicating with the server: Transport endpoint is not connected.*",
+            ".*Connection aborted: connection error: error communicating with the server: Broken pipe.*",
+            ".*Connection aborted: connection error: error communicating with the server: Transport endpoint is not connected.*",
+            ".*Connection aborted: connection error: error communicating with the server: Connection reset by peer.*",
             # FIXME: replication patch for tokio_postgres regards  any but CopyDone/CopyData message in CopyBoth stream as unexpected
-            ".*Connection aborted: unexpected message from server*",
+            ".*Connection aborted: connection error: unexpected message from server*",
             ".*kill_and_wait_impl.*: wait successful.*",
-            ".*: db error:.*ending streaming to Some.*",
+            ".*Replication stream finished: db error:.*ending streaming to Some*",
             ".*query handler for 'pagestream.*failed: Broken pipe.*",  # pageserver notices compute shut down
             ".*query handler for 'pagestream.*failed: Connection reset by peer.*",  # pageserver notices compute shut down
             # safekeeper connection can fail with this, in the window between timeline creation
@@ -1613,12 +1606,13 @@ class NeonPageserver(PgProtocol):
             ".*manual_gc.*is_shutdown_requested\\(\\) called in an unexpected task or thread.*",
             ".*tenant_list: timeline is not found in remote index while it is present in the tenants registry.*",
             ".*Removing intermediate uninit mark file.*",
+            # FIXME: known race condition in TaskHandle: https://github.com/neondatabase/neon/issues/2885
+            ".*sender is dropped while join handle is still alive.*",
             # Tenant::delete_timeline() can cause any of the four following errors.
             # FIXME: we shouldn't be considering it an error: https://github.com/neondatabase/neon/issues/2946
             ".*could not flush frozen layer.*queue is in state Stopped",  # when schedule layer upload fails because queued got closed before compaction got killed
             ".*wait for layer upload ops to complete.*",  # .*Caused by:.*wait_completion aborted because upload queue was stopped
             ".*gc_loop.*Gc failed, retrying in.*timeline is Stopping",  # When gc checks timeline state after acquiring layer_removal_cs
-            ".*gc_loop.*Gc failed, retrying in.*: Cannot run GC iteration on inactive tenant",  # Tenant::gc precondition
             ".*compaction_loop.*Compaction failed, retrying in.*timeline is Stopping",  # When compaction checks timeline state after acquiring layer_removal_cs
             ".*query handler for 'pagestream.*failed: Timeline .* was not found",  # postgres reconnects while timeline_delete doesn't hold the tenant's timelines.lock()
             ".*query handler for 'pagestream.*failed: Timeline .* is not active",  # timeline delete in progress
@@ -1696,9 +1690,6 @@ class NeonPageserver(PgProtocol):
                 else:
                     errors.append(line)
 
-        for error in errors:
-            log.info(f"not allowed error: {error.strip()}")
-
         assert not errors
 
     def log_contains(self, pattern: str) -> Optional[str]:
@@ -2291,24 +2282,17 @@ class Endpoint(PgProtocol):
     """An object representing a Postgres compute endpoint managed by the control plane."""
 
     def __init__(
-        self,
-        env: NeonEnv,
-        tenant_id: TenantId,
-        pg_port: int,
-        http_port: int,
-        check_stop_result: bool = True,
+        self, env: NeonEnv, tenant_id: TenantId, port: int, check_stop_result: bool = True
     ):
-        super().__init__(host="localhost", port=pg_port, user="cloud_admin", dbname="postgres")
+        super().__init__(host="localhost", port=port, user="cloud_admin", dbname="postgres")
         self.env = env
         self.running = False
         self.branch_name: Optional[str] = None  # dubious
         self.endpoint_id: Optional[str] = None  # dubious, see asserts below
         self.pgdata_dir: Optional[str] = None  # Path to computenode PGDATA
         self.tenant_id = tenant_id
-        self.pg_port = pg_port
-        self.http_port = http_port
+        self.port = port
         self.check_stop_result = check_stop_result
-        self.active_safekeepers: List[int] = list(map(lambda sk: sk.id, env.safekeepers))
         # path to conf is <repo_dir>/endpoints/<endpoint_id>/pgdata/postgresql.conf
 
     def create(
@@ -2338,8 +2322,7 @@ class Endpoint(PgProtocol):
             tenant_id=self.tenant_id,
             lsn=lsn,
             hot_standby=hot_standby,
-            pg_port=self.pg_port,
-            http_port=self.http_port,
+            port=self.port,
         )
         path = Path("endpoints") / self.endpoint_id / "pgdata"
         self.pgdata_dir = os.path.join(self.env.repo_dir, path)
@@ -2364,13 +2347,7 @@ class Endpoint(PgProtocol):
 
         log.info(f"Starting postgres endpoint {self.endpoint_id}")
 
-        self.env.neon_cli.endpoint_start(
-            self.endpoint_id,
-            pg_port=self.pg_port,
-            http_port=self.http_port,
-            tenant_id=self.tenant_id,
-            safekeepers=self.active_safekeepers,
-        )
+        self.env.neon_cli.endpoint_start(self.endpoint_id, tenant_id=self.tenant_id, port=self.port)
         self.running = True
 
         return self
@@ -2394,8 +2371,32 @@ class Endpoint(PgProtocol):
         return os.path.join(self.pg_data_dir_path(), "pg_twophase")
 
     def config_file_path(self) -> str:
-        """Path to the postgresql.conf in the endpoint directory (not the one in pgdata)"""
-        return os.path.join(self.endpoint_path(), "postgresql.conf")
+        """Path to postgresql.conf"""
+        return os.path.join(self.pg_data_dir_path(), "postgresql.conf")
+
+    def adjust_for_safekeepers(self, safekeepers: str) -> "Endpoint":
+        """
+        Adjust instance config for working with wal acceptors instead of
+        pageserver (pre-configured by CLI) directly.
+        """
+
+        # TODO: reuse config()
+        with open(self.config_file_path(), "r") as f:
+            cfg_lines = f.readlines()
+        with open(self.config_file_path(), "w") as f:
+            for cfg_line in cfg_lines:
+                # walproposer uses different application_name
+                if (
+                    "synchronous_standby_names" in cfg_line
+                    or
+                    # don't repeat safekeepers/wal_acceptors multiple times
+                    "neon.safekeepers" in cfg_line
+                ):
+                    continue
+                f.write(cfg_line)
+            f.write("synchronous_standby_names = 'walproposer'\n")
+            f.write("neon.safekeepers = '{}'\n".format(safekeepers))
+        return self
 
     def config(self, lines: List[str]) -> "Endpoint":
         """
@@ -2500,8 +2501,7 @@ class EndpointFactory:
         ep = Endpoint(
             self.env,
             tenant_id=tenant_id or self.env.initial_tenant,
-            pg_port=self.env.port_distributor.get_port(),
-            http_port=self.env.port_distributor.get_port(),
+            port=self.env.port_distributor.get_port(),
         )
         self.num_instances += 1
         self.endpoints.append(ep)
@@ -2526,8 +2526,7 @@ class EndpointFactory:
         ep = Endpoint(
             self.env,
             tenant_id=tenant_id or self.env.initial_tenant,
-            pg_port=self.env.port_distributor.get_port(),
-            http_port=self.env.port_distributor.get_port(),
+            port=self.env.port_distributor.get_port(),
         )
 
         if endpoint_id is None:
@@ -2910,7 +2909,6 @@ SKIP_FILES = frozenset(
         "pg_internal.init",
         "pg.log",
         "zenith.signal",
-        "pg_hba.conf",
         "postgresql.conf",
         "postmaster.opts",
         "postmaster.pid",

test_runner/fixtures/pageserver/utils.py

@@ -1,5 +1,5 @@
 import time
-from typing import Any, Dict, Optional
+from typing import Optional
 
 from fixtures.log_helper import log
 from fixtures.pageserver.http import PageserverHttpClient
@@ -72,7 +72,7 @@ def wait_until_tenant_state(
     expected_state: str,
     iterations: int,
     period: float = 1.0,
-) -> Dict[str, Any]:
+) -> bool:
     """
     Does not use `wait_until` for debugging purposes
     """
@@ -81,7 +81,7 @@ def wait_until_tenant_state(
             tenant = pageserver_http.tenant_status(tenant_id=tenant_id)
             log.debug(f"Tenant {tenant_id} data: {tenant}")
             if tenant["state"]["slug"] == expected_state:
-                return tenant
+                return True
         except Exception as e:
             log.debug(f"Tenant {tenant_id} state retrieval failure: {e}")
 

test_runner/regress/test_attach_tenant_config.py

@@ -20,11 +20,6 @@ def positive_env(neon_env_builder: NeonEnvBuilder) -> NeonEnv:
         test_name="test_attach_tenant_config",
     )
     env = neon_env_builder.init_start()
-
-    # eviction might be the first one after an attach to access the layers
-    env.pageserver.allowed_errors.append(
-        ".*unexpectedly on-demand downloading remote layer remote.* for task kind Eviction"
-    )
     assert isinstance(env.remote_storage, LocalFsStorage)
     return env
 

test_runner/regress/test_compatibility.py

@@ -383,9 +383,6 @@ def check_neon_works(
     cli_target = NeonCli(config_target)
 
     # And the current binaries to launch computes
-    snapshot_config["neon_distrib_dir"] = str(neon_current_binpath)
-    with (snapshot_config_toml).open("w") as f:
-        toml.dump(snapshot_config, f)
     config_current = copy.copy(config)
     config_current.neon_binpath = neon_current_binpath
     cli_current = NeonCli(config_current)
@@ -394,8 +391,7 @@ def check_neon_works(
     request.addfinalizer(lambda: cli_target.raw_cli(["stop"]))
 
     pg_port = port_distributor.get_port()
-    http_port = port_distributor.get_port()
-    cli_current.endpoint_start("main", pg_port=pg_port, http_port=http_port)
+    cli_current.endpoint_start("main", port=pg_port)
     request.addfinalizer(lambda: cli_current.endpoint_stop("main"))
 
     connstr = f"host=127.0.0.1 port={pg_port} user=cloud_admin dbname=postgres"

test_runner/regress/test_compute_ctl.py

@@ -0,0 +1,253 @@
+import os
+from pathlib import Path
+from subprocess import TimeoutExpired
+
+from fixtures.log_helper import log
+from fixtures.neon_fixtures import ComputeCtl, NeonEnvBuilder, PgBin
+
+
+# Test that compute_ctl works and prints "--sync-safekeepers" logs.
+def test_sync_safekeepers_logs(neon_env_builder: NeonEnvBuilder, pg_bin: PgBin):
+    neon_env_builder.num_safekeepers = 3
+    env = neon_env_builder.init_start()
+    ctl = ComputeCtl(env)
+
+    env.neon_cli.create_branch("test_compute_ctl", "main")
+    endpoint = env.endpoints.create_start("test_compute_ctl")
+    endpoint.safe_psql("CREATE TABLE t(key int primary key, value text)")
+
+    with open(endpoint.config_file_path(), "r") as f:
+        cfg_lines = f.readlines()
+    cfg_map = {}
+    for line in cfg_lines:
+        if "=" in line:
+            k, v = line.split("=")
+            cfg_map[k] = v.strip("\n '\"")
+    log.info(f"postgres config: {cfg_map}")
+    pgdata = endpoint.pg_data_dir_path()
+    pg_bin_path = os.path.join(pg_bin.pg_bin_path, "postgres")
+
+    endpoint.stop_and_destroy()
+
+    # stop_and_destroy removes the whole endpoint directory. Recreate it.
+    Path(pgdata).mkdir(parents=True)
+
+    spec = (
+        """
+{
+    "format_version": 1.0,
+
+    "timestamp": "2021-05-23T18:25:43.511Z",
+    "operation_uuid": "0f657b36-4b0f-4a2d-9c2e-1dcd615e7d8b",
+
+    "cluster": {
+        "cluster_id": "test-cluster-42",
+        "name": "Neon Test",
+        "state": "restarted",
+        "roles": [
+        ],
+        "databases": [
+        ],
+        "settings": [
+            {
+                "name": "fsync",
+                "value": "off",
+                "vartype": "bool"
+            },
+            {
+                "name": "wal_level",
+                "value": "replica",
+                "vartype": "enum"
+            },
+            {
+                "name": "neon.safekeepers",
+                "value": """
+        + f'"{cfg_map["neon.safekeepers"]}"'
+        + """,
+                "vartype": "string"
+            },
+            {
+                "name": "wal_log_hints",
+                "value": "on",
+                "vartype": "bool"
+            },
+            {
+                "name": "log_connections",
+                "value": "on",
+                "vartype": "bool"
+            },
+            {
+                "name": "shared_buffers",
+                "value": "32768",
+                "vartype": "integer"
+            },
+            {
+                "name": "port",
+                "value": """
+        + f'"{cfg_map["port"]}"'
+        + """,
+                "vartype": "integer"
+            },
+            {
+                "name": "max_connections",
+                "value": "100",
+                "vartype": "integer"
+            },
+            {
+                "name": "max_wal_senders",
+                "value": "10",
+                "vartype": "integer"
+            },
+            {
+                "name": "listen_addresses",
+                "value": "0.0.0.0",
+                "vartype": "string"
+            },
+            {
+                "name": "wal_sender_timeout",
+                "value": "0",
+                "vartype": "integer"
+            },
+            {
+                "name": "password_encryption",
+                "value": "md5",
+                "vartype": "enum"
+            },
+            {
+                "name": "maintenance_work_mem",
+                "value": "65536",
+                "vartype": "integer"
+            },
+            {
+                "name": "max_parallel_workers",
+                "value": "8",
+                "vartype": "integer"
+            },
+            {
+                "name": "max_worker_processes",
+                "value": "8",
+                "vartype": "integer"
+            },
+            {
+                "name": "neon.tenant_id",
+                "value": """
+        + f'"{cfg_map["neon.tenant_id"]}"'
+        + """,
+                "vartype": "string"
+            },
+            {
+                "name": "max_replication_slots",
+                "value": "10",
+                "vartype": "integer"
+            },
+            {
+                "name": "neon.timeline_id",
+                "value": """
+        + f'"{cfg_map["neon.timeline_id"]}"'
+        + """,
+                "vartype": "string"
+            },
+            {
+                "name": "shared_preload_libraries",
+                "value": "neon",
+                "vartype": "string"
+            },
+            {
+                "name": "synchronous_standby_names",
+                "value": "walproposer",
+                "vartype": "string"
+            },
+            {
+                "name": "neon.pageserver_connstring",
+                "value": """
+        + f'"{cfg_map["neon.pageserver_connstring"]}"'
+        + """,
+                "vartype": "string"
+            }
+        ]
+    },
+    "delta_operations": [
+    ]
+}
+"""
+    )
+
+    ps_connstr = cfg_map["neon.pageserver_connstring"]
+    log.info(f"ps_connstr: {ps_connstr}, pgdata: {pgdata}")
+
+    # run compute_ctl and wait for 10s
+    try:
+        ctl.raw_cli(
+            [
+                "--connstr",
+                "postgres://invalid/",
+                "--pgdata",
+                pgdata,
+                "--spec",
+                spec,
+                "--pgbin",
+                pg_bin_path,
+            ],
+            timeout=10,
+        )
+    except TimeoutExpired as exc:
+        ctl_logs = (exc.stderr or b"").decode("utf-8")
+        log.info(f"compute_ctl stderr:\n{ctl_logs}")
+
+    with ExternalProcessManager(Path(pgdata) / "postmaster.pid"):
+        start = "starting safekeepers syncing"
+        end = "safekeepers synced at LSN"
+        start_pos = ctl_logs.index(start)
+        assert start_pos != -1
+        end_pos = ctl_logs.index(end, start_pos)
+        assert end_pos != -1
+        sync_safekeepers_logs = ctl_logs[start_pos : end_pos + len(end)]
+        log.info("sync_safekeepers_logs:\n" + sync_safekeepers_logs)
+
+        # assert that --sync-safekeepers logs are present in the output
+        assert "connecting with node" in sync_safekeepers_logs
+        assert "connected with node" in sync_safekeepers_logs
+        assert "proposer connected to quorum (2)" in sync_safekeepers_logs
+        assert "got votes from majority (2)" in sync_safekeepers_logs
+        assert "sending elected msg to node" in sync_safekeepers_logs
+
+
+class ExternalProcessManager:
+    """
+    Context manager that kills a process with a pid file on exit.
+    """
+
+    def __init__(self, pid_file: Path):
+        self.path = pid_file
+        self.pid_file = open(pid_file, "r")
+        self.pid = int(self.pid_file.readline().strip())
+
+    def __enter__(self):
+        return self
+
+    def leave_alive(self):
+        self.pid_file.close()
+
+    def __exit__(self, _type, _value, _traceback):
+        import signal
+        import time
+
+        if self.pid_file.closed:
+            return
+
+        with self.pid_file:
+            try:
+                os.kill(self.pid, signal.SIGTERM)
+            except OSError as e:
+                if not self.path.is_file():
+                    return
+                log.info(f"Failed to kill {self.pid}, but the pidfile remains: {e}")
+                return
+
+            for _ in range(20):
+                if not self.path.is_file():
+                    return
+                time.sleep(0.2)
+
+            log.info("Process failed to stop after SIGTERM: {self.pid}")
+            os.kill(self.pid, signal.SIGKILL)

test_runner/regress/test_disk_usage_eviction.py

@@ -110,12 +110,6 @@ class EvictionEnv:
             overrides=(
                 "--pageserver-config-override=disk_usage_based_eviction="
                 + enc.dump_inline_table(disk_usage_config).replace("\n", " "),
-                # Disk usage based eviction runs as a background task.
-                # But pageserver startup delays launch of background tasks for some time, to prioritize initial logical size calculations during startup.
-                # But, initial logical size calculation may not be triggered if safekeepers don't publish new broker messages.
-                # But, we only have a 10-second-timeout in this test.
-                # So, disable the delay for this test.
-                "--pageserver-config-override=background_task_maximum_delay='0s'",
             ),
         )
 

test_runner/regress/test_neon_local_cli.py

@@ -9,18 +9,11 @@ def test_neon_cli_basics(neon_env_builder: NeonEnvBuilder, port_distributor: Por
     try:
         env.neon_cli.start()
         env.neon_cli.create_tenant(tenant_id=env.initial_tenant, set_default=True)
-
-        pg_port = port_distributor.get_port()
-        http_port = port_distributor.get_port()
-        env.neon_cli.endpoint_start(
-            endpoint_id="ep-basic-main", pg_port=pg_port, http_port=http_port
-        )
+        env.neon_cli.endpoint_start(endpoint_id="ep-main", port=port_distributor.get_port())
 
         env.neon_cli.create_branch(new_branch_name="migration_check")
-        pg_port = port_distributor.get_port()
-        http_port = port_distributor.get_port()
         env.neon_cli.endpoint_start(
-            endpoint_id="ep-migration_check", pg_port=pg_port, http_port=http_port
+            endpoint_id="ep-migration_check", port=port_distributor.get_port()
         )
     finally:
         env.neon_cli.stop()

test_runner/regress/test_ondemand_download.py

@@ -58,8 +58,11 @@ def test_ondemand_download_large_rel(
     )
 
     ##### First start, insert secret data and upload it to the remote storage
-    env = neon_env_builder.init_start(
-        initial_tenant_conf={
+    env = neon_env_builder.init_start()
+
+    # Override defaults, to create more layers
+    tenant, _ = env.neon_cli.create_tenant(
+        conf={
             # disable background GC
             "gc_period": "0s",
             "gc_horizon": f"{10 * 1024 ** 3}",  # 10 GB
@@ -72,6 +75,7 @@ def test_ondemand_download_large_rel(
             "compaction_period": "0s",
         }
     )
+    env.initial_tenant = tenant
 
     endpoint = env.endpoints.create_start("main")
 

test_runner/regress/test_pageserver_restarts_under_workload.py

@@ -17,6 +17,12 @@ def test_pageserver_restarts_under_worload(neon_simple_env: NeonEnv, pg_bin: PgB
     n_restarts = 10
     scale = 10
 
+    # the background task may complete the init task delay after finding an
+    # active tenant, but shutdown starts right before Tenant::gc_iteration
+    env.pageserver.allowed_errors.append(
+        r".*Gc failed, retrying in \S+: Cannot run GC iteration on inactive tenant"
+    )
+
     def run_pgbench(connstr: str):
         log.info(f"Start a pgbench workload on pg {connstr}")
         pg_bin.run_capture(["pgbench", "-i", f"-s{scale}", connstr])

test_runner/regress/test_remote_storage.py

@@ -147,12 +147,7 @@ def test_remote_storage_backup_and_restore(
     # listing the remote timelines will fail because of the failpoint,
     # and the tenant will be marked as Broken.
     client.tenant_attach(tenant_id)
-
-    tenant_info = wait_until_tenant_state(pageserver_http, tenant_id, "Broken", 15)
-    assert tenant_info["attachment_status"] == {
-        "slug": "failed",
-        "data": {"reason": "storage-sync-list-remote-timelines"},
-    }
+    wait_until_tenant_state(pageserver_http, tenant_id, "Broken", 15)
 
     # Ensure that even though the tenant is broken, we can't attach it again.
     with pytest.raises(Exception, match=f"tenant {tenant_id} already exists, state: Broken"):
@@ -698,15 +693,15 @@ def test_empty_branch_remote_storage_upload_on_restart(
         f".*POST.* path=/v1/tenant/{env.initial_tenant}/timeline.* request was dropped before completing"
     )
 
-    # index upload is now hitting the failpoint, it should block the shutdown
-    env.pageserver.stop(immediate=True)
+    # index upload is now hitting the failpoint, should not block the shutdown
+    env.pageserver.stop()
 
     timeline_path = (
         Path("tenants") / str(env.initial_tenant) / "timelines" / str(new_branch_timeline_id)
     )
 
     local_metadata = env.repo_dir / timeline_path / "metadata"
-    assert local_metadata.is_file()
+    assert local_metadata.is_file(), "timeout cancelled timeline branching, not the upload"
 
     assert isinstance(env.remote_storage, LocalFsStorage)
     new_branch_on_remote_storage = env.remote_storage.root / timeline_path

test_runner/regress/test_tenant_detach.py

@@ -59,13 +59,6 @@ def test_tenant_reattach(
     # create new nenant
     tenant_id, timeline_id = env.neon_cli.create_tenant()
 
-    # Attempts to connect from compute to pageserver while the tenant is
-    # temporarily detached produces these errors in the pageserver log.
-    env.pageserver.allowed_errors.append(f".*Tenant {tenant_id} not found.*")
-    env.pageserver.allowed_errors.append(
-        f".*Tenant {tenant_id} will not become active\\. Current state: Stopping.*"
-    )
-
     with env.endpoints.create_start("main", tenant_id=tenant_id) as endpoint:
         with endpoint.cursor() as cur:
             cur.execute("CREATE TABLE t(key int primary key, value text)")
@@ -230,6 +223,13 @@ def test_tenant_reattach_while_busy(
     )
     env = neon_env_builder.init_start()
 
+    # Attempts to connect from compute to pageserver while the tenant is
+    # temporarily detached produces these errors in the pageserver log.
+    env.pageserver.allowed_errors.append(".*Tenant .* not found.*")
+    env.pageserver.allowed_errors.append(
+        ".*Tenant .* will not become active\\. Current state: Stopping.*"
+    )
+
     pageserver_http = env.pageserver.http_client()
 
     # create new nenant
@@ -238,13 +238,6 @@ def test_tenant_reattach_while_busy(
         conf={"checkpoint_distance": "100000"}
     )
 
-    # Attempts to connect from compute to pageserver while the tenant is
-    # temporarily detached produces these errors in the pageserver log.
-    env.pageserver.allowed_errors.append(f".*Tenant {tenant_id} not found.*")
-    env.pageserver.allowed_errors.append(
-        f".*Tenant {tenant_id} will not become active\\. Current state: Stopping.*"
-    )
-
     endpoint = env.endpoints.create_start("main", tenant_id=tenant_id)
 
     cur = endpoint.connect().cursor()
@@ -282,13 +275,6 @@ def test_tenant_detach_smoke(neon_env_builder: NeonEnvBuilder):
     # create new nenant
     tenant_id, timeline_id = env.neon_cli.create_tenant()
 
-    # Attempts to connect from compute to pageserver while the tenant is
-    # temporarily detached produces these errors in the pageserver log.
-    env.pageserver.allowed_errors.append(f".*Tenant {tenant_id} not found.*")
-    env.pageserver.allowed_errors.append(
-        f".*Tenant {tenant_id} will not become active\\. Current state: Stopping.*"
-    )
-
     # assert tenant exists on disk
     assert (env.repo_dir / "tenants" / str(tenant_id)).exists()
 
@@ -350,13 +336,6 @@ def test_tenant_detach_ignored_tenant(neon_simple_env: NeonEnv):
     # create a new tenant
     tenant_id, _ = env.neon_cli.create_tenant()
 
-    # Attempts to connect from compute to pageserver while the tenant is
-    # temporarily detached produces these errors in the pageserver log.
-    env.pageserver.allowed_errors.append(f".*Tenant {tenant_id} not found.*")
-    env.pageserver.allowed_errors.append(
-        f".*Tenant {tenant_id} will not become active\\. Current state: Stopping.*"
-    )
-
     # assert tenant exists on disk
     assert (env.repo_dir / "tenants" / str(tenant_id)).exists()
 
@@ -406,13 +385,6 @@ def test_tenant_detach_regular_tenant(neon_simple_env: NeonEnv):
     # create a new tenant
     tenant_id, _ = env.neon_cli.create_tenant()
 
-    # Attempts to connect from compute to pageserver while the tenant is
-    # temporarily detached produces these errors in the pageserver log.
-    env.pageserver.allowed_errors.append(f".*Tenant {tenant_id} not found.*")
-    env.pageserver.allowed_errors.append(
-        f".*Tenant {tenant_id} will not become active\\. Current state: Stopping.*"
-    )
-
     # assert tenant exists on disk
     assert (env.repo_dir / "tenants" / str(tenant_id)).exists()
 
@@ -427,7 +399,6 @@ def test_tenant_detach_regular_tenant(neon_simple_env: NeonEnv):
 
     log.info("detaching regular tenant with detach ignored flag")
     client.tenant_detach(tenant_id, True)
-
     log.info("regular tenant detached without error")
 
     # check that nothing is left on disk for deleted tenant
@@ -461,13 +432,6 @@ def test_detach_while_attaching(
     tenant_id = TenantId(endpoint.safe_psql("show neon.tenant_id")[0][0])
     timeline_id = TimelineId(endpoint.safe_psql("show neon.timeline_id")[0][0])
 
-    # Attempts to connect from compute to pageserver while the tenant is
-    # temporarily detached produces these errors in the pageserver log.
-    env.pageserver.allowed_errors.append(f".*Tenant {tenant_id} not found.*")
-    env.pageserver.allowed_errors.append(
-        f".*Tenant {tenant_id} will not become active\\. Current state: Stopping.*"
-    )
-
     # Create table, and insert some rows. Make it big enough that it doesn't fit in
     # shared_buffers, otherwise the SELECT after restart will just return answer
     # from shared_buffers without hitting the page server, which defeats the point
@@ -532,7 +496,7 @@ def test_ignored_tenant_reattach(
 ):
     neon_env_builder.enable_remote_storage(
         remote_storage_kind=remote_storage_kind,
-        test_name="test_ignored_tenant_reattach",
+        test_name="test_remote_storage_backup_and_restore",
     )
     env = neon_env_builder.init_start()
     pageserver_http = env.pageserver.http_client()
@@ -613,13 +577,6 @@ def test_ignored_tenant_download_missing_layers(
     tenant_id = TenantId(endpoint.safe_psql("show neon.tenant_id")[0][0])
     timeline_id = TimelineId(endpoint.safe_psql("show neon.timeline_id")[0][0])
 
-    # Attempts to connect from compute to pageserver while the tenant is
-    # temporarily detached produces these errors in the pageserver log.
-    env.pageserver.allowed_errors.append(f".*Tenant {tenant_id} not found.*")
-    env.pageserver.allowed_errors.append(
-        f".*Tenant {tenant_id} will not become active\\. Current state: Stopping.*"
-    )
-
     data_id = 1
     data_secret = "very secret secret"
     insert_test_data(pageserver_http, tenant_id, timeline_id, data_id, data_secret, endpoint)
@@ -679,13 +636,6 @@ def test_ignored_tenant_stays_broken_without_metadata(
     tenant_id = TenantId(endpoint.safe_psql("show neon.tenant_id")[0][0])
     timeline_id = TimelineId(endpoint.safe_psql("show neon.timeline_id")[0][0])
 
-    # Attempts to connect from compute to pageserver while the tenant is
-    # temporarily detached produces these errors in the pageserver log.
-    env.pageserver.allowed_errors.append(f".*Tenant {tenant_id} not found.*")
-    env.pageserver.allowed_errors.append(
-        f".*Tenant {tenant_id} will not become active\\. Current state: Broken.*"
-    )
-
     # ignore the tenant and remove its metadata
     pageserver_http.tenant_ignore(tenant_id)
     tenant_timeline_dir = env.repo_dir / "tenants" / str(tenant_id) / "timelines" / str(timeline_id)
@@ -722,13 +672,6 @@ def test_load_attach_negatives(
 
     tenant_id = TenantId(endpoint.safe_psql("show neon.tenant_id")[0][0])
 
-    # Attempts to connect from compute to pageserver while the tenant is
-    # temporarily detached produces these errors in the pageserver log.
-    env.pageserver.allowed_errors.append(f".*Tenant {tenant_id} not found.*")
-    env.pageserver.allowed_errors.append(
-        f".*Tenant {tenant_id} will not become active\\. Current state: Stopping.*"
-    )
-
     env.pageserver.allowed_errors.append(".*tenant .*? already exists, state:.*")
     with pytest.raises(
         expected_exception=PageserverApiException,
@@ -771,13 +714,6 @@ def test_ignore_while_attaching(
     tenant_id = TenantId(endpoint.safe_psql("show neon.tenant_id")[0][0])
     timeline_id = TimelineId(endpoint.safe_psql("show neon.timeline_id")[0][0])
 
-    # Attempts to connect from compute to pageserver while the tenant is
-    # temporarily detached produces these errors in the pageserver log.
-    env.pageserver.allowed_errors.append(f".*Tenant {tenant_id} not found.*")
-    env.pageserver.allowed_errors.append(
-        f".*Tenant {tenant_id} will not become active\\. Current state: Stopping.*"
-    )
-
     data_id = 1
     data_secret = "very secret secret"
     insert_test_data(pageserver_http, tenant_id, timeline_id, data_id, data_secret, endpoint)

test_runner/regress/test_tenant_size.py

@@ -318,7 +318,7 @@ def test_only_heads_within_horizon(neon_simple_env: NeonEnv, test_output_dir: Pa
 
 
 def test_single_branch_get_tenant_size_grows(
-    neon_env_builder: NeonEnvBuilder, test_output_dir: Path, pg_version: PgVersion
+    neon_env_builder: NeonEnvBuilder, test_output_dir: Path
 ):
     """
     Operate on single branch reading the tenants size after each transaction.
@@ -333,13 +333,6 @@ def test_single_branch_get_tenant_size_grows(
     # that there next_gc_cutoff could be smaller than initdb_lsn, which will
     # obviously lead to issues when calculating the size.
     gc_horizon = 0x38000
-
-    # it's a bit of a hack, but different versions of postgres have different
-    # amount of WAL generated for the same amount of data. so we need to
-    # adjust the gc_horizon accordingly.
-    if pg_version == PgVersion.V14:
-        gc_horizon = 0x40000
-
     neon_env_builder.pageserver_config_override = f"tenant_config={{compaction_period='0s', gc_period='0s', pitr_interval='0sec', gc_horizon={gc_horizon}}}"
 
     env = neon_env_builder.init_start()

test_runner/regress/test_tenants.py

@@ -267,7 +267,6 @@ def test_pageserver_metrics_removed_after_detach(
                 cur.execute("INSERT INTO t SELECT generate_series(1,100000), 'payload'")
                 cur.execute("SELECT sum(key) FROM t")
                 assert cur.fetchone() == (5000050000,)
-        endpoint.stop()
 
     def get_ps_metric_samples_for_tenant(tenant_id: TenantId) -> List[Sample]:
         ps_metrics = env.pageserver.http_client().get_metrics()