-
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
Don't pass InvalidTransactionId to update_next_xid. #6410
Conversation
2268 tests run: 2179 passed, 0 failed, 89 skipped (full report)Code coverage (full report)
The comment gets automatically updated with the latest test results
4a8d33e at 2024-01-20T14:47:46.807Z :recycle: |
|
6ee90cf
to
5da65ec
Compare
5da65ec
to
771f9d3
Compare
update_next_xid() doesn't have any special treatment for the invalid or other special XIDs, so it will treat InvalidTransactionId (0) as a regular XID. If old nextXid is smaller than 2^31, 0 will look like a very old XID, and nothing happens. But if nextXid is greater than 2^31 0 will look like a very new XID, and update_next_xid() will incorrectly bump up nextXID.
771f9d3
to
f10adb6
Compare
e19df29
to
6a2187f
Compare
Without this PR, when importing a vanilla Postgres datadir with nextXid < 1024, importing the CHECKPOINT record would bump it up to 1024. That caused a new delta layer to be created at that point. After fixing that, if the WAL that's part of the basebackup didn't contain any updates to actual relations, importing the WAL would generate no delta layers. That caused a test failure, because the test waited for the generated delta layers to be uploaded, but none were generated. To fix, update the checkpoint key-value pair on every CHECKPOINT record. That's not really required, but seems nice anyway, and it forces a delta layer to be generated in this corner case.
One test failure in the e2e public API tests: https://github.com/neondatabase/cloud/actions/runs/7595014557/job/20687120459. I'm pretty sure that's a flaky test, because all the other variants of the e2e tests failed, and they don't exercise this codepath in any special manner anyway. I will relaunch it. |
The e2e test suite failed again, at a different test. I still believe it's flakiness, but would appreciate a second opinion from someone more in touch with the e2e tests |
update_next_xid() doesn't have any special treatment for the invalid or other special XIDs, so it will treat InvalidTransactionId (0) as a regular XID. If old nextXid is smaller than 2^31, 0 will look like a very old XID, and nothing happens. But if nextXid is greater than 2^31 0 will look like a very new XID, and update_next_xid() will incorrectly bump up nextXID.
update_next_xid() doesn't have any special treatment for the invalid or other special XIDs, so it will treat InvalidTransactionId (0) as a regular XID. If old nextXid is smaller than 2^31, 0 will look like a very old XID, and nothing happens. But if nextXid is greater than 2^31 0 will look like a very new XID, and update_next_xid() will incorrectly bump up nextXID.