Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 79e573f

Browse files
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 980224b commit 79e573f

File tree

9 files changed

+174
-41
lines changed

9 files changed

+174
-41
lines changed

src/backend/catalog/dependency.c

Lines changed: 47 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -543,14 +543,18 @@ findDependentObjects(const ObjectAddress *object,
543543
ObjectIdGetDatum(object->objectId));
544544
if (object->objectSubId != 0)
545545
{
546+
/* Consider only dependencies of this sub-object */
546547
ScanKeyInit(&key[2],
547548
Anum_pg_depend_objsubid,
548549
BTEqualStrategyNumber, F_INT4EQ,
549550
Int32GetDatum(object->objectSubId));
550551
nkeys = 3;
551552
}
552553
else
554+
{
555+
/* Consider dependencies of this object and any sub-objects it has */
553556
nkeys = 2;
557+
}
554558

555559
scan = systable_beginscan(*depRel, DependDependerIndexId, true,
556560
NULL, nkeys, key);
@@ -567,6 +571,18 @@ findDependentObjects(const ObjectAddress *object,
567571
otherObject.objectId = foundDep->refobjid;
568572
otherObject.objectSubId = foundDep->refobjsubid;
569573

574+
/*
575+
* When scanning dependencies of a whole object, we may find rows
576+
* linking sub-objects of the object to the object itself. (Normally,
577+
* such a dependency is implicit, but we must make explicit ones in
578+
* some cases involving partitioning.) We must ignore such rows to
579+
* avoid infinite recursion.
580+
*/
581+
if (otherObject.classId == object->classId &&
582+
otherObject.objectId == object->objectId &&
583+
object->objectSubId == 0)
584+
continue;
585+
570586
switch (foundDep->deptype)
571587
{
572588
case DEPENDENCY_NORMAL:
@@ -854,6 +870,16 @@ findDependentObjects(const ObjectAddress *object,
854870
otherObject.objectId = foundDep->objid;
855871
otherObject.objectSubId = foundDep->objsubid;
856872

873+
/*
874+
* If what we found is a sub-object of the current object, just ignore
875+
* it. (Normally, such a dependency is implicit, but we must make
876+
* explicit ones in some cases involving partitioning.)
877+
*/
878+
if (otherObject.classId == object->classId &&
879+
otherObject.objectId == object->objectId &&
880+
object->objectSubId == 0)
881+
continue;
882+
857883
/*
858884
* Must lock the dependent object before recursing to it.
859885
*/
@@ -1588,8 +1614,10 @@ recordDependencyOnExpr(const ObjectAddress *depender,
15881614
* As above, but only one relation is expected to be referenced (with
15891615
* varno = 1 and varlevelsup = 0). Pass the relation OID instead of a
15901616
* range table. An additional frammish is that dependencies on that
1591-
* relation (or its component columns) will be marked with 'self_behavior',
1592-
* whereas 'behavior' is used for everything else.
1617+
* relation's component columns will be marked with 'self_behavior',
1618+
* whereas 'behavior' is used for everything else; also, if 'reverse_self'
1619+
* is true, those dependencies are reversed so that the columns are made
1620+
* to depend on the table not vice versa.
15931621
*
15941622
* NOTE: the caller should ensure that a whole-table dependency on the
15951623
* specified relation is created separately, if one is needed. In particular,
@@ -1602,7 +1630,7 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
16021630
Node *expr, Oid relId,
16031631
DependencyType behavior,
16041632
DependencyType self_behavior,
1605-
bool ignore_self)
1633+
bool reverse_self)
16061634
{
16071635
find_expr_references_context context;
16081636
RangeTblEntry rte;
@@ -1626,7 +1654,8 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
16261654
eliminate_duplicate_dependencies(context.addrs);
16271655

16281656
/* Separate self-dependencies if necessary */
1629-
if (behavior != self_behavior && context.addrs->numrefs > 0)
1657+
if ((behavior != self_behavior || reverse_self) &&
1658+
context.addrs->numrefs > 0)
16301659
{
16311660
ObjectAddresses *self_addrs;
16321661
ObjectAddress *outobj;
@@ -1657,11 +1686,23 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
16571686
}
16581687
context.addrs->numrefs = outrefs;
16591688

1660-
/* Record the self-dependencies */
1661-
if (!ignore_self)
1689+
/* Record the self-dependencies with the appropriate direction */
1690+
if (!reverse_self)
16621691
recordMultipleDependencies(depender,
16631692
self_addrs->refs, self_addrs->numrefs,
16641693
self_behavior);
1694+
else
1695+
{
1696+
/* Can't use recordMultipleDependencies, so do it the hard way */
1697+
int selfref;
1698+
1699+
for (selfref = 0; selfref < self_addrs->numrefs; selfref++)
1700+
{
1701+
ObjectAddress *thisobj = self_addrs->refs + selfref;
1702+
1703+
recordDependencyOn(thisobj, depender, self_behavior);
1704+
}
1705+
}
16651706

16661707
free_object_addresses(self_addrs);
16671708
}

src/backend/catalog/heap.c

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

35303530
/*
3531-
* Anything mentioned in the expressions. We must ignore the column
3532-
* references, which will depend on the table itself; there is no separate
3533-
* partition key object.
3531+
* The partitioning columns are made internally dependent on the table,
3532+
* because we cannot drop any of them without dropping the whole table.
3533+
* (ATExecDropColumn independently enforces that, but it's not bulletproof
3534+
* so we need the dependencies too.)
3535+
*/
3536+
for (i = 0; i < partnatts; i++)
3537+
{
3538+
if (partattrs[i] == 0)
3539+
continue; /* ignore expressions here */
3540+
3541+
referenced.classId = RelationRelationId;
3542+
referenced.objectId = RelationGetRelid(rel);
3543+
referenced.objectSubId = partattrs[i];
3544+
3545+
recordDependencyOn(&referenced, &myself, DEPENDENCY_INTERNAL);
3546+
}
3547+
3548+
/*
3549+
* Also consider anything mentioned in partition expressions. External
3550+
* references (e.g. functions) get NORMAL dependencies. Table columns
3551+
* mentioned in the expressions are handled the same as plain partitioning
3552+
* columns, i.e. they become internally dependent on the whole table.
35343553
*/
35353554
if (partexprs)
35363555
recordDependencyOnSingleRelExpr(&myself,
35373556
(Node *) partexprs,
35383557
RelationGetRelid(rel),
35393558
DEPENDENCY_NORMAL,
3540-
DEPENDENCY_AUTO, true);
3559+
DEPENDENCY_INTERNAL,
3560+
true /* reverse the self-deps */ );
35413561

35423562
/*
35433563
* 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
@@ -7023,27 +7023,28 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
70237023
errmsg("cannot drop system column \"%s\"",
70247024
colName)));
70257025

7026-
/* Don't drop inherited columns */
7026+
/*
7027+
* Don't drop inherited columns, unless recursing (presumably from a drop
7028+
* of the parent column)
7029+
*/
70277030
if (targetatt->attinhcount > 0 && !recursing)
70287031
ereport(ERROR,
70297032
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
70307033
errmsg("cannot drop inherited column \"%s\"",
70317034
colName)));
70327035

7033-
/* Don't drop columns used in the partition key */
7036+
/*
7037+
* Don't drop columns used in the partition key, either. (If we let this
7038+
* go through, the key column's dependencies would cause a cascaded drop
7039+
* of the whole table, which is surely not what the user expected.)
7040+
*/
70347041
if (has_partition_attrs(rel,
70357042
bms_make_singleton(attnum - FirstLowInvalidHeapAttributeNumber),
70367043
&is_expr))
7037-
{
7038-
if (!is_expr)
7039-
ereport(ERROR,
7040-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
7041-
errmsg("cannot drop column named in partition key")));
7042-
else
7043-
ereport(ERROR,
7044-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
7045-
errmsg("cannot drop column referenced in partition key expression")));
7046-
}
7044+
ereport(ERROR,
7045+
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
7046+
errmsg("cannot drop column \"%s\" because it is part of the partition key of relation \"%s\"",
7047+
colName, RelationGetRelationName(rel))));
70477048

70487049
ReleaseSysCache(tuple);
70497050

@@ -10256,16 +10257,10 @@ ATPrepAlterColumnType(List **wqueue,
1025610257
if (has_partition_attrs(rel,
1025710258
bms_make_singleton(attnum - FirstLowInvalidHeapAttributeNumber),
1025810259
&is_expr))
10259-
{
10260-
if (!is_expr)
10261-
ereport(ERROR,
10262-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
10263-
errmsg("cannot alter type of column named in partition key")));
10264-
else
10265-
ereport(ERROR,
10266-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
10267-
errmsg("cannot alter type of column referenced in partition key expression")));
10268-
}
10260+
ereport(ERROR,
10261+
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
10262+
errmsg("cannot alter column \"%s\" because it is part of the partition key of relation \"%s\"",
10263+
colName, RelationGetRelationName(rel))));
1026910264

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

src/bin/pg_dump/pg_dump_sort.c

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1104,7 +1104,15 @@ repairDependencyLoop(DumpableObject **loop,
11041104
}
11051105
}
11061106

1107-
/* Loop of table with itself, happens with generated columns */
1107+
/*
1108+
* Loop of table with itself --- just ignore it.
1109+
*
1110+
* (Actually, what this arises from is a dependency of a table column on
1111+
* another column, which happens with generated columns; or a dependency
1112+
* of a table column on the whole table, which happens with partitioning.
1113+
* But we didn't pay attention to sub-object IDs while collecting the
1114+
* dependency data, so we can't see that here.)
1115+
*/
11081116
if (nLoop == 1)
11091117
{
11101118
if (loop[0]->objType == DO_TABLE)

src/include/catalog/catversion.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,6 @@
5353
*/
5454

5555
/* yyyymmddN */
56-
#define CATALOG_VERSION_NO 201907032
56+
#define CATALOG_VERSION_NO 201907221
5757

5858
#endif

src/include/catalog/dependency.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ extern void recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
156156
Node *expr, Oid relId,
157157
DependencyType behavior,
158158
DependencyType self_behavior,
159-
bool ignore_self);
159+
bool reverse_self);
160160

161161
extern ObjectClass getObjectClass(const ObjectAddress *object);
162162

src/test/regress/expected/alter_table.out

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3446,13 +3446,13 @@ LINE 1: ALTER TABLE partitioned ADD EXCLUDE USING gist (a WITH &&);
34463446
^
34473447
-- cannot drop column that is part of the partition key
34483448
ALTER TABLE partitioned DROP COLUMN a;
3449-
ERROR: cannot drop column named in partition key
3449+
ERROR: cannot drop column "a" because it is part of the partition key of relation "partitioned"
34503450
ALTER TABLE partitioned ALTER COLUMN a TYPE char(5);
3451-
ERROR: cannot alter type of column named in partition key
3451+
ERROR: cannot alter column "a" because it is part of the partition key of relation "partitioned"
34523452
ALTER TABLE partitioned DROP COLUMN b;
3453-
ERROR: cannot drop column referenced in partition key expression
3453+
ERROR: cannot drop column "b" because it is part of the partition key of relation "partitioned"
34543454
ALTER TABLE partitioned ALTER COLUMN b TYPE char(5);
3455-
ERROR: cannot alter type of column referenced in partition key expression
3455+
ERROR: cannot alter column "b" because it is part of the partition key of relation "partitioned"
34563456
-- partitioned table cannot participate in regular inheritance
34573457
CREATE TABLE nonpartitioned (
34583458
a int,
@@ -3945,9 +3945,9 @@ ERROR: cannot change inheritance of a partition
39453945
-- partitioned tables; for example, part_5, which is list_parted2's
39463946
-- partition, is partitioned on b;
39473947
ALTER TABLE list_parted2 DROP COLUMN b;
3948-
ERROR: cannot drop column named in partition key
3948+
ERROR: cannot drop column "b" because it is part of the partition key of relation "part_5"
39493949
ALTER TABLE list_parted2 ALTER COLUMN b TYPE text;
3950-
ERROR: cannot alter type of column named in partition key
3950+
ERROR: cannot alter column "b" because it is part of the partition key of relation "part_5"
39513951
-- dropping non-partition key columns should be allowed on the parent table.
39523952
ALTER TABLE list_parted DROP COLUMN b;
39533953
SELECT * FROM list_parted;

src/test/regress/expected/create_table.out

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -501,6 +501,42 @@ Partition of: partitioned2 FOR VALUES FROM ('-1', 'aaaaa') TO (100, 'ccccc')
501501
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))))
502502

503503
DROP TABLE partitioned, partitioned2;
504+
-- check that dependencies of partition columns are handled correctly
505+
create domain intdom1 as int;
506+
create table partitioned (
507+
a intdom1,
508+
b text
509+
) partition by range (a);
510+
alter table partitioned drop column a; -- fail
511+
ERROR: cannot drop column "a" because it is part of the partition key of relation "partitioned"
512+
drop domain intdom1; -- fail, requires cascade
513+
ERROR: cannot drop type intdom1 because other objects depend on it
514+
DETAIL: table partitioned depends on type intdom1
515+
HINT: Use DROP ... CASCADE to drop the dependent objects too.
516+
drop domain intdom1 cascade;
517+
NOTICE: drop cascades to table partitioned
518+
table partitioned; -- gone
519+
ERROR: relation "partitioned" does not exist
520+
LINE 1: table partitioned;
521+
^
522+
-- likewise for columns used in partition expressions
523+
create domain intdom1 as int;
524+
create table partitioned (
525+
a intdom1,
526+
b text
527+
) partition by range (plusone(a));
528+
alter table partitioned drop column a; -- fail
529+
ERROR: cannot drop column "a" because it is part of the partition key of relation "partitioned"
530+
drop domain intdom1; -- fail, requires cascade
531+
ERROR: cannot drop type intdom1 because other objects depend on it
532+
DETAIL: table partitioned depends on type intdom1
533+
HINT: Use DROP ... CASCADE to drop the dependent objects too.
534+
drop domain intdom1 cascade;
535+
NOTICE: drop cascades to table partitioned
536+
table partitioned; -- gone
537+
ERROR: relation "partitioned" does not exist
538+
LINE 1: table partitioned;
539+
^
504540
--
505541
-- Partitions
506542
--

src/test/regress/sql/create_table.sql

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

450450
DROP TABLE partitioned, partitioned2;
451451

452+
-- check that dependencies of partition columns are handled correctly
453+
create domain intdom1 as int;
454+
455+
create table partitioned (
456+
a intdom1,
457+
b text
458+
) partition by range (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+
-- likewise for columns used in partition expressions
469+
create domain intdom1 as int;
470+
471+
create table partitioned (
472+
a intdom1,
473+
b text
474+
) partition by range (plusone(a));
475+
476+
alter table partitioned drop column a; -- fail
477+
478+
drop domain intdom1; -- fail, requires cascade
479+
480+
drop domain intdom1 cascade;
481+
482+
table partitioned; -- gone
483+
484+
452485
--
453486
-- Partitions
454487
--

0 commit comments

Comments
 (0)