Skip to content

PG-1710 Create helpers for decrypting/encrypting archived WAL #487

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 3 commits into from
Jul 29, 2025

Conversation

jeltz
Copy link
Collaborator

@jeltz jeltz commented Jul 25, 2025

To support some common WAL archiving tools, e.g. PgBackRest, we implement an archive_command and a restore_command which can wrap any command and use pipe() to create fake file to either read from or write to. The restore command makes sure to write encrypted files if WAL encryption is enabled. It uses the fresh WAL key generated by the server on the current start which works fine because we then just let the first invocation of the restore command set the start LSN of the key.

For e.g. PgBackRest you would have the following commands:

  archive_command = 'pg_tde_archive_decrypt %p pgbackrest --stanza=demo archive-push %p'
  restore_command = 'pg_tde_restore_encrypt %f %p pgbackrest --stanza=demo archive-get %f "%p"'

An alternative if we want to make sure to use system() like archive_command and restore_command it would instead be, but feels a bit risky since people would have to remember to do %% for it to work.

  archive_command = 'pg_tde_archive_decrypt %p "pgbackrest --stanza=demo archive-push %%p"'
  restore_command = 'pg_tde_restore_encrypt %f %p "pgbackrest --stanza=demo archive-get %%f \"%%p\""'

@jeltz jeltz requested review from dutow and dAdAbird as code owners July 25, 2025 02:28
@jeltz jeltz force-pushed the tde/archive-restore-helpers branch from 5dc0f74 to 6c75dba Compare July 25, 2025 02:28
@jeltz
Copy link
Collaborator Author

jeltz commented Jul 25, 2025

I am aware documentation us missing but I want some early feedback so that I don't waste more time if this is wrong.

@jeltz jeltz force-pushed the tde/archive-restore-helpers branch from 6c75dba to 39cdf06 Compare July 25, 2025 02:36
@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2025

Codecov Report

❌ Patch coverage is 58.92857% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.87%. Comparing base (a329aeb) to head (1903471).
⚠️ Report is 4 commits behind head on TDE_REL_17_STABLE.

❌ Your project status has failed because the head coverage (81.87%) 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     #487      +/-   ##
=====================================================
- Coverage              83.68%   81.87%   -1.82%     
=====================================================
  Files                     21       24       +3     
  Lines                   2771     3001     +230     
  Branches                 435      489      +54     
=====================================================
+ Hits                    2319     2457     +138     
- Misses                   368      446      +78     
- Partials                  84       98      +14     
Components Coverage Δ
access 81.17% <0.00%> (-1.10%) ⬇️
catalog 87.85% <ø> (ø)
common 77.77% <ø> (ø)
encryption 73.45% <ø> (ø)
keyring 73.21% <ø> (ø)
src 94.15% <ø> (+6.59%) ⬆️
smgr 94.85% <ø> (ø)
transam ∅ <ø> (∅)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jeltz jeltz force-pushed the tde/archive-restore-helpers branch 16 times, most recently from aea6c32 to 1903471 Compare July 28, 2025 09:29
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.

Looks good to me. Though I'm not that familiar with the archive_command, but from what I've read, this PR looks sound.

pg_fatal("mismatch of segment size in WAL file \"%s\" (header: %d bytes, file size: %ld bytes)",
segname, walsegsz, fsize);

if (!IsValidWalSegSize(walsegsz))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't this part be a helper function to avoid the duplication in the two tools? And is_segment is also common

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think the duplicated code is enough to bother but I could give it a shot and see if anything improves.

jeltz added 2 commits July 28, 2025 23:19
Now that we will soon be adding more bianries having them at the top
level only makes things confusing for developers.
@jeltz jeltz force-pushed the tde/archive-restore-helpers branch 3 times, most recently from e555fa8 to 56f4484 Compare July 29, 2025 10:05
To support some common WAL archiving tools, e.g. PgBackRest, we
implement an archive_command and a restore_command which can wrap any
command and use pipe() to create fake file to either read from or write
to. The restore command makes sure to write encrypted files if WAL
encryption is enabled. It uses the fresh WAL key generated by the server
on the current start which works fine because we then just let the first
invocation of the restore command set the start LSN of the key.

For e.g. PgBackRest you would have the following commands:

  archive_command = 'pg_tde_archive_decrypt %p pgbackrest --stanza=demo archive-push %p'
  restore_command = 'pg_tde_restore_encrypt %f %p pgbackrest --stanza=demo archive-get %f "%p"'
@jeltz jeltz force-pushed the tde/archive-restore-helpers branch from 56f4484 to 675d3f0 Compare July 29, 2025 10:08
@jeltz jeltz merged commit f6f1ae3 into percona:TDE_REL_17_STABLE Jul 29, 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.

6 participants