From ed923b6102014af5014f5ae8408b417b3a16d3f8 Mon Sep 17 00:00:00 2001 From: Alexey Kondratov Date: Wed, 25 Oct 2023 20:25:03 +0200 Subject: [PATCH] Apply changes of 3689 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There are tiny modifications in the comments and the tests, and also we did the routes.rs changes in an earlier PR. Otherwise, this is just the changes from that PR. --------- Co-authored-by: Arpad Müller --- pageserver/src/pgdatadir_mapping.rs | 48 ++++++++++++++----------- test_runner/regress/test_lsn_mapping.py | 12 ++++--- 2 files changed, 36 insertions(+), 24 deletions(-) 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