8000 PG-1507 PG-1508 Disallow ALTER TABLE when there is a mix of encrypted… · percona/postgres@effdc95 · GitHub
[go: up one dir, main page]
Skip to content

Commit effdc95

Browse files
jeltzjeltz
committed
PG-1507 PG-1508 Disallow ALTER TABLE when there is a mix of encrypted and unencrypted
Since ALTER TABLE can modify multiple due to inheritance or typed tables which can for example result in TOAST tables being created for some or all of the modified tables while the event trigger is only fired once we cannot rely on the event stack to make sure we get the correct encryption status. Our solution is to be cautious and only modify tables when all tables with storage are either encrypt or not encrypted. If there is a mix we will throw an error. The result of this is also used to properly inform the SMGR of the current encryption status. We apply the same logic to ALTER TYPE for typed tables. A possibility for future improvement would be to limit this check only to commands which recurse down to child tables and/or can create new relfilenodes.
1 parent e5e04ee commit effdc95

File tree

5 files changed

+201
-9
lines changed

5 files changed

+201
-9
lines changed

‎contrib/pg_tde/expected/partition_table.out

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ SELECT pg_tde_is_encrypted('partition_q4_2024');
5252
(1 row)
5353

5454
ALTER TABLE partitioned_table SET ACCESS METHOD heap;
55+
ERROR: Recursive ALTER TABLE on a mix of encrypted and unencrypted relations is not supported
5556
ALTER TABLE partition_q1_2024 SET ACCESS METHOD heap;
5657
ALTER TABLE partition_q2_2024 SET ACCESS METHOD tde_heap;
5758
ALTER TABLE partition_q3_2024 SET ACCESS METHOD heap;
@@ -86,6 +87,12 @@ SELECT pg_tde_is_encrypted('partition_q4_2024');
8687
t
8788
(1 row)
8889

90+
-- Does not care about parent AM as long as all children with storage use the same
91+
ALTER TABLE partition_q1_2024 SET ACCESS METHOD tde_heap;
92+
ALTER TABLE partition_q2_2024 SET ACCESS METHOD tde_heap;
93+
ALTER TABLE partition_q3_2024 SET ACCESS METHOD tde_heap;
94+
ALTER TABLE partition_q4_2024 SET ACCESS METHOD tde_heap;
95+
ALTER TABLE partitioned_table SET ACCESS METHOD heap;
8996
DROP TABLE partitioned_table;
9097
-- Partition inherits encryption status from parent table if default is heap and parent is tde_heap
9198
SET default_table_access_method = "heap";

‎contrib/pg_tde/expected/recreate_storage.out

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,49 @@ SELECT pg_tde_is_encrypted('plain_table_id2_seq2');
229229
f
230230
(1 row)
231231

232+
-- Enforce that we do not mess up encryption status for toast table
233+
CREATE TABLE cities (
234+
name varchar(8),
235+
population real,
236+
elevation int
237+
) USING tde_heap;
238+
CREATE TABLE state_capitals (
239+
state char(2) UNIQUE NOT NULL
240+
) INHERITS (cities) USING heap;
241+
CREATE TABLE capitals (
242+
country char(2) UNIQUE NOT NULL
243+
) INHERITS (cities) USING tde_heap;
244+
ALTER TABLE cities ALTER COLUMN name TYPE TEXT;
245+
ERROR: Recursive ALTER TABLE on a mix of encrypted and unencrypted relations is not supported
246+
-- Enforce the same for typed tables
247+
CREATE TYPE people_type AS (age int, name varchar(8), dob date);
248+
CREATE TABLE sales_staff OF people_type USING tde_heap;
249+
CREATE TABLE other_staff OF people_type USING heap;
250+
ALTER TYPE people_type ALTER ATTRIBUTE name TYPE text CASCADE;
251+
ERROR: Recursive ALTER TABLE on a mix of encrypted and unencrypted relations is not supported
252+
-- If all tpyed tables are encrypted everything should work as usual
253+
ALTER TABLE other_staff SET ACCESS METHOD tde_heap;
254+
ALTER TYPE people_type ALTER ATTRIBUTE name TYPE text CASCADE;
255+
SELECT pg_tde_is_encrypted('pg_toast.pg_toast_' || 'sales_staff'::regclass::oid);
256+
pg_tde_is_encrypted
257+
---------------------
258+
t
259+
(1 row)
260+
261+
SELECT pg_tde_is_encrypted('pg_toast.pg_toast_' || 'other_staff'::regclass::oid);
262+
pg_tde_is_encrypted
263+
---------------------
264+
t
265+
(1 row)
266+
267+
DROP TYPE people_type CASCADE;
268+
NOTICE: drop cascades to 2 other objects
269+
DETAIL: drop cascades to table sales_staff
270+
drop cascades to table other_staff
271+
DROP TABLE cities CASCADE;
272+
NOTICE: drop cascades to 2 other objects
273+
DETAIL: drop cascades to table state_capitals
274+
drop cascades to table capitals
232275
DROP TABLE plain_table;
233276
DROP EXTENSION pg_tde CASCADE;
234277
NOTICE: drop cascades to 7 other objects

‎contrib/pg_tde/sql/partition_table.sql

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,13 @@ SELECT pg_tde_is_encrypted('partition_q2_2024');
3131
SELECT pg_tde_is_encrypted('partition_q3_2024');
3232
SELECT pg_tde_is_encrypted('partition_q4_2024');
3333

34+
-- Does not care about parent AM as long as all children with storage use the same
35+
ALTER TABLE partition_q1_2024 SET ACCESS METHOD tde_heap;
36+
ALTER TABLE partition_q2_2024 SET ACCESS METHOD tde_heap;
37+
ALTER TABLE partition_q3_2024 SET ACCESS METHOD tde_heap;
38+
ALTER TABLE partition_q4_2024 SET ACCESS METHOD tde_heap;
39+
ALTER TABLE partitioned_table SET ACCESS METHOD heap;
40+
3441
DROP TABLE partitioned_table;
3542

3643
-- Partition inherits encryption status from parent table if default is heap and parent is tde_heap

‎contrib/pg_tde/sql/recreate_storage.sql

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,39 @@ SELECT pg_tde_is_encrypted('plain_table_id2_seq2');
9696
ALTER SEQUENCE plain_table_id2_seq2 OWNED BY NONE;
9797
SELECT pg_tde_is_encrypted('plain_table_id2_seq2');
9898

99+
-- Enforce that we do not mess up encryption status for toast table
100+
CREATE TABLE cities (
101+
name varchar(8),
102+
population real,
103+
elevation int
104+
) USING tde_heap;
105+
106+
CREATE TABLE state_capitals (
107+
state char(2) UNIQUE NOT NULL
108+
) INHERITS (cities) USING heap;
109+
110+
CREATE TABLE capitals (
111+
country char(2) UNIQUE NOT NULL
112+
) INHERITS (cities) USING tde_heap;
113+
114+
ALTER TABLE cities ALTER COLUMN name TYPE TEXT;
115+
116+
-- Enforce the same for typed tables
117+
CREATE TYPE people_type AS (age int, name varchar(8), dob date);
118+
CREATE TABLE sales_staff OF people_type USING tde_heap;
119+
CREATE TABLE other_staff OF people_type USING heap;
120+
121+
ALTER TYPE people_type ALTER ATTRIBUTE name TYPE text CASCADE;
122+
123+
-- If all tpyed tables are encrypted everything should work as usual
124+
ALTER TABLE other_staff SET ACCESS METHOD tde_heap;
125+
ALTER TYPE people_type ALTER ATTRIBUTE name TYPE text CASCADE;
126+
127+
SELECT pg_tde_is_encrypted('pg_toast.pg_toast_' || 'sales_staff'::regclass::oid);
128+
SELECT pg_tde_is_encrypted('pg_toast.pg_toast_' || 'other_staff'::regclass::oid);
129+
130+
DROP TYPE people_type CASCADE;
131+
DROP TABLE cities CASCADE;
99132
DROP TABLE plain_table;
100133
DROP EXTENSION pg_tde CASCADE;
101134
RESET default_table_access_method;

‎contrib/pg_tde/src/pg_tde_event_capture.c

Lines changed: 111 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,10 @@
1717
#include "utils/lsyscache.h"
1818
#include "catalog/pg_class.h"
1919
#include "catalog/pg_database.h"
20+
#include "catalog/pg_inherits.h"
2021
#include "commands/defrem.h"
2122
#include "commands/sequence.h"
23+
#include "access/heapam.h"
2224
#include "access/table.h"
2325
#include "access/relation.h"
2426
#include "catalog/pg_event_trigger.h"
@@ -141,6 +143,97 @@ push_event_stack(const TdeDdlEvent *event)
141143
MemoryContextSwitchTo(oldCtx);
142144
}
143145

146+
static List *
147+
find_typed_table_dependencies(Oid typeOid)
148+
{
149+
Relation classRel;
150+
ScanKeyData key[1];
151+
TableScanDesc scan;
152+
HeapTuple tuple;
153+
List *result = NIL;
154+
155+
classRel = table_open(RelationRelationId, AccessShareLock);
156+
157+
ScanKeyInit(&key[0],
158+
Anum_pg_class_reloftype,
159+
BTEqualStrategyNumber, F_OIDEQ,
160+
ObjectIdGetDatum(typeOid));
161+
162+
scan = table_beginscan_catalog(classRel, 1, key);
163+
164+
while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
165+
{
166+
Form_pg_class classform = (Form_pg_class) GETSTRUCT(tuple);
167+
168+
LockRelationOid(classform->oid, AccessShareLock);
169+
result = lappend_oid(result, classform->oid);
170+
}
171+
172+
table_endscan(scan);
173+
table_close(classRel, AccessShareLock);
174+
175+
return result;
176+
}
177+
178+
typedef enum
179+
{
180+
ENC_MIX_UNKNOWN,
181+
ENC_MIX_PLAIN,
182+
ENC_MIX_ENCRYPTED,
183+
ENC_MIX_MIXED,
184+
} EncryptionMix;
185+
186+
/*
187+
* Since ALTER TABLE can modify multiple tables due to inheritance or typed
188+
* tables which can for example result in TOAST tables being created for some
189+
* or all of the modified tables while the event trigger is only fired once we
190+
* cannot rely on the event stack to make sure we get the correct encryption
191+
* status.
192+
*
193+
* Our solution is to be cautious and only modify tables when all tables with
194+
* storage are either encrypted or not encrypted. If there is a mix we will
195+
* throw and error. The result of this is also used to properly inform the
196+
* SMGR of the current encryption status.
197+
*/
198+
static EncryptionMix
199+
alter_table_encryption_mix(Oid relid)
200+
{
201+
EncryptionMix enc = ENC_MIX_UNKNOWN;
202+
Relation rel;
203+
List *children;
204+
ListCell *lc;
205+
206+
rel = relation_open(relid, NoLock);
207+
208+
if (RELKIND_HAS_STORAGE(rel->rd_rel->relkind))
209+
enc = rel->rd_rel->relam == get_tde_table_am_oid() ? ENC_MIX_ENCRYPTED : ENC_MIX_PLAIN;
210+
211+
if (rel->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
212+
children = find_typed_table_dependencies(rel->rd_rel->reltype);
213+
else
214+
children = find_inheritance_children(relid, AccessShareLock);
215+
216+
relation_close(rel, NoLock);
217+
218+
foreach(lc, children)
219+
{
220+
Oid childid = lfirst_oid(lc);
221+
EncryptionMix childenc;
222+
223+
childenc = alter_table_encryption_mix(childid);
224+
225+
if (childenc != ENC_MIX_UNKNOWN)
226+
{
227+
if (enc == ENC_MIX_UNKNOWN)
228+
enc = childenc;
229+
else if (enc != childenc)
230+
return ENC_MIX_MIXED;
231+
}
232+
}
233+
234+
return enc;
235+
}
236+
144237
/*
145238
* pg_tde_ddl_command_start_capture is an event trigger function triggered
146239
* at the start of any DDL command execution.
@@ -267,6 +360,7 @@ pg_tde_ddl_command_start_capture(PG_FUNCTION_ARGS)
267360
AlterTableCmd *setAccessMethod = NULL;
268361
ListCell *lcmd;
269362
TdeDdlEvent event = {.parsetree = parsetree};
363+
EncryptionMix encmix;
270364

271365
foreach(lcmd, stmt->cmds)
272366
{
@@ -276,6 +370,20 @@ pg_tde_ddl_command_start_capture(PG_FUNCTION_ARGS)
276370
setAccessMethod = cmd;
277371
}
278372

373+
encmix = alter_table_encryption_mix(relid);
374+
375+
/*
376+
* This check is very braod and could be limited only to commands
377+
* which recurse to child tables or to those which may create new
378+
* relfilenodes, but this restrictive code is good enough for now.
379+
*/
380+
if (encmix == ENC_MIX_MIXED)
381+
{
382+
ereport(ERROR,
383+
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
384+
errmsg("Recursive ALTER TABLE on a mix of encrypted and unencrypted relations is not supported"));
385+
}
386+
279387
/*
280388
* With a SET ACCESS METHOD clause, use that as the basis for
281389
* decisions. But if it's not present, look up encryption status
@@ -292,19 +400,13 @@ pg_tde_ddl_command_start_capture(PG_FUNCTION_ARGS)
292400
}
293401
else
294402
{
295-
Relation rel = relation_open(relid, AccessShareLock);
296-
297-
if (rel->rd_rel->relam == get_tde_table_am_oid())
403+
if (encmix == ENC_MIX_ENCRYPTED)
298404
{
299-
/*
300-
* We are altering an encrypted table and ALTER TABLE can
301-
* possibly create new files so set the global state.
302-
*/
303405
event.encryptMode = TDE_ENCRYPT_MODE_ENCRYPT;
304406
checkPrincipalKeyConfigured();
305407
}
306-
307-
relation_close(rel, NoLock);
408+
else if (encmix == ENC_MIX_PLAIN)
409+
event.encryptMode = TDE_ENCRYPT_MODE_PLAIN;
308410
}
309411

310412
push_event_stack(&event);

0 commit comments

Comments
 (0)
0