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.

Sign up for GitHub

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

Jump to bottom

Fix CREATE/DROP tde_heap in non-default tablespace #329

Closed
wants to merge 3 commits into from
Closed

Fix CREATE/DROP tde_heap in non-default tablespace #329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8d89c80
Fix CREATE tde_heap in non-default tablespace
dAdAbird Nov 6, 2024
ac08362
Fix drop/move tde_heap in non-default tablespace
dAdAbird Nov 12, 2024
0d646b0
A non-error option for keys clean-up
dAdAbird Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions expected/tablespace.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,9 @@ SELECT * FROM test WHERE num1=110;

DROP TABLE test;
DROP TABLESPACE test_tblspace;
-- check CREATE in non-default tablespace
CREATE TABLESPACE test_tblspace2 LOCATION '';
CREATE TABLE test(num1 bigint, num2 double precision, t text) USING :tde_am TABLESPACE test_tblspace2;
DROP TABLE test;
DROP TABLESPACE test_tblspace2;
DROP EXTENSION pg_tde;
5 changes: 5 additions & 0 deletions expected/tablespace_basic.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,9 @@ SELECT * FROM test WHERE num1=110;

DROP TABLE test;
DROP TABLESPACE test_tblspace;
-- check CREATE in non-default tablespace
CREATE TABLESPACE test_tblspace2 LOCATION '';
CREATE TABLE test(num1 bigint, num2 double precision, t text) USING :tde_am TABLESPACE test_tblspace2;
DROP TABLE test;
DROP TABLESPACE test_tblspace2;
DROP EXTENSION pg_tde;
7 changes: 7 additions & 0 deletions sql/tablespace.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,11 @@ SELECT * FROM test WHERE num1=110;

DROP TABLE test;
DROP TABLESPACE test_tblspace;

-- check CREATE in non-default tablespace
CREATE TABLESPACE test_tblspace2 LOCATION '';
CREATE TABLE test(num1 bigint, num2 double precision, t text) USING :tde_am TABLESPACE test_tblspace2;

DROP TABLE test;
DROP TABLESPACE test_tblspace2;
DROP EXTENSION pg_tde;
140 changes: 94 additions & 46 deletions src/access/pg_tde_tdemap.c
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion src/include/access/pg_tde_tdemap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ extern RelKeyData *tde_create_rel_key(RelFileNumber rel_num, InternalKey *key, T
extern RelKeyData *tde_encrypt_rel_key(TDEPrincipalKey *principal_key, RelKeyData *rel_key_data, const RelFileLocator *rlocator);
extern RelKeyData *tde_decrypt_rel_key(TDEPrincipalKey *principal_key, RelKeyData *enc_rel_key_data, const RelFileLocator *rlocator);
extern RelKeyData *pg_tde_get_key_from_file(const RelFileLocator *rlocator, uint32 key_type, bool no_map_ok);
extern bool pg_tde_move_rel_key(const RelFileLocator *newrlocator, const RelFileLocator *oldrlocator);
extern void pg_tde_move_rel_key(const RelFileLocator *newrlocator, const RelFileLocator *oldrlocator);
extern void pg_tde_clean_map_data(const RelFileLocator *rlocator, bool error_on_failure);

extern void pg_tde_set_db_file_paths(Oid dbOid, Oid spcOid, char *map_path, char *keydata_path);

Expand Down
2 changes: 1 addition & 1 deletion src/pg_tde_event_capture.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ pg_tde_ddl_command_start_capture(PG_FUNCTION_ARGS)
foreach(lcmd, stmt->cmds)
{
AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd);
if (cmd->subtype == AT_SetAccessMethod &&
if ((cmd->subtype == AT_SetAccessMethod || cmd->subtype == AT_SetTableSpace) &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure about this? I think name will refer to the tablespace name for SET TABLESPACE, not the access method name, making the next access method name checks incorrect. In case of SET TABLESPACE, we should open the table instead and check the current access method.

Also, the TODO comment below is outdated with these changes.

((cmd->name != NULL && strcmp(cmd->name, "tde_heap")==0) ||
(cmd->name == NULL && strcmp(default_table_access_method, "tde_heap") == 0))
)
Expand Down
54 changes: 39 additions & 15 deletions src/smgr/pg_tde_smgr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ tde_is_encryption_required(TDESMgrRelation tdereln, ForkNumber forknum)
}

static RelKeyData *
tde_smgr_get_key(SMgrRelation reln, RelFileLocator* old_locator)
tde_smgr_get_key(SMgrRelation reln, RelFileLocator* old_locator, bool createIfNone)
{
TdeCreateEvent *event;
RelKeyData *rkd;
Expand Down Expand Up @@ -68,19 +68,23 @@ tde_smgr_get_key(SMgrRelation reln, RelFileLocator* old_locator)
return rkd;
}

if (!createIfNone)
return NULL;

/* if this is a CREATE TABLE, we have to generate the key */
if (event->encryptMode == true && event->eventType == TDE_TABLE_CREATE_EVENT)
if(event->encryptMode == true && event->eventType == TDE_TABLE_CREATE_EVENT)
Copy link
Collaborator

Choose a reason for hiding this comment

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

accidental whitespace change?

{
return pg_tde_create_smgr_key(&reln->smgr_rlocator.locator);
}

/* if this is a CREATE INDEX, we have to load the key based on the table */
if (event->encryptMode == true && event->eventType == TDE_INDEX_CREATE_EVENT)
{
/* For now keep it simple and create separate key for indexes */
/*
/*
* For now keep it simple and create separate key for indexes.
*
* Later we might modify the map infrastructure to support the same
* keys
* keys.
*/
return pg_tde_create_smgr_key(&reln->smgr_rlocator.locator);
}
Expand Down Expand Up @@ -226,21 +230,23 @@ tde_mdreadv(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
static void
tde_mdcreate(RelFileLocator relold, SMgrRelation reln, ForkNumber forknum, bool isRedo)
{
RelKeyData* key;
TDESMgrRelation tdereln = (TDESMgrRelation) reln;

/*
* This is the only function that gets called during actual CREATE
* TABLE/INDEX (EVENT TRIGGER)
/*
* We have to ensure that all direcories are created before moving on with
* the possible key createion. This is crucial for non-default tablespaces.
*/
/* so we create the key here by loading it */

mdcreate(relold, reln, forknum, isRedo);

/*
* This is the only function that gets called during actual CREATE
* TABLE/INDEX (EVENT TRIGGER) so we create the key here by loading it.
* Later calls then decide to encrypt or not based on the existence of the
* key
* key.
* So we create the key here by loading it.
*/
RelKeyData *key = tde_smgr_get_key(reln, &relold);
key = tde_smgr_get_key(reln, &relold, true);

if (key)
{
Expand All @@ -254,14 +260,20 @@ tde_mdcreate(RelFileLocator relold, SMgrRelation reln, ForkNumber forknum, bool
}

/*
* mdopen() -- Initialize newly-opened relation.
* tde_mdopen() -- Initialize newly-opened relation.
*/
static void
tde_mdopen(SMgrRelation reln)
{
TDESMgrRelation tdereln = (TDESMgrRelation) reln;
RelKeyData *key = tde_smgr_get_key(reln, NULL);

/*
* tde_mdopen() is called before tde_mdcreate() during CREATE. And in case
* of non-default tablespace, the creation of the key will fail here, as
* the storage is yet to be created.
*/
RelKeyData *key = tde_smgr_get_key(reln, NULL, false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

But we will emit warnings even with false, that could be confusing.


if (key)
{
tdereln->encrypted_relation = true;
Expand All @@ -274,6 +286,18 @@ tde_mdopen(SMgrRelation reln)
mdopen(reln);
}

static void
tde_mdunlink(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo)
{
/*
* tde_mdunlink() being called after the transaction is commited, so we
* cannot have any errors down the call stack.
*/
pg_tde_clean_map_data(&rlocator.locator, false);

mdunlink(rlocator, forknum, isRedo);
}

static SMgrId tde_smgr_id;
static const struct f_smgr tde_smgr = {
.name = "tde",
Expand All @@ -283,7 +307,7 @@ static const struct f_smgr tde_smgr = {
.smgr_close = mdclose,
.smgr_create = tde_mdcreate,
.smgr_exists = mdexists,
.smgr_unlink = mdunlink,
.smgr_unlink = tde_mdunlink,
.smgr_extend = tde_mdextend,
.smgr_zeroextend = mdzeroextend,
.smgr_prefetch = mdprefetch,
Expand Down
Loading