Skip to content

Commit e7502a3

Browse files
DimasKovasjcsp
andauthored
pageserver: return 412 PreconditionFailed in get_timestamp_of_lsn if timestamp is not found (#11491)
## Problem Now `get_timestamp_of_lsn` returns `404 NotFound` if there is no clog pages for given LSN, and it's difficult to distinguish from other 404 errors. A separate status code for this error will allow the control plane to handle this case. - Closes: #11439 - Corresponding PR in control plane: neondatabase/cloud#27125 ## Summary of changes - Return `412 PreconditionFailed` instead of `404 NotFound` if no timestamp is fond for given LSN. I looked briefly through the current error handling code in cloud.git and the status code change should not affect anything for the existing code. Change from the corresponding PR also looks fine and should work with the current PS status code. Additionally, here is OK to merge it from control plane team: #11439 (comment) --------- Co-authored-by: John Spray <[email protected]>
1 parent ef8101a commit e7502a3

File tree

4 files changed

+44
-7
lines changed

4 files changed

+44
-7
lines changed

pageserver/src/http/openapi_spec.yml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,12 @@ paths:
212212
schema:
213213
type: string
214214
format: date-time
215+
"412":
216+
description: No timestamp is found for given LSN, e.g. if there had been no commits till LSN
217+
content:
218+
application/json:
219+
schema:
220+
$ref: "#/components/schemas/PreconditionFailedError"
215221

216222
/v1/tenant/{tenant_id}/timeline/{timeline_id}/get_lsn_by_timestamp:
217223
parameters:

pageserver/src/http/routes.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -989,7 +989,7 @@ async fn get_lsn_by_timestamp_handler(
989989
if !tenant_shard_id.is_shard_zero() {
990990
// Requires SLRU contents, which are only stored on shard zero
991991
return Err(ApiError::BadRequest(anyhow!(
992-
"Size calculations are only available on shard zero"
992+
"Lsn calculations by timestamp are only available on shard zero"
993993
)));
994994
}
995995

@@ -1064,7 +1064,7 @@ async fn get_timestamp_of_lsn_handler(
10641064
if !tenant_shard_id.is_shard_zero() {
10651065
// Requires SLRU contents, which are only stored on shard zero
10661066
return Err(ApiError::BadRequest(anyhow!(
1067-
"Size calculations are only available on shard zero"
1067+
"Timestamp calculations by lsn are only available on shard zero"
10681068
)));
10691069
}
10701070

@@ -1090,8 +1090,8 @@ async fn get_timestamp_of_lsn_handler(
10901090
.to_string();
10911091
json_response(StatusCode::OK, time)
10921092
}
1093-
None => Err(ApiError::NotFound(
1094-
anyhow::anyhow!("Timestamp for lsn {} not found", lsn).into(),
1093+
None => Err(ApiError::PreconditionFailed(
1094+
format!("Timestamp for lsn {} not found", lsn).into(),
10951095
)),
10961096
}
10971097
}

pageserver/src/pgdatadir_mapping.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -691,7 +691,7 @@ impl Timeline {
691691
Ok(buf.get_u32_le())
692692
}
693693

694-
/// Get size of an SLRU segment
694+
/// Does the slru segment exist?
695695
pub(crate) async fn get_slru_segment_exists(
696696
&self,
697697
kind: SlruKind,
@@ -844,9 +844,9 @@ impl Timeline {
844844
.await
845845
}
846846

847-
/// Obtain the possible timestamp range for the given lsn.
847+
/// Obtain the timestamp for the given lsn.
848848
///
849-
/// If the lsn has no timestamps, returns None. returns `(min, max, median)` if it has timestamps.
849+
/// If the lsn has no timestamps (e.g. no commits), returns None.
850850
pub(crate) async fn get_timestamp_for_lsn(
851851
&self,
852852
probe_lsn: Lsn,

test_runner/regress/test_lsn_mapping.py

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -276,3 +276,34 @@ def test_ts_of_lsn_api(neon_env_builder: NeonEnvBuilder):
276276
if i > 1:
277277
before_timestamp = tbl[i - step_size][1]
278278
assert timestamp >= before_timestamp, "before_timestamp before timestamp"
279+
280+
281+
def test_timestamp_of_lsn_empty_branch(neon_env_builder: NeonEnvBuilder):
282+
"""
283+
Test that getting the timestamp of the head LSN of a newly created branch works.
284+
This verifies that we don't get a 404 error when trying to get the timestamp
285+
of the head LSN of a branch that was just created.
286+
We now return a special status code 412 to indicate if there is no timestamp found for lsn.
287+
288+
Reproducer for https://github.com/neondatabase/neon/issues/11439
289+
"""
290+
env = neon_env_builder.init_start()
291+
292+
# Create a new branch
293+
new_timeline_id = env.create_branch("test_timestamp_of_lsn_empty_branch")
294+
295+
# Retrieve the commit LSN of the empty branch, which we have never run postgres on
296+
detail = env.pageserver.http_client().timeline_detail(
297+
tenant_id=env.initial_tenant, timeline_id=new_timeline_id
298+
)
299+
head_lsn = detail["last_record_lsn"]
300+
301+
# Verify that we get 412 status code
302+
with env.pageserver.http_client() as client:
303+
with pytest.raises(PageserverApiException) as err:
304+
client.timeline_get_timestamp_of_lsn(
305+
env.initial_tenant,
306+
new_timeline_id,
307+
head_lsn,
308+
)
309+
assert err.value.status_code == 412

0 commit comments

Comments
 (0)