Move SMGR code out of pg_tde_tdemap.c
#361
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jeltz
force-pushed
the
tde/smgr-refactor
branch
10 times, most recently
from
cc70455 to
4a15334
Compare
| @@ -962,14 +908,56 @@ pg_tde_has_smgr_key(RelFileLocator rel) | |||
| } | |||
|
|
|||
| /* | |||
| * Returns TDE key for a given relation. | |||
| * Reads the key of the required relation. It identifies its map entry and then simply | |||
There was a problem hiding this comment.
map entry and keydata file are no longer separate things
| { | ||
| TDESMgrRelation *tdereln = (TDESMgrRelation *) reln; | ||
|
|
||
| if (reln->smgr_which != OurSMgrId) |
There was a problem hiding this comment.
This is the first time we actually use our SMgrId afaics. Should we use it more? Why haven't we needed it before?
There was a problem hiding this comment.
Because the other calls go through the smgr*() functions while this one does not.
| @@ -328,7 +417,67 @@ RegisterStorageMgr(void) | |||
| { | |||
| if (storage_manager_id != MdSMgrId) | |||
| elog(FATAL, "Another storage manager was loaded before pg_tde. Multiple storage managers is unsupported."); | |||
| storage_manager_id = smgr_register(&tde_smgr, sizeof(TDESMgrRelation)); | |||
| storage_manager_id = OurSMgrId = smgr_register(&tde_smgr, sizeof(TDESMgrRelation)); | |||
There was a problem hiding this comment.
I'm not a big fan of chained assignments like this. I think they're always harder than necessary to read.
jeltz
force-pushed
the
tde/smgr-refactor
branch
2 times, most recently
from
be58a0a to
187449b
Compare
dutow
approved these changes
May 27, 2025
|
Yeah, maybe it should be in a separate file. I did consider that but will merge this as-is. |
Since we rely on the SMGR relation cache for fast key lookup when opening a SMGR relation and that the catalog should be open basically all the time this optimization adds little value and only complicates the code.
Especially all code realted to the keys of temporary tables did not belong in the TDE map code and fit better in the SMGR code. Additionally we speed up pg_tde_is_encrypted() by relying on the SMGR to cache the relations. This might in some cases lead to blow up of the SMGR relation cache if you query every relation in the database but given the small size I am not overly worried.
Since the caller of the function became a very thin wrapper having both functions no longer make any sense.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Especially all code related to the keys of temporary tables did not belong in the TDE map code and fit better in the SMGR code.
Additionally we speed up
pg_tde_is_encrypted()by relying on the SMGR to cache the relations. This might in some cases lead to blow up of the SMGR relation cache if you query every relation in the database but given the small size I am not overly worried.Also removes an optimization where we skipped trying to look for a key during SMGR open for catalog tables. It made the code harder to read and I do not think it was a very useful optimization.
So what do you think about the removed catalog optimization and the potential cache issue? Also I am not entirely happy with the naming of some of the functions.