Fix CREATE/DROP tde_heap in non-default tablespace #329
Conversation
This file contains 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
dAdAbird
force-pushed
the
tablespaces_tde_heap_am
branch
from
e84f04e
to
479a9b4
Compare
dAdAbird
changed the title
dAdAbird
requested review from
codeforall and
dutow
and removed request for
dutow
tde_mdopen() is called before tde_mdcreate() during CREATE. And in case of non-default tablespace, the creation of the key will fail in tde_mdopen(), as the storage is yet to be created (namely database dir). This doesn't happen with default table space because the db dir had been already created before the CREATE TABLE call.
We must clean up *.tde files in the non-default tablespace after dropping/moving tde_heap relations. There is no way for `tde_mdunlink` to determine the relation type so we check any (tde and non-tde) relations.
dAdAbird
force-pushed
the
tablespaces_tde_heap_am
branch
from
479a9b4
to
0d646b0
Compare
dutow
reviewed
Nov 13, 2024
| @@ -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) && | |||
There was a problem hiding this comment.
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.
| /* 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) |
There was a problem hiding this comment.
accidental whitespace change?
| * 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); |
There was a problem hiding this comment.
But we will emit warnings even with false, that could be confusing.
|
Will fix in https://perconadev.atlassian.net/browse/PG-1209 |
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.
tde_mdopen()is called beforetde_mdcreate()during CREATE. In the case of non-default tablespace, the creation of the key will fail in tde_mdopen(), as the storage is yet to be created (namely database dir). This doesn't happen with default table space because the db dir had been already created before the CREATE TABLE call.*.tdefiles. And remove those files physically if there is nothing (tde related) left in the table space. OtherwiseDROP TABLESPACEwill fail.One caveat is that
tde_mdunlink()is called after the transaction is commited, so we must not have any errors down the call stack. For that, we need related tdemap* functions to have an option of not failing on error.Fixes PG-1196