Skip to content

WIP: Re-encrypt partially copied relations when needed#528

Draft
dAdAbird wants to merge 3 commits intopercona:mainfrom
dAdAbird:pg_rewind
Draft

WIP: Re-encrypt partially copied relations when needed#528
dAdAbird wants to merge 3 commits intopercona:mainfrom
dAdAbird:pg_rewind

Conversation

@dAdAbird
Copy link
Member

@dAdAbird dAdAbird commented Mar 13, 2026

This is a WIP for fixing various issues with pg_tde_rewind and encrypted tables (https://perconadev.atlassian.net/browse/PG-2234)

Currently changes are only for pg18. I'll port to pg17 after we approve/solve everything for pg18

What's here:
First, it prevent pgdata/pg_tde/ should not be replaced on the target. Otherwise it won't be able to read it own tables on start

Then, for every partially updated relation we do:

  1. Parse RelFileLocator based on the file path.
  2. Check for the internal key on the source cluster.
  3. Check for the internal key on the target cluster.
  4. If the source key exists, decrypt data with it after reading.
  5. If the target key exists, encrypt data with it before writing to disk.

Concerns:

  1. We always assume MAIN_FORK. But I believe it's alright for now - we don't encrypt any other forks (do we?)
  2. It doesn't handle custom table spaces out of pgdata. Does pg_rewind mess with such files at all?
  3. This doesn't help with VACUUM FULL - when there is an encrypted table with new dbOid on the source but no key on the target. For such cases we have to copy internal keys (from pg_tde/xxxxx_keys) to from the source which doesn't exists on the target.
  4. Everything here works only for local source. For the remote, we have to handle the issue of server sending encrypted data
  5. Need to fix relation's providers. Currently even merge won't help since the redo on first start wipes out destinations providers (redo records have position in the file, so they don't add provider but rather replace bytes) and then the redo of key_rotation fails as there is no old key...
  6. Oh, and we need tests of course
@jeltz
Copy link
Collaborator

jeltz commented Mar 16, 2026

We always assume MAIN_FORK. But I believe it's alright for now - we don't encrypt any other forks (do we?)

We do encrypt other forks. Not sure if this was an error or not.

It doesn't handle custom table spaces out of pgdata. Does pg_rewind mess with such files at all?

I sure hope pg_rewind does so, otherwise pg_rewind would be broken.

This doesn't help with VACUUM FULL - when there is an encrypted table with new dbOid on the source but no key on the target. For such cases we have to copy internal keys (from pg_tde/xxxxx_keys) to from the source which doesn't exists on the target.

Would an idea be to find that we need to do so from reading the WAL and looking for key deletion events?

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.

From a quick look the changes seem sound but I need to think more.


/* Skip pg_tde key data */
if (strstr(path, "pg_tde/") != NULL)
return FILE_ACTION_NONE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this mean it should not be in excludeDirContents? Also should the comment explain it better like for global/pg_control?

@dutow
Copy link
Collaborator

dutow commented Mar 17, 2026

First, it prevent pgdata/pg_tde/ should not be replaced on the target

This means we also don't copy wal keys, which means this doesn't work properly with wal encryption.

@dutow
Copy link
Collaborator

dutow commented Mar 18, 2026

I realized one more thing: while we have to keep using our own pg_tde directory at the beginning, if we want to do a consistent recovery, we also have to ensure that we are using exactly the same key providers/principal keys as the source at the end.

@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 65.67164% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.56%. Comparing base (e50afc9) to head (4f90e1f).
⚠️ Report is 3 commits behind head on main.

❌ Your project status has failed because the head coverage (79.70%) 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 @@ ## main #528 +/- ## ========================================== - Coverage 58.63% 58.56% -0.08%  ========================================== Files 68 68 Lines 10696 10783 +87 Branches 1842 1857 +15 ========================================== + Hits 6272 6315 +43  - Misses 3557 3593 +36  - Partials 867 875 +8 
Components Coverage Δ
access 80.77% <ø> (ø)
bin 69.82% <ø> (ø)
catalog 84.63% <ø> (ø)
common 72.22% <ø> (ø)
encryption 65.24% <100.00%> (+6.42%) ⬆️
keyring 67.80% <ø> (ø)
src 90.05% <ø> (ø)
smgr 91.28% <100.00%> (-0.05%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants