Skip to content

[PG-1637] Remove any existing keys for tables on create #388

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 Jun 4, 2025

Some crash scenarios can leave keys behind in the relation key file for OIDs that are unused. If these OIDs are later used when creating a non-encrypted relation the key has to be removed or we will do the wrong thing since the existence of a key makes us assume the relation is encrypted.

@codecov-commenter
Copy link

codecov-commenter commented Jun 4, 2025

Codecov Report

Attention: Patch coverage is 97.22222% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.55%. Comparing base (0529424) to head (45303eb).

❌ Your project status has failed because the head coverage (85.55%) 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     #388      +/-   ##
=====================================================
- Coverage              85.72%   85.55%   -0.18%     
=====================================================
  Files                     22       22              
  Lines                   2529     2547      +18     
  Branches                 387      388       +1     
=====================================================
+ Hits                    2168     2179      +11     
- Misses                   287      292       +5     
- Partials                  74       76       +2     
Components Coverage Δ
access 83.73% <100.00%> (-0.67%) ⬇️
catalog 88.90% <ø> (ø)
common 91.80% <ø> (ø)
encryption 73.45% <ø> (+0.88%) ⬆️
keyring 72.94% <ø> (ø)
src 91.42% <100.00%> (ø)
smgr 94.85% <96.77%> (-2.57%) ⬇️
transam ∅ <ø> (∅)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@@ -93,6 +93,7 @@ _PG_init(void)

check_percona_api_version();

pg_tde_init_data_dir();
Copy link
Member

Choose a reason for hiding this comment

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

what was the reason to move it here? I'm not objecting it, just curious

Copy link
Collaborator Author

@AndersAstrand AndersAstrand Jun 4, 2025

Choose a reason for hiding this comment

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

As I mentioned in the commit message, this directory is used by the SMgr, and the SMgr is loaded and used regardless of whether any database have run CREATE EXTENSION. So the directory should be created regardless.

Practically it was because the call to pg_free_key_map_entry() crashed when the directory didn't exist :).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, sorry, I missed it. Thanks

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.

My first thought is: why can't we always delete keys, even for encrypted relations? And I do not find an answer in the PR.


if (key)
if (unlikely(key))
Copy link
Collaborator

Choose a reason for hiding this comment

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

When can this even happen? Is it a thing which only happens if there are leftover keys or we are doing a recovery/replay on the replica? This branch should be explained.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New version up that is much more strict about what can happen in each case.

@AndersAstrand
Copy link
Collaborator Author

AndersAstrand commented Jun 4, 2025

My first thought is: why can't we always delete keys, even for encrypted relations? And I do not find an answer in the PR.

I have to think about this. I was worried about the recovery case, but there will be a WAL record for creating the key after this function in called, so it shouldn't be an issue. That would also remove the need for the branch you asked about I'm pretty sure. I need to verify though.

I also need to understand exactly how and when the forks are created et.c. because my understanding of this is a little hazy.

Only create keys when MAIN fork is created, and trust
tde_smgr_should_encrypt() to know when to encrypt.

Also trust that the key has already been created if we're in recovery or
replication.
Instead of it being done down in tdemap code controlled with a boolean,
we just do it where the decision to do it is made instead.
There are crash scenarios where keys are left behind in the key file
even though the OID for the table goes unused. This meant that we could
have keys laying around for newly created plaintext relations after OID
wraparound.

Simply removing any existing keys when relations are created seems
appriopriate.

Also move creation of pg_tde data dir to library init. This directory is
used by the SMgr which is loaded regardless of whether any database are
yet to create extension or not.
@AndersAstrand AndersAstrand force-pushed the tde/remove-unused-relation-keys branch from de81c34 to 45303eb Compare June 9, 2025 10:58
@AndersAstrand AndersAstrand changed the title [PG-1637] Remove any existing keys for plaintext tables on create [PG-1637] Remove any existing keys for tables on create Jun 9, 2025
@AndersAstrand AndersAstrand merged commit 27d05dd into percona:TDE_REL_17_STABLE Jun 9, 2025
16 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.

4 participants