Skip to content

Conversation

AndersAstrand
Copy link
Collaborator

@AndersAstrand AndersAstrand commented Aug 12, 2025

Use a single argument for the wrapped command in the archivation wrappers.

Instead of giving all of the arguments of the command separately and trying to figure out which one should be replaced by the path to the unencrypted WAL segment, we take a single argument and do % parameter replacement similar to what postgres does with archive_command and restore_command.

This also mean that we can simplify by using system() instead of exec().

Replaces: #522

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

It's decrypt, not deceypt.
@AndersAstrand
Copy link
Collaborator Author

AndersAstrand commented Aug 12, 2025

I am not certain of whether it's safe to derive the name in the restore command in order to figure out whether it's a wal segment that needs encrypting or something else.

I think it is, but I might be missing something.

EDIT: I was missing something.

@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2025

Codecov Report

❌ Patch coverage is 34.88372% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.07%. Comparing base (b2bb77c) to head (423d614).
⚠️ Report is 4 commits behind head on TDE_REL_17_STABLE.

❌ Your project status has failed because the head coverage (82.07%) 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     #526      +/-   ##
=====================================================
- Coverage              82.19%   82.07%   -0.13%     
=====================================================
  Files                     25       25              
  Lines                   3174     3168       -6     
  Branches                 515      504      -11     
=====================================================
- Hits                    2609     2600       -9     
- Misses                   456      461       +5     
+ Partials                 109      107       -2     
Components Coverage Δ
access 83.47% <ø> (ø)
catalog 87.60% <ø> (ø)
common 77.77% <ø> (ø)
encryption 72.97% <ø> (ø)
keyring 73.21% <ø> (ø)
src 94.15% <ø> (ø)
smgr 95.37% <ø> (+0.08%) ⬆️
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/use-system-for-archiving branch 2 times, most recently from 0924dc5 to 81fea7b Compare August 12, 2025 12:31
@AndersAstrand AndersAstrand changed the title Use system() over exec() for archive/restore commands PG-1862 Use system() over exec() for archive/restore commands Aug 12, 2025
@AndersAstrand AndersAstrand force-pushed the tde/use-system-for-archiving branch 2 times, most recently from 7a825ad to f074219 Compare August 13, 2025 07:49
Use a single argument for the wrapped command in the archivation
wrappers.

Instead of giving all of the arguments of the command separately and
trying to figure out which one should be replaced by the path to the
unencrypted WAL segment, we take a single argument and do % parameter
replacement similar to what postgres does with archive_command and
restore_command.

This also mean that we can simplify by using system() instead of exec().

We also clean up usage instructions and make the two wrappers more
symmetrical by requiring the same parameters.

Co-authored-by: Andreas Karlsson <[email protected]>
@AndersAstrand AndersAstrand force-pushed the tde/use-system-for-archiving branch from f074219 to 423d614 Compare August 13, 2025 07:59
Copy link
Collaborator

@jeltz jeltz left a comment

Choose a reason for hiding this comment

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

Really nice PR!

@AndersAstrand AndersAstrand merged commit f508287 into percona:TDE_REL_17_STABLE Aug 13, 2025
18 of 19 checks passed
@AndersAstrand AndersAstrand deleted the tde/use-system-for-archiving branch August 13, 2025 09:03
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