-
Notifications
You must be signed in to change notification settings - Fork 434
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
Compute <-> pageserver protocol version 2 #7377
Conversation
2ef7279
to
0c7f083
Compare
2820 tests run: 2699 passed, 0 failed, 121 skipped (full report)Flaky tests (4)Postgres 16Postgres 15Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
36c2a88 at 2024-04-25T17:53:08.701Z :recycle: |
0c7f083
to
05039d6
Compare
TODOs:
|
8afdacd
to
f6c9fa8
Compare
As noted in the comment, the craft_internal() function fails if the inserted WAL happens to land at page boundary. I bumped into that with PR #7377; it changed the arguments of a few SQL functions in neon_test_utils extension, which changed the WAL positions slightly, and caused a test failure.
As noted in the comment, the craft_internal() function fails if the inserted WAL happens to land at page boundary. I bumped into that with PR #7377; it changed the arguments of a few SQL functions in neon_test_utils extension, which changed the WAL positions slightly, and caused a test failure.
f6c9fa8
to
47cf645
Compare
As noted in the comment, the craft_internal() function fails if the inserted WAL happens to land at page boundary. I bumped into that with PR #7377; it changed the arguments of a few SQL functions in neon_test_utils extension, which changed the WAL positions slightly, and caused a test failure.
As noted in the comment, the craft_internal() function fails if the inserted WAL happens to land at page boundary. I bumped into that with PR #7377; it changed the arguments of a few SQL functions in neon_test_utils extension, which changed the WAL positions slightly, and caused a test failure.
Added one more test, to demonstrate the original error: 34f75f5. |
Replying to @problame's comment on the original PR:
added: 34f75f5
Removing the old version:
We are coordinating this on the #escalation-2024-04-12-tried-to-request-a-page-version-that-was-garbage-coll channel now. |
The 'latest' argument was passed to the functions in pgdatadir_mapping.rs to know when they can update the relsize cache. Commit e69ff3f changed how the relsize cache is updated, making the 'latest' argument unused.
In the old protocol version, the client sent with each request: - latest: bool. If true, the client requested the latest page version, and the 'lsn' was just a hint of when the page was last modified - lsn: Lsn, the page version to return This protocol didn't allow requesting a page at a particular non-latest LSN and *also* sending a hint on when the page was last modified. That put a read only compute into an awkward position where it had to either request each page at the replay-LSN, which could be very close to the last LSN written in the primary and therefore require the pageserver to wait for it to arrive, or an older LSN which could already be garbage collected in the pageserver, resulting in an error. The new protocol version fixes that by allowing a read only compute to send both LSNs. To use the new protocol version, use "pagestream_v2" command instead of just "pagestream". The old protocol version is still supported, for compatibility with old computes (and in fact there is no client support yet, it is added by the next commit).
Instead of thinking in terms of 'latest' and 'lsn' of the request, each request has two LSNs: the request LSN and 'not_modified_since' LSN. The request is nominally made at the request LSN, that determines what page version we want to see. But as a hint, we also include 'not_modified_since'. It tells the pageserver that the page has not been modified since that LSN, which allows the pageserver to skip waiting for newer WAL to arrive, and could allow more optimizations in the future. Refactor the internal functions to calculate the request LSN to calculate both LSNs. Sending two LSNs to the pageserver requires using the new protocol version 2. The previous commit added the server support for it, but we still default to the old protocol for compatibility with old pageservers. The 'neon.protocol_version' GUC can be used to use the new protocol. The new protocol addresses one cause of issue #6211, although you can still get the same error if you have a standby that is lagging behind so that the page version it needs is genuinely GC'd away.
Before PR #7377, on-demand SLRU download always used the basebackup's LSN in the SLRU download, but that LSN might get garbage-collected away in the pageserver. We should request the latest LSN, like with GetPage requests, with the LSN just indicating that we know that the page hasn't been changed since the LSN (since the basebackup in this case). Add test to demonstrate the problem. Without the fix, it fails with "tried to request a page version that was garbage collected" error from the pageserver. I wrote this test as part of earlier PR #6693, but that fell through the cracks and was never applied. PR #7377 superseded the fix from that older PR, but the test is still valid.
34f75f5
to
3e9a139
Compare
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.
Reviewed on behalf of safekeepers group. Someone with deeper pageserver knowledge still should review to check for potential bugs.
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.
Looks good to me. Thanks for the informative commit messages.
The 'latest' argument was passed to the functions in pgdatadir_mapping.rs to know when they can update the relsize cache. Commit e69ff3f changed how the relsize cache is updated, making the 'latest' argument unused.
In the old protocol version, the client sent with each request: - latest: bool. If true, the client requested the latest page version, and the 'lsn' was just a hint of when the page was last modified - lsn: Lsn, the page version to return This protocol didn't allow requesting a page at a particular non-latest LSN and *also* sending a hint on when the page was last modified. That put a read only compute into an awkward position where it had to either request each page at the replay-LSN, which could be very close to the last LSN written in the primary and therefore require the pageserver to wait for it to arrive, or an older LSN which could already be garbage collected in the pageserver, resulting in an error. The new protocol version fixes that by allowing a read only compute to send both LSNs. To use the new protocol version, use "pagestream_v2" command instead of just "pagestream". The old protocol version is still supported, for compatibility with old computes (and in fact there is no client support yet, it is added by the next commit).
Instead of thinking in terms of 'latest' and 'lsn' of the request, each request has two LSNs: the request LSN and 'not_modified_since' LSN. The request is nominally made at the request LSN, that determines what page version we want to see. But as a hint, we also include 'not_modified_since'. It tells the pageserver that the page has not been modified since that LSN, which allows the pageserver to skip waiting for newer WAL to arrive, and could allow more optimizations in the future. Refactor the internal functions to calculate the request LSN to calculate both LSNs. Sending two LSNs to the pageserver requires using the new protocol version 2. The previous commit added the server support for it, but we still default to the old protocol for compatibility with old pageservers. The 'neon.protocol_version' GUC can be used to use the new protocol. The new protocol addresses one cause of issue #6211, although you can still get the same error if you have a standby that is lagging behind so that the page version it needs is genuinely GC'd away.
Before PR #7377, on-demand SLRU download always used the basebackup's LSN in the SLRU download, but that LSN might get garbage-collected away in the pageserver. We should request the latest LSN, like with GetPage requests, with the LSN just indicating that we know that the page hasn't been changed since the LSN (since the basebackup in this case). Add test to demonstrate the problem. Without the fix, it fails with "tried to request a page version that was garbage collected" error from the pageserver. I wrote this test as part of earlier PR #6693, but that fell through the cracks and was never applied. PR #7377 superseded the fix from that older PR, but the test is still valid.
3e9a139
to
36c2a88
Compare
The 'latest' argument was passed to the functions in pgdatadir_mapping.rs to know when they can update the relsize cache. Commit e69ff3f changed how the relsize cache is updated, making the 'latest' argument unused.
In the old protocol version, the client sent with each request: - latest: bool. If true, the client requested the latest page version, and the 'lsn' was just a hint of when the page was last modified - lsn: Lsn, the page version to return This protocol didn't allow requesting a page at a particular non-latest LSN and *also* sending a hint on when the page was last modified. That put a read only compute into an awkward position where it had to either request each page at the replay-LSN, which could be very close to the last LSN written in the primary and therefore require the pageserver to wait for it to arrive, or an older LSN which could already be garbage collected in the pageserver, resulting in an error. The new protocol version fixes that by allowing a read only compute to send both LSNs. To use the new protocol version, use "pagestream_v2" command instead of just "pagestream". The old protocol version is still supported, for compatibility with old computes (and in fact there is no client support yet, it is added by the next commit).
Instead of thinking in terms of 'latest' and 'lsn' of the request, each request has two LSNs: the request LSN and 'not_modified_since' LSN. The request is nominally made at the request LSN, that determines what page version we want to see. But as a hint, we also include 'not_modified_since'. It tells the pageserver that the page has not been modified since that LSN, which allows the pageserver to skip waiting for newer WAL to arrive, and could allow more optimizations in the future. Refactor the internal functions to calculate the request LSN to calculate both LSNs. Sending two LSNs to the pageserver requires using the new protocol version 2. The previous commit added the server support for it, but we still default to the old protocol for compatibility with old pageservers. The 'neon.protocol_version' GUC can be used to use the new protocol. The new protocol addresses one cause of issue #6211, although you can still get the same error if you have a standby that is lagging behind so that the page version it needs is genuinely GC'd away.
Before PR #7377, on-demand SLRU download always used the basebackup's LSN in the SLRU download, but that LSN might get garbage-collected away in the pageserver. We should request the latest LSN, like with GetPage requests, with the LSN just indicating that we know that the page hasn't been changed since the LSN (since the basebackup in this case). Add test to demonstrate the problem. Without the fix, it fails with "tried to request a page version that was garbage collected" error from the pageserver. I wrote this test as part of earlier PR #6693, but that fell through the cracks and was never applied. PR #7377 superseded the fix from that older PR, but the test is still valid.
This is the same functionality as #6718, with a bunch of cleanups and refactoring based on comments there. Main changes:
pagestream_v2
command when the pagestream is started. The message tags of individual request are unchanged and are the same in both versions (per Send LSN range in getpage request #6718 (comment))request_lsn
andnot_modified_since
. This is more clear than the concept of ahorizon
from that previous PR. There are no magic values, a request must provide a valid LSN for both. Lsn::MAX is valid forrequest_lsn
as long as `not_modified_since is a valid value, however. That can be used to fetch the latest version of the page. It's used by some tests, but not by the actual compute. The actual compute always passes a valid LSN for both.