Skip to content

Refactor WAL SMGER FE/BE split and let frontend code also write encrypted WAL #479

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

Merged
merged 4 commits into from
Jul 24, 2025

Conversation

jeltz
Copy link
Collaborator

@jeltz jeltz commented Jul 22, 2025

With ifdefs all over the palce it was hard to expose the write functions to frontend tools so we reduce the number of ifdefs by having one clear set of data structures for backend and one for frontend.

Additionally we give access to WAL key generation and setting the start_lsn of a key to the frontend code.

The performance improvement should be tiny but this also makes the code
easier to follow.
@jeltz jeltz requested review from dutow and dAdAbird as code owners July 22, 2025 09:56
@jeltz jeltz changed the title Tde/refactor wal write fe be Refactor WAL SMGER FE/BE split and let frontend code also write encryopted WAL Jul 22, 2025
@jeltz jeltz force-pushed the tde/refactor-wal-write-fe-be branch from 78c72e1 to 3ef324d Compare July 22, 2025 09:56
@jeltz jeltz marked this pull request as draft July 22, 2025 09:57
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 87.62887% with 12 lines in your changes missing coverage. Please review.

Project coverage is 83.64%. Comparing base (54d0128) to head (3ef324d).
Report is 6 commits behind head on TDE_REL_17_STABLE.

❌ Your project status has failed because the head coverage (83.64%) 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     #479      +/-   ##
=====================================================
+ Coverage              83.47%   83.64%   +0.16%     
=====================================================
  Files                     21       21              
  Lines                   2766     2769       +3     
  Branches                 435      435              
=====================================================
+ Hits                    2309     2316       +7     
+ Misses                   371      368       -3     
+ Partials                  86       85       -1     
Components Coverage Δ
access 81.95% <87.50%> (+0.84%) ⬆️
catalog 87.89% <ø> (ø)
common 77.77% <ø> (ø)
encryption 73.45% <ø> (ø)
keyring 73.21% <ø> (ø)
src 87.55% <100.00%> (+0.02%) ⬆️
smgr 94.88% <ø> (ø)
transam ∅ <ø> (∅)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jeltz added 2 commits July 23, 2025 16:32
Since some frontend tools will need to write WAL while others will not
it makes sense to split the initalization so only some frontend tools
and the backend needs to initialize the WAL write related stuff.
Only the read took the segment size while both writing and reading
actually needs it. Either both or neither should take it as an argument.
@jeltz jeltz force-pushed the tde/refactor-wal-write-fe-be branch 2 times, most recently from e50b2db to c088a92 Compare July 23, 2025 14:58
@jeltz jeltz changed the title Refactor WAL SMGER FE/BE split and let frontend code also write encryopted WAL Refactor WAL SMGER FE/BE split and let frontend code also write encrypted WAL Jul 23, 2025
…nd tools

With ifdefs all over the place it was hard to expose the write functions
to frontend tools so we reduce the number of ifdefs by having one clear
set of data structures fror backend and one for frontend.

Additionally we give access to WAL key generation and setting the start_lsn
of a key to the frontend code.
@jeltz jeltz force-pushed the tde/refactor-wal-write-fe-be branch from c088a92 to f9fcc9d Compare July 23, 2025 15:02
@jeltz jeltz marked this pull request as ready for review July 23, 2025 15:03
Copy link
Collaborator

@artemgavrilov artemgavrilov left a comment

Choose a reason for hiding this comment

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

I checked my pg_resetwal changes on top of this PR. Everything works 👍

Comment on lines +200 to +201
void
TDEXLogSmgrInitWrite(bool encrypt_xlog)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In my pg_resetwal PR I used last WAL key state to check if I need to write encrypted block or not. But with this init function it looks like that better solution will be to read GUC parameter from server, because I guess we don't want to read keys explicitly in resetwal code.

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question. I am not sure which is the best but I do not think either prevents this from being merged.

@jeltz jeltz merged commit a329aeb into percona:TDE_REL_17_STABLE Jul 24, 2025
17 of 19 checks passed
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