-
Notifications
You must be signed in to change notification settings - Fork 438
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
Add endpoint for querying time info for lsn #5497
Conversation
it isnt really an issue as it's in a test but whatever, couldn't find ways to disable it for the tests dir.
2322 tests run: 2207 passed, 0 failed, 115 skipped (full report)Flaky tests (2)Postgres 16
Postgres 14
Code coverage (full report)
The comment gets automatically updated with the latest test results
f6946e9 at 2023-10-19T02:08:35.574Z :recycle: |
@xenomote I have implemented this. Is this what you were asking for and does it fit your use cases? |
yea seems good to me, I can update the go client once this is merged in 👍 |
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.
My only concern about this is how large that vector of timestamps will grow, since I don't know how many SLRU blocks we can expect to see. I've asked for some guidance in #postgres -- maybe this is no issue at all, but from the existing code it isn't obvious.
We could avoid storing the whole list if we didn't output the median, so if this becomes a problem in practice we might need to do that.
@arpad-m if the median is not a requirement and imposes issues, lets drop it |
This test failure is interesting:
With:
On the other hand, on my local box it works, with:
Apparently parsing time strings with |
------ Co-authored-by: Shany Pozin <shany@neon.tech>
For rerun attempt 2 for the latest commit "merge branch 'main' into arpad/ts_by_lsn", of the 6 regress tests, 2 failed. logs for regress-tests (debug v15):
and regress-tests (release v15):
So the same error two times. The rerun attempt 1 for the same commit showed different failures. For regress-tests (debug v14):
For regress-tests (debug v15), regress-tests (release v15) we have the 404 error again, and for regress-tests (release v16) we have the assertion failure again:
We can get this PR probably merged by pressing on the "retry" button a few times but the test would still be extremely flaky. Therefore, I have added a commit that adds a few sleeps and don't consider the first lsn (where we likely encounter the 404). let's see if this helps. |
The `get_timestamp_of_lsn` pageserver endpoint has been added in #5497, but the yml it added was wrong: the lsn is expected in hex format, not in integer (decimal) format.
Problem
See #5468.
Summary of changes
Add a new
get_timestamp_of_lsn
endpoint, returning the timestampassociated with the given lsn.
Fixes #5468.
Checklist before requesting a review
Checklist before merging