PG-1604 fix: preallocate one more record for the cache #539
Conversation
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (82.49%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## TDE_REL_17_STABLE #539 +/- ##
=====================================================
- Coverage 82.50% 82.49% -0.01%
=====================================================
Files 25 25
Lines 3207 3217 +10
Branches 508 510 +2
=====================================================
+ Hits 2646 2654 +8
Misses 452 452
- Partials 109 111 +2
🚀 New features to boost your workflow:
|
|
I find the names a bit confusing. Maybe To me it's not clear what "extra" refers to at all at least, while "preallocated" is clear. |
There was a problem hiding this comment.
This is a good solution for now. We need to clean up the whole wal key cache setup at some point anyways.
I think the naming could be made clearer. And maybe InitWrite should always call the preallocate function with a comment like Ensure there are preallocated key and cache entries because they're sometimes needed in the critical section would make it all easier to understand than understanding why this is only done when loading all of the keys.
Technically, that will happen, that check there is only a safety net. It could be an assert. But since the init function is the first function that tries to use the WAL cache, it will be the first function to fetch the keys. |
There is at lesat one corner case scenario where we have to load the last record into the cache during a write: * replica crashes, receives last segment from primary * replica replays last segment, reaches end * replica activtes new key * replica replays prepared transaction, has to use old keys again * old key write function sees that we generated a new key, tries to load it In this scenario we could get away by detecting that we are in a write, and asserting if we tried to use the last key. But in a release build assertions are not fired, and we would end up writing some non encrypted data to disk, and later if we have to run recovery failing. It could be a FATAL, but that would still crash the server, and the next startup would crash again and again... Instead, to properly avoid this situation we preallocate memory for one more key in the cache during initialization. Since we can only add one extra key to the cache during the servers run, this means we no longer try to allocate in the critical section in any corner case. While this is not the nicest solution, it is simple and keeps the current cache and decrypt/encrypt logic the same as before. Any other solution would be more complex, and even more of a hack, as it would require dealing with a possibly out of date cache.
There is at lesat one corner case scenario where we have to load the last record into the cache during a write:
In this scenario we could get away by detecting that we are in a write, and asserting if we tried to use the last key.
But in a release build assertions are not fired, and we would end up writing some non encrypted data to disk, and later if we have to run recovery failing.
It could be a FATAL, but that would still crash the server, and the next startup would crash again and again...
Instead, to properly avoid this situation we preallocate memory for one more key in the cache during initialization. Since we can only add one extra key to the cache during the servers run, this means we no longer try to allocate in the critical section in any corner case.
While this is not the nicest solution, it is simple and keeps the current cache and decrypt/encrypt logic the same as before. Any other solution would be more complex, and even more of a hack, as it would require dealing with a possibly out of date cache.