Skip to content
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

Refactoring the code and adding TDEMasterKeyInfo to TDE file headers. #141

Closed
wants to merge 0 commits into from

Conversation

hqakhtar
Copy link
Contributor

This also eliminates the master key info file in favour of saving the same information in the TDE map and key data files.

This is the first step towards master key rotation as we are eliminating the need for making changes in multiple files and thereby, making the master key rotation an atomic operation.

/* Set the file paths */
pg_tde_set_db_file_paths();

/* Open and vaidate file for basic correctness. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* Open and vaidate file for basic correctness. */

{
/* Return if the values are already set */
if (*db_path && *db_map_path && *db_keydata_path)
return;

/* Fill in the values */
snprintf(db_path, MAXPGPATH, "%s", GetDatabasePath(rlocator->dbOid, rlocator->spcOid));
snprintf(db_path, MAXPGPATH, "%s", GetDatabasePath(MyDatabaseId, DEFAULTTABLESPACE_OID));
Copy link
Member

Choose a reason for hiding this comment

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

MyDatabaseId won't work for replication as XLog replay has value. We would need to use pg_tde_write_map_entry() and pg_tde_delete_tde_files() there and those functions call pg_tde_set_db_file_paths(). So we have to have an option to pass a databaseId here. I'm not sure about table space though.


static int32 pg_tde_write_map_entry(const RelFileLocator *rlocator, char *db_map_path, const char *master_key_name);
static int32 pg_tde_write_map_entry(const RelFileLocator *rlocator, char *db_map_path, TDEMasterKey *master_key);
Copy link
Member

Choose a reason for hiding this comment

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

I feel like TDEMasterKeyInfo is more appropriate here and in the rest of the functions in this file as we dealing with the key info only. It would be more clear for the code readers.

Comment on lines 301 to 302
PathNameDeleteTemporaryFile(db_map_path, true);
PathNameDeleteTemporaryFile(db_keydata_path, true);
Copy link
Member

Choose a reason for hiding this comment

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

Won't durable_unlink() be better here? "TemporaryFile" a bit looks confusing in this context plus durable_unlink() ensures the remove effect is persist before returning

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.

2 participants