From 699f20081171c716ea35e1549294888f15fe6caf Mon Sep 17 00:00:00 2001
From: Arseny Sher <sher-ars@yandex.ru>
Date: Mon, 20 Mar 2023 12:49:56 +0400
Subject: [PATCH 01/19] Send error context chain to the client when Copy stream
 errors.

---
 libs/postgres_backend/src/lib.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libs/postgres_backend/src/lib.rs b/libs/postgres_backend/src/lib.rs
index 4d88b958f0..60932a5950 100644
--- a/libs/postgres_backend/src/lib.rs
+++ b/libs/postgres_backend/src/lib.rs
@@ -767,7 +767,7 @@ impl<IO: AsyncRead + AsyncWrite + Unpin> PostgresBackend<IO> {
 
         let err_to_send_and_errcode = match &end {
             ServerInitiated(_) => Some((end.to_string(), SQLSTATE_SUCCESSFUL_COMPLETION)),
-            Other(_) => Some((end.to_string(), SQLSTATE_INTERNAL_ERROR)),
+            Other(_) => Some((format!("{end:#}"), SQLSTATE_INTERNAL_ERROR)),
             // Note: CopyFail in duplex copy is somewhat unexpected (at least to
             // PG walsender; evidently and per my docs reading client should
             // finish it with CopyDone). It is not a problem to recover from it

From 5a786fab4f2877bc94f47ff2e369efd081baf846 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki@neon.tech>
Date: Mon, 20 Mar 2023 20:51:32 +0200
Subject: [PATCH 02/19] Remove duplicated global variables in neon extension.

Walproposer used to live in the backend, while pagestore_smgr was an
extension. But now that both are part of the neon extension,
walproposer can access the same 'neon_tenant' and 'neon_timeline'
variables as the pageserver_smgr code.
---
 pgxn/neon/libpagestore.c |  8 ++------
 pgxn/neon/neon.h         |  5 +++++
 pgxn/neon/walproposer.c  | 28 ++++++++++++----------------
 pgxn/neon/walproposer.h  |  4 ----
 4 files changed, 19 insertions(+), 26 deletions(-)

diff --git a/pgxn/neon/libpagestore.c b/pgxn/neon/libpagestore.c
index 3fe6d38251..a3f34247bb 100644
--- a/pgxn/neon/libpagestore.c
+++ b/pgxn/neon/libpagestore.c
@@ -536,10 +536,6 @@ pg_init_libpagestore(void)
 	/* substitute password in pageserver_connstring */
 	page_server_connstring = substitute_pageserver_password(page_server_connstring_raw);
 
-	/* Is there more correct way to pass CustomGUC to postgres code? */
-	neon_timeline_walproposer = neon_timeline;
-	neon_tenant_walproposer = neon_tenant;
-
 	/* retrieve the token for Safekeeper, if present */
 	if (safekeeper_token_env != NULL) {
 		if (safekeeper_token_env[0] != '$') {
@@ -548,8 +544,8 @@ pg_init_libpagestore(void)
 							errmsg("expected safekeeper auth token environment variable's name starting with $ but found: %s",
 								   safekeeper_token_env)));
 		}
-		neon_safekeeper_token_walproposer = getenv(&safekeeper_token_env[1]);
-		if (!neon_safekeeper_token_walproposer) {
+		neon_safekeeper_token = getenv(&safekeeper_token_env[1]);
+		if (!neon_safekeeper_token) {
 			ereport(ERROR,
 					(errcode(ERRCODE_CONNECTION_EXCEPTION),
 							errmsg("cannot get safekeeper auth token, environment variable %s is not set",
diff --git a/pgxn/neon/neon.h b/pgxn/neon/neon.h
index 6b9ba372fb..da441b783d 100644
--- a/pgxn/neon/neon.h
+++ b/pgxn/neon/neon.h
@@ -12,6 +12,11 @@
 #ifndef NEON_H
 #define NEON_H
 
+/* GUCs */
+extern char *neon_safekeeper_token;
+extern char *neon_timeline;
+extern char *neon_tenant;
+
 extern void pg_init_libpagestore(void);
 extern void pg_init_walproposer(void);
 
diff --git a/pgxn/neon/walproposer.c b/pgxn/neon/walproposer.c
index bf8bb02493..aef2465e54 100644
--- a/pgxn/neon/walproposer.c
+++ b/pgxn/neon/walproposer.c
@@ -78,10 +78,6 @@ int			wal_acceptor_reconnect_timeout;
 int			wal_acceptor_connection_timeout;
 bool		am_wal_proposer;
 
-char	   *neon_timeline_walproposer = NULL;
-char	   *neon_tenant_walproposer = NULL;
-char	   *neon_safekeeper_token_walproposer = NULL;
-
 #define WAL_PROPOSER_SLOT_NAME "wal_proposer_slot"
 
 static int	n_safekeepers = 0;
@@ -514,15 +510,15 @@ WalProposerInit(XLogRecPtr flushRecPtr, uint64 systemId)
 			Safekeeper *sk = &safekeeper[n_safekeepers];
 			int written = 0;
 
-			if (neon_safekeeper_token_walproposer != NULL) {
+			if (neon_safekeeper_token != NULL) {
 				written = snprintf((char *) &sk->conninfo, MAXCONNINFO,
 								   "host=%s port=%s password=%s dbname=replication options='-c timeline_id=%s tenant_id=%s'",
-								   sk->host, sk->port, neon_safekeeper_token_walproposer, neon_timeline_walproposer,
-								   neon_tenant_walproposer);
+								   sk->host, sk->port, neon_safekeeper_token, neon_timeline,
+								   neon_tenant);
 			} else {
 				written = snprintf((char *) &sk->conninfo, MAXCONNINFO,
 								   "host=%s port=%s dbname=replication options='-c timeline_id=%s tenant_id=%s'",
-								   sk->host, sk->port, neon_timeline_walproposer, neon_tenant_walproposer);
+								   sk->host, sk->port, neon_timeline, neon_tenant);
 			}
 
 			if (written > MAXCONNINFO || written < 0)
@@ -550,16 +546,16 @@ WalProposerInit(XLogRecPtr flushRecPtr, uint64 systemId)
 	greetRequest.pgVersion = PG_VERSION_NUM;
 	pg_strong_random(&greetRequest.proposerId, sizeof(greetRequest.proposerId));
 	greetRequest.systemId = systemId;
-	if (!neon_timeline_walproposer)
+	if (!neon_timeline)
 		elog(FATAL, "neon.timeline_id is not provided");
-	if (*neon_timeline_walproposer != '\0' &&
-		!HexDecodeString(greetRequest.timeline_id, neon_timeline_walproposer, 16))
-		elog(FATAL, "Could not parse neon.timeline_id, %s", neon_timeline_walproposer);
-	if (!neon_tenant_walproposer)
+	if (*neon_timeline != '\0' &&
+		!HexDecodeString(greetRequest.timeline_id, neon_timeline, 16))
+		elog(FATAL, "Could not parse neon.timeline_id, %s", neon_timeline);
+	if (!neon_tenant)
 		elog(FATAL, "neon.tenant_id is not provided");
-	if (*neon_tenant_walproposer != '\0' &&
-		!HexDecodeString(greetRequest.tenant_id, neon_tenant_walproposer, 16))
-		elog(FATAL, "Could not parse neon.tenant_id, %s", neon_tenant_walproposer);
+	if (*neon_tenant != '\0' &&
+		!HexDecodeString(greetRequest.tenant_id, neon_tenant, 16))
+		elog(FATAL, "Could not parse neon.tenant_id, %s", neon_tenant);
 
 #if PG_VERSION_NUM >= 150000
 	/* FIXME don't use hardcoded timeline id */
diff --git a/pgxn/neon/walproposer.h b/pgxn/neon/walproposer.h
index 1abaab2cc6..357d6378f8 100644
--- a/pgxn/neon/walproposer.h
+++ b/pgxn/neon/walproposer.h
@@ -39,10 +39,6 @@ typedef struct WalProposerConn WalProposerConn;
 struct WalMessage;
 typedef struct WalMessage WalMessage;
 
-extern char *neon_timeline_walproposer;
-extern char *neon_tenant_walproposer;
-extern char *neon_safekeeper_token_walproposer;
-
 /* Possible return values from ReadPGAsync */
 typedef enum
 {

From 299db9d0288d28bf6b8205140ce443c5c54e215d Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki@neon.tech>
Date: Mon, 20 Mar 2023 20:51:36 +0200
Subject: [PATCH 03/19] Simplify and clean up the $NEON_AUTH_TOKEN stuff in
 compute

- Remove the neon.safekeeper_token_env GUC. It was used to set the
  name of an environment variable, which was then used in pageserver
  and safekeeper connection strings to in place of the
  password. Instead, always look up the environment variable called
  NEON_AUTH_TOKEN. That's what neon.safekeeper_token_env was always
  set to in practice, and I don't see the need for the extra level of
  indirection or configurability.

- Instead of substituting $NEON_AUTH_TOKEN in the connection strings,
  pass $NEON_AUTH_TOKEN "out-of-band" as the password, when we connect
  to the pageserver or safekeepers. That's simpler.

- Also use the password from $NEON_AUTH_TOKEN in compute_ctl, when it
  connects to the pageserver to get the "base backup".
---
 compute_tools/src/compute.rs |  18 +++-
 control_plane/src/compute.rs |  28 ++----
 docs/authentication.md       |  26 +++---
 pgxn/neon/libpagestore.c     | 173 ++++++++---------------------------
 pgxn/neon/libpqwalproposer.c |  31 ++++++-
 pgxn/neon/neon.h             |   2 +-
 pgxn/neon/pagestore_smgr.c   |   8 --
 pgxn/neon/walproposer.c      |  16 +---
 pgxn/neon/walproposer.h      |   2 +-
 9 files changed, 112 insertions(+), 192 deletions(-)

diff --git a/compute_tools/src/compute.rs b/compute_tools/src/compute.rs
index 8ceef44d61..09272262de 100644
--- a/compute_tools/src/compute.rs
+++ b/compute_tools/src/compute.rs
@@ -18,6 +18,7 @@ use std::fs;
 use std::os::unix::fs::PermissionsExt;
 use std::path::Path;
 use std::process::{Command, Stdio};
+use std::str::FromStr;
 use std::sync::atomic::{AtomicU64, Ordering};
 use std::sync::RwLock;
 
@@ -126,7 +127,22 @@ impl ComputeNode {
     fn get_basebackup(&self, lsn: &str) -> Result<()> {
         let start_time = Utc::now();
 
-        let mut client = Client::connect(&self.pageserver_connstr, NoTls)?;
+        let mut config = postgres::Config::from_str(&self.pageserver_connstr)?;
+
+        // Like in the neon extension, if the $NEON_AUTH_TOKEN env variable is
+        // set, use it as the password when connecting to pageserver.
+        //
+        // Note: this overrides any password set in the connection string.
+        match std::env::var("NEON_AUTH_TOKEN") {
+            Ok(val) => {
+                info!("Got pageserver auth token from NEON_AUTH_TOKEN env variable");
+                config.password(val);
+            }
+            Err(std::env::VarError::NotPresent) => info!("NEON_AUTH_TOKEN env variable not set"),
+            Err(e) => info!("could not parse NEON_AUTH_TOKEN env variable: {}", e),
+        };
+
+        let mut client = config.connect(NoTls)?;
         let basebackup_cmd = match lsn {
             "0/0" => format!("basebackup {} {}", &self.tenant, &self.timeline), // First start of the compute
             _ => format!("basebackup {} {} {}", &self.tenant, &self.timeline, lsn),
diff --git a/control_plane/src/compute.rs b/control_plane/src/compute.rs
index 094d2add8d..730cacf40b 100644
--- a/control_plane/src/compute.rs
+++ b/control_plane/src/compute.rs
@@ -11,7 +11,6 @@ use std::sync::Arc;
 use std::time::Duration;
 
 use anyhow::{Context, Result};
-use postgres_backend::AuthType;
 use utils::{
     id::{TenantId, TimelineId},
     lsn::Lsn,
@@ -97,7 +96,7 @@ impl ComputeControlPlane {
         });
 
         node.create_pgdata()?;
-        node.setup_pg_conf(self.env.pageserver.pg_auth_type)?;
+        node.setup_pg_conf()?;
 
         self.nodes
             .insert((tenant_id, node.name.clone()), Arc::clone(&node));
@@ -278,7 +277,7 @@ impl PostgresNode {
 
     // Write postgresql.conf with default configuration
     // and PG_VERSION file to the data directory of a new node.
-    fn setup_pg_conf(&self, auth_type: AuthType) -> Result<()> {
+    fn setup_pg_conf(&self) -> Result<()> {
         let mut conf = PostgresConf::new();
         conf.append("max_wal_senders", "10");
         conf.append("wal_log_hints", "off");
@@ -302,29 +301,12 @@ impl PostgresNode {
             let config = &self.pageserver.pg_connection_config;
             let (host, port) = (config.host(), config.port());
 
-            // Set up authentication
-            //
-            // $NEON_AUTH_TOKEN will be replaced with value from environment
-            // variable during compute pg startup. It is done this way because
-            // otherwise user will be able to retrieve the value using SHOW
-            // command or pg_settings
-            let password = if let AuthType::NeonJWT = auth_type {
-                "$NEON_AUTH_TOKEN"
-            } else {
-                ""
-            };
-            // NOTE avoiding spaces in connection string, because it is less error prone if we forward it somewhere.
-            // Also note that not all parameters are supported here. Because in compute we substitute $NEON_AUTH_TOKEN
-            // We parse this string and build it back with token from env var, and for simplicity rebuild
-            // uses only needed variables namely host, port, user, password.
-            format!("postgresql://no_user:{password}@{host}:{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);
-        if let AuthType::NeonJWT = auth_type {
-            conf.append("neon.safekeeper_token_env", "$NEON_AUTH_TOKEN");
-        }
         conf.append("neon.tenant_id", &self.tenant_id.to_string());
         conf.append("neon.timeline_id", &self.timeline_id.to_string());
         if let Some(lsn) = self.lsn {
@@ -447,6 +429,8 @@ impl PostgresNode {
             "DYLD_LIBRARY_PATH",
             self.env.pg_lib_dir(self.pg_version)?.to_str().unwrap(),
         );
+
+        // Pass authentication token used for the connections to pageserver and safekeepers
         if let Some(token) = auth_token {
             cmd.env("NEON_AUTH_TOKEN", token);
         }
diff --git a/docs/authentication.md b/docs/authentication.md
index dc402d1bca..f768b04c5b 100644
--- a/docs/authentication.md
+++ b/docs/authentication.md
@@ -106,20 +106,22 @@ Their authentication is just plain PostgreSQL authentication and out of scope fo
 There is no administrative API except those provided by PostgreSQL.
 
 #### Outgoing connections
-Compute connects to Pageserver for getting pages.
-The connection string is configured by the `neon.pageserver_connstring` PostgreSQL GUC, e.g. `postgresql://no_user:$NEON_AUTH_TOKEN@localhost:15028`.
-The environment variable inside the connection string is substituted with
-the JWT token.
+Compute connects to Pageserver for getting pages. The connection string is
+configured by the `neon.pageserver_connstring` PostgreSQL GUC,
+e.g. `postgresql://no_user@localhost:15028`. If the `$NEON_AUTH_TOKEN`
+environment variable is set, it is used as the password for the connection. (The
+pageserver uses JWT tokens for authentication, so the password is really a
+token.)
 
-Compute connects to Safekeepers to write and commit data.
-The token is the same for all safekeepers.
-It's stored in an environment variable, whose name is configured
-by the `neon.safekeeper_token_env` PostgreSQL GUC.
-If the GUC is unset, no token is passed.
+Compute connects to Safekeepers to write and commit data. The list of safekeeper
+addresses is given in the `neon.safekeepers` GUC. The connections to the
+safekeepers take the password from the `$NEON_AUTH_TOKEN` environment
+variable, if set.
 
-Note that both tokens can be (and typically are) the same;
-the scope is the tenant and the token is usually passed through the
-`$NEON_AUTH_TOKEN` environment variable.
+The `compute_ctl` binary that runs before the PostgreSQL server, and launches
+PostgreSQL, also makes a connection to the pageserver. It uses it to fetch the
+initial "base backup" dump, to initialize the PostgreSQL data directory. It also
+uses `$NEON_AUTH_TOKEN` as the password for the connection.
 
 ### Pageserver
 #### Overview
diff --git a/pgxn/neon/libpagestore.c b/pgxn/neon/libpagestore.c
index a3f34247bb..c44e8fcda5 100644
--- a/pgxn/neon/libpagestore.c
+++ b/pgxn/neon/libpagestore.c
@@ -46,8 +46,12 @@ PGconn	   *pageserver_conn = NULL;
  */
 WaitEventSet *pageserver_conn_wes = NULL;
 
-char	   *page_server_connstring_raw;
-char	   *safekeeper_token_env;
+/* GUCs */
+char	   *neon_timeline;
+char	   *neon_tenant;
+int32		max_cluster_size;
+char	   *page_server_connstring;
+char	   *neon_auth_token;
 
 int			n_unflushed_requests = 0;
 int			flush_every_n_requests = 8;
@@ -60,10 +64,37 @@ pageserver_connect(int elevel)
 {
 	char	   *query;
 	int			ret;
+	const char *keywords[3];
+	const char *values[3];
+	int			n;
 
 	Assert(!connected);
 
-	pageserver_conn = PQconnectdb(page_server_connstring);
+	/*
+	 * Connect using the connection string we got from the
+	 * neon.pageserver_connstring GUC. If the NEON_AUTH_TOKEN environment
+	 * variable was set, use that as the password.
+	 *
+	 * The connection options are parsed in the order they're given, so
+	 * when we set the password before the connection string, the
+	 * connection string can override the password from the env variable.
+	 * Seems useful, although we don't currently use that capability
+	 * anywhere.
+	 */
+	n = 0;
+	if (neon_auth_token)
+	{
+		keywords[n] = "password";
+		values[n] = neon_auth_token;
+		n++;
+	}
+	keywords[n] = "dbname";
+	values[n] = page_server_connstring;
+	n++;
+	keywords[n] = NULL;
+	values[n] = NULL;
+	n++;
+	pageserver_conn = PQconnectdbParams(keywords, values, 1);
 
 	if (PQstatus(pageserver_conn) == CONNECTION_BAD)
 	{
@@ -125,7 +156,7 @@ pageserver_connect(int elevel)
 		}
 	}
 
-	neon_log(LOG, "libpagestore: connected to '%s'", page_server_connstring_raw);
+	neon_log(LOG, "libpagestore: connected to '%s'", page_server_connstring);
 
 	connected = true;
 	return true;
@@ -354,105 +385,6 @@ check_neon_id(char **newval, void **extra, GucSource source)
 	return **newval == '\0' || HexDecodeString(id, *newval, 16);
 }
 
-static char *
-substitute_pageserver_password(const char *page_server_connstring_raw)
-{
-	char	   *host = NULL;
-	char	   *port = NULL;
-	char	   *user = NULL;
-	char	   *auth_token = NULL;
-	char	   *err = NULL;
-	char	   *page_server_connstring = NULL;
-	PQconninfoOption *conn_options;
-	PQconninfoOption *conn_option;
-	MemoryContext oldcontext;
-
-	/*
-	 * Here we substitute password in connection string with an environment
-	 * variable. To simplify things we construct a connection string back with
-	 * only known options. In particular: host port user and password. We do
-	 * not currently use other options and constructing full connstring in an
-	 * URI shape is quite messy.
-	 */
-
-	if (page_server_connstring_raw == NULL || page_server_connstring_raw[0] == '\0')
-		return NULL;
-
-	/* extract the auth token from the connection string */
-	conn_options = PQconninfoParse(page_server_connstring_raw, &err);
-	if (conn_options == NULL)
-	{
-		/* The error string is malloc'd, so we must free it explicitly */
-		char	   *errcopy = err ? pstrdup(err) : "out of memory";
-
-		PQfreemem(err);
-		ereport(ERROR,
-				(errcode(ERRCODE_SYNTAX_ERROR),
-				 errmsg("invalid connection string syntax: %s", errcopy)));
-	}
-
-	/*
-	 * Trying to populate pageserver connection string with auth token from
-	 * environment. We are looking for password in with placeholder value like
-	 * $ENV_VAR_NAME, so if password field is present and starts with $ we try
-	 * to fetch environment variable value and fail loudly if it is not set.
-	 */
-	for (conn_option = conn_options; conn_option->keyword != NULL; conn_option++)
-	{
-		if (strcmp(conn_option->keyword, "host") == 0)
-		{
-			if (conn_option->val != NULL && conn_option->val[0] != '\0')
-				host = conn_option->val;
-		}
-		else if (strcmp(conn_option->keyword, "port") == 0)
-		{
-			if (conn_option->val != NULL && conn_option->val[0] != '\0')
-				port = conn_option->val;
-		}
-		else if (strcmp(conn_option->keyword, "user") == 0)
-		{
-			if (conn_option->val != NULL && conn_option->val[0] != '\0')
-				user = conn_option->val;
-		}
-		else if (strcmp(conn_option->keyword, "password") == 0)
-		{
-			if (conn_option->val != NULL && conn_option->val[0] != '\0')
-			{
-				/* ensure that this is a template */
-				if (strncmp(conn_option->val, "$", 1) != 0)
-					ereport(ERROR,
-							(errcode(ERRCODE_CONNECTION_EXCEPTION),
-							 errmsg("expected placeholder value in pageserver password starting from $ but found: %s", &conn_option->val[1])));
-
-				neon_log(LOG, "found auth token placeholder in pageserver conn string '%s'", &conn_option->val[1]);
-				auth_token = getenv(&conn_option->val[1]);
-				if (!auth_token)
-				{
-					ereport(ERROR,
-							(errcode(ERRCODE_CONNECTION_EXCEPTION),
-							 errmsg("cannot get auth token, environment variable %s is not set", &conn_option->val[1])));
-				}
-				else
-				{
-					neon_log(LOG, "using auth token from environment passed via env");
-				}
-			}
-		}
-	}
-
-	/*
-	 * allocate connection string in TopMemoryContext to make sure it is not
-	 * freed
-	 */
-	oldcontext = CurrentMemoryContext;
-	MemoryContextSwitchTo(TopMemoryContext);
-	page_server_connstring = psprintf("postgresql://%s:%s@%s:%s", user, auth_token ? auth_token : "", host, port);
-	MemoryContextSwitchTo(oldcontext);
-
-	PQconninfoFree(conn_options);
-	return page_server_connstring;
-}
-
 /*
  * Module initialization function
  */
@@ -462,21 +394,12 @@ pg_init_libpagestore(void)
 	DefineCustomStringVariable("neon.pageserver_connstring",
 							   "connection string to the page server",
 							   NULL,
-							   &page_server_connstring_raw,
+							   &page_server_connstring,
 							   "",
 							   PGC_POSTMASTER,
 							   0,	/* no flags required */
 							   NULL, NULL, NULL);
 
-    DefineCustomStringVariable("neon.safekeeper_token_env",
-                               "the environment variable containing JWT token for authentication with Safekeepers, the convention is to either unset or set to $NEON_AUTH_TOKEN",
-                               NULL,
-                               &safekeeper_token_env,
-                               NULL,
-                               PGC_POSTMASTER,
-                               0,	/* no flags required */
-                               NULL, NULL, NULL);
-
 	DefineCustomStringVariable("neon.timeline_id",
 							   "Neon timeline_id the server is running on",
 							   NULL,
@@ -533,26 +456,10 @@ pg_init_libpagestore(void)
 	neon_log(PageStoreTrace, "libpagestore already loaded");
 	page_server = &api;
 
-	/* substitute password in pageserver_connstring */
-	page_server_connstring = substitute_pageserver_password(page_server_connstring_raw);
-
-	/* retrieve the token for Safekeeper, if present */
-	if (safekeeper_token_env != NULL) {
-		if (safekeeper_token_env[0] != '$') {
-			ereport(ERROR,
-					(errcode(ERRCODE_CONNECTION_EXCEPTION),
-							errmsg("expected safekeeper auth token environment variable's name starting with $ but found: %s",
-								   safekeeper_token_env)));
-		}
-		neon_safekeeper_token = getenv(&safekeeper_token_env[1]);
-		if (!neon_safekeeper_token) {
-			ereport(ERROR,
-					(errcode(ERRCODE_CONNECTION_EXCEPTION),
-							errmsg("cannot get safekeeper auth token, environment variable %s is not set",
-								   &safekeeper_token_env[1])));
-		}
-		neon_log(LOG, "using safekeeper auth token from environment variable");
-	}
+	/* Retrieve the auth token to use when connecting to pageserver and safekeepers */
+	neon_auth_token = getenv("NEON_AUTH_TOKEN");
+	if (neon_auth_token)
+		neon_log(LOG, "using storage auth token from NEON_AUTH_TOKEN environment variable");
 
 	if (page_server_connstring && page_server_connstring[0])
 	{
diff --git a/pgxn/neon/libpqwalproposer.c b/pgxn/neon/libpqwalproposer.c
index 6b1e6a8bcc..9b6175a621 100644
--- a/pgxn/neon/libpqwalproposer.c
+++ b/pgxn/neon/libpqwalproposer.c
@@ -51,12 +51,39 @@ walprop_status(WalProposerConn *conn)
 }
 
 WalProposerConn *
-walprop_connect_start(char *conninfo)
+walprop_connect_start(char *conninfo, char *password)
 {
 	WalProposerConn *conn;
 	PGconn	   *pg_conn;
+	const char *keywords[3];
+	const char *values[3];
+	int			n;
 
-	pg_conn = PQconnectStart(conninfo);
+	/*
+	 * Connect using the given connection string. If the
+	 * NEON_AUTH_TOKEN environment variable was set, use that as
+	 * the password.
+	 *
+	 * The connection options are parsed in the order they're given, so
+	 * when we set the password before the connection string, the
+	 * connection string can override the password from the env variable.
+	 * Seems useful, although we don't currently use that capability
+	 * anywhere.
+	 */
+	n = 0;
+	if (password)
+	{
+		keywords[n] = "password";
+		values[n] = neon_auth_token;
+		n++;
+	}
+	keywords[n] = "dbname";
+	values[n] = conninfo;
+	n++;
+	keywords[n] = NULL;
+	values[n] = NULL;
+	n++;
+	pg_conn = PQconnectStartParams(keywords, values, 1);
 
 	/*
 	 * Allocation of a PQconn can fail, and will return NULL. We want to fully
diff --git a/pgxn/neon/neon.h b/pgxn/neon/neon.h
index da441b783d..3eac8f4570 100644
--- a/pgxn/neon/neon.h
+++ b/pgxn/neon/neon.h
@@ -13,7 +13,7 @@
 #define NEON_H
 
 /* GUCs */
-extern char *neon_safekeeper_token;
+extern char *neon_auth_token;
 extern char *neon_timeline;
 extern char *neon_tenant;
 
diff --git a/pgxn/neon/pagestore_smgr.c b/pgxn/neon/pagestore_smgr.c
index ca91112195..5b30641856 100644
--- a/pgxn/neon/pagestore_smgr.c
+++ b/pgxn/neon/pagestore_smgr.c
@@ -92,14 +92,6 @@ const int	SmgrTrace = DEBUG5;
 
 page_server_api *page_server;
 
-/* GUCs */
-char	   *page_server_connstring;
-
-/*with substituted password*/
-char	   *neon_timeline;
-char	   *neon_tenant;
-int32		max_cluster_size;
-
 /* unlogged relation build states */
 typedef enum
 {
diff --git a/pgxn/neon/walproposer.c b/pgxn/neon/walproposer.c
index aef2465e54..b0b2a23e3c 100644
--- a/pgxn/neon/walproposer.c
+++ b/pgxn/neon/walproposer.c
@@ -510,17 +510,9 @@ WalProposerInit(XLogRecPtr flushRecPtr, uint64 systemId)
 			Safekeeper *sk = &safekeeper[n_safekeepers];
 			int written = 0;
 
-			if (neon_safekeeper_token != NULL) {
-				written = snprintf((char *) &sk->conninfo, MAXCONNINFO,
-								   "host=%s port=%s password=%s dbname=replication options='-c timeline_id=%s tenant_id=%s'",
-								   sk->host, sk->port, neon_safekeeper_token, neon_timeline,
-								   neon_tenant);
-			} else {
-				written = snprintf((char *) &sk->conninfo, MAXCONNINFO,
-								   "host=%s port=%s dbname=replication options='-c timeline_id=%s tenant_id=%s'",
-								   sk->host, sk->port, neon_timeline, neon_tenant);
-			}
-
+			written = snprintf((char *) &sk->conninfo, MAXCONNINFO,
+							   "host=%s port=%s dbname=replication options='-c timeline_id=%s tenant_id=%s'",
+							   sk->host, sk->port, neon_timeline, neon_tenant);
 			if (written > MAXCONNINFO || written < 0)
 				elog(FATAL, "could not create connection string for safekeeper %s:%s", sk->host, sk->port);
 		}
@@ -696,7 +688,7 @@ ResetConnection(Safekeeper *sk)
 	/*
 	 * Try to establish new connection
 	 */
-	sk->conn = walprop_connect_start((char *) &sk->conninfo);
+	sk->conn = walprop_connect_start((char *) &sk->conninfo, neon_auth_token);
 
 	/*
 	 * "If the result is null, then libpq has been unable to allocate a new
diff --git a/pgxn/neon/walproposer.h b/pgxn/neon/walproposer.h
index 357d6378f8..537c733850 100644
--- a/pgxn/neon/walproposer.h
+++ b/pgxn/neon/walproposer.h
@@ -454,7 +454,7 @@ extern char *walprop_error_message(WalProposerConn *conn);
 extern WalProposerConnStatusType walprop_status(WalProposerConn *conn);
 
 /* Re-exported PQconnectStart */
-extern WalProposerConn * walprop_connect_start(char *conninfo);
+extern WalProposerConn * walprop_connect_start(char *conninfo, char *password);
 
 /* Re-exported PQconectPoll */
 extern WalProposerConnectPollStatusType walprop_connect_poll(WalProposerConn *conn);

From 809acb5fa93411f44091ab6a56dddeffdcee0d89 Mon Sep 17 00:00:00 2001
From: Shany Pozin <shany@neon.tech>
Date: Tue, 21 Mar 2023 19:32:36 +0200
Subject: [PATCH 04/19] Move neon-image-depot to a larger runner (#3860)

## Describe your changes
https://neondb.slack.com/archives/C039YKBRZB4/p1679413279637059
## Issue ticket number and link

## Checklist before requesting a review
- [ ] I have performed a self-review of my code.
- [ ] If it is a core feature, I have added thorough tests.
- [ ] Do we need to implement analytics? if so did you add the relevant
metrics to the dashboard?
- [ ] If this PR requires public announcement, mark it with
/release-notes label and add several sentences in this section.
---
 .github/workflows/build_and_test.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml
index e056cf0fcf..d50a42d83c 100644
--- a/.github/workflows/build_and_test.yml
+++ b/.github/workflows/build_and_test.yml
@@ -552,7 +552,7 @@ jobs:
   neon-image-depot:
     # For testing this will run side-by-side for a few merges.
     # This action is not really optimized yet, but gets the job done
-    runs-on: [ self-hosted, gen3, small ]
+    runs-on: [ self-hosted, gen3, large ]
     needs: [ tag ]
     container: 369495373322.dkr.ecr.eu-central-1.amazonaws.com/base:pinned
     permissions:

From 4158e24e60d294e0f039395ea95dd87f8ab317d9 Mon Sep 17 00:00:00 2001
From: Dmitry Rodionov <dmitry@neon.tech>
Date: Tue, 21 Mar 2023 20:03:27 +0200
Subject: [PATCH 05/19] rfc: delete pageserver data from s3 (#3792)

[Rendered](https://github.com/neondatabase/neon/blob/main/docs/rfcs/022-pageserver-delete-from-s3.md)

---------

Co-authored-by: Joonas Koivunen <joonas@neon.tech>
---
 docs/rfcs/022-pageserver-delete-from-s3.md | 269 +++++++++++++++++++++
 1 file changed, 269 insertions(+)
 create mode 100644 docs/rfcs/022-pageserver-delete-from-s3.md

diff --git a/docs/rfcs/022-pageserver-delete-from-s3.md b/docs/rfcs/022-pageserver-delete-from-s3.md
new file mode 100644
index 0000000000..260e549670
--- /dev/null
+++ b/docs/rfcs/022-pageserver-delete-from-s3.md
@@ -0,0 +1,269 @@
+# Deleting pageserver part of tenants data from s3
+
+Created on 08.03.23
+
+## Motivation
+
+Currently we dont delete pageserver part of the data from s3 when project is deleted. (The same is true for safekeepers, but this outside of the scope of this RFC).
+
+This RFC aims to spin a discussion to come to a robust deletion solution that wont put us in into a corner for features like postponed deletion (when we keep data for user to be able to restore a project if it was deleted by accident)
+
+## Summary
+
+TLDR; There are two options, one based on control plane issuing actual delete requests to s3 and the other one that keeps s3 stuff bound to pageserver. Each one has its pros and cons.
+
+The decision is to stick with pageserver centric approach. For motivation see [Decision](#decision).
+
+## Components
+
+pageserver, control-plane
+
+## Requirements
+
+Deletion should successfully finish (eventually) without leaving dangling files in presense of:
+
+- component restarts
+- component outage
+- pageserver loss
+
+## Proposed implementation
+
+Before the options are discussed, note that deletion can be quite long process. For deletion from s3 the obvious choice is [DeleteObjects](https://docs.aws.amazon.com/AmazonS3/latest/API/API_DeleteObjects.html) API call. It allows to batch deletion of up to 1k objects in one API call. So deletion operation linearly depends on number of layer files.
+
+Another design limitation is that there is no cheap `mv` operation available for s3. `mv` from `aws s3 mv` uses `copy(src, dst) + delete(src)`. So `mv`-like operation is not feasible as a building block because it actually amplifies the problem with both duration and resulting cost of the operation.
+
+The case when there are multiple pageservers handling the same tenants is largely out of scope of the RFC. We still consider case with migration from one PS to another, but do not consider case when tenant exists on multiple pageservers for extended period of time. The case with multiple pageservers can be reduced to case with one pageservers by calling detach on all pageservers except the last one, for it actual delete needs to be called.
+
+For simplicity lets look into deleting tenants. Differences in deletion process between tenants and timelines are mentioned in paragraph ["Differences between tenants and timelines"](#differences-between-tenants-and-timelines)
+
+### 1. Pageserver owns deletion machinery
+
+#### The sequence
+
+TLDR; With this approach control plane needs to call delete on a tenant and poll for progress. As much as possible is handled on pageserver. Lets see the sequence.
+
+Happy path:
+
+```mermaid
+sequenceDiagram
+    autonumber
+    participant CP as Control Plane
+    participant PS as Pageserver
+    participant S3
+
+    CP->>PS: Delete tenant
+    PS->>S3: Create deleted mark file at <br> /tenant/meta/deleted
+    PS->>PS: Create deleted mark file locally
+    PS->>CP: Accepted
+    PS->>PS: delete local files other than deleted mark
+    loop Delete layers for each timeline
+        PS->>S3: delete(..)
+        CP->>PS: Finished?
+        PS->>CP: False
+    end
+    PS->>S3: Delete mark file
+    PS->>PS: Delete local mark file
+
+    loop Poll for status
+        CP->>PS: Finished?
+        PS->>CP: True or False
+    end
+```
+
+Why two mark files?
+Remote one is needed for cases when pageserver is lost during deletion so other pageserver can learn the deletion from s3 during attach.
+
+Why local mark file is needed?
+
+If we dont have one, we have two choices, delete local data before deleting the remote part or do that after.
+
+If we delete local data before remote then during restart pageserver wont pick up remote tenant at all because nothing is available locally (pageserver looks for remote conuterparts of locally available tenants).
+
+If we delete local data after remote then at the end of the sequence when remote mark file is deleted if pageserver restart happens then the state is the same to situation when pageserver just missing data on remote without knowing the fact that this data is intended to be deleted. In this case the current behavior is upload everything local-only to remote.
+
+Thus we need local record of tenant being deleted as well.
+
+##### Handle pageserver crashes
+
+Lets explore sequences with various crash points.
+
+Pageserver crashes before `deleted` mark file is persisted in s3:
+
+```mermaid
+sequenceDiagram
+    autonumber
+    participant CP as Control Plane
+    participant PS as Pageserver
+    participant S3
+
+    CP->>PS: Delete tenant
+    note over PS: Crash point 1.
+    CP->>PS: Retry delete request
+
+    PS->>S3: Create deleted mark file at <br> /tenant/meta/deleted
+    PS->>PS: Create deleted mark file locally
+
+    PS->>CP: Accepted
+
+    PS->>PS: delete local files other than deleted mark
+
+    loop Delete layers for each timeline
+        PS->>S3: delete(..)
+        CP->>PS: Finished?
+        PS->>CP: False
+    end
+    PS->>S3: Delete mark file
+    PS->>PS: Delete local mark file
+
+    CP->>PS: Finished?
+    PS->>CP: True
+```
+
+Pageserver crashed when deleted mark was about to be persisted in s3, before Control Plane gets a response:
+
+```mermaid
+sequenceDiagram
+    autonumber
+    participant CP as Control Plane
+    participant PS as Pageserver
+    participant S3
+
+    CP->>PS: Delete tenant
+    PS->>S3: Create deleted mark file at <br> /tenant/meta/deleted
+
+    note over PS: Crash point 2.
+    note over PS: During startup we reconcile <br> with remote and see <br> whether the remote mark exists
+    alt Remote mark exists
+        PS->>PS: create local mark if its missing
+        PS->>PS: delete local files other than deleted mark
+        loop Delete layers for each timeline
+            PS->>S3: delete(..)
+        end
+
+        note over CP: Eventually console should <br> retry delete request
+
+        CP->>PS: Retry delete tenant
+        PS->>CP: Not modified
+    else Mark is missing
+        note over PS: Continue to operate the tenant as if deletion didnt happen
+
+        note over CP: Eventually console should <br> retry delete request
+
+        CP->>PS: Retry delete tenant
+        PS->>S3: Create deleted mark file at <br> /tenant/meta/deleted
+        PS->>CP: Delete tenant
+    end
+
+    PS->>PS: Continue with layer file deletions
+    loop Delete layers for each timeline
+        PS->>S3: delete(..)
+        CP->>PS: Finished?
+        PS->>CP: False
+    end
+
+    PS->>S3: Delete mark file
+    PS->>PS: Delete local mark file
+
+    CP->>PS: Finished?
+    PS->>CP: True
+```
+
+Similar sequence applies when both local and remote marks were persisted but Control Plane still didnt receive a response.
+
+If pageserver crashes after both mark files were deleted then it will reply to control plane status poll request with 404 which should be treated by control plane as success.
+
+The same applies if pageserver crashes in the end, when remote mark is deleted but before local one gets deleted. In this case on restart pageserver moves forward with deletion of local mark and Control Plane will receive 404.
+
+##### Differences between tenants and timelines
+
+For timeline the sequence is the same with the following differences:
+
+- remote delete mark file can be replaced with a boolean "deleted" flag in index_part.json
+- local deletion mark is not needed, because whole tenant is kept locally so situation described in motivation for local mark is impossible
+
+##### Handle pageserver loss
+
+If pageseserver is lost then the deleted tenant should be attached to different pageserver and delete request needs to be retried against new pageserver. Then attach logic is shared with one described for pageserver restarts (local deletion mark wont be available so needs to be created).
+
+##### Restrictions for tenant that is in progress of being deleted
+
+I propose to add another state to tenant/timeline - PendingDelete. This state shouldnt allow executing any operations aside from polling the deletion status.
+
+#### Summary
+
+Pros:
+
+- Storage is not dependent on control plane. Storage can be restarted even if control plane is not working.
+- Allows for easier dogfooding, console can use Neon backed database as primary operational data store. If storage depends on control plane and control plane depends on storage we're stuck.
+- No need to share inner s3 workings with control plane. Pageserver presents api contract and S3 paths are not part of this contract.
+- No need to pass list of alive timelines to attach call. This will be solved by pageserver observing deleted flag. See
+
+Cons:
+
+- Logic is a tricky, needs good testing
+- Anything else?
+
+### 2. Control plane owns deletion machinery
+
+In this case the only action performed on pageserver is removal of local files.
+
+Everything else is done by control plane. The steps are as follows:
+
+1. Control plane marks tenant as "delete pending" in its database
+2. It lists the s3 for all the files and repeatedly calls delete until nothing is left behind
+3. When no files are left marks deletion as completed
+
+In case of restart it selects all tenants marked as "delete pending" and continues the deletion.
+
+For tenants it is simple. For timelines there are caveats.
+
+Assume that the same workflow is used for timelines.
+
+If a tenant gets relocated during timeline deletion the attach call with its current logic will pick up deleted timeline in its half deleted state.
+
+Available options:
+
+- require list of alive timelines to be passed to attach call
+- use the same schema with flag in index_part.json (again part of the caveats around pageserver restart applies). In this case nothing stops pageserver from implementing deletion inside if we already have these deletion marks.
+
+With first option the following problem becomes apparent:
+
+Who is the source of truth regarding timeline liveness?
+
+Imagine:
+PS1 fails.
+PS2 gets assigned the tenant.
+New branch gets created
+PS1 starts up (is it possible or we just recycle it?)
+PS1 is unaware of the new branch. It can either fall back to s3 ls, or ask control plane.
+
+So here comes the dependency of storage on control plane. During restart storage needs to know which timelines are valid for operation. If there is nothing on s3 that can answer that question storage neeeds to ask control plane.
+
+### Summary
+
+Cons:
+
+- Potential thundering herd-like problem during storage restart (requests to control plane)
+- Potential increase in storage startup time (additional request to control plane)
+- Storage startup starts to depend on console
+- Erroneous attach call can attach tenant in half deleted state
+
+Pros:
+
+- Easier to reason about if you dont have to account for pageserver restarts
+
+### Extra notes
+
+There was a concern that having deletion code in pageserver is a littlebit scary, but we need to have this code somewhere. So to me it is equally scary to have that in whatever place it ends up at.
+
+Delayed deletion can be done with both approaches. As discussed with Anna (@stepashka) this is only relevant for tenants (projects) not for timelines. For first approach detach can be called immediately and deletion can be done later with attach + delete. With second approach control plane needs to start the deletion whenever necessary.
+
+## Decision
+
+After discussion in comments I see that we settled on two options (though a bit different from ones described in rfc). First one is the same - pageserver owns as much as possible. The second option is that pageserver owns markers thing, but actual deletion happens in control plane by repeatedly calling ls + delete.
+
+To my mind the only benefit of the latter approach is possible code reuse between safekeepers and pageservers. Otherwise poking around integrating s3 library into control plane, configuring shared knowledge abouth paths in s3 - are the downsides. Another downside of relying on control plane is the testing process. Control plane resides in different repository so it is quite hard to test pageserver related changes there. e2e test suite there doesnt support shutting down pageservers, which are separate docker containers there instead of just processes.
+
+With pageserver owning everything we still give the retry logic to control plane but its easier to duplicate if needed compared to sharing inner s3 workings. We will have needed tests for retry logic in neon repo.
+
+So the decision is to proceed with pageserver centric approach.

From 6fdd9c10d18270a5e30704f17e573ea14ee978ce Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki@neon.tech>
Date: Tue, 21 Mar 2023 15:24:49 +0200
Subject: [PATCH 06/19] Read storage auth token from spec file.

We read the pageserver connection string from the spec file, so let's
read the auth token from the same place.

We've been talking about pre-launching compute nodes that are not
associated with any particular tenant at startup, so that the spec
file is delivered to the compute node later. We cannot change the env
variables after the process has been launched.

We still pass the token to 'postgres' binary in the NEON_AUTH_TOKEN
env variable, but compute_ctl is now responsible for setting it.
---
 compute_tools/src/bin/compute_ctl.rs |  2 ++
 compute_tools/src/compute.rs         | 29 +++++++++++++++++-----------
 compute_tools/src/spec.rs            |  2 ++
 3 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/compute_tools/src/bin/compute_ctl.rs b/compute_tools/src/bin/compute_ctl.rs
index a4e9262072..b96842e416 100644
--- a/compute_tools/src/bin/compute_ctl.rs
+++ b/compute_tools/src/bin/compute_ctl.rs
@@ -133,6 +133,7 @@ fn main() -> Result<()> {
         .settings
         .find("neon.pageserver_connstring")
         .expect("pageserver connstr should be provided");
+    let storage_auth_token = spec.storage_auth_token.clone();
     let tenant = spec
         .cluster
         .settings
@@ -153,6 +154,7 @@ fn main() -> Result<()> {
         tenant,
         timeline,
         pageserver_connstr,
+        storage_auth_token,
         metrics: ComputeMetrics::default(),
         state: RwLock::new(ComputeState::new()),
     };
diff --git a/compute_tools/src/compute.rs b/compute_tools/src/compute.rs
index 09272262de..00d1e234ab 100644
--- a/compute_tools/src/compute.rs
+++ b/compute_tools/src/compute.rs
@@ -45,6 +45,7 @@ pub struct ComputeNode {
     pub tenant: String,
     pub timeline: String,
     pub pageserver_connstr: String,
+    pub storage_auth_token: Option<String>,
     pub metrics: ComputeMetrics,
     /// Volatile part of the `ComputeNode` so should be used under `RwLock`
     /// to allow HTTP API server to serve status requests, while configuration
@@ -129,18 +130,14 @@ impl ComputeNode {
 
         let mut config = postgres::Config::from_str(&self.pageserver_connstr)?;
 
-        // Like in the neon extension, if the $NEON_AUTH_TOKEN env variable is
-        // set, use it as the password when connecting to pageserver.
-        //
+        // Use the storage auth token from the config file, if given.
         // Note: this overrides any password set in the connection string.
-        match std::env::var("NEON_AUTH_TOKEN") {
-            Ok(val) => {
-                info!("Got pageserver auth token from NEON_AUTH_TOKEN env variable");
-                config.password(val);
-            }
-            Err(std::env::VarError::NotPresent) => info!("NEON_AUTH_TOKEN env variable not set"),
-            Err(e) => info!("could not parse NEON_AUTH_TOKEN env variable: {}", e),
-        };
+        if let Some(storage_auth_token) = &self.storage_auth_token {
+            info!("Got storage auth token from spec file");
+            config.password(storage_auth_token);
+        } else {
+            info!("Storage auth token not set");
+        }
 
         let mut client = config.connect(NoTls)?;
         let basebackup_cmd = match lsn {
@@ -179,6 +176,11 @@ impl ComputeNode {
         let sync_handle = Command::new(&self.pgbin)
             .args(["--sync-safekeepers"])
             .env("PGDATA", &self.pgdata) // we cannot use -D in this mode
+            .envs(if let Some(storage_auth_token) = &self.storage_auth_token {
+                vec![("NEON_AUTH_TOKEN", storage_auth_token)]
+            } else {
+                vec![]
+            })
             .stdout(Stdio::piped())
             .spawn()
             .expect("postgres --sync-safekeepers failed to start");
@@ -256,6 +258,11 @@ impl ComputeNode {
         // Run postgres as a child process.
         let mut pg = Command::new(&self.pgbin)
             .args(["-D", &self.pgdata])
+            .envs(if let Some(storage_auth_token) = &self.storage_auth_token {
+                vec![("NEON_AUTH_TOKEN", storage_auth_token)]
+            } else {
+                vec![]
+            })
             .spawn()
             .expect("cannot start postgres process");
 
diff --git a/compute_tools/src/spec.rs b/compute_tools/src/spec.rs
index 47f1d69cff..9694ba9a88 100644
--- a/compute_tools/src/spec.rs
+++ b/compute_tools/src/spec.rs
@@ -24,6 +24,8 @@ pub struct ComputeSpec {
     pub cluster: Cluster,
     pub delta_operations: Option<Vec<DeltaOp>>,
 
+    pub storage_auth_token: Option<String>,
+
     pub startup_tracing_context: Option<HashMap<String, String>>,
 }
 

From dd22c871003275d1087a9a5a4948f030ad6a8eda Mon Sep 17 00:00:00 2001
From: Kirill Bulatov <kirill@neon.tech>
Date: Tue, 21 Mar 2023 23:33:28 +0200
Subject: [PATCH 07/19] Remove older layer metadata format support code (#3854)

The PR enforces current newest `index_part.json` format in the type
system (version `1`), not allowing any previous forms of it, that were
used in the past.
Similarly, the code to mitigate the
https://github.com/neondatabase/neon/issues/3024 issue is now also
removed.

Current code does not produce old formats and extra files in the
index_part.json, in the future we will be able to use
https://github.com/neondatabase/aversion or other approach to make
version transitions more explicit.

See https://neondb.slack.com/archives/C033RQ5SPDH/p1679134185248119 for
the justification on the breaking changes.
---
 libs/pageserver_api/src/models.rs             |   4 +-
 pageserver/src/http/routes.rs                 |   4 +-
 .../src/tenant/remote_timeline_client.rs      |  38 +--
 .../tenant/remote_timeline_client/download.rs |  21 +-
 .../tenant/remote_timeline_client/index.rs    | 219 +++++-------------
 .../tenant/remote_timeline_client/upload.rs   |  10 +-
 pageserver/src/tenant/storage_layer.rs        |   2 +-
 .../src/tenant/storage_layer/delta_layer.rs   |   6 +-
 .../src/tenant/storage_layer/filename.rs      |   9 +
 .../src/tenant/storage_layer/image_layer.rs   |   6 +-
 .../src/tenant/storage_layer/remote_layer.rs  |   2 +-
 pageserver/src/tenant/timeline.rs             |  88 +++----
 pageserver/src/tenant/upload_queue.rs         |  15 +-
 .../test_tenants_with_remote_storage.py       | 200 ----------------
 14 files changed, 132 insertions(+), 492 deletions(-)

diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs
index 7a43100ba5..0f860d0a6d 100644
--- a/libs/pageserver_api/src/models.rs
+++ b/libs/pageserver_api/src/models.rs
@@ -346,7 +346,7 @@ pub enum InMemoryLayerInfo {
 pub enum HistoricLayerInfo {
     Delta {
         layer_file_name: String,
-        layer_file_size: Option<u64>,
+        layer_file_size: u64,
 
         #[serde_as(as = "DisplayFromStr")]
         lsn_start: Lsn,
@@ -357,7 +357,7 @@ pub enum HistoricLayerInfo {
     },
     Image {
         layer_file_name: String,
-        layer_file_size: Option<u64>,
+        layer_file_size: u64,
 
         #[serde_as(as = "DisplayFromStr")]
         lsn_start: Lsn,
diff --git a/pageserver/src/http/routes.rs b/pageserver/src/http/routes.rs
index 39f2776952..d91e421a52 100644
--- a/pageserver/src/http/routes.rs
+++ b/pageserver/src/http/routes.rs
@@ -185,7 +185,7 @@ fn build_timeline_info_common(
             None
         }
     };
-    let current_physical_size = Some(timeline.layer_size_sum().approximate_is_ok());
+    let current_physical_size = Some(timeline.layer_size_sum());
     let state = timeline.current_state();
     let remote_consistent_lsn = timeline.get_remote_consistent_lsn().unwrap_or(Lsn(0));
 
@@ -451,7 +451,7 @@ async fn tenant_status(request: Request<Body>) -> Result<Response<Body>, ApiErro
         // Calculate total physical size of all timelines
         let mut current_physical_size = 0;
         for timeline in tenant.list_timelines().iter() {
-            current_physical_size += timeline.layer_size_sum().approximate_is_ok();
+            current_physical_size += timeline.layer_size_sum();
         }
 
         let state = tenant.current_state();
diff --git a/pageserver/src/tenant/remote_timeline_client.rs b/pageserver/src/tenant/remote_timeline_client.rs
index f3943298f2..28c4943dbd 100644
--- a/pageserver/src/tenant/remote_timeline_client.rs
+++ b/pageserver/src/tenant/remote_timeline_client.rs
@@ -210,7 +210,6 @@ pub use download::{is_temp_download_file, list_remote_timelines};
 use std::sync::atomic::{AtomicU32, Ordering};
 use std::sync::{Arc, Mutex};
 
-use anyhow::ensure;
 use remote_storage::{DownloadError, GenericRemoteStorage};
 use std::ops::DerefMut;
 use tokio::runtime::Runtime;
@@ -347,7 +346,7 @@ impl RemoteTimelineClient {
                 .layer_metadata
                 .values()
                 // If we don't have the file size for the layer, don't account for it in the metric.
-                .map(|ilmd| ilmd.file_size.unwrap_or(0))
+                .map(|ilmd| ilmd.file_size)
                 .sum()
         } else {
             0
@@ -420,34 +419,6 @@ impl RemoteTimelineClient {
             .await?
         };
 
-        // Update the metadata for given layer file. The remote index file
-        // might be missing some information for the file; this allows us
-        // to fill in the missing details.
-        if layer_metadata.file_size().is_none() {
-            let new_metadata = LayerFileMetadata::new(downloaded_size);
-            let mut guard = self.upload_queue.lock().unwrap();
-            let upload_queue = guard.initialized_mut()?;
-            if let Some(upgraded) = upload_queue.latest_files.get_mut(layer_file_name) {
-                if upgraded.merge(&new_metadata) {
-                    upload_queue.latest_files_changes_since_metadata_upload_scheduled += 1;
-                }
-                // If we don't do an index file upload inbetween here and restart,
-                // the value will go back down after pageserver restart, since we will
-                // have lost this data point.
-                // But, we upload index part fairly frequently, and restart pageserver rarely.
-                // So, by accounting eagerly, we present a most-of-the-time-more-accurate value sooner.
-                self.metrics
-                    .remote_physical_size_gauge()
-                    .add(downloaded_size);
-            } else {
-                // The file should exist, since we just downloaded it.
-                warn!(
-                    "downloaded file {:?} not found in local copy of the index file",
-                    layer_file_name
-                );
-            }
-        }
-
         REMOTE_ONDEMAND_DOWNLOADED_LAYERS.inc();
         REMOTE_ONDEMAND_DOWNLOADED_BYTES.inc_by(downloaded_size);
 
@@ -550,13 +521,6 @@ impl RemoteTimelineClient {
         let mut guard = self.upload_queue.lock().unwrap();
         let upload_queue = guard.initialized_mut()?;
 
-        // The file size can be missing for files that were created before we tracked that
-        // in the metadata, but it should be present for any new files we create.
-        ensure!(
-            layer_metadata.file_size().is_some(),
-            "file size not initialized in metadata"
-        );
-
         upload_queue
             .latest_files
             .insert(layer_file_name.clone(), layer_metadata.clone());
diff --git a/pageserver/src/tenant/remote_timeline_client/download.rs b/pageserver/src/tenant/remote_timeline_client/download.rs
index ea8d9858c3..bda095d850 100644
--- a/pageserver/src/tenant/remote_timeline_client/download.rs
+++ b/pageserver/src/tenant/remote_timeline_client/download.rs
@@ -21,7 +21,7 @@ use remote_storage::{DownloadError, GenericRemoteStorage};
 use utils::crashsafe::path_with_suffix_extension;
 use utils::id::{TenantId, TimelineId};
 
-use super::index::{IndexPart, IndexPartUnclean, LayerFileMetadata};
+use super::index::{IndexPart, LayerFileMetadata};
 use super::{FAILED_DOWNLOAD_RETRIES, FAILED_DOWNLOAD_WARN_THRESHOLD};
 
 async fn fsync_path(path: impl AsRef<std::path::Path>) -> Result<(), std::io::Error> {
@@ -113,16 +113,11 @@ pub async fn download_layer_file<'a>(
         })
         .map_err(DownloadError::Other)?;
 
-    match layer_metadata.file_size() {
-        Some(expected) if expected != bytes_amount => {
-            return Err(DownloadError::Other(anyhow!(
-                "According to layer file metadata should have downloaded {expected} bytes but downloaded {bytes_amount} bytes into file '{}'",
-                temp_file_path.display()
-            )));
-        }
-        Some(_) | None => {
-            // matches, or upgrading from an earlier IndexPart version
-        }
+    let expected = layer_metadata.file_size();
+    if expected != bytes_amount {
+        return Err(DownloadError::Other(anyhow!(
+            "According to layer file metadata should have downloaded {expected} bytes but downloaded {bytes_amount} bytes into file {temp_file_path:?}",
+        )));
     }
 
     // not using sync_data because it can lose file size update
@@ -261,14 +256,12 @@ pub(super) async fn download_index_part(
     )
     .await?;
 
-    let index_part: IndexPartUnclean = serde_json::from_slice(&index_part_bytes)
+    let index_part: IndexPart = serde_json::from_slice(&index_part_bytes)
         .with_context(|| {
             format!("Failed to deserialize index part file into file {index_part_path:?}")
         })
         .map_err(DownloadError::Other)?;
 
-    let index_part = index_part.remove_unclean_layer_file_names();
-
     Ok(index_part)
 }
 
diff --git a/pageserver/src/tenant/remote_timeline_client/index.rs b/pageserver/src/tenant/remote_timeline_client/index.rs
index 420edae6cd..9c84f8e977 100644
--- a/pageserver/src/tenant/remote_timeline_client/index.rs
+++ b/pageserver/src/tenant/remote_timeline_client/index.rs
@@ -6,7 +6,6 @@ use std::collections::{HashMap, HashSet};
 
 use serde::{Deserialize, Serialize};
 use serde_with::{serde_as, DisplayFromStr};
-use tracing::warn;
 
 use crate::tenant::metadata::TimelineMetadata;
 use crate::tenant::storage_layer::LayerFileName;
@@ -20,7 +19,7 @@ use utils::lsn::Lsn;
 #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)]
 #[cfg_attr(test, derive(Default))]
 pub struct LayerFileMetadata {
-    file_size: Option<u64>,
+    file_size: u64,
 }
 
 impl From<&'_ IndexLayerMetadata> for LayerFileMetadata {
@@ -33,36 +32,16 @@ impl From<&'_ IndexLayerMetadata> for LayerFileMetadata {
 
 impl LayerFileMetadata {
     pub fn new(file_size: u64) -> Self {
-        LayerFileMetadata {
-            file_size: Some(file_size),
-        }
+        LayerFileMetadata { file_size }
     }
 
-    /// This is used to initialize the metadata for remote layers, for which
-    /// the metadata was missing from the index part file.
-    pub const MISSING: Self = LayerFileMetadata { file_size: None };
-
-    pub fn file_size(&self) -> Option<u64> {
+    pub fn file_size(&self) -> u64 {
         self.file_size
     }
-
-    /// Metadata has holes due to version upgrades. This method is called to upgrade self with the
-    /// other value.
-    ///
-    /// This is called on the possibly outdated version. Returns true if any changes
-    /// were made.
-    pub fn merge(&mut self, other: &Self) -> bool {
-        let mut changed = false;
-
-        if self.file_size != other.file_size {
-            self.file_size = other.file_size.or(self.file_size);
-            changed = true;
-        }
-
-        changed
-    }
 }
 
+// TODO seems like another part of the remote storage file format
+// compatibility issue, see https://github.com/neondatabase/neon/issues/3072
 /// In-memory representation of an `index_part.json` file
 ///
 /// Contains the data about all files in the timeline, present remotely and its metadata.
@@ -71,10 +50,7 @@ impl LayerFileMetadata {
 /// remember to add a test case for the changed version.
 #[serde_as]
 #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)]
-pub struct IndexPartImpl<L>
-where
-    L: std::hash::Hash + PartialEq + Eq,
-{
+pub struct IndexPart {
     /// Debugging aid describing the version of this type.
     #[serde(default)]
     version: usize,
@@ -82,14 +58,13 @@ where
     /// Layer names, which are stored on the remote storage.
     ///
     /// Additional metadata can might exist in `layer_metadata`.
-    pub timeline_layers: HashSet<L>,
+    pub timeline_layers: HashSet<LayerFileName>,
 
     /// Per layer file name metadata, which can be present for a present or missing layer file.
     ///
     /// Older versions of `IndexPart` will not have this property or have only a part of metadata
     /// that latest version stores.
-    #[serde(default = "HashMap::default")]
-    pub layer_metadata: HashMap<L, IndexLayerMetadata>,
+    pub layer_metadata: HashMap<LayerFileName, IndexLayerMetadata>,
 
     // 'disk_consistent_lsn' is a copy of the 'disk_consistent_lsn' in the metadata.
     // It's duplicated here for convenience.
@@ -98,101 +73,6 @@ where
     metadata_bytes: Vec<u8>,
 }
 
-// TODO seems like another part of the remote storage file format
-// compatibility issue, see https://github.com/neondatabase/neon/issues/3072
-pub type IndexPart = IndexPartImpl<LayerFileName>;
-
-pub type IndexPartUnclean = IndexPartImpl<UncleanLayerFileName>;
-
-#[derive(Debug, PartialEq, Eq, Hash, Clone)]
-pub enum UncleanLayerFileName {
-    Clean(LayerFileName),
-    BackupFile(String),
-}
-
-impl<'de> serde::Deserialize<'de> for UncleanLayerFileName {
-    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
-    where
-        D: serde::Deserializer<'de>,
-    {
-        deserializer.deserialize_string(UncleanLayerFileNameVisitor)
-    }
-}
-
-struct UncleanLayerFileNameVisitor;
-
-impl<'de> serde::de::Visitor<'de> for UncleanLayerFileNameVisitor {
-    type Value = UncleanLayerFileName;
-
-    fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
-        write!(
-            formatter,
-            "a string that is a valid LayerFileName or '.old' backup file name"
-        )
-    }
-
-    fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
-    where
-        E: serde::de::Error,
-    {
-        let maybe_clean: Result<LayerFileName, _> = v.parse();
-        match maybe_clean {
-            Ok(clean) => Ok(UncleanLayerFileName::Clean(clean)),
-            Err(e) => {
-                if v.ends_with(".old") || v == "metadata_backup" {
-                    Ok(UncleanLayerFileName::BackupFile(v.to_owned()))
-                } else {
-                    Err(E::custom(e))
-                }
-            }
-        }
-    }
-}
-
-impl UncleanLayerFileName {
-    fn into_clean(self) -> Option<LayerFileName> {
-        match self {
-            UncleanLayerFileName::Clean(clean) => Some(clean),
-            UncleanLayerFileName::BackupFile(_) => None,
-        }
-    }
-}
-
-impl IndexPartUnclean {
-    pub fn remove_unclean_layer_file_names(self) -> IndexPart {
-        let IndexPartUnclean {
-            version,
-            timeline_layers,
-            layer_metadata,
-            disk_consistent_lsn,
-            metadata_bytes,
-        } = self;
-
-        IndexPart {
-            version,
-            timeline_layers: timeline_layers
-                .into_iter()
-                .filter_map(|unclean_file_name| match unclean_file_name {
-                    UncleanLayerFileName::Clean(clean_name) => Some(clean_name),
-                    UncleanLayerFileName::BackupFile(backup_file_name) => {
-                        // For details see https://github.com/neondatabase/neon/issues/3024
-                        warn!(
-                            "got backup file on the remote storage, ignoring it {backup_file_name}"
-                        );
-                        None
-                    }
-                })
-                .collect(),
-            layer_metadata: layer_metadata
-                .into_iter()
-                .filter_map(|(l, m)| l.into_clean().map(|l| (l, m)))
-                .collect(),
-            disk_consistent_lsn,
-            metadata_bytes,
-        }
-    }
-}
-
 impl IndexPart {
     /// When adding or modifying any parts of `IndexPart`, increment the version so that it can be
     /// used to understand later versions.
@@ -232,7 +112,7 @@ impl IndexPart {
 /// Serialized form of [`LayerFileMetadata`].
 #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize, Default)]
 pub struct IndexLayerMetadata {
-    pub(super) file_size: Option<u64>,
+    pub(super) file_size: u64,
 }
 
 impl From<&'_ LayerFileMetadata> for IndexLayerMetadata {
@@ -247,27 +127,6 @@ impl From<&'_ LayerFileMetadata> for IndexLayerMetadata {
 mod tests {
     use super::*;
 
-    #[test]
-    fn v0_indexpart_is_parsed() {
-        let example = r#"{
-            "timeline_layers":["000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__0000000001696070-00000000016960E9"],
-            "disk_consistent_lsn":"0/16960E8",
-            "metadata_bytes":[113,11,159,210,0,54,0,4,0,0,0,0,1,105,96,232,1,0,0,0,0,1,105,96,112,0,0,0,0,0,0,0,0,0,0,0,0,0,1,105,96,112,0,0,0,0,1,105,96,112,0,0,0,14,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]
-        }"#;
-
-        let expected = IndexPart {
-            version: 0,
-            timeline_layers: HashSet::from(["000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__0000000001696070-00000000016960E9".parse().unwrap()]),
-            layer_metadata: HashMap::default(),
-            disk_consistent_lsn: "0/16960E8".parse::<Lsn>().unwrap(),
-            metadata_bytes: [113,11,159,210,0,54,0,4,0,0,0,0,1,105,96,232,1,0,0,0,0,1,105,96,112,0,0,0,0,0,0,0,0,0,0,0,0,0,1,105,96,112,0,0,0,0,1,105,96,112,0,0,0,14,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0].to_vec(),
-        };
-
-        let part: IndexPartUnclean = serde_json::from_str(example).unwrap();
-        let part = part.remove_unclean_layer_file_names();
-        assert_eq!(part, expected);
-    }
-
     #[test]
     fn v1_indexpart_is_parsed() {
         let example = r#"{
@@ -287,21 +146,19 @@ mod tests {
             timeline_layers: HashSet::from(["000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__0000000001696070-00000000016960E9".parse().unwrap()]),
             layer_metadata: HashMap::from([
                 ("000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__0000000001696070-00000000016960E9".parse().unwrap(), IndexLayerMetadata {
-                    file_size: Some(25600000),
+                    file_size: 25600000,
                 }),
                 ("000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__00000000016B59D8-00000000016B5A51".parse().unwrap(), IndexLayerMetadata {
                     // serde_json should always parse this but this might be a double with jq for
                     // example.
-                    file_size: Some(9007199254741001),
+                    file_size: 9007199254741001,
                 })
             ]),
             disk_consistent_lsn: "0/16960E8".parse::<Lsn>().unwrap(),
             metadata_bytes: [113,11,159,210,0,54,0,4,0,0,0,0,1,105,96,232,1,0,0,0,0,1,105,96,112,0,0,0,0,0,0,0,0,0,0,0,0,0,1,105,96,112,0,0,0,0,1,105,96,112,0,0,0,14,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0].to_vec(),
         };
 
-        let part = serde_json::from_str::<IndexPartUnclean>(example)
-            .unwrap()
-            .remove_unclean_layer_file_names();
+        let part = serde_json::from_str::<IndexPart>(example).unwrap();
         assert_eq!(part, expected);
     }
 
@@ -325,20 +182,64 @@ mod tests {
             timeline_layers: HashSet::from(["000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__0000000001696070-00000000016960E9".parse().unwrap()]),
             layer_metadata: HashMap::from([
                 ("000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__0000000001696070-00000000016960E9".parse().unwrap(), IndexLayerMetadata {
-                    file_size: Some(25600000),
+                    file_size: 25600000,
                 }),
                 ("000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__00000000016B59D8-00000000016B5A51".parse().unwrap(), IndexLayerMetadata {
                     // serde_json should always parse this but this might be a double with jq for
                     // example.
-                    file_size: Some(9007199254741001),
+                    file_size: 9007199254741001,
                 })
             ]),
             disk_consistent_lsn: "0/16960E8".parse::<Lsn>().unwrap(),
             metadata_bytes: [112,11,159,210,0,54,0,4,0,0,0,0,1,105,96,232,1,0,0,0,0,1,105,96,112,0,0,0,0,0,0,0,0,0,0,0,0,0,1,105,96,112,0,0,0,0,1,105,96,112,0,0,0,14,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0].to_vec(),
         };
 
-        let part = serde_json::from_str::<IndexPartUnclean>(example).unwrap();
-        let part = part.remove_unclean_layer_file_names();
+        let part = serde_json::from_str::<IndexPart>(example).unwrap();
         assert_eq!(part, expected);
     }
+
+    #[test]
+    fn empty_layers_are_parsed() {
+        let empty_layers_json = r#"{
+            "version":1,
+            "timeline_layers":[],
+            "layer_metadata":{},
+            "disk_consistent_lsn":"0/2532648",
+            "metadata_bytes":[136,151,49,208,0,70,0,4,0,0,0,0,2,83,38,72,1,0,0,0,0,2,83,38,32,1,87,198,240,135,97,119,45,125,38,29,155,161,140,141,255,210,0,0,0,0,2,83,38,72,0,0,0,0,1,73,240,192,0,0,0,0,1,73,240,192,0,0,0,15,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]
+        }"#;
+
+        let expected = IndexPart {
+            version: 1,
+            timeline_layers: HashSet::new(),
+            layer_metadata: HashMap::new(),
+            disk_consistent_lsn: "0/2532648".parse::<Lsn>().unwrap(),
+            metadata_bytes: [
+                136, 151, 49, 208, 0, 70, 0, 4, 0, 0, 0, 0, 2, 83, 38, 72, 1, 0, 0, 0, 0, 2, 83,
+                38, 32, 1, 87, 198, 240, 135, 97, 119, 45, 125, 38, 29, 155, 161, 140, 141, 255,
+                210, 0, 0, 0, 0, 2, 83, 38, 72, 0, 0, 0, 0, 1, 73, 240, 192, 0, 0, 0, 0, 1, 73,
+                240, 192, 0, 0, 0, 15, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0,
+            ]
+            .to_vec(),
+        };
+
+        let empty_layers_parsed = serde_json::from_str::<IndexPart>(empty_layers_json).unwrap();
+
+        assert_eq!(empty_layers_parsed, expected);
+    }
 }
diff --git a/pageserver/src/tenant/remote_timeline_client/upload.rs b/pageserver/src/tenant/remote_timeline_client/upload.rs
index 5082fa1634..ce9f4d9bf8 100644
--- a/pageserver/src/tenant/remote_timeline_client/upload.rs
+++ b/pageserver/src/tenant/remote_timeline_client/upload.rs
@@ -64,13 +64,9 @@ pub(super) async fn upload_timeline_layer<'a>(
         })?
         .len();
 
-    // FIXME: this looks bad
-    if let Some(metadata_size) = known_metadata.file_size() {
-        if metadata_size != fs_size {
-            bail!("File {source_path:?} has its current FS size {fs_size} diferent from initially determined {metadata_size}");
-        }
-    } else {
-        // this is a silly state we would like to avoid
+    let metadata_size = known_metadata.file_size();
+    if metadata_size != fs_size {
+        bail!("File {source_path:?} has its current FS size {fs_size} diferent from initially determined {metadata_size}");
     }
 
     let fs_size = usize::try_from(fs_size).with_context(|| {
diff --git a/pageserver/src/tenant/storage_layer.rs b/pageserver/src/tenant/storage_layer.rs
index 52ce2cab42..c36b6121c0 100644
--- a/pageserver/src/tenant/storage_layer.rs
+++ b/pageserver/src/tenant/storage_layer.rs
@@ -378,7 +378,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) -> Option<u64>;
+    fn file_size(&self) -> u64;
 
     fn info(&self, reset: LayerAccessStatsReset) -> HistoricLayerInfo;
 
diff --git a/pageserver/src/tenant/storage_layer/delta_layer.rs b/pageserver/src/tenant/storage_layer/delta_layer.rs
index 37719dfce5..98cbcc5f07 100644
--- a/pageserver/src/tenant/storage_layer/delta_layer.rs
+++ b/pageserver/src/tenant/storage_layer/delta_layer.rs
@@ -444,8 +444,8 @@ impl PersistentLayer for DeltaLayer {
         Ok(())
     }
 
-    fn file_size(&self) -> Option<u64> {
-        Some(self.file_size)
+    fn file_size(&self) -> u64 {
+        self.file_size
     }
 
     fn info(&self, reset: LayerAccessStatsReset) -> HistoricLayerInfo {
@@ -456,7 +456,7 @@ impl PersistentLayer for DeltaLayer {
 
         HistoricLayerInfo::Delta {
             layer_file_name,
-            layer_file_size: Some(self.file_size),
+            layer_file_size: self.file_size,
             lsn_start: lsn_range.start,
             lsn_end: lsn_range.end,
             remote: false,
diff --git a/pageserver/src/tenant/storage_layer/filename.rs b/pageserver/src/tenant/storage_layer/filename.rs
index efd0769886..e2112fc388 100644
--- a/pageserver/src/tenant/storage_layer/filename.rs
+++ b/pageserver/src/tenant/storage_layer/filename.rs
@@ -258,6 +258,15 @@ impl serde::Serialize for LayerFileName {
     }
 }
 
+impl<'de> serde::Deserialize<'de> for LayerFileName {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        deserializer.deserialize_string(LayerFileNameVisitor)
+    }
+}
+
 struct LayerFileNameVisitor;
 
 impl<'de> serde::de::Visitor<'de> for LayerFileNameVisitor {
diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs
index e37e001eda..a99b1b491f 100644
--- a/pageserver/src/tenant/storage_layer/image_layer.rs
+++ b/pageserver/src/tenant/storage_layer/image_layer.rs
@@ -258,8 +258,8 @@ impl PersistentLayer for ImageLayer {
         Ok(())
     }
 
-    fn file_size(&self) -> Option<u64> {
-        Some(self.file_size)
+    fn file_size(&self) -> u64 {
+        self.file_size
     }
 
     fn info(&self, reset: LayerAccessStatsReset) -> HistoricLayerInfo {
@@ -268,7 +268,7 @@ impl PersistentLayer for ImageLayer {
 
         HistoricLayerInfo::Image {
             layer_file_name,
-            layer_file_size: Some(self.file_size),
+            layer_file_size: self.file_size,
             lsn_start: lsn_range.start,
             remote: false,
             access_stats: self.access_stats.as_api_model(reset),
diff --git a/pageserver/src/tenant/storage_layer/remote_layer.rs b/pageserver/src/tenant/storage_layer/remote_layer.rs
index dbce2e7888..2eb7eb0cb6 100644
--- a/pageserver/src/tenant/storage_layer/remote_layer.rs
+++ b/pageserver/src/tenant/storage_layer/remote_layer.rs
@@ -167,7 +167,7 @@ impl PersistentLayer for RemoteLayer {
         true
     }
 
-    fn file_size(&self) -> Option<u64> {
+    fn file_size(&self) -> u64 {
         self.layer_metadata.file_size()
     }
 
diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs
index f5dbe63b0b..4d03a78883 100644
--- a/pageserver/src/tenant/timeline.rs
+++ b/pageserver/src/tenant/timeline.rs
@@ -334,25 +334,6 @@ impl LogicalSize {
     }
 }
 
-/// Returned by [`Timeline::layer_size_sum`]
-pub enum LayerSizeSum {
-    /// The result is accurate.
-    Accurate(u64),
-    // We don't know the layer file size of one or more layers.
-    // They contribute to the sum with a value of 0.
-    // Hence, the sum is a lower bound for the actualy layer file size sum.
-    ApproximateLowerBound(u64),
-}
-
-impl LayerSizeSum {
-    pub fn approximate_is_ok(self) -> u64 {
-        match self {
-            LayerSizeSum::Accurate(v) => v,
-            LayerSizeSum::ApproximateLowerBound(v) => v,
-        }
-    }
-}
-
 pub struct WalReceiverInfo {
     pub wal_source_connconf: PgConnectionConfig,
     pub last_received_msg_lsn: Lsn,
@@ -550,20 +531,13 @@ impl Timeline {
     /// The sum of the file size of all historic layers in the layer map.
     /// This method makes no distinction between local and remote layers.
     /// Hence, the result **does not represent local filesystem usage**.
-    pub fn layer_size_sum(&self) -> LayerSizeSum {
+    pub fn layer_size_sum(&self) -> u64 {
         let layer_map = self.layers.read().unwrap();
         let mut size = 0;
-        let mut no_size_cnt = 0;
         for l in layer_map.iter_historic_layers() {
-            let (l_size, l_no_size) = l.file_size().map(|s| (s, 0)).unwrap_or((0, 1));
-            size += l_size;
-            no_size_cnt += l_no_size;
-        }
-        if no_size_cnt == 0 {
-            LayerSizeSum::Accurate(size)
-        } else {
-            LayerSizeSum::ApproximateLowerBound(size)
+            size += l.file_size();
         }
+        size
     }
 
     pub fn get_resident_physical_size(&self) -> u64 {
@@ -1047,9 +1021,7 @@ impl Timeline {
             return Ok(false);
         }
 
-        let layer_file_size = local_layer
-            .file_size()
-            .expect("Local layer should have a file size");
+        let layer_file_size = local_layer.file_size();
 
         let local_layer_mtime = local_layer
             .local_path()
@@ -1514,7 +1486,12 @@ impl Timeline {
                 .layer_metadata
                 .get(remote_layer_name)
                 .map(LayerFileMetadata::from)
-                .unwrap_or(LayerFileMetadata::MISSING);
+                .with_context(|| {
+                    format!(
+                        "No remote layer metadata found for layer {}",
+                        remote_layer_name.file_name()
+                    )
+                })?;
 
             // Is the local layer's size different from the size stored in the
             // remote index file?
@@ -1530,34 +1507,27 @@ impl Timeline {
                     local_layer_path.display()
                 );
 
-                if let Some(remote_size) = remote_layer_metadata.file_size() {
-                    let metadata = local_layer_path.metadata().with_context(|| {
-                        format!(
-                            "get file size of local layer {}",
-                            local_layer_path.display()
-                        )
-                    })?;
-                    let local_size = metadata.len();
-                    if local_size != remote_size {
-                        warn!("removing local file {local_layer_path:?} because it has unexpected length {local_size}; length in remote index is {remote_size}");
-                        if let Err(err) = rename_to_backup(&local_layer_path) {
-                            assert!(local_layer_path.exists(), "we would leave the local_layer without a file if this does not hold: {}", local_layer_path.display());
-                            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);
-                            // fall-through to adding the remote layer
-                        }
+                let remote_size = remote_layer_metadata.file_size();
+                let metadata = local_layer_path.metadata().with_context(|| {
+                    format!(
+                        "get file size of local layer {}",
+                        local_layer_path.display()
+                    )
+                })?;
+                let local_size = metadata.len();
+                if local_size != remote_size {
+                    warn!("removing local file {local_layer_path:?} because it has unexpected length {local_size}; length in remote index is {remote_size}");
+                    if let Err(err) = rename_to_backup(&local_layer_path) {
+                        assert!(local_layer_path.exists(), "we would leave the local_layer without a file if this does not hold: {}", local_layer_path.display());
+                        anyhow::bail!("could not rename file {local_layer_path:?}: {err:?}");
                     } else {
-                        debug!(
-                            "layer is present locally and file size matches remote, using it: {}",
-                            local_layer_path.display()
-                        );
-                        continue;
+                        self.metrics.resident_physical_size_gauge.sub(local_size);
+                        updates.remove_historic(local_layer);
+                        // fall-through to adding the remote layer
                     }
                 } else {
                     debug!(
-                        "layer is present locally and remote does not have file size, using it: {}",
+                        "layer is present locally and file size matches remote, using it: {}",
                         local_layer_path.display()
                     );
                     continue;
@@ -1984,9 +1954,7 @@ impl Timeline {
     ) -> anyhow::Result<()> {
         if !layer.is_remote_layer() {
             layer.delete_resident_layer_file()?;
-            let layer_file_size = layer
-                .file_size()
-                .expect("Local layer should have a file size");
+            let layer_file_size = layer.file_size();
             self.metrics
                 .resident_physical_size_gauge
                 .sub(layer_file_size);
diff --git a/pageserver/src/tenant/upload_queue.rs b/pageserver/src/tenant/upload_queue.rs
index 790b2f59aa..08bc1f219d 100644
--- a/pageserver/src/tenant/upload_queue.rs
+++ b/pageserver/src/tenant/upload_queue.rs
@@ -127,12 +127,21 @@ impl UploadQueue {
 
         let mut files = HashMap::with_capacity(index_part.timeline_layers.len());
         for layer_name in &index_part.timeline_layers {
-            let layer_metadata = index_part
+            match index_part
                 .layer_metadata
                 .get(layer_name)
                 .map(LayerFileMetadata::from)
-                .unwrap_or(LayerFileMetadata::MISSING);
-            files.insert(layer_name.to_owned(), layer_metadata);
+            {
+                Some(layer_metadata) => {
+                    files.insert(layer_name.to_owned(), layer_metadata);
+                }
+                None => {
+                    anyhow::bail!(
+                        "No remote layer metadata found for layer {}",
+                        layer_name.file_name()
+                    );
+                }
+            }
         }
 
         let index_part_metadata = index_part.parse_metadata()?;
diff --git a/test_runner/regress/test_tenants_with_remote_storage.py b/test_runner/regress/test_tenants_with_remote_storage.py
index 769bc10280..c786f8a8e1 100644
--- a/test_runner/regress/test_tenants_with_remote_storage.py
+++ b/test_runner/regress/test_tenants_with_remote_storage.py
@@ -9,7 +9,6 @@
 import asyncio
 import json
 import os
-import shutil
 from pathlib import Path
 from typing import List, Tuple
 
@@ -217,208 +216,9 @@ def test_tenants_attached_after_download(
     assert env.pageserver.log_contains(".*download .* succeeded after 1 retries.*")
 
 
-@pytest.mark.parametrize("remote_storage_kind", [RemoteStorageKind.LOCAL_FS])
-def test_tenant_upgrades_index_json_from_v0(
-    neon_env_builder: NeonEnvBuilder, remote_storage_kind: RemoteStorageKind
-):
-    # the "image" for the v0 index_part.json. the fields themselves are
-    # replaced with values read from the later version because of #2592 (initdb
-    # lsn not reproducible).
-    v0_skeleton = json.loads(
-        """{
-        "timeline_layers":[
-            "000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__0000000001696070-00000000016960E9"
-        ],
-        "missing_layers":["This should not fail as its not used anymore"],
-        "disk_consistent_lsn":"0/16960E8",
-        "metadata_bytes":[]
-    }"""
-    )
-
-    # getting a too eager compaction happening for this test would not play
-    # well with the strict assertions.
-    neon_env_builder.pageserver_config_override = "tenant_config.compaction_period='1h'"
-
-    neon_env_builder.enable_remote_storage(
-        remote_storage_kind, "test_tenant_upgrades_index_json_from_v0"
-    )
-
-    # launch pageserver, populate the default tenants timeline, wait for it to be uploaded,
-    # then go ahead and modify the "remote" version as if it was downgraded, needing upgrade
-    env = neon_env_builder.init_start()
-
-    pageserver_http = env.pageserver.http_client()
-    pg = env.postgres.create_start("main")
-
-    tenant_id = TenantId(pg.safe_psql("show neon.tenant_id")[0][0])
-    timeline_id = TimelineId(pg.safe_psql("show neon.timeline_id")[0][0])
-
-    with pg.cursor() as cur:
-        cur.execute("CREATE TABLE t0 AS VALUES (123, 'second column as text');")
-        current_lsn = Lsn(query_scalar(cur, "SELECT pg_current_wal_flush_lsn()"))
-
-    # flush, wait until in remote storage
-    wait_for_last_record_lsn(pageserver_http, tenant_id, timeline_id, current_lsn)
-    pageserver_http.timeline_checkpoint(tenant_id, timeline_id)
-    wait_for_upload(pageserver_http, tenant_id, timeline_id, current_lsn)
-    env.postgres.stop_all()
-    env.pageserver.stop()
-
-    # remove all local data for the tenant to force redownloading and subsequent upgrade
-    shutil.rmtree(Path(env.repo_dir) / "tenants" / str(tenant_id))
-
-    # downgrade the remote file
-    timeline_path = local_fs_index_part_path(env, tenant_id, timeline_id)
-    with open(timeline_path, "r+") as timeline_file:
-        # keep the deserialized for later inspection
-        orig_index_part = json.load(timeline_file)
-
-        v0_index_part = {
-            key: orig_index_part[key]
-            for key in v0_skeleton.keys() - ["missing_layers"]  # pgserver doesn't have it anymore
-        }
-
-        timeline_file.seek(0)
-        json.dump(v0_index_part, timeline_file)
-        timeline_file.truncate(timeline_file.tell())
-
-    env.pageserver.start()
-    pageserver_http = env.pageserver.http_client()
-    pageserver_http.tenant_attach(tenant_id)
-
-    wait_until(
-        number_of_iterations=5,
-        interval=1,
-        func=lambda: assert_tenant_status(pageserver_http, tenant_id, "Active"),
-    )
-
-    pg = env.postgres.create_start("main")
-
-    with pg.cursor() as cur:
-        cur.execute("INSERT INTO t0 VALUES (234, 'test data');")
-        current_lsn = Lsn(query_scalar(cur, "SELECT pg_current_wal_flush_lsn()"))
-
-    wait_for_last_record_lsn(pageserver_http, tenant_id, timeline_id, current_lsn)
-    pageserver_http.timeline_checkpoint(tenant_id, timeline_id)
-    wait_for_upload(pageserver_http, tenant_id, timeline_id, current_lsn)
-
-    # not needed anymore
-    env.postgres.stop_all()
-    env.pageserver.stop()
-
-    # make sure the file has been upgraded back to how it started
-    index_part = local_fs_index_part(env, tenant_id, timeline_id)
-    assert index_part["version"] == orig_index_part["version"]
-    assert "missing_layers" not in index_part.keys()
-
-    # expect one more layer because of the forced checkpoint
-    assert len(index_part["timeline_layers"]) == len(orig_index_part["timeline_layers"]) + 1
-
-    # all of the same layer files are there, but they might be shuffled around
-    orig_layers = set(orig_index_part["timeline_layers"])
-    later_layers = set(index_part["timeline_layers"])
-    assert later_layers.issuperset(orig_layers)
-
-    added_layers = later_layers - orig_layers
-    assert len(added_layers) == 1
-
-    # all of metadata has been regenerated (currently just layer file size)
-    all_metadata_keys = set()
-    for layer in orig_layers:
-        orig_metadata = orig_index_part["layer_metadata"][layer]
-        new_metadata = index_part["layer_metadata"][layer]
-        assert (
-            orig_metadata == new_metadata
-        ), f"metadata for layer {layer} should not have changed {orig_metadata} vs. {new_metadata}"
-        all_metadata_keys |= set(orig_metadata.keys())
-
-    one_new_layer = next(iter(added_layers))
-    assert one_new_layer in index_part["layer_metadata"], "new layer should have metadata"
-
-    only_new_metadata = index_part["layer_metadata"][one_new_layer]
-
-    assert (
-        set(only_new_metadata.keys()).symmetric_difference(all_metadata_keys) == set()
-    ), "new layer metadata has same metadata as others"
-
-
 # FIXME: test index_part.json getting downgraded from imaginary new version
 
 
-@pytest.mark.parametrize("remote_storage_kind", [RemoteStorageKind.LOCAL_FS])
-def test_tenant_ignores_backup_file(
-    neon_env_builder: NeonEnvBuilder, remote_storage_kind: RemoteStorageKind
-):
-    # getting a too eager compaction happening for this test would not play
-    # well with the strict assertions.
-    neon_env_builder.pageserver_config_override = "tenant_config.compaction_period='1h'"
-
-    neon_env_builder.enable_remote_storage(remote_storage_kind, "test_tenant_ignores_backup_file")
-
-    # launch pageserver, populate the default tenants timeline, wait for it to be uploaded,
-    # then go ahead and modify the "remote" version as if it was downgraded, needing upgrade
-    env = neon_env_builder.init_start()
-
-    env.pageserver.allowed_errors.append(".*got backup file on the remote storage, ignoring it.*")
-
-    pageserver_http = env.pageserver.http_client()
-    pg = env.postgres.create_start("main")
-
-    tenant_id = TenantId(pg.safe_psql("show neon.tenant_id")[0][0])
-    timeline_id = TimelineId(pg.safe_psql("show neon.timeline_id")[0][0])
-
-    with pg.cursor() as cur:
-        cur.execute("CREATE TABLE t0 AS VALUES (123, 'second column as text');")
-        current_lsn = Lsn(query_scalar(cur, "SELECT pg_current_wal_flush_lsn()"))
-
-    # flush, wait until in remote storage
-    wait_for_last_record_lsn(pageserver_http, tenant_id, timeline_id, current_lsn)
-    pageserver_http.timeline_checkpoint(tenant_id, timeline_id)
-    wait_for_upload(pageserver_http, tenant_id, timeline_id, current_lsn)
-
-    env.postgres.stop_all()
-    env.pageserver.stop()
-
-    # change the remote file to have entry with .0.old suffix
-    timeline_path = local_fs_index_part_path(env, tenant_id, timeline_id)
-    with open(timeline_path, "r+") as timeline_file:
-        # keep the deserialized for later inspection
-        orig_index_part = json.load(timeline_file)
-        backup_layer_name = orig_index_part["timeline_layers"][0] + ".0.old"
-        orig_index_part["timeline_layers"].append(backup_layer_name)
-
-        timeline_file.seek(0)
-        json.dump(orig_index_part, timeline_file)
-
-    env.pageserver.start()
-    pageserver_http = env.pageserver.http_client()
-
-    wait_until(
-        number_of_iterations=5,
-        interval=1,
-        func=lambda: assert_tenant_status(pageserver_http, tenant_id, "Active"),
-    )
-
-    pg = env.postgres.create_start("main")
-
-    with pg.cursor() as cur:
-        cur.execute("INSERT INTO t0 VALUES (234, 'test data');")
-        current_lsn = Lsn(query_scalar(cur, "SELECT pg_current_wal_flush_lsn()"))
-
-    wait_for_last_record_lsn(pageserver_http, tenant_id, timeline_id, current_lsn)
-    pageserver_http.timeline_checkpoint(tenant_id, timeline_id)
-    wait_for_upload(pageserver_http, tenant_id, timeline_id, current_lsn)
-
-    # not needed anymore
-    env.postgres.stop_all()
-    env.pageserver.stop()
-
-    # the .old file is gone from newly serialized index_part
-    new_index_part = local_fs_index_part(env, tenant_id, timeline_id)
-    backup_layers = filter(lambda x: x.endswith(".old"), new_index_part["timeline_layers"])
-    assert len(list(backup_layers)) == 0
-
-
 @pytest.mark.parametrize("remote_storage_kind", [RemoteStorageKind.LOCAL_FS])
 def test_tenant_redownloads_truncated_file_on_startup(
     neon_env_builder: NeonEnvBuilder, remote_storage_kind: RemoteStorageKind

From 0f7de847856510807b5794d5c3903e058b07d066 Mon Sep 17 00:00:00 2001
From: Shany Pozin <shany@neon.tech>
Date: Wed, 22 Mar 2023 09:17:00 +0200
Subject: [PATCH 08/19] Allow calling detach on ignored tenant (#3834)

## Describe your changes
Added a query param to detach API
Allow to remove local state of a tenant even if its not in the memory
(following ignore API)
## Issue ticket number and link
#3828
## Checklist before requesting a review
- [x] I have performed a self-review of my code.
- [ ] If it is a core feature, I have added thorough tests.
- [ ] Do we need to implement analytics? if so did you add the relevant
metrics to the dashboard?
- [ ] If this PR requires public announcement, mark it with
/release-notes label and add several sentences in this section.

---------

Co-authored-by: Kirill Bulatov <kirill@neon.tech>
---
 pageserver/src/http/openapi_spec.yml      |  7 ++
 pageserver/src/http/routes.rs             |  3 +-
 pageserver/src/tenant/mgr.rs              | 32 +++++---
 test_runner/fixtures/neon_fixtures.py     | 14 +++-
 test_runner/regress/test_tenant_detach.py | 90 ++++++++++++++++++++++-
 5 files changed, 130 insertions(+), 16 deletions(-)

diff --git a/pageserver/src/http/openapi_spec.yml b/pageserver/src/http/openapi_spec.yml
index 3d3a9892bf..2098f848d5 100644
--- a/pageserver/src/http/openapi_spec.yml
+++ b/pageserver/src/http/openapi_spec.yml
@@ -351,6 +351,13 @@ paths:
         schema:
           type: string
           format: hex
+      - name: detach_ignored
+        in: query
+        required: false
+        schema:
+          type: boolean
+        description: |
+          When true, allow to detach a tenant which state is ignored.
     post:
       description: |
         Remove tenant data (including all corresponding timelines) from pageserver's memory and file system.
diff --git a/pageserver/src/http/routes.rs b/pageserver/src/http/routes.rs
index d91e421a52..04b7928d31 100644
--- a/pageserver/src/http/routes.rs
+++ b/pageserver/src/http/routes.rs
@@ -384,10 +384,11 @@ async fn timeline_delete_handler(request: Request<Body>) -> Result<Response<Body
 async fn tenant_detach_handler(request: Request<Body>) -> Result<Response<Body>, ApiError> {
     let tenant_id: TenantId = parse_request_param(&request, "tenant_id")?;
     check_permission(&request, Some(tenant_id))?;
+    let detach_ignored: Option<bool> = parse_query_param(&request, "detach_ignored")?;
 
     let state = get_state(&request);
     let conf = state.conf;
-    mgr::detach_tenant(conf, tenant_id)
+    mgr::detach_tenant(conf, tenant_id, detach_ignored.unwrap_or(false))
         .instrument(info_span!("tenant_detach", tenant = %tenant_id))
         .await?;
 
diff --git a/pageserver/src/tenant/mgr.rs b/pageserver/src/tenant/mgr.rs
index a4212ea8a6..26a2bb972c 100644
--- a/pageserver/src/tenant/mgr.rs
+++ b/pageserver/src/tenant/mgr.rs
@@ -315,10 +315,6 @@ pub async fn get_tenant(
         .get(&tenant_id)
         .ok_or(TenantStateError::NotFound(tenant_id))?;
     if active_only && !tenant.is_active() {
-        tracing::warn!(
-            "Tenant {tenant_id} is not active. Current state: {:?}",
-            tenant.current_state()
-        );
         Err(TenantStateError::NotActive(tenant_id))
     } else {
         Ok(Arc::clone(tenant))
@@ -350,17 +346,35 @@ pub enum TenantStateError {
 pub async fn detach_tenant(
     conf: &'static PageServerConf,
     tenant_id: TenantId,
+    detach_ignored: bool,
 ) -> Result<(), TenantStateError> {
-    remove_tenant_from_memory(tenant_id, async {
-        let local_tenant_directory = conf.tenant_path(&tenant_id);
+    let local_files_cleanup_operation = |tenant_id_to_clean| async move {
+        let local_tenant_directory = conf.tenant_path(&tenant_id_to_clean);
         fs::remove_dir_all(&local_tenant_directory)
             .await
             .with_context(|| {
-                format!("Failed to remove local tenant directory {local_tenant_directory:?}")
+                format!("local tenant directory {local_tenant_directory:?} removal")
             })?;
         Ok(())
-    })
-    .await
+    };
+
+    let removal_result =
+        remove_tenant_from_memory(tenant_id, local_files_cleanup_operation(tenant_id)).await;
+
+    // Ignored tenants are not present in memory and will bail the removal from memory operation.
+    // Before returning the error, check for ignored tenant removal case — we only need to clean its local files then.
+    if detach_ignored && matches!(removal_result, Err(TenantStateError::NotFound(_))) {
+        let tenant_ignore_mark = conf.tenant_ignore_mark_file_path(tenant_id);
+        if tenant_ignore_mark.exists() {
+            info!("Detaching an ignored tenant");
+            local_files_cleanup_operation(tenant_id)
+                .await
+                .with_context(|| format!("Ignored tenant {tenant_id} local files cleanup"))?;
+            return Ok(());
+        }
+    }
+
+    removal_result
 }
 
 pub async fn load_tenant(
diff --git a/test_runner/fixtures/neon_fixtures.py b/test_runner/fixtures/neon_fixtures.py
index 6429b1e940..9929d3e66b 100644
--- a/test_runner/fixtures/neon_fixtures.py
+++ b/test_runner/fixtures/neon_fixtures.py
@@ -1119,7 +1119,9 @@ def neon_env_builder(
 
 
 class PageserverApiException(Exception):
-    pass
+    def __init__(self, message, status_code: int):
+        super().__init__(message)
+        self.status_code = status_code
 
 
 class PageserverHttpClient(requests.Session):
@@ -1140,7 +1142,7 @@ class PageserverHttpClient(requests.Session):
                 msg = res.json()["msg"]
             except:  # noqa: E722
                 msg = ""
-            raise PageserverApiException(msg) from e
+            raise PageserverApiException(msg, res.status_code) from e
 
     def check_status(self):
         self.get(f"http://localhost:{self.port}/v1/status").raise_for_status()
@@ -1190,8 +1192,12 @@ class PageserverHttpClient(requests.Session):
         res = self.post(f"http://localhost:{self.port}/v1/tenant/{tenant_id}/attach")
         self.verbose_error(res)
 
-    def tenant_detach(self, tenant_id: TenantId):
-        res = self.post(f"http://localhost:{self.port}/v1/tenant/{tenant_id}/detach")
+    def tenant_detach(self, tenant_id: TenantId, detach_ignored=False):
+        params = {}
+        if detach_ignored:
+            params["detach_ignored"] = "true"
+
+        res = self.post(f"http://localhost:{self.port}/v1/tenant/{tenant_id}/detach", params=params)
         self.verbose_error(res)
 
     def tenant_load(self, tenant_id: TenantId):
diff --git a/test_runner/regress/test_tenant_detach.py b/test_runner/regress/test_tenant_detach.py
index e061ab92a4..5db79eef4a 100644
--- a/test_runner/regress/test_tenant_detach.py
+++ b/test_runner/regress/test_tenant_detach.py
@@ -264,9 +264,11 @@ def test_tenant_detach_smoke(neon_env_builder: NeonEnvBuilder):
     with pytest.raises(
         expected_exception=PageserverApiException,
         match=f"NotFound: tenant {tenant_id}",
-    ):
+    ) as excinfo:
         pageserver_http.tenant_detach(tenant_id)
 
+    assert excinfo.value.status_code == 404
+
     # the error will be printed to the log too
     env.pageserver.allowed_errors.append(".*NotFound: tenant *")
 
@@ -325,7 +327,91 @@ def test_tenant_detach_smoke(neon_env_builder: NeonEnvBuilder):
         pageserver_http.timeline_gc(tenant_id, timeline_id, 0)
 
 
-#
+# Creates and ignores a tenant, then detaches it: first, with no parameters (should fail),
+# then with parameters to force ignored tenant detach (should not fail).
+def test_tenant_detach_ignored_tenant(neon_simple_env: NeonEnv):
+    env = neon_simple_env
+    client = env.pageserver.http_client()
+
+    # create a new tenant
+    tenant_id, _ = env.neon_cli.create_tenant()
+
+    # assert tenant exists on disk
+    assert (env.repo_dir / "tenants" / str(tenant_id)).exists()
+
+    pg = env.postgres.create_start("main", tenant_id=tenant_id)
+    # we rely upon autocommit after each statement
+    pg.safe_psql_many(
+        queries=[
+            "CREATE TABLE t(key int primary key, value text)",
+            "INSERT INTO t SELECT generate_series(1,100000), 'payload'",
+        ]
+    )
+
+    # ignore tenant
+    client.tenant_ignore(tenant_id)
+    env.pageserver.allowed_errors.append(".*NotFound: tenant .*")
+    # ensure tenant couldn't be detached without the special flag for ignored tenant
+    log.info("detaching ignored tenant WITHOUT required flag")
+    with pytest.raises(
+        expected_exception=PageserverApiException, match=f"NotFound: tenant {tenant_id}"
+    ):
+        client.tenant_detach(tenant_id)
+
+    log.info("tenant detached failed as expected")
+
+    # ensure tenant is detached with ignore state
+    log.info("detaching ignored tenant with required flag")
+    client.tenant_detach(tenant_id, True)
+    log.info("ignored tenant detached without error")
+
+    # check that nothing is left on disk for deleted tenant
+    assert not (env.repo_dir / "tenants" / str(tenant_id)).exists()
+
+    # assert the tenant does not exists in the Pageserver
+    tenants_after_detach = [tenant["id"] for tenant in client.tenant_list()]
+    assert (
+        tenant_id not in tenants_after_detach
+    ), f"Ignored and then detached tenant {tenant_id} \
+        should not be present in pageserver's memory"
+
+
+# Creates a tenant, and detaches it with extra paremeter that forces ignored tenant detach.
+# Tenant should be detached without issues.
+def test_tenant_detach_regular_tenant(neon_simple_env: NeonEnv):
+    env = neon_simple_env
+    client = env.pageserver.http_client()
+
+    # create a new tenant
+    tenant_id, _ = env.neon_cli.create_tenant()
+
+    # assert tenant exists on disk
+    assert (env.repo_dir / "tenants" / str(tenant_id)).exists()
+
+    pg = env.postgres.create_start("main", tenant_id=tenant_id)
+    # we rely upon autocommit after each statement
+    pg.safe_psql_many(
+        queries=[
+            "CREATE TABLE t(key int primary key, value text)",
+            "INSERT INTO t SELECT generate_series(1,100000), 'payload'",
+        ]
+    )
+
+    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
+    assert not (env.repo_dir / "tenants" / str(tenant_id)).exists()
+
+    # assert the tenant does not exists in the Pageserver
+    tenants_after_detach = [tenant["id"] for tenant in client.tenant_list()]
+    assert (
+        tenant_id not in tenants_after_detach
+    ), f"Ignored and then detached tenant {tenant_id} \
+        should not be present in pageserver's memory"
+
+
 @pytest.mark.parametrize("remote_storage_kind", available_remote_storages())
 def test_detach_while_attaching(
     neon_env_builder: NeonEnvBuilder,

From 14a40c9ca67568beac3783f597e44990c2c9a9e4 Mon Sep 17 00:00:00 2001
From: mikecaat <35882227+mikecaat@users.noreply.github.com>
Date: Wed, 22 Mar 2023 17:10:53 +0900
Subject: [PATCH 09/19] Fix minor things for the docker-compose file (#3862)

* Add the REPOSITORY env to build args to avoid the following error when
executing without the credentials for the repository.

```
ERROR: Service 'compute' failed to build: Head
"https://369495373322.dkr.ecr.eu-central-1.amazonaws.com/v2/compute-node-v15/manifests/2221":
no basic auth credentials
```

* update the tag version in the documentation to support storage broker
---
 docker-compose/docker-compose.yml | 1 +
 docs/docker.md                    | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/docker-compose/docker-compose.yml b/docker-compose/docker-compose.yml
index b24cb80ce4..4926dad932 100644
--- a/docker-compose/docker-compose.yml
+++ b/docker-compose/docker-compose.yml
@@ -160,6 +160,7 @@ services:
     build:
       context: ./compute_wrapper/
       args:
+        - REPOSITORY=${REPOSITORY:-neondatabase}
         - COMPUTE_IMAGE=compute-node-v${PG_VERSION:-14}
         - TAG=${TAG:-latest}
         - http_proxy=$http_proxy
diff --git a/docs/docker.md b/docs/docker.md
index d264a1a748..704044377f 100644
--- a/docs/docker.md
+++ b/docs/docker.md
@@ -37,9 +37,9 @@ You can specify version of neon cluster using following environment values.
 - PG_VERSION: postgres version for compute (default is 14)
 - TAG: the tag version of [docker image](https://registry.hub.docker.com/r/neondatabase/neon/tags) (default is latest), which is tagged in [CI test](/.github/workflows/build_and_test.yml)
 ```
-$ cd docker-compose/docker-compose.yml
+$ cd docker-compose/
 $ docker-compose down   # remove the conainers if exists
-$ PG_VERSION=15 TAG=2221 docker-compose up --build -d  # You can specify the postgres and image version
+$ PG_VERSION=15 TAG=2937 docker-compose up --build -d  # You can specify the postgres and image version
 Creating network "dockercompose_default" with the default driver
 Creating docker-compose_storage_broker_1       ... done
 (...omit...)

From 6033dfdf4a9cbc5f81db551d6a8b259445f390c5 Mon Sep 17 00:00:00 2001
From: Joonas Koivunen <joonas@neon.tech>
Date: Wed, 22 Mar 2023 16:26:27 +0200
Subject: [PATCH 10/19] Re-access layers before threshold eviction (#3867)

To avoid re-downloading evicted files on restart, re-compute logical
size and partitioning before each threshold based eviction run.

Cc: #3802

Co-authored-by: Christian Schwarz <christian@neon.tech>
---
 .../src/tenant/timeline/eviction_task.rs      | 81 +++++++++++++++++--
 1 file changed, 74 insertions(+), 7 deletions(-)

diff --git a/pageserver/src/tenant/timeline/eviction_task.rs b/pageserver/src/tenant/timeline/eviction_task.rs
index 2aad0ef0f3..666768ff87 100644
--- a/pageserver/src/tenant/timeline/eviction_task.rs
+++ b/pageserver/src/tenant/timeline/eviction_task.rs
@@ -1,5 +1,18 @@
-//! The per-timeline layer eviction task.
-
+//! The per-timeline layer eviction task, which evicts data which has not been accessed for more
+//! than a given threshold.
+//!
+//! Data includes all kinds of caches, namely:
+//! - (in-memory layers)
+//! - on-demand downloaded layer files on disk
+//! - (cached layer file pages)
+//! - derived data from layer file contents, namely:
+//!     - initial logical size
+//!     - partitioning
+//!     - (other currently missing unknowns)
+//!
+//! Items with parentheses are not (yet) touched by this task.
+//!
+//! See write-up on restart on-demand download spike: <https://gist.github.com/problame/2265bf7b8dc398be834abfead36c76b5>
 use std::{
     ops::ControlFlow,
     sync::Arc,
@@ -12,6 +25,7 @@ use tokio_util::sync::CancellationToken;
 use tracing::{debug, error, info, instrument, warn};
 
 use crate::{
+    context::{DownloadBehavior, RequestContext},
     task_mgr::{self, TaskKind, BACKGROUND_RUNTIME},
     tenant::{
         config::{EvictionPolicy, EvictionPolicyLayerAccessThreshold},
@@ -54,9 +68,10 @@ impl Timeline {
             }
         }
 
+        let ctx = RequestContext::new(TaskKind::Eviction, DownloadBehavior::Warn);
         loop {
             let policy = self.get_eviction_policy();
-            let cf = self.eviction_iteration(&policy, cancel.clone()).await;
+            let cf = self.eviction_iteration(&policy, &cancel, &ctx).await;
 
             match cf {
                 ControlFlow::Break(()) => break,
@@ -77,7 +92,8 @@ impl Timeline {
     async fn eviction_iteration(
         self: &Arc<Self>,
         policy: &EvictionPolicy,
-        cancel: CancellationToken,
+        cancel: &CancellationToken,
+        ctx: &RequestContext,
     ) -> ControlFlow<(), Instant> {
         debug!("eviction iteration: {policy:?}");
         match policy {
@@ -87,7 +103,7 @@ impl Timeline {
             }
             EvictionPolicy::LayerAccessThreshold(p) => {
                 let start = Instant::now();
-                match self.eviction_iteration_threshold(p, cancel).await {
+                match self.eviction_iteration_threshold(p, cancel, ctx).await {
                     ControlFlow::Break(()) => return ControlFlow::Break(()),
                     ControlFlow::Continue(()) => (),
                 }
@@ -101,7 +117,8 @@ impl Timeline {
     async fn eviction_iteration_threshold(
         self: &Arc<Self>,
         p: &EvictionPolicyLayerAccessThreshold,
-        cancel: CancellationToken,
+        cancel: &CancellationToken,
+        ctx: &RequestContext,
     ) -> ControlFlow<()> {
         let now = SystemTime::now();
 
@@ -114,6 +131,20 @@ impl Timeline {
             not_evictable: usize,
             skipped_for_shutdown: usize,
         }
+
+        // what we want is to invalidate any caches which haven't been accessed for `p.threshold`,
+        // but we cannot actually do it for current limitations except by restarting pageserver. we
+        // just recompute the values which would be recomputed on startup.
+        //
+        // for active tenants this will likely materialized page cache or in-memory layers. for
+        // inactive tenants it will refresh the last_access timestamps so that we will not evict
+        // and re-download on restart these layers.
+        self.refresh_layers_required_in_restart(cancel, ctx).await;
+
+        if cancel.is_cancelled() {
+            return ControlFlow::Break(());
+        }
+
         let mut stats = EvictionStats::default();
         // Gather layers for eviction.
         // NB: all the checks can be invalidated as soon as we release the layer map lock.
@@ -174,7 +205,7 @@ impl Timeline {
         };
 
         let results = match self
-            .evict_layer_batch(remote_client, &candidates[..], cancel)
+            .evict_layer_batch(remote_client, &candidates[..], cancel.clone())
             .await
         {
             Err(pre_err) => {
@@ -216,4 +247,40 @@ impl Timeline {
         }
         ControlFlow::Continue(())
     }
+
+    /// Recompute the values which would cause on-demand downloads during restart.
+    async fn refresh_layers_required_in_restart(
+        &self,
+        cancel: &CancellationToken,
+        ctx: &RequestContext,
+    ) {
+        let lsn = self.get_last_record_lsn();
+
+        // imitiate on-restart initial logical size
+        let size = self.calculate_logical_size(lsn, cancel.clone(), ctx).await;
+
+        match &size {
+            Ok(_size) => {
+                // good, don't log it to avoid confusion
+            }
+            Err(_) => {
+                // we have known issues for which we already log this on consumption metrics,
+                // gc, and compaction. leave logging out for now.
+                //
+                // https://github.com/neondatabase/neon/issues/2539
+            }
+        }
+
+        // imitiate repartiting on first compactation
+        if let Err(e) = self.collect_keyspace(lsn, ctx).await {
+            // if this failed, we probably failed logical size because these use the same keys
+            if size.is_err() {
+                // ignore, see above comment
+            } else {
+                warn!(
+                    "failed to collect keyspace but succeeded in calculating logical size: {e:#}"
+                );
+            }
+        }
+    }
 }

From 8bd565e09ede7b3af0e0dddce68968ba02ac8b54 Mon Sep 17 00:00:00 2001
From: Kirill Bulatov <kirill@neon.tech>
Date: Wed, 22 Mar 2023 17:42:31 +0200
Subject: [PATCH 11/19] Ensure branches with no layers have their remote
 storage counterpart created eventually (#3857)

Discovered during writing a test for
https://github.com/neondatabase/neon/pull/3843
---
 pageserver/src/tenant.rs                   |  39 +++--
 pageserver/src/tenant/timeline.rs          |  51 +++++--
 test_runner/regress/test_remote_storage.py | 164 ++++++++++++++++++---
 3 files changed, 205 insertions(+), 49 deletions(-)

diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs
index 5f1e23b873..b462c93b2d 100644
--- a/pageserver/src/tenant.rs
+++ b/pageserver/src/tenant.rs
@@ -478,7 +478,7 @@ impl Tenant {
 
             let dummy_timeline = self.create_timeline_data(
                 timeline_id,
-                up_to_date_metadata.clone(),
+                up_to_date_metadata,
                 ancestor.clone(),
                 remote_client,
             )?;
@@ -503,7 +503,7 @@ impl Tenant {
                     let broken_timeline = self
                         .create_timeline_data(
                             timeline_id,
-                            up_to_date_metadata.clone(),
+                            up_to_date_metadata,
                             ancestor.clone(),
                             None,
                         )
@@ -1142,7 +1142,7 @@ impl Tenant {
         );
         self.prepare_timeline(
             new_timeline_id,
-            new_metadata,
+            &new_metadata,
             timeline_uninit_mark,
             true,
             None,
@@ -1700,7 +1700,7 @@ impl Tenant {
     fn create_timeline_data(
         &self,
         new_timeline_id: TimelineId,
-        new_metadata: TimelineMetadata,
+        new_metadata: &TimelineMetadata,
         ancestor: Option<Arc<Timeline>>,
         remote_client: Option<RemoteTimelineClient>,
     ) -> anyhow::Result<Arc<Timeline>> {
@@ -2160,13 +2160,25 @@ impl Tenant {
         let new_timeline = self
             .prepare_timeline(
                 dst_id,
-                metadata,
+                &metadata,
                 timeline_uninit_mark,
                 false,
                 Some(Arc::clone(src_timeline)),
             )?
             .initialize_with_lock(&mut timelines, true, true)?;
         drop(timelines);
+
+        // Root timeline gets its layers during creation and uploads them along with the metadata.
+        // A branch timeline though, when created, can get no writes for some time, hence won't get any layers created.
+        // We still need to upload its metadata eagerly: if other nodes `attach` the tenant and miss this timeline, their GC
+        // could get incorrect information and remove more layers, than needed.
+        // See also https://github.com/neondatabase/neon/issues/3865
+        if let Some(remote_client) = new_timeline.remote_client.as_ref() {
+            remote_client
+                .schedule_index_upload_for_metadata_update(&metadata)
+                .context("branch initial metadata upload")?;
+        }
+
         info!("branched timeline {dst_id} from {src_id} at {start_lsn}");
 
         Ok(new_timeline)
@@ -2229,7 +2241,7 @@ impl Tenant {
             pg_version,
         );
         let raw_timeline =
-            self.prepare_timeline(timeline_id, new_metadata, timeline_uninit_mark, true, None)?;
+            self.prepare_timeline(timeline_id, &new_metadata, timeline_uninit_mark, true, None)?;
 
         let tenant_id = raw_timeline.owning_tenant.tenant_id;
         let unfinished_timeline = raw_timeline.raw_timeline()?;
@@ -2283,7 +2295,7 @@ impl Tenant {
     fn prepare_timeline(
         &self,
         new_timeline_id: TimelineId,
-        new_metadata: TimelineMetadata,
+        new_metadata: &TimelineMetadata,
         uninit_mark: TimelineUninitMark,
         init_layers: bool,
         ancestor: Option<Arc<Timeline>>,
@@ -2297,7 +2309,7 @@ impl Tenant {
                 tenant_id,
                 new_timeline_id,
             );
-            remote_client.init_upload_queue_for_empty_remote(&new_metadata)?;
+            remote_client.init_upload_queue_for_empty_remote(new_metadata)?;
             Some(remote_client)
         } else {
             None
@@ -2336,17 +2348,12 @@ impl Tenant {
         &self,
         timeline_path: &Path,
         new_timeline_id: TimelineId,
-        new_metadata: TimelineMetadata,
+        new_metadata: &TimelineMetadata,
         ancestor: Option<Arc<Timeline>>,
         remote_client: Option<RemoteTimelineClient>,
     ) -> anyhow::Result<Arc<Timeline>> {
         let timeline_data = self
-            .create_timeline_data(
-                new_timeline_id,
-                new_metadata.clone(),
-                ancestor,
-                remote_client,
-            )
+            .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")?;
 
@@ -2358,7 +2365,7 @@ impl Tenant {
             self.conf,
             new_timeline_id,
             self.tenant_id,
-            &new_metadata,
+            new_metadata,
             true,
         )
         .context("Failed to create timeline metadata")?;
diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs
index 4d03a78883..33909e749b 100644
--- a/pageserver/src/tenant/timeline.rs
+++ b/pageserver/src/tenant/timeline.rs
@@ -1163,7 +1163,7 @@ impl Timeline {
     pub(super) fn new(
         conf: &'static PageServerConf,
         tenant_conf: Arc<RwLock<TenantConfOpt>>,
-        metadata: TimelineMetadata,
+        metadata: &TimelineMetadata,
         ancestor: Option<Arc<Timeline>>,
         timeline_id: TimelineId,
         tenant_id: TenantId,
@@ -1629,6 +1629,8 @@ impl Timeline {
             .map(|l| (l.filename(), l))
             .collect::<HashMap<_, _>>();
 
+        // If no writes happen, new branches do not have any layers, only the metadata file.
+        let has_local_layers = !local_layers.is_empty();
         let local_only_layers = match index_part {
             Some(index_part) => {
                 info!(
@@ -1646,21 +1648,40 @@ impl Timeline {
             }
         };
 
-        // Are there local files that don't exist remotely? Schedule uploads for them
-        for (layer_name, layer) in &local_only_layers {
-            // XXX solve this in the type system
-            let layer_path = layer
-                .local_path()
-                .expect("local_only_layers only contains local layers");
-            let layer_size = layer_path
-                .metadata()
-                .with_context(|| format!("failed to get file {layer_path:?} metadata"))?
-                .len();
-            info!("scheduling {layer_path:?} for upload");
-            remote_client
-                .schedule_layer_file_upload(layer_name, &LayerFileMetadata::new(layer_size))?;
+        if has_local_layers {
+            // Are there local files that don't exist remotely? Schedule uploads for them.
+            // Local timeline metadata will get uploaded to remove along witht he layers.
+            for (layer_name, layer) in &local_only_layers {
+                // XXX solve this in the type system
+                let layer_path = layer
+                    .local_path()
+                    .expect("local_only_layers only contains local layers");
+                let layer_size = layer_path
+                    .metadata()
+                    .with_context(|| format!("failed to get file {layer_path:?} metadata"))?
+                    .len();
+                info!("scheduling {layer_path:?} for upload");
+                remote_client
+                    .schedule_layer_file_upload(layer_name, &LayerFileMetadata::new(layer_size))?;
+            }
+            remote_client.schedule_index_upload_for_file_changes()?;
+        } else if index_part.is_none() {
+            // No data on the remote storage, no local layers, local metadata file.
+            //
+            // TODO https://github.com/neondatabase/neon/issues/3865
+            // Currently, console does not wait for the timeline data upload to the remote storage
+            // and considers the timeline created, expecting other pageserver nodes to work with it.
+            // Branch metadata upload could get interrupted (e.g pageserver got killed),
+            // hence any locally existing branch metadata with no remote counterpart should be uploaded,
+            // otherwise any other pageserver won't see the branch on `attach`.
+            //
+            // After the issue gets implemented, pageserver should rather remove the branch,
+            // since absence on S3 means we did not acknowledge the branch creation and console will have to retry,
+            // no need to keep the old files.
+            remote_client.schedule_index_upload_for_metadata_update(up_to_date_metadata)?;
+        } else {
+            // Local timeline has a metadata file, remote one too, both have no layers to sync.
         }
-        remote_client.schedule_index_upload_for_file_changes()?;
 
         info!("Done");
 
diff --git a/test_runner/regress/test_remote_storage.py b/test_runner/regress/test_remote_storage.py
index 1f6f0c67cc..f6600e8974 100644
--- a/test_runner/regress/test_remote_storage.py
+++ b/test_runner/regress/test_remote_storage.py
@@ -11,8 +11,10 @@ from typing import Dict, List, Tuple
 import pytest
 from fixtures.log_helper import log
 from fixtures.neon_fixtures import (
+    LocalFsStorage,
     NeonEnvBuilder,
     PageserverApiException,
+    PageserverHttpClient,
     RemoteStorageKind,
     available_remote_storages,
     wait_for_last_flush_lsn,
@@ -421,23 +423,6 @@ def test_remote_timeline_client_calls_started_metric(
         )
         wait_for_last_flush_lsn(env, pg, tenant_id, timeline_id)
 
-    def get_queued_count(file_kind, op_kind):
-        val = client.get_remote_timeline_client_metric(
-            "pageserver_remote_timeline_client_calls_unfinished",
-            tenant_id,
-            timeline_id,
-            file_kind,
-            op_kind,
-        )
-        if val is None:
-            return val
-        return int(val)
-
-    def wait_upload_queue_empty():
-        wait_until(2, 1, lambda: get_queued_count(file_kind="layer", op_kind="upload") == 0)
-        wait_until(2, 1, lambda: get_queued_count(file_kind="index", op_kind="upload") == 0)
-        wait_until(2, 1, lambda: get_queued_count(file_kind="layer", op_kind="delete") == 0)
-
     calls_started: Dict[Tuple[str, str], List[int]] = {
         ("layer", "upload"): [0],
         ("index", "upload"): [0],
@@ -478,7 +463,7 @@ def test_remote_timeline_client_calls_started_metric(
     # create some layers & wait for uploads to finish
     churn("a", "b")
 
-    wait_upload_queue_empty()
+    wait_upload_queue_empty(client, tenant_id, timeline_id)
 
     # ensure that we updated the calls_started metric
     fetch_calls_started()
@@ -637,4 +622,147 @@ def test_timeline_deletion_with_files_stuck_in_upload_queue(
     time.sleep(10)
 
 
+# Branches off a root branch, but does not write anything to the new branch, so it has a metadata file only.
+# Ensures that such branch is still persisted on the remote storage, and can be restored during tenant (re)attach.
+@pytest.mark.parametrize("remote_storage_kind", [RemoteStorageKind.LOCAL_FS])
+def test_empty_branch_remote_storage_upload(
+    neon_env_builder: NeonEnvBuilder,
+    remote_storage_kind: RemoteStorageKind,
+):
+    neon_env_builder.enable_remote_storage(
+        remote_storage_kind=remote_storage_kind,
+        test_name="test_empty_branch_remote_storage_upload",
+    )
+
+    env = neon_env_builder.init_start()
+    client = env.pageserver.http_client()
+
+    new_branch_name = "new_branch"
+    new_branch_timeline_id = env.neon_cli.create_branch(new_branch_name, "main", env.initial_tenant)
+
+    with env.postgres.create_start(new_branch_name, tenant_id=env.initial_tenant) as pg:
+        wait_for_last_flush_lsn(env, pg, env.initial_tenant, new_branch_timeline_id)
+    wait_upload_queue_empty(client, env.initial_tenant, new_branch_timeline_id)
+
+    timelines_before_detach = set(
+        map(
+            lambda t: TimelineId(t["timeline_id"]),
+            client.timeline_list(env.initial_tenant),
+        )
+    )
+    expected_timelines = set([env.initial_timeline, new_branch_timeline_id])
+    assert (
+        timelines_before_detach == expected_timelines
+    ), f"Expected to have an initial timeline and the branch timeline only, but got {timelines_before_detach}"
+
+    client.tenant_detach(env.initial_tenant)
+    client.tenant_attach(env.initial_tenant)
+    wait_until_tenant_state(client, env.initial_tenant, "Active", 5)
+
+    timelines_after_detach = set(
+        map(
+            lambda t: TimelineId(t["timeline_id"]),
+            client.timeline_list(env.initial_tenant),
+        )
+    )
+
+    assert (
+        timelines_before_detach == timelines_after_detach
+    ), f"Expected to have same timelines after reattach, but got {timelines_after_detach}"
+
+
+# Branches off a root branch, but does not write anything to the new branch, so it has a metadata file only.
+# Ensures the branch is not on the remote storage and restarts the pageserver — the branch should be uploaded after the restart.
+@pytest.mark.parametrize("remote_storage_kind", [RemoteStorageKind.LOCAL_FS])
+def test_empty_branch_remote_storage_upload_on_restart(
+    neon_env_builder: NeonEnvBuilder,
+    remote_storage_kind: RemoteStorageKind,
+):
+    neon_env_builder.enable_remote_storage(
+        remote_storage_kind=remote_storage_kind,
+        test_name="test_empty_branch_remote_storage_upload_on_restart",
+    )
+
+    env = neon_env_builder.init_start()
+    client = env.pageserver.http_client()
+
+    new_branch_name = "new_branch"
+    new_branch_timeline_id = env.neon_cli.create_branch(new_branch_name, "main", env.initial_tenant)
+
+    with env.postgres.create_start(new_branch_name, tenant_id=env.initial_tenant) as pg:
+        wait_for_last_flush_lsn(env, pg, env.initial_tenant, new_branch_timeline_id)
+    wait_upload_queue_empty(client, env.initial_tenant, new_branch_timeline_id)
+
+    env.pageserver.stop()
+
+    # Remove new branch from the remote storage
+    assert isinstance(env.remote_storage, LocalFsStorage)
+    new_branch_on_remote_storage = (
+        env.remote_storage.root
+        / "tenants"
+        / str(env.initial_tenant)
+        / "timelines"
+        / str(new_branch_timeline_id)
+    )
+    assert (
+        new_branch_on_remote_storage.is_dir()
+    ), f"'{new_branch_on_remote_storage}' path does not exist on the remote storage"
+    shutil.rmtree(new_branch_on_remote_storage)
+
+    env.pageserver.start()
+
+    wait_upload_queue_empty(client, env.initial_tenant, new_branch_timeline_id)
+    assert (
+        new_branch_on_remote_storage.is_dir()
+    ), f"New branch should have been reuploaded on pageserver restart to the remote storage path '{new_branch_on_remote_storage}'"
+
+
+def wait_upload_queue_empty(
+    client: PageserverHttpClient, tenant_id: TenantId, timeline_id: TimelineId
+):
+    wait_until(
+        2,
+        1,
+        lambda: get_queued_count(
+            client, tenant_id, timeline_id, file_kind="layer", op_kind="upload"
+        )
+        == 0,
+    )
+    wait_until(
+        2,
+        1,
+        lambda: get_queued_count(
+            client, tenant_id, timeline_id, file_kind="index", op_kind="upload"
+        )
+        == 0,
+    )
+    wait_until(
+        2,
+        1,
+        lambda: get_queued_count(
+            client, tenant_id, timeline_id, file_kind="layer", op_kind="delete"
+        )
+        == 0,
+    )
+
+
+def get_queued_count(
+    client: PageserverHttpClient,
+    tenant_id: TenantId,
+    timeline_id: TimelineId,
+    file_kind: str,
+    op_kind: str,
+):
+    val = client.get_remote_timeline_client_metric(
+        "pageserver_remote_timeline_client_calls_unfinished",
+        tenant_id,
+        timeline_id,
+        file_kind,
+        op_kind,
+    )
+    if val is None:
+        return val
+    return int(val)
+
+
 # TODO Test that we correctly handle GC of files that are stuck in upload queue.

From f5ca897292d78faf6a447a8f4542b1b1400e6dd1 Mon Sep 17 00:00:00 2001
From: Joonas Koivunen <joonas@neon.tech>
Date: Thu, 23 Mar 2023 12:00:52 +0200
Subject: [PATCH 12/19] fix: less logging at shutdown (#3866)

Log less during shutdown; don't log anything for quickly (less than 1s)
exiting tasks.
---
 pageserver/src/task_mgr.rs | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/pageserver/src/task_mgr.rs b/pageserver/src/task_mgr.rs
index 2734031a09..44b1bbb06d 100644
--- a/pageserver/src/task_mgr.rs
+++ b/pageserver/src/task_mgr.rs
@@ -481,13 +481,25 @@ pub async fn shutdown_tasks(
     for task in victim_tasks {
         let join_handle = {
             let mut task_mut = task.mutable.lock().unwrap();
-            info!("waiting for {} to shut down", task.name);
-            let join_handle = task_mut.join_handle.take();
-            drop(task_mut);
-            join_handle
+            task_mut.join_handle.take()
         };
-        if let Some(join_handle) = join_handle {
-            let _ = join_handle.await;
+        if let Some(mut join_handle) = join_handle {
+            let completed = tokio::select! {
+                _ = &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
+                    // lines.
+                    info!("waiting for {} to shut down", task.name);
+                    false
+                }
+            };
+            if !completed {
+                // we never handled this return value, but:
+                // - we don't deschedule which would lead to is_cancelled
+                // - panics are already logged (is_panicked)
+                // - task errors are already logged in the wrapper
+                let _ = join_handle.await;
+            }
         } else {
             // Possibly one of:
             //  * The task had not even fully started yet.

From 870ba43a1ff6840b56d64709a59e3616e4040870 Mon Sep 17 00:00:00 2001
From: Dmitry Rodionov <dmitry@neon.tech>
Date: Fri, 24 Mar 2023 20:25:39 +0300
Subject: [PATCH 13/19] return proper http codes in timeline delete endpoint
 (#3876)

return proper http codes in timeline delete endpoint
+ fix openapi spec for detach to include 404 responses
---
 pageserver/src/http/openapi_spec.yml        | 12 +++++++++++
 pageserver/src/http/routes.rs               | 23 +++++++++++++++++++++
 pageserver/src/tenant.rs                    | 22 ++++++++++++++------
 pageserver/src/tenant/mgr.rs                | 11 +++++++++-
 test_runner/regress/test_timeline_delete.py | 12 ++++++++---
 5 files changed, 70 insertions(+), 10 deletions(-)

diff --git a/pageserver/src/http/openapi_spec.yml b/pageserver/src/http/openapi_spec.yml
index 2098f848d5..b8c3bffcd5 100644
--- a/pageserver/src/http/openapi_spec.yml
+++ b/pageserver/src/http/openapi_spec.yml
@@ -183,6 +183,12 @@ paths:
             application/json:
               schema:
                 $ref: "#/components/schemas/ForbiddenError"
+        "404":
+          description: Timeline not found
+          content:
+            application/json:
+              schema:
+                $ref: "#/components/schemas/NotFoundError"
         "500":
           description: Generic operation error
           content:
@@ -383,6 +389,12 @@ paths:
             application/json:
               schema:
                 $ref: "#/components/schemas/ForbiddenError"
+        "404":
+          description: Tenant not found
+          content:
+            application/json:
+              schema:
+                $ref: "#/components/schemas/NotFoundError"
         "500":
           description: Generic operation error
           content:
diff --git a/pageserver/src/http/routes.rs b/pageserver/src/http/routes.rs
index 04b7928d31..ba53729ea9 100644
--- a/pageserver/src/http/routes.rs
+++ b/pageserver/src/http/routes.rs
@@ -131,6 +131,29 @@ impl From<TenantStateError> for ApiError {
     }
 }
 
+impl From<crate::tenant::DeleteTimelineError> for ApiError {
+    fn from(value: crate::tenant::DeleteTimelineError) -> Self {
+        use crate::tenant::DeleteTimelineError::*;
+        match value {
+            NotFound => ApiError::NotFound(anyhow::anyhow!("timeline not found")),
+            HasChildren => ApiError::BadRequest(anyhow::anyhow!(
+                "Cannot delete timeline which has child timelines"
+            )),
+            Other(e) => ApiError::InternalServerError(e),
+        }
+    }
+}
+
+impl From<crate::tenant::mgr::DeleteTimelineError> for ApiError {
+    fn from(value: crate::tenant::mgr::DeleteTimelineError) -> Self {
+        use crate::tenant::mgr::DeleteTimelineError::*;
+        match value {
+            Tenant(t) => ApiError::from(t),
+            Timeline(t) => ApiError::from(t),
+        }
+    }
+}
+
 // Helper function to construct a TimelineInfo struct for a timeline
 async fn build_timeline_info(
     timeline: &Arc<Timeline>,
diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs
index b462c93b2d..0a167fd787 100644
--- a/pageserver/src/tenant.rs
+++ b/pageserver/src/tenant.rs
@@ -431,6 +431,16 @@ remote:
     }
 }
 
+#[derive(Debug, thiserror::Error)]
+pub enum DeleteTimelineError {
+    #[error("NotFound")]
+    NotFound,
+    #[error("HasChildren")]
+    HasChildren,
+    #[error(transparent)]
+    Other(#[from] anyhow::Error),
+}
+
 struct RemoteStartupData {
     index_part: IndexPart,
     remote_metadata: TimelineMetadata,
@@ -1307,7 +1317,7 @@ impl Tenant {
         &self,
         timeline_id: TimelineId,
         _ctx: &RequestContext,
-    ) -> anyhow::Result<()> {
+    ) -> Result<(), DeleteTimelineError> {
         // Transition the timeline into TimelineState::Stopping.
         // This should prevent new operations from starting.
         let timeline = {
@@ -1319,13 +1329,13 @@ impl Tenant {
                 .iter()
                 .any(|(_, entry)| entry.get_ancestor_timeline_id() == Some(timeline_id));
 
-            anyhow::ensure!(
-                !children_exist,
-                "Cannot delete timeline which has child timelines"
-            );
+            if children_exist {
+                return Err(DeleteTimelineError::HasChildren);
+            }
+
             let timeline_entry = match timelines.entry(timeline_id) {
                 Entry::Occupied(e) => e,
-                Entry::Vacant(_) => bail!("timeline not found"),
+                Entry::Vacant(_) => return Err(DeleteTimelineError::NotFound),
             };
 
             let timeline = Arc::clone(timeline_entry.get());
diff --git a/pageserver/src/tenant/mgr.rs b/pageserver/src/tenant/mgr.rs
index 26a2bb972c..4971186206 100644
--- a/pageserver/src/tenant/mgr.rs
+++ b/pageserver/src/tenant/mgr.rs
@@ -321,11 +321,20 @@ pub async fn get_tenant(
     }
 }
 
+#[derive(Debug, thiserror::Error)]
+pub enum DeleteTimelineError {
+    #[error("Tenant {0}")]
+    Tenant(#[from] TenantStateError),
+
+    #[error("Timeline {0}")]
+    Timeline(#[from] crate::tenant::DeleteTimelineError),
+}
+
 pub async fn delete_timeline(
     tenant_id: TenantId,
     timeline_id: TimelineId,
     ctx: &RequestContext,
-) -> Result<(), TenantStateError> {
+) -> Result<(), DeleteTimelineError> {
     let tenant = get_tenant(tenant_id, true).await?;
     tenant.delete_timeline(timeline_id, ctx).await?;
     Ok(())
diff --git a/test_runner/regress/test_timeline_delete.py b/test_runner/regress/test_timeline_delete.py
index b9c4f5b83f..30d894e04c 100644
--- a/test_runner/regress/test_timeline_delete.py
+++ b/test_runner/regress/test_timeline_delete.py
@@ -25,9 +25,11 @@ def test_timeline_delete(neon_simple_env: NeonEnv):
     with pytest.raises(
         PageserverApiException,
         match=f"NotFound: tenant {invalid_tenant_id}",
-    ):
+    ) as exc:
         ps_http.timeline_delete(tenant_id=invalid_tenant_id, timeline_id=invalid_timeline_id)
 
+    assert exc.value.status_code == 404
+
     # construct pair of branches to validate that pageserver prohibits
     # deletion of ancestor timelines when they have child branches
     parent_timeline_id = env.neon_cli.create_branch("test_ancestor_branch_delete_parent", "empty")
@@ -39,7 +41,7 @@ def test_timeline_delete(neon_simple_env: NeonEnv):
     ps_http = env.pageserver.http_client()
     with pytest.raises(
         PageserverApiException, match="Cannot delete timeline which has child timelines"
-    ):
+    ) as exc:
         timeline_path = (
             env.repo_dir
             / "tenants"
@@ -53,6 +55,8 @@ def test_timeline_delete(neon_simple_env: NeonEnv):
 
         assert not timeline_path.exists()
 
+    assert exc.value.status_code == 400
+
     timeline_path = (
         env.repo_dir / "tenants" / str(env.initial_tenant) / "timelines" / str(leaf_timeline_id)
     )
@@ -71,7 +75,7 @@ def test_timeline_delete(neon_simple_env: NeonEnv):
     with pytest.raises(
         PageserverApiException,
         match=f"Timeline {env.initial_tenant}/{leaf_timeline_id} was not found",
-    ):
+    ) as exc:
         ps_http.timeline_detail(env.initial_tenant, leaf_timeline_id)
 
         # FIXME leaves tenant without timelines, should we prevent deletion of root timeline?
@@ -80,3 +84,5 @@ def test_timeline_delete(neon_simple_env: NeonEnv):
             interval=0.2,
             func=lambda: ps_http.timeline_delete(env.initial_tenant, parent_timeline_id),
         )
+
+    assert exc.value.status_code == 404

From 4071ff8c7b699565d79f772e44ca2423e00a6a3b Mon Sep 17 00:00:00 2001
From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com>
Date: Sat, 25 Mar 2023 12:33:39 +0000
Subject: [PATCH 14/19] Bump openssl from 0.10.45 to 0.10.48 in
 /test_runner/pg_clients/rust/tokio-postgres (#3879)

---
 test_runner/pg_clients/rust/tokio-postgres/Cargo.lock | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/test_runner/pg_clients/rust/tokio-postgres/Cargo.lock b/test_runner/pg_clients/rust/tokio-postgres/Cargo.lock
index 96989ee5ee..a0067b183e 100644
--- a/test_runner/pg_clients/rust/tokio-postgres/Cargo.lock
+++ b/test_runner/pg_clients/rust/tokio-postgres/Cargo.lock
@@ -389,9 +389,9 @@ checksum = "b7e5500299e16ebb147ae15a00a942af264cf3688f47923b8fc2cd5858f23ad3"
 
 [[package]]
 name = "openssl"
-version = "0.10.45"
+version = "0.10.48"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "b102428fd03bc5edf97f62620f7298614c45cedf287c271e7ed450bbaf83f2e1"
+checksum = "518915b97df115dd36109bfa429a48b8f737bd05508cf9588977b599648926d2"
 dependencies = [
  "bitflags",
  "cfg-if",
@@ -421,9 +421,9 @@ checksum = "ff011a302c396a5197692431fc1948019154afc178baf7d8e37367442a4601cf"
 
 [[package]]
 name = "openssl-sys"
-version = "0.9.80"
+version = "0.9.83"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "23bbbf7854cd45b83958ebe919f0e8e516793727652e27fda10a8384cfc790b7"
+checksum = "666416d899cf077260dac8698d60a60b435a46d57e82acb1be3d0dad87284e5b"
 dependencies = [
  "autocfg",
  "cc",

From 4d8c7654852cdae2375243aabd02e2f8a183e026 Mon Sep 17 00:00:00 2001
From: Dmitry Rodionov <dmitry@neon.tech>
Date: Mon, 27 Mar 2023 12:04:48 +0300
Subject: [PATCH 15/19] remove redundant dyn (#3878)

remove redundant dyn
---
 libs/utils/src/id.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libs/utils/src/id.rs b/libs/utils/src/id.rs
index f84bcb793f..b27c5cda35 100644
--- a/libs/utils/src/id.rs
+++ b/libs/utils/src/id.rs
@@ -23,7 +23,7 @@ pub enum IdError {
 struct Id([u8; 16]);
 
 impl Id {
-    pub fn get_from_buf(buf: &mut dyn bytes::Buf) -> Id {
+    pub fn get_from_buf(buf: &mut impl bytes::Buf) -> Id {
         let mut arr = [0u8; 16];
         buf.copy_to_slice(&mut arr);
         Id::from(arr)
@@ -112,7 +112,7 @@ impl fmt::Debug for Id {
 macro_rules! id_newtype {
     ($t:ident) => {
         impl $t {
-            pub fn get_from_buf(buf: &mut dyn bytes::Buf) -> $t {
+            pub fn get_from_buf(buf: &mut impl bytes::Buf) -> $t {
                 $t(Id::get_from_buf(buf))
             }
 

From 8d783299919ac4c8d86fb5c5187450cc49c0108a Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki@neon.tech>
Date: Sat, 25 Mar 2023 13:43:04 +0200
Subject: [PATCH 16/19] Remove some dead code.

whoami() was never called, 'is_test' was never set.

'restart()' might be useful, but it wasn't hooked up the CLI so it was
dead code. It's not clear what kind of a restart it should perform,
anyway: just restart Postgres, or re-initialize the data directory
from a fresh basebackup like "stop"+"start" does.
---
 control_plane/src/compute.rs | 29 -----------------------------
 1 file changed, 29 deletions(-)

diff --git a/control_plane/src/compute.rs b/control_plane/src/compute.rs
index 730cacf40b..46f0ad8d4f 100644
--- a/control_plane/src/compute.rs
+++ b/control_plane/src/compute.rs
@@ -87,7 +87,6 @@ impl ComputeControlPlane {
             address: SocketAddr::new("127.0.0.1".parse().unwrap(), port),
             env: self.env.clone(),
             pageserver: Arc::clone(&self.pageserver),
-            is_test: false,
             timeline_id,
             lsn,
             tenant_id,
@@ -113,7 +112,6 @@ pub struct PostgresNode {
     name: String,
     pub env: LocalEnv,
     pageserver: Arc<PageServerNode>,
-    is_test: bool,
     pub timeline_id: TimelineId,
     pub lsn: Option<Lsn>, // if it's a read-only node. None for primary
     pub tenant_id: TenantId,
@@ -171,7 +169,6 @@ impl PostgresNode {
             name,
             env: env.clone(),
             pageserver: Arc::clone(pageserver),
-            is_test: false,
             timeline_id,
             lsn: recovery_target_lsn,
             tenant_id,
@@ -480,10 +477,6 @@ impl PostgresNode {
         self.pg_ctl(&["start"], auth_token)
     }
 
-    pub fn restart(&self, auth_token: &Option<String>) -> Result<()> {
-        self.pg_ctl(&["restart"], auth_token)
-    }
-
     pub fn stop(&self, destroy: bool) -> Result<()> {
         // If we are going to destroy data directory,
         // use immediate shutdown mode, otherwise,
@@ -514,26 +507,4 @@ impl PostgresNode {
             "postgres"
         )
     }
-
-    // XXX: cache that in control plane
-    pub fn whoami(&self) -> String {
-        let output = Command::new("whoami")
-            .output()
-            .expect("failed to execute whoami");
-
-        assert!(output.status.success(), "whoami failed");
-
-        String::from_utf8(output.stdout).unwrap().trim().to_string()
-    }
-}
-
-impl Drop for PostgresNode {
-    // destructor to clean up state after test is done
-    // XXX: we may detect failed test by setting some flag in catch_unwind()
-    // and checking it here. But let just clean datadirs on start.
-    fn drop(&mut self) {
-        if self.is_test {
-            let _ = self.stop(true);
-        }
-    }
 }

From e3cbcc2ea759a58f90c07e6ece984310bbb43492 Mon Sep 17 00:00:00 2001
From: Vadim Kharitonov <vadim@neon.tech>
Date: Mon, 27 Mar 2023 10:13:34 +0200
Subject: [PATCH 17/19] Revert "Add `neondatabase/release` team as a default
 reviewers for storage"

This reverts commit daeaa767c405532f0c8bdb8a5765f0c13fd83aee.
---
 .github/workflows/release.yml | 1 -
 1 file changed, 1 deletion(-)

diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
index 014084c410..4bce9cdd1e 100644
--- a/.github/workflows/release.yml
+++ b/.github/workflows/release.yml
@@ -31,4 +31,3 @@ jobs:
         head: releases/${{ steps.date.outputs.date }}
         base: release
         title: Release ${{ steps.date.outputs.date }}
-        team_reviewers: release

From ff51e96fbd864504494ab301edfe955a2f030d47 Mon Sep 17 00:00:00 2001
From: Christian Schwarz <christian@neon.tech>
Date: Mon, 27 Mar 2023 12:45:10 +0200
Subject: [PATCH 18/19] fix synthetic size for (last_record_lsn - gc_horizon) <
 initdb_lsn (#3874)

fix synthetic size for (last_record_lsn - gc_horizon) < initdb_lsn

Assume a single-timeline project.
If the gc_horizon covers all WAL (last_record_lsn < gc_horizon)
but we have written more data than just initdb, the synthetic
size calculation worker needs to calculate the logical size
at LSN initdb_lsn (Segment BranchStart).

Before this patch, that calculation would incorrectly return
the initial logical size calculation result that we cache in
the Timeline::initial_logical_size. Presumably, because there
was confusion around initdb_lsn vs. initial size calculation.

The fix is to only hand out the initialized_size() only if
the LSN matches.

The distinction in the metrics between "init logical size" and "logical
size" was also incorrect because of the above. So, remove it.

There was a special case for `size != 0`. This was to cover the case of
LogicalSize::empty_initial(), but `initial_part_end` is `None` in that
case, so the new `LogicalSize::initialized_size()` will return None
in that case as well.

Lastly, to prevent confusion like this in the future, rename all
occurrences of `init_lsn` to either just `lsn` or a more specific name.

Co-authored-by: Joonas Koivunen <joonas@neon.tech>
Co-authored-by: Heikki Linnakangas <heikki@neon.tech>
---
 pageserver/src/metrics.rs         |  4 ---
 pageserver/src/tenant/timeline.rs | 43 ++++++++++++++-----------------
 2 files changed, 19 insertions(+), 28 deletions(-)

diff --git a/pageserver/src/metrics.rs b/pageserver/src/metrics.rs
index b5563ad186..6cb245aed7 100644
--- a/pageserver/src/metrics.rs
+++ b/pageserver/src/metrics.rs
@@ -586,7 +586,6 @@ pub struct TimelineMetrics {
     pub flush_time_histo: StorageTimeMetrics,
     pub compact_time_histo: StorageTimeMetrics,
     pub create_images_time_histo: StorageTimeMetrics,
-    pub init_logical_size_histo: StorageTimeMetrics,
     pub logical_size_histo: StorageTimeMetrics,
     pub load_layer_map_histo: StorageTimeMetrics,
     pub garbage_collect_histo: StorageTimeMetrics,
@@ -619,8 +618,6 @@ impl TimelineMetrics {
         let compact_time_histo = StorageTimeMetrics::new("compact", &tenant_id, &timeline_id);
         let create_images_time_histo =
             StorageTimeMetrics::new("create images", &tenant_id, &timeline_id);
-        let init_logical_size_histo =
-            StorageTimeMetrics::new("init logical size", &tenant_id, &timeline_id);
         let logical_size_histo = StorageTimeMetrics::new("logical size", &tenant_id, &timeline_id);
         let load_layer_map_histo =
             StorageTimeMetrics::new("load layer map", &tenant_id, &timeline_id);
@@ -657,7 +654,6 @@ impl TimelineMetrics {
             flush_time_histo,
             compact_time_histo,
             create_images_time_histo,
-            init_logical_size_histo,
             logical_size_histo,
             garbage_collect_histo,
             load_layer_map_histo,
diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs
index 33909e749b..5fde1a77e0 100644
--- a/pageserver/src/tenant/timeline.rs
+++ b/pageserver/src/tenant/timeline.rs
@@ -328,9 +328,13 @@ impl LogicalSize {
             .fetch_add(delta, AtomicOrdering::SeqCst);
     }
 
-    /// Returns the initialized (already calculated) value, if any.
-    fn initialized_size(&self) -> Option<u64> {
-        self.initial_logical_size.get().copied()
+    /// Make the value computed by initial logical size computation
+    /// available for re-use. This doesn't contain the incremental part.
+    fn initialized_size(&self, lsn: Lsn) -> Option<u64> {
+        match self.initial_part_end {
+            Some(v) if v == lsn => self.initial_logical_size.get().copied(),
+            _ => None,
+        }
     }
 }
 
@@ -806,11 +810,11 @@ impl Timeline {
 
         let mut is_exact = true;
         let size = current_size.size();
-        if let (CurrentLogicalSize::Approximate(_), Some(init_lsn)) =
+        if let (CurrentLogicalSize::Approximate(_), Some(initial_part_end)) =
             (current_size, self.current_logical_size.initial_part_end)
         {
             is_exact = false;
-            self.try_spawn_size_init_task(init_lsn, ctx);
+            self.try_spawn_size_init_task(initial_part_end, ctx);
         }
 
         Ok((size, is_exact))
@@ -1688,7 +1692,7 @@ impl Timeline {
         Ok(())
     }
 
-    fn try_spawn_size_init_task(self: &Arc<Self>, init_lsn: Lsn, ctx: &RequestContext) {
+    fn try_spawn_size_init_task(self: &Arc<Self>, lsn: Lsn, ctx: &RequestContext) {
         let permit = match Arc::clone(&self.current_logical_size.initial_size_computation)
             .try_acquire_owned()
         {
@@ -1726,7 +1730,7 @@ impl Timeline {
             // NB: don't log errors here, task_mgr will do that.
             async move {
                 let calculated_size = match self_clone
-                    .logical_size_calculation_task(init_lsn, &background_ctx)
+                    .logical_size_calculation_task(lsn, &background_ctx)
                     .await
                 {
                     Ok(s) => s,
@@ -1811,7 +1815,7 @@ impl Timeline {
     #[instrument(skip_all, fields(tenant = %self.tenant_id, timeline = %self.timeline_id))]
     async fn logical_size_calculation_task(
         self: &Arc<Self>,
-        init_lsn: Lsn,
+        lsn: Lsn,
         ctx: &RequestContext,
     ) -> Result<u64, CalculateLogicalSizeError> {
         let mut timeline_state_updates = self.subscribe_for_state_updates();
@@ -1822,7 +1826,7 @@ impl Timeline {
             let cancel = cancel.child_token();
             let ctx = ctx.attached_child();
             self_calculation
-                .calculate_logical_size(init_lsn, cancel, &ctx)
+                .calculate_logical_size(lsn, cancel, &ctx)
                 .await
         };
         let timeline_state_cancellation = async {
@@ -1906,21 +1910,12 @@ impl Timeline {
             // need to return something
             Ok(0)
         });
-        let timer = if up_to_lsn == self.initdb_lsn {
-            if let Some(size) = self.current_logical_size.initialized_size() {
-                if size != 0 {
-                    // non-zero size means that the size has already been calculated by this method
-                    // after startup. if the logical size is for a new timeline without layers the
-                    // size will be zero, and we cannot use that, or this caching strategy until
-                    // pageserver restart.
-                    return Ok(size);
-                }
-            }
-
-            self.metrics.init_logical_size_histo.start_timer()
-        } else {
-            self.metrics.logical_size_histo.start_timer()
-        };
+        // See if we've already done the work for initial size calculation.
+        // This is a short-cut for timelines that are mostly unused.
+        if let Some(size) = self.current_logical_size.initialized_size(up_to_lsn) {
+            return Ok(size);
+        }
+        let timer = self.metrics.logical_size_histo.start_timer();
         let logical_size = self
             .get_current_logical_size_non_incremental(up_to_lsn, cancel, ctx)
             .await?;

From fe156245708525d87bd3682595a3383e389efc65 Mon Sep 17 00:00:00 2001
From: Christian Schwarz <christian@neon.tech>
Date: Mon, 27 Mar 2023 13:33:40 +0200
Subject: [PATCH 19/19] eviction_task: only refresh layer accesses once per
 p.threshold (#3877)

Without this, we run it every p.period, which can be quite low. For
example, the running experiment with 3000 tenants in prod uses a period
of 1 minute.

Doing it once per p.threshold is enough to prevent eviction.
---
 pageserver/src/tenant/timeline.rs               |  8 ++++++++
 pageserver/src/tenant/timeline/eviction_task.rs | 15 ++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs
index 5fde1a77e0..dfa0e842f1 100644
--- a/pageserver/src/tenant/timeline.rs
+++ b/pageserver/src/tenant/timeline.rs
@@ -71,6 +71,8 @@ use crate::ZERO_PAGE;
 use crate::{is_temporary, task_mgr};
 use walreceiver::spawn_connection_manager_task;
 
+use self::eviction_task::EvictionTaskTimelineState;
+
 use super::layer_map::BatchedUpdates;
 use super::remote_timeline_client::index::IndexPart;
 use super::remote_timeline_client::RemoteTimelineClient;
@@ -216,6 +218,8 @@ pub struct Timeline {
     download_all_remote_layers_task_info: RwLock<Option<DownloadRemoteLayersTaskInfo>>,
 
     state: watch::Sender<TimelineState>,
+
+    eviction_task_timeline_state: tokio::sync::Mutex<EvictionTaskTimelineState>,
 }
 
 /// Internal structure to hold all data needed for logical size calculation.
@@ -1252,6 +1256,10 @@ impl Timeline {
                 download_all_remote_layers_task_info: RwLock::new(None),
 
                 state,
+
+                eviction_task_timeline_state: tokio::sync::Mutex::new(
+                    EvictionTaskTimelineState::default(),
+                ),
             };
             result.repartition_threshold = result.get_checkpoint_distance() / 10;
             result
diff --git a/pageserver/src/tenant/timeline/eviction_task.rs b/pageserver/src/tenant/timeline/eviction_task.rs
index 666768ff87..06dfe7a0b9 100644
--- a/pageserver/src/tenant/timeline/eviction_task.rs
+++ b/pageserver/src/tenant/timeline/eviction_task.rs
@@ -35,6 +35,11 @@ use crate::{
 
 use super::Timeline;
 
+#[derive(Default)]
+pub struct EvictionTaskTimelineState {
+    last_refresh_required_in_restart: Option<tokio::time::Instant>,
+}
+
 impl Timeline {
     pub(super) fn launch_eviction_task(self: &Arc<Self>) {
         let self_clone = Arc::clone(self);
@@ -139,7 +144,15 @@ impl Timeline {
         // for active tenants this will likely materialized page cache or in-memory layers. for
         // inactive tenants it will refresh the last_access timestamps so that we will not evict
         // and re-download on restart these layers.
-        self.refresh_layers_required_in_restart(cancel, ctx).await;
+        let mut state = self.eviction_task_timeline_state.lock().await;
+        match state.last_refresh_required_in_restart {
+            Some(ts) if ts.elapsed() < p.threshold => { /* no need to run */ }
+            _ => {
+                self.refresh_layers_required_in_restart(cancel, ctx).await;
+                state.last_refresh_required_in_restart = Some(tokio::time::Instant::now())
+            }
+        }
+        drop(state);
 
         if cancel.is_cancelled() {
             return ControlFlow::Break(());