[pageserver] Do several adjustements to the find_lsn_for_timestamp

1. Use `max(find_lsn_for_timestamp, ancestor_lsn)` as a lower boundary for search. We use this method to figure out the branching LSN for new branch, but GC cutoff could be before branching point and we cannot create new branch with LSN < `ancestor_lsn`. 2. Search for the first commit **before** specified timestamp. This solves two drawbacks: i) newly created branch won't contain physical changes from later commits that will be marked as aborted, and will need to be vacuumed away; and ii) we can still figure out a reasonable branching LSN if there were no new commits since the specified timestamp. 3. Change `get_lsn_by_timestamp` API method to return LSN even if we only found commit **before** the specified timestamp. Resolves #3414
2026-01-17 10:22:56 +00:00 · 2023-02-22 20:33:05 +01:00
3 changed files with 42 additions and 26 deletions
--- a/pageserver/src/http/routes.rs
+++ b/pageserver/src/http/routes.rs
@@ -330,7 +330,7 @@ async fn get_lsn_by_timestamp_handler(request: Request<Body>) -> Result<Response

    let result = match result {
        LsnForTimestamp::Present(lsn) => format!("{lsn}"),
-        LsnForTimestamp::Future(_lsn) => "future".into(),
+        LsnForTimestamp::Future(lsn) => format!("{lsn}"),
        LsnForTimestamp::Past(_lsn) => "past".into(),
        LsnForTimestamp::NoData(_lsn) => "nodata".into(),
    };
--- a/pageserver/src/pgdatadir_mapping.rs
+++ b/pageserver/src/pgdatadir_mapping.rs
@@ -290,6 +290,7 @@ impl Timeline {
        }
    }

+    ///
    /// Locate LSN, such that all transactions that committed before
    /// 'search_timestamp' are visible, but nothing newer is.
    ///
@@ -303,7 +304,11 @@ impl Timeline {
        ctx: &RequestContext,
    ) -> Result<LsnForTimestamp, PageReconstructError> {
        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 new branch, but
+        // GC cutoff could be before branching point and we cannot create new branch
+        // with LSN < `ancestor_lsn`. Thus, pick the maximum of these two just 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
@@ -327,12 +332,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); and 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).
+        // Substitute 1 to get exactly the 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.
@@ -340,32 +355,26 @@ 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,
--- a/test_runner/regress/test_lsn_mapping.py
+++ b/test_runner/regress/test_lsn_mapping.py
@@ -17,7 +17,9 @@ def test_lsn_mapping(neon_env_builder: NeonEnvBuilder):

    cur = pgmain.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.
@@ -32,20 +34,23 @@ def test_lsn_mapping(neon_env_builder: NeonEnvBuilder):

    # Execute one more transaction with synchronous_commit enabled, to flush
    # all the previous transactions
+    cur.execute("SET synchronous_commit=on")
    cur.execute("INSERT INTO foo VALUES (-1)")

    # Wait until WAL is received by pageserver
    wait_for_last_flush_lsn(env, pgmain, 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"
        )
-        assert result == "future"
+        # We should still return LSN of the first commit before timestamp
+        assert result not in ["past", "nodata"]

-        # 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"
@@ -55,10 +60,12 @@ def test_lsn_mapping(neon_env_builder: NeonEnvBuilder):
        # Probe a bunch of timestamps in the valid range
        for i in range(1, len(tbl), 100):
            probe_timestamp = tbl[i][1]
+            # Call get_lsn_by_timestamp to get the LSN
            lsn = client.timeline_get_lsn_by_timestamp(
                env.initial_tenant, new_timeline_id, f"{probe_timestamp.isoformat()}Z"
            )
-            # Call get_lsn_by_timestamp to get the LSN
+            assert lsn not in ["past", "nodata"]
+
            # Launch a new read-only node at that LSN, and check that only the rows
            # that were supposed to be committed at that point in time are visible.
            pg_here = env.postgres.create_start(