diff --git a/pageserver/src/pgdatadir_mapping.rs b/pageserver/src/pgdatadir_mapping.rs index daadf6abd4..61949f5c32 100644 --- a/pageserver/src/pgdatadir_mapping.rs +++ b/pageserver/src/pgdatadir_mapping.rs @@ -314,7 +314,11 @@ impl Timeline { ctx: &RequestContext, ) -> Result { let gc_cutoff_lsn_guard = self.get_latest_gc_cutoff_lsn(); - let min_lsn = *gc_cutoff_lsn_guard; + // We use this method to figure out the branching LSN for the new branch, but the + // GC cutoff could be before the branching point and we cannot create a new branch + // with LSN < `ancestor_lsn`. Thus, pick the maximum of these two to be + // on the safe side. + let min_lsn = std::cmp::max(*gc_cutoff_lsn_guard, self.get_ancestor_lsn()); let max_lsn = self.get_last_record_lsn(); // LSNs are always 8-byte aligned. low/mid/high represent the @@ -338,12 +342,22 @@ impl Timeline { ) .await?; - if cmp { - high = mid; - } else { + if !cmp { + // We either 1) found commit with timestamp **before** `search_timestamp`; + // or 2) we haven't found any commit records at all. + // Search with a larger LSN. In case 1), to try to find a more recent commit + // (but still **before** target timestamp). In case 2), to fetch more + // SLRU segments for `clog`. low = mid + 1; + } else { + // We found only more recent commits, search in the older range. + high = mid; } } + // If `found_smaller == true`, `low` is the LSN of the first commit record + // **before** the `search_timestamp` + 1 (to hit the while loop exit condition). + // Subtract 1 to get back the exact commit LSN. + let commit_lsn = Lsn((low - 1) * 8); match (found_smaller, found_larger) { (false, false) => { // This can happen if no commit records have been processed yet, e.g. @@ -351,31 +365,25 @@ impl Timeline { Ok(LsnForTimestamp::NoData(max_lsn)) } (true, false) => { - // Didn't find any commit timestamps larger than the request - Ok(LsnForTimestamp::Future(max_lsn)) + // Only found a commit with timestamp smaller than the request. + // It's still a valid case for branch creation, return it. + // And `update_gc_info()` ignores LSN for a `LsnForTimestamp::Future` + // case, anyway. + Ok(LsnForTimestamp::Future(commit_lsn)) } (false, true) => { // Didn't find any commit timestamps smaller than the request Ok(LsnForTimestamp::Past(max_lsn)) } - (true, true) => { - // low is the LSN of the first commit record *after* the search_timestamp, - // Back off by one to get to the point just before the commit. - // - // FIXME: it would be better to get the LSN of the previous commit. - // Otherwise, if you restore to the returned LSN, the database will - // include physical changes from later commits that will be marked - // as aborted, and will need to be vacuumed away. - Ok(LsnForTimestamp::Present(Lsn((low - 1) * 8))) - } + (true, true) => Ok(LsnForTimestamp::Present(commit_lsn)), } } - /// Subroutine of find_lsn_for_timestamp(). Returns true, if there are any - /// commits that committed after 'search_timestamp', at LSN 'probe_lsn'. + /// Subroutine of `find_lsn_for_timestamp()`. Returns `true`, if there are any + /// commits that committed after `search_timestamp`, at LSN `probe_lsn`. /// - /// Additionally, sets 'found_smaller'/'found_Larger, if encounters any commits - /// with a smaller/larger timestamp. + /// Additionally, sets `found_smaller` / `found_larger`, if encounters any commits + /// with a smaller / larger timestamp. /// pub async fn is_latest_commit_timestamp_ge_than( &self, diff --git a/test_runner/regress/test_lsn_mapping.py b/test_runner/regress/test_lsn_mapping.py index 726bfa5f29..cb0e9a358c 100644 --- a/test_runner/regress/test_lsn_mapping.py +++ b/test_runner/regress/test_lsn_mapping.py @@ -85,7 +85,9 @@ def test_lsn_mapping(neon_env_builder: NeonEnvBuilder): cur = endpoint_main.connect().cursor() # Create table, and insert rows, each in a separate transaction - # Disable synchronous_commit to make this initialization go faster. + # Disable `synchronous_commit` to make this initialization go faster. + # XXX: on my laptop this test takes 7s, and setting `synchronous_commit=off` + # doesn't change anything. # # Each row contains current insert LSN and the current timestamp, when # the row was inserted. @@ -107,14 +109,15 @@ def test_lsn_mapping(neon_env_builder: NeonEnvBuilder): wait_for_last_flush_lsn(env, endpoint_main, env.initial_tenant, new_timeline_id) with env.pageserver.http_client() as client: - # Check edge cases: timestamp in the future + # Check edge cases + # Timestamp is in the future probe_timestamp = tbl[-1][1] + timedelta(hours=1) result = client.timeline_get_lsn_by_timestamp( env.initial_tenant, new_timeline_id, f"{probe_timestamp.isoformat()}Z", 2 ) - assert result["kind"] == "future" + assert result["kind"] in ["present", "future"] - # timestamp too the far history + # Timestamp is in the unreachable past probe_timestamp = tbl[0][1] - timedelta(hours=10) result = client.timeline_get_lsn_by_timestamp( env.initial_tenant, new_timeline_id, f"{probe_timestamp.isoformat()}Z", 2 @@ -127,6 +130,7 @@ def test_lsn_mapping(neon_env_builder: NeonEnvBuilder): result = client.timeline_get_lsn_by_timestamp( env.initial_tenant, new_timeline_id, f"{probe_timestamp.isoformat()}Z", 2 ) + assert result["kind"] not in ["past", "nodata"] lsn = result["lsn"] # Call get_lsn_by_timestamp to get the LSN # Launch a new read-only node at that LSN, and check that only the rows