Skip to content

Commit 0e1deaa

Browse files
tglsfdctglsfdc
committed
Install dependencies to prevent dropping partition key columns.
The logic in ATExecDropColumn that rejects dropping partition key columns is quite an inadequate defense, because it doesn't execute in cases where a column needs to be dropped due to cascade from something that only the column, not the whole partitioned table, depends on. That leaves us with a badly broken partitioned table; even an attempt to load its relcache entry will fail. We really need to have explicit pg_depend entries that show that the column can't be dropped without dropping the whole table. Hence, add those entries. In v12 and HEAD, bump catversion to ensure that partitioned tables will have such entries. We can't do that in released branches of course, so in v10 and v11 this patch affords protection only to partitioned tables created after the patch is installed. Given the lack of field complaints (this bug was found by fuzz-testing not by end users), that's probably good enough. In passing, fix ATExecDropColumn and ATPrepAlterColumnType messages to be more specific about which partition key column they're complaining about. Per report from Manuel Rigger. Back-patch to v10 where partitioned tables were added. Discussion: https://postgr.es/m/CA+u7OA4JKCPFrdrAbOs7XBiCyD61XJxeNav4LefkSmBLQ-Vobg@mail.gmail.com Discussion: https://postgr.es/m/31920.1562526703@sss.pgh.pa.us
1 parent 8a4fa29 commit 0e1deaa

File tree

8 files changed

+182
-39
lines changed

8 files changed

+182
-39
lines changed

‎src/backend/catalog/dependency.c

Lines changed: 47 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -526,14 +526,18 @@ findDependentObjects(const ObjectAddress *object,
526526
ObjectIdGetDatum(object->objectId));
527527
if (object->objectSubId != 0)
528528
{
529+
/* Consider only dependencies of this sub-object */
529530
ScanKeyInit(&key[2],
530531
Anum_pg_depend_objsubid,
531532
BTEqualStrategyNumber, F_INT4EQ,
532533
Int32GetDatum(object->objectSubId));
533534
nkeys = 3;
534535
}
535536
else
537+
{
538+
/* Consider dependencies of this object and any sub-objects it has */
536539
nkeys = 2;
540+
}
537541

538542
scan = systable_beginscan(*depRel, DependDependerIndexId, true,
539543
NULL, nkeys, key);
@@ -546,6 +550,18 @@ findDependentObjects(const ObjectAddress *object,
546550
otherObject.objectId = foundDep->refobjid;
547551
otherObject.objectSubId = foundDep->refobjsubid;
548552

553+
/*
554+
* When scanning dependencies of a whole object, we may find rows
555+
* linking sub-objects of the object to the object itself. (Normally,
556+
* such a dependency is implicit, but we must make explicit ones in
557+
* some cases involving partitioning.) We must ignore such rows to
558+
* avoid infinite recursion.
559+
*/
560+
if (otherObject.classId == object->classId &&
561+
otherObject.objectId == object->objectId &&
562+
object->objectSubId == 0)
563+
continue;
564+
549565
switch (foundDep->deptype)
550566
{
551567
case DEPENDENCY_NORMAL:
@@ -732,6 +748,16 @@ findDependentObjects(const ObjectAddress *object,
732748
otherObject.objectId = foundDep->objid;
733749
otherObject.objectSubId = foundDep->objsubid;
734750

751+
/*
752+
* If what we found is a sub-object of the current object, just ignore
753+
* it. (Normally, such a dependency is implicit, but we must make
754+
* explicit ones in some cases involving partitioning.)
755+
*/
756+
if (otherObject.classId == object->classId &&
757+
otherObject.objectId == object->objectId &&
758+
object->objectSubId == 0)
759+
continue;
760+
735761
/*
736762
* Must lock the dependent object before recursing to it.
737763
*/
@@ -1379,8 +1405,10 @@ recordDependencyOnExpr(const ObjectAddress *depender,
13791405
* As above, but only one relation is expected to be referenced (with
13801406
* varno = 1 and varlevelsup = 0). Pass the relation OID instead of a
13811407
* range table. An additional frammish is that dependencies on that
1382-
* relation (or its component columns) will be marked with 'self_behavior',
1383-
* whereas 'behavior' is used for everything else.
1408+
* relation's component columns will be marked with 'self_behavior',
1409+
* whereas 'behavior' is used for everything else; also, if 'reverse_self'
1410+
* is true, those dependencies are reversed so that the columns are made
1411+
* to depend on the table not vice versa.
13841412
*
13851413
* NOTE: the caller should ensure that a whole-table dependency on the
13861414
* specified relation is created separately, if one is needed. In particular,
@@ -1393,7 +1421,7 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
13931421
Node *expr, Oid relId,
13941422
DependencyType behavior,
13951423
DependencyType self_behavior,
1396-
bool ignore_self)
1424+
bool reverse_self)
13971425
{
13981426
find_expr_references_context context;
13991427
RangeTblEntry rte;
@@ -1416,7 +1444,8 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
14161444
eliminate_duplicate_dependencies(context.addrs);
14171445

14181446
/* Separate self-dependencies if necessary */
1419-
if (behavior != self_behavior && context.addrs->numrefs > 0)
1447+
if ((behavior != self_behavior || reverse_self) &&
1448+
context.addrs->numrefs > 0)
14201449
{
14211450
ObjectAddresses *self_addrs;
14221451
ObjectAddress *outobj;
@@ -1447,11 +1476,23 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
14471476
}
14481477
context.addrs->numrefs = outrefs;
14491478

1450-
/* Record the self-dependencies */
1451-
if (!ignore_self)
1479+
/* Record the self-dependencies with the appropriate direction */
1480+
if (!reverse_self)
14521481
recordMultipleDependencies(depender,
14531482
self_addrs->refs, self_addrs->numrefs,
14541483
self_behavior);
1484+
else
1485+
{
1486+
/* Can't use recordMultipleDependencies, so do it the hard way */
1487+
int selfref;
1488+
1489+
for (selfref = 0; selfref < self_addrs->numrefs; selfref++)
1490+
{
1491+
ObjectAddress *thisobj = self_addrs->refs + selfref;
1492+
1493+
recordDependencyOn(thisobj, depender, self_behavior);
1494+
}
1495+
}
14551496

14561497
free_object_addresses(self_addrs);
14571498
}

‎src/backend/catalog/heap.c

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3170,16 +3170,36 @@ StorePartitionKey(Relation rel,
31703170
}
31713171

31723172
/*
3173-
* Anything mentioned in the expressions. We must ignore the column
3174-
* references, which will depend on the table itself; there is no separate
3175-
* partition key object.
3173+
* The partitioning columns are made internally dependent on the table,
3174+
* because we cannot drop any of them without dropping the whole table.
3175+
* (ATExecDropColumn independently enforces that, but it's not bulletproof
3176+
* so we need the dependencies too.)
3177+
*/
3178+
for (i = 0; i < partnatts; i++)
3179+
{
3180+
if (partattrs[i] == 0)
3181+
continue; /* ignore expressions here */
3182+
3183+
referenced.classId = RelationRelationId;
3184+
referenced.objectId = RelationGetRelid(rel);
3185+
referenced.objectSubId = partattrs[i];
3186+
3187+
recordDependencyOn(&referenced, &myself, DEPENDENCY_INTERNAL);
3188+
}
3189+
3190+
/*
3191+
* Also consider anything mentioned in partition expressions. External
3192+
* references (e.g. functions) get NORMAL dependencies. Table columns
3193+
* mentioned in the expressions are handled the same as plain partitioning
3194+
* columns, i.e. they become internally dependent on the whole table.
31763195
*/
31773196
if (partexprs)
31783197
recordDependencyOnSingleRelExpr(&myself,
31793198
(Node *) partexprs,
31803199
RelationGetRelid(rel),
31813200
DEPENDENCY_NORMAL,
3182-
DEPENDENCY_AUTO, true);
3201+
DEPENDENCY_INTERNAL,
3202+
true /* reverse the self-deps */ );
31833203

31843204
/*
31853205
* We must invalidate the relcache so that the next

‎src/backend/commands/tablecmds.c

Lines changed: 17 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -6549,25 +6549,26 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
65496549
errmsg("cannot drop system column \"%s\"",
65506550
colName)));
65516551

6552-
/* Don't drop inherited columns */
6552+
/*
6553+
* Don't drop inherited columns, unless recursing (presumably from a drop
6554+
* of the parent column)
6555+
*/
65536556
if (targetatt->attinhcount > 0 && !recursing)
65546557
ereport(ERROR,
65556558
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
65566559
errmsg("cannot drop inherited column \"%s\"",
65576560
colName)));
65586561

6559-
/* Don't drop columns used in the partition key */
6562+
/*
6563+
* Don't drop columns used in the partition key, either. (If we let this
6564+
* go through, the key column's dependencies would cause a cascaded drop
6565+
* of the whole table, which is surely not what the user expected.)
6566+
*/
65606567
if (is_partition_attr(rel, attnum, &is_expr))
6561-
{
6562-
if (!is_expr)
6563-
ereport(ERROR,
6564-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
6565-
errmsg("cannot drop column named in partition key")));
6566-
else
6567-
ereport(ERROR,
6568-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
6569-
errmsg("cannot drop column referenced in partition key expression")));
6570-
}
6568+
ereport(ERROR,
6569+
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
6570+
errmsg("cannot drop column \"%s\" because it is part of the partition key of relation \"%s\"",
6571+
colName, RelationGetRelationName(rel))));
65716572

65726573
ReleaseSysCache(tuple);
65736574

@@ -8776,16 +8777,10 @@ ATPrepAlterColumnType(List **wqueue,
87768777

87778778
/* Don't alter columns used in the partition key */
87788779
if (is_partition_attr(rel, attnum, &is_expr))
8779-
{
8780-
if (!is_expr)
8781-
ereport(ERROR,
8782-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
8783-
errmsg("cannot alter type of column named in partition key")));
8784-
else
8785-
ereport(ERROR,
8786-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
8787-
errmsg("cannot alter type of column referenced in partition key expression")));
8788-
}
8780+
ereport(ERROR,
8781+
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
8782+
errmsg("cannot alter column \"%s\" because it is part of the partition key of relation \"%s\"",
8783+
colName, RelationGetRelationName(rel))));
87898784

87908785
/* Look up the target type */
87918786
typenameTypeIdAndMod(NULL, typeName, &targettype, &targettypmod);

‎src/bin/pg_dump/pg_dump_sort.c

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1173,6 +1173,24 @@ repairDependencyLoop(DumpableObject **loop,
11731173
}
11741174
}
11751175

1176+
/*
1177+
* Loop of table with itself --- just ignore it.
1178+
*
1179+
* (Actually, what this arises from is a dependency of a table column on
1180+
* another column, which happens with generated columns; or a dependency
1181+
* of a table column on the whole table, which happens with partitioning.
1182+
* But we didn't pay attention to sub-object IDs while collecting the
1183+
* dependency data, so we can't see that here.)
1184+
*/
1185+
if (nLoop == 1)
1186+
{
1187+
if (loop[0]->objType == DO_TABLE)
1188+
{
1189+
removeObjectDependency(loop[0], loop[0]->dumpId);
1190+
return;
1191+
}
1192+
}
1193+
11761194
/*
11771195
* If all the objects are TABLE_DATA items, what we must have is a
11781196
* circular set of foreign key constraints (or a single self-referential

‎src/include/catalog/dependency.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@ extern void recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
194194
Node *expr, Oid relId,
195195
DependencyType behavior,
196196
DependencyType self_behavior,
197-
bool ignore_self);
197+
bool reverse_self);
198198

199199
extern ObjectClass getObjectClass(const ObjectAddress *object);
200200

‎src/test/regress/expected/alter_table.out

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3309,13 +3309,13 @@ LINE 1: ALTER TABLE partitioned ADD EXCLUDE USING gist (a WITH &&);
33093309
^
33103310
-- cannot drop column that is part of the partition key
33113311
ALTER TABLE partitioned DROP COLUMN a;
3312-
ERROR: cannot drop column named in partition key
3312+
ERROR: cannot drop column "a" because it is part of the partition key of relation "partitioned"
33133313
ALTER TABLE partitioned ALTER COLUMN a TYPE char(5);
3314-
ERROR: cannot alter type of column named in partition key
3314+
ERROR: cannot alter column "a" because it is part of the partition key of relation "partitioned"
33153315
ALTER TABLE partitioned DROP COLUMN b;
3316-
ERROR: cannot drop column referenced in partition key expression
3316+
ERROR: cannot drop column "b" because it is part of the partition key of relation "partitioned"
33173317
ALTER TABLE partitioned ALTER COLUMN b TYPE char(5);
3318-
ERROR: cannot alter type of column referenced in partition key expression
3318+
ERROR: cannot alter column "b" because it is part of the partition key of relation "partitioned"
33193319
-- partitioned table cannot participate in regular inheritance
33203320
CREATE TABLE nonpartitioned (
33213321
a int,
@@ -3697,9 +3697,9 @@ ERROR: cannot change inheritance of a partition
36973697
-- partitioned tables; for example, part_5, which is list_parted2's
36983698
-- partition, is partitioned on b;
36993699
ALTER TABLE list_parted2 DROP COLUMN b;
3700-
ERROR: cannot drop column named in partition key
3700+
ERROR: cannot drop column "b" because it is part of the partition key of relation "part_5"
37013701
ALTER TABLE list_parted2 ALTER COLUMN b TYPE text;
3702-
ERROR: cannot alter type of column named in partition key
3702+
ERROR: cannot alter column "b" because it is part of the partition key of relation "part_5"
37033703
-- cleanup
37043704
DROP TABLE list_parted, list_parted2, range_parted;
37053705
-- more tests for certain multi-level partitioning scenarios

‎src/test/regress/expected/create_table.out

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -461,6 +461,42 @@ Partition of: partitioned2 FOR VALUES FROM ('-1', 'aaaaa') TO (100, 'ccccc')
461461
Partition constraint: (((a + 1) IS NOT NULL) AND (substr(b, 1, 5) IS NOT NULL) AND (((a + 1) > '-1'::integer) OR (((a + 1) = '-1'::integer) AND (substr(b, 1, 5) >= 'aaaaa'::text))) AND (((a + 1) < 100) OR (((a + 1) = 100) AND (substr(b, 1, 5) < 'ccccc'::text))))
462462

463463
DROP TABLE partitioned, partitioned2;
464+
-- check that dependencies of partition columns are handled correctly
465+
create domain intdom1 as int;
466+
create table partitioned (
467+
a intdom1,
468+
b text
469+
) partition by range (a);
470+
alter table partitioned drop column a; -- fail
471+
ERROR: cannot drop column "a" because it is part of the partition key of relation "partitioned"
472+
drop domain intdom1; -- fail, requires cascade
473+
ERROR: cannot drop type intdom1 because other objects depend on it
474+
DETAIL: table partitioned depends on type intdom1
475+
HINT: Use DROP ... CASCADE to drop the dependent objects too.
476+
drop domain intdom1 cascade;
477+
NOTICE: drop cascades to table partitioned
478+
table partitioned; -- gone
479+
ERROR: relation "partitioned" does not exist
480+
LINE 1: table partitioned;
481+
^
482+
-- likewise for columns used in partition expressions
483+
create domain intdom1 as int;
484+
create table partitioned (
485+
a intdom1,
486+
b text
487+
) partition by range (plusone(a));
488+
alter table partitioned drop column a; -- fail
489+
ERROR: cannot drop column "a" because it is part of the partition key of relation "partitioned"
490+
drop domain intdom1; -- fail, requires cascade
491+
ERROR: cannot drop type intdom1 because other objects depend on it
492+
DETAIL: table partitioned depends on type intdom1
493+
HINT: Use DROP ... CASCADE to drop the dependent objects too.
494+
drop domain intdom1 cascade;
495+
NOTICE: drop cascades to table partitioned
496+
table partitioned; -- gone
497+
ERROR: relation "partitioned" does not exist
498+
LINE 1: table partitioned;
499+
^
464500
--
465501
-- Partitions
466502
--

‎src/test/regress/sql/create_table.sql

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -433,6 +433,39 @@ CREATE TABLE part2_1 PARTITION OF partitioned2 FOR VALUES FROM (-1, 'aaaaa') TO
433433

434434
DROP TABLE partitioned, partitioned2;
435435

436+
-- check that dependencies of partition columns are handled correctly
437+
create domain intdom1 as int;
438+
439+
create table partitioned (
440+
a intdom1,
441+
b text
442+
) partition by range (a);
443+
444+
alter table partitioned drop column a; -- fail
445+
446+
drop domain intdom1; -- fail, requires cascade
447+
448+
drop domain intdom1 cascade;
449+
450+
table partitioned; -- gone
451+
452+
-- likewise for columns used in partition expressions
453+
create domain intdom1 as int;
454+
455+
create table partitioned (
456+
a intdom1,
457+
b text
458+
) partition by range (plusone(a));
459+
460+
alter table partitioned drop column a; -- fail
461+
462+
drop domain intdom1; -- fail, requires cascade
463+
464+
drop domain intdom1 cascade;
465+
466+
table partitioned; -- gone
467+
468+
436469
--
437470
-- Partitions
438471
--

0 commit comments

Comments
 (0)