Skip to content

Separate file format for relation and wal keys #500

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

Conversation

AndersAstrand
Copy link
Collaborator

@AndersAstrand AndersAstrand commented Aug 5, 2025

There is a lot of cleanup that will have to be done on both sides of this separation, but this is a bit of "it gets worse before it gets better".

There is also plenty of code, especially around the principal keys, that will need to be re-unified after having properly abstracted them from these key files.

Changes to either format should at least not affect the other after this.

https://perconadev.atlassian.net/browse/PG-1813

This is the only field from the InternalKey structure that's actually
used, and other "raw" crypto functions doesn't use these structures.
This function probably belongs elsewhere than in the key file code, but
that's where it currently resides so expose it so it can also be used
elsewhere.
@AndersAstrand AndersAstrand force-pushed the tde/split-relation-and-wal-key-files branch from 384e0db to a1ff94c Compare August 5, 2025 22:27
@AndersAstrand
Copy link
Collaborator Author

AndersAstrand commented Aug 6, 2025

Seems like it doesn't build properly with make, I only used meson. Will take a look.

@AndersAstrand AndersAstrand force-pushed the tde/split-relation-and-wal-key-files branch from a1ff94c to 29b44d4 Compare August 6, 2025 08:28
@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2025

Codecov Report

❌ Patch coverage is 88.34951% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.03%. Comparing base (0319cb3) to head (75b5032).
⚠️ Report is 1 commits behind head on TDE_REL_17_STABLE.

❌ Your project status has failed because the head coverage (82.03%) 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     #500      +/-   ##
=====================================================
+ Coverage              81.77%   82.03%   +0.26%     
=====================================================
  Files                     24       25       +1     
  Lines                   2991     3168     +177     
  Branches                 485      514      +29     
=====================================================
+ Hits                    2446     2599     +153     
- Misses                   444      460      +16     
- Partials                 101      109       +8     
Components Coverage Δ
access 82.87% <88.77%> (+1.50%) ⬆️
catalog 87.61% <81.81%> (-0.26%) ⬇️
common 77.77% <ø> (ø)
encryption 73.45% <100.00%> (ø)
keyring 73.21% <ø> (ø)
src 94.15% <ø> (ø)
smgr 95.29% <ø> (+0.43%) ⬆️
transam ∅ <ø> (∅)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AndersAstrand AndersAstrand force-pushed the tde/split-relation-and-wal-key-files branch from 7f2d183 to 7f969ef Compare August 6, 2025 09:44
Copy link
Member

@dAdAbird dAdAbird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It screams for abstractions but I reckon that's the best we can do right now. Thanks!

We want to add timeline information to the wal keys and cannot easily do
so without affecting existing clusters' relation key files.

This commit does the bare minimum to separate the two completely and as
such contains a fair bit of duplicated code.

The file format for the WAL key file is exactly the same before and
after this commit.

There is _a lot_ of cleanup that will have to be done on both sides of
this separation, but this is a bit of "it gets worse before it gets
better".
This is to prevent this file from ever getting mixed up with a relation
key file as these might no longer be of the same format.
Instead of 1664_keys it's now called wal_encryption_keys.

This lets us use a constant name for it instead of generating it from an
Oid pretending it's a relation key file.

Also remove some now unused Oid parameters to functions.
@AndersAstrand AndersAstrand force-pushed the tde/split-relation-and-wal-key-files branch from 7f969ef to 75b5032 Compare August 7, 2025 08:53
@AndersAstrand AndersAstrand merged commit 583f8ef into percona:TDE_REL_17_STABLE Aug 7, 2025
19 checks passed
@AndersAstrand AndersAstrand deleted the tde/split-relation-and-wal-key-files branch August 7, 2025 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants