-
Notifications
You must be signed in to change notification settings - Fork 409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
get_lsn_by_timestamp: clamp commit_lsn to be >= min_lsn #7488
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Is there any way to craft a test case for this?
2766 tests run: 2645 passed, 0 failed, 121 skipped (full report)Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
623c3a6 at 2024-04-23T17:06:16.779Z :recycle: |
This is an edge case: I'll be thinking about a test case. Depending on urgency, @koivunej feel free to merge this if you think a fix is urgent. |
Hmmm tried for two hours to create a reproducer but the issue is that this is really an edge case. This (added to # Regression test for an edge case fixed by #7488
# Create a bunch of branches and probe the returned lsn at that timestamp
found_with_present = False
for i in range(10, len(tbl), 100):
probe_timestamp_before = cast(datetime, tbl[i - 1][1])
probe_timestamp = cast(datetime, tbl[i][1])
result_before = client.timeline_get_lsn_by_timestamp(
tenant_id, timeline_id, probe_timestamp_before
)
assert result_before["kind"] not in ["past", "nodata"]
parent_lsn_before = Lsn(result_before["lsn"])
result = client.timeline_get_lsn_by_timestamp(tenant_id, timeline_id, probe_timestamp)
assert result["kind"] not in ["past", "nodata"]
parent_lsn = Lsn(result["lsn"])
for branch_lsn in range(int(parent_lsn_before), int(parent_lsn), 8):
branch_lsn = Lsn(branch_lsn)
timeline_id_child = env.neon_cli.create_branch(
f"test_lsn_mapping_child_loop_{i}_{branch_lsn}",
tenant_id=tenant_id,
ancestor_branch_name="test_lsn_mapping",
ancestor_start_lsn=branch_lsn,
)
result = client.timeline_get_lsn_by_timestamp(
tenant_id, timeline_id_child, probe_timestamp_before
)
assert result["kind"] != "nodata"
if result["kind"] in ["present", "future"]:
found_with_present = True
lsn = Lsn(result["lsn"])
assert lsn >= branch_lsn, "Grandchild lsn needs to be valid"
timeline_id_child = env.neon_cli.create_branch(
f"test_lsn_mapping_child_loop_{i}_{branch_lsn}_grandchild",
tenant_id=tenant_id,
ancestor_branch_name="test_lsn_mapping",
ancestor_start_lsn=lsn,
)
assert found_with_present |
…7520) Implements an approach different from the one #7488 chose: We now return `past` instead of `present` (or`future`) when encountering the edge case where commit_lsn < min_lsn. In my opinion, both `past` and `present` are correct responses, but past is slightly better as the lsn returned by `present` with #7488 is one too "new". In practice, this shouldn't matter much, but shrug. We agreed in slack that this is the better approach: https://neondb.slack.com/archives/C03F5SM1N02/p1713871064147029
There was an edge case where
get_lsn_by_timestamp
/find_lsn_for_timestamp
could have returned an lsn that is before the limits we enforce: when we did find SLRU entries with timestamps before the one we search for.The API contract of
get_lsn_by_timestamp
is to not return something before the anchestor lsn.cc https://neondb.slack.com/archives/C03F5SM1N02/p1713871064147029