PG-1863 Do not try to delete keys or log WAL for temporary tables

[jeltz]

We forgot to have a check against trying to delete leftover SMGR keys for temporary tables which is a useless operation since they are in stored in memory.

Additionally we forgot to prevent WAL from being written when creating or removing a key in smgrcreate() for temporary tables.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 88.88889% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.21%. Comparing base (b2bb77c) to head (64b432a).

❌ Your project status has failed because the head coverage (82.21%) 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     #512      +/-   ##
=====================================================
+ Coverage              82.19%   82.21%   +0.01%     
=====================================================
  Files                     25       25              
  Lines                   3174     3177       +3     
  Branches                 515      516       +1     
=====================================================
+ Hits                    2609     2612       +3     
  Misses                   456      456              
  Partials                 109      109              

Components	Coverage Δ
access	83.47% <40.00%> (ø)
catalog	87.60% <ø> (ø)
common	77.77% <ø> (ø)
encryption	72.97% <ø> (ø)
keyring	73.21% <ø> (ø)
src	94.15% <ø> (ø)
smgr	95.37% <100.00%> (+0.08%)	⬆️
transam	∅ <ø> (∅)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[AndersAstrand]

Why do we prefer "remove" over "delete" here? We actually delete the keys, not just remove them from our collection of keys while they still exist elsewhere. Right?

@artemgavrilov any comments?

[artemgavrilov]

@artemgavrilov any comments?

Yep, looks a little bit messy. tde_smgr_log_create_key but XLOG_TDE_ADD_RELATION_KEY, tde_smgr_log_remove_leftover_key but XLOG_TDE_DELETE_RELATION_KEY.

As for my preference it should be delete for things that we created and remove for things that we added

[jeltz]

Fixed naming to make it slightly more consistent. I did not fix the issues in pg_tde_tdemap.c.

[AndersAstrand]

I'm not sure why we first rename functions from "delete" to "remove" to then name them back.

And with regards to the principal keys XLOG_ constant I think "add" is appropriate since we don't create anything just add "knowledge" of a key that exists elsewhere (in the KMS).

[jeltz]

Add does not just add knowledge, it generates a new key too.

[jeltz]

I had planned to fix the renaming back and forth later after the bikeshedding is finished.

[AndersAstrand]

Add does not just add knowledge, it generates a new key too.

I'm confused. We only create principal keys with the pg_tde_create_key_using_..._() calls, right? And those are then "added" using pg_set_key_using_..._() calls which emits the WAL record in question.

The change for the relation keys from add to create looks fine to me :).

[AndersAstrand]

I had planned to fix the renaming back and forth later after the bikeshedding is finished.

Yeah, it's dangerous to suggest repainting the shed in the same PR as doing real work 😆

[jeltz]

Ah, I missread. Fixed now.

[AndersAstrand]

I think the second commit should be done first, or as a separate PR.

I think the "leftover" clarification in the names from the second commit should be done in the currently first commit though, because it's unrelated to renaming the XLOG_ constants.

[AndersAstrand]

Looks good!

I'm sorry for the confusion. This is why I shouldn't have pushed cleaning the naming up until after the patch I did for adding the WAL record to remove leftover keys. Sorry about that.