diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c index 6315fc4b2f..dd0a7d8dac 100644 --- a/src/backend/catalog/dependency.c +++ b/src/backend/catalog/dependency.c @@ -543,6 +543,7 @@ findDependentObjects(const ObjectAddress *object, ObjectIdGetDatum(object->objectId)); if (object->objectSubId != 0) { + /* Consider only dependencies of this sub-object */ ScanKeyInit(&key[2], Anum_pg_depend_objsubid, BTEqualStrategyNumber, F_INT4EQ, @@ -550,7 +551,10 @@ findDependentObjects(const ObjectAddress *object, nkeys = 3; } else + { + /* Consider dependencies of this object and any sub-objects it has */ nkeys = 2; + } scan = systable_beginscan(*depRel, DependDependerIndexId, true, NULL, nkeys, key); @@ -567,6 +571,18 @@ findDependentObjects(const ObjectAddress *object, otherObject.objectId = foundDep->refobjid; otherObject.objectSubId = foundDep->refobjsubid; + /* + * When scanning dependencies of a whole object, we may find rows + * linking sub-objects of the object to the object itself. (Normally, + * such a dependency is implicit, but we must make explicit ones in + * some cases involving partitioning.) We must ignore such rows to + * avoid infinite recursion. + */ + if (otherObject.classId == object->classId && + otherObject.objectId == object->objectId && + object->objectSubId == 0) + continue; + switch (foundDep->deptype) { case DEPENDENCY_NORMAL: @@ -854,6 +870,16 @@ findDependentObjects(const ObjectAddress *object, otherObject.objectId = foundDep->objid; otherObject.objectSubId = foundDep->objsubid; + /* + * If what we found is a sub-object of the current object, just ignore + * it. (Normally, such a dependency is implicit, but we must make + * explicit ones in some cases involving partitioning.) + */ + if (otherObject.classId == object->classId && + otherObject.objectId == object->objectId && + object->objectSubId == 0) + continue; + /* * Must lock the dependent object before recursing to it. */ @@ -1588,8 +1614,10 @@ recordDependencyOnExpr(const ObjectAddress *depender, * As above, but only one relation is expected to be referenced (with * varno = 1 and varlevelsup = 0). Pass the relation OID instead of a * range table. An additional frammish is that dependencies on that - * relation (or its component columns) will be marked with 'self_behavior', - * whereas 'behavior' is used for everything else. + * relation's component columns will be marked with 'self_behavior', + * whereas 'behavior' is used for everything else; also, if 'reverse_self' + * is true, those dependencies are reversed so that the columns are made + * to depend on the table not vice versa. * * NOTE: the caller should ensure that a whole-table dependency on the * specified relation is created separately, if one is needed. In particular, @@ -1602,7 +1630,7 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender, Node *expr, Oid relId, DependencyType behavior, DependencyType self_behavior, - bool ignore_self) + bool reverse_self) { find_expr_references_context context; RangeTblEntry rte; @@ -1626,7 +1654,8 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender, eliminate_duplicate_dependencies(context.addrs); /* Separate self-dependencies if necessary */ - if (behavior != self_behavior && context.addrs->numrefs > 0) + if ((behavior != self_behavior || reverse_self) && + context.addrs->numrefs > 0) { ObjectAddresses *self_addrs; ObjectAddress *outobj; @@ -1657,11 +1686,23 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender, } context.addrs->numrefs = outrefs; - /* Record the self-dependencies */ - if (!ignore_self) + /* Record the self-dependencies with the appropriate direction */ + if (!reverse_self) recordMultipleDependencies(depender, self_addrs->refs, self_addrs->numrefs, self_behavior); + else + { + /* Can't use recordMultipleDependencies, so do it the hard way */ + int selfref; + + for (selfref = 0; selfref < self_addrs->numrefs; selfref++) + { + ObjectAddress *thisobj = self_addrs->refs + selfref; + + recordDependencyOn(thisobj, depender, self_behavior); + } + } free_object_addresses(self_addrs); } diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 032fab9ac4..b7bcdd9d0f 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -3491,16 +3491,36 @@ StorePartitionKey(Relation rel, } /* - * Anything mentioned in the expressions. We must ignore the column - * references, which will depend on the table itself; there is no separate - * partition key object. + * The partitioning columns are made internally dependent on the table, + * because we cannot drop any of them without dropping the whole table. + * (ATExecDropColumn independently enforces that, but it's not bulletproof + * so we need the dependencies too.) + */ + for (i = 0; i < partnatts; i++) + { + if (partattrs[i] == 0) + continue; /* ignore expressions here */ + + referenced.classId = RelationRelationId; + referenced.objectId = RelationGetRelid(rel); + referenced.objectSubId = partattrs[i]; + + recordDependencyOn(&referenced, &myself, DEPENDENCY_INTERNAL); + } + + /* + * Also consider anything mentioned in partition expressions. External + * references (e.g. functions) get NORMAL dependencies. Table columns + * mentioned in the expressions are handled the same as plain partitioning + * columns, i.e. they become internally dependent on the whole table. */ if (partexprs) recordDependencyOnSingleRelExpr(&myself, (Node *) partexprs, RelationGetRelid(rel), DEPENDENCY_NORMAL, - DEPENDENCY_AUTO, true); + DEPENDENCY_INTERNAL, + true /* reverse the self-deps */ ); /* * We must invalidate the relcache so that the next diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index f39f993e01..9265f0ef55 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -7021,27 +7021,28 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName, errmsg("cannot drop system column \"%s\"", colName))); - /* Don't drop inherited columns */ + /* + * Don't drop inherited columns, unless recursing (presumably from a drop + * of the parent column) + */ if (targetatt->attinhcount > 0 && !recursing) ereport(ERROR, (errcode(ERRCODE_INVALID_TABLE_DEFINITION), errmsg("cannot drop inherited column \"%s\"", colName))); - /* Don't drop columns used in the partition key */ + /* + * Don't drop columns used in the partition key, either. (If we let this + * go through, the key column's dependencies would cause a cascaded drop + * of the whole table, which is surely not what the user expected.) + */ if (has_partition_attrs(rel, bms_make_singleton(attnum - FirstLowInvalidHeapAttributeNumber), &is_expr)) - { - if (!is_expr) - ereport(ERROR, - (errcode(ERRCODE_INVALID_TABLE_DEFINITION), - errmsg("cannot drop column named in partition key"))); - else - ereport(ERROR, - (errcode(ERRCODE_INVALID_TABLE_DEFINITION), - errmsg("cannot drop column referenced in partition key expression"))); - } + ereport(ERROR, + (errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("cannot drop column \"%s\" because it is part of the partition key of relation \"%s\"", + colName, RelationGetRelationName(rel)))); ReleaseSysCache(tuple); @@ -10255,16 +10256,10 @@ ATPrepAlterColumnType(List **wqueue, if (has_partition_attrs(rel, bms_make_singleton(attnum - FirstLowInvalidHeapAttributeNumber), &is_expr)) - { - if (!is_expr) - ereport(ERROR, - (errcode(ERRCODE_INVALID_TABLE_DEFINITION), - errmsg("cannot alter type of column named in partition key"))); - else - ereport(ERROR, - (errcode(ERRCODE_INVALID_TABLE_DEFINITION), - errmsg("cannot alter type of column referenced in partition key expression"))); - } + ereport(ERROR, + (errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("cannot alter column \"%s\" because it is part of the partition key of relation \"%s\"", + colName, RelationGetRelationName(rel)))); /* Look up the target type */ typenameTypeIdAndMod(NULL, typeName, &targettype, &targettypmod); diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c index 54430a5004..31fc06a255 100644 --- a/src/bin/pg_dump/pg_dump_sort.c +++ b/src/bin/pg_dump/pg_dump_sort.c @@ -1104,7 +1104,15 @@ repairDependencyLoop(DumpableObject **loop, } } - /* Loop of table with itself, happens with generated columns */ + /* + * Loop of table with itself --- just ignore it. + * + * (Actually, what this arises from is a dependency of a table column on + * another column, which happens with generated columns; or a dependency + * of a table column on the whole table, which happens with partitioning. + * But we didn't pay attention to sub-object IDs while collecting the + * dependency data, so we can't see that here.) + */ if (nLoop == 1) { if (loop[0]->objType == DO_TABLE) diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h index 19ccb54796..34466b432c 100644 --- a/src/include/catalog/catversion.h +++ b/src/include/catalog/catversion.h @@ -53,6 +53,6 @@ */ /* yyyymmddN */ -#define CATALOG_VERSION_NO 201907142 +#define CATALOG_VERSION_NO 201907222 #endif diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h index 12296b6130..ff50d594f6 100644 --- a/src/include/catalog/dependency.h +++ b/src/include/catalog/dependency.h @@ -156,7 +156,7 @@ extern void recordDependencyOnSingleRelExpr(const ObjectAddress *depender, Node *expr, Oid relId, DependencyType behavior, DependencyType self_behavior, - bool ignore_self); + bool reverse_self); extern ObjectClass getObjectClass(const ObjectAddress *object); diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 3578b8009b..e5407bbf0f 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -3446,13 +3446,13 @@ LINE 1: ALTER TABLE partitioned ADD EXCLUDE USING gist (a WITH &&); ^ -- cannot drop column that is part of the partition key ALTER TABLE partitioned DROP COLUMN a; -ERROR: cannot drop column named in partition key +ERROR: cannot drop column "a" because it is part of the partition key of relation "partitioned" ALTER TABLE partitioned ALTER COLUMN a TYPE char(5); -ERROR: cannot alter type of column named in partition key +ERROR: cannot alter column "a" because it is part of the partition key of relation "partitioned" ALTER TABLE partitioned DROP COLUMN b; -ERROR: cannot drop column referenced in partition key expression +ERROR: cannot drop column "b" because it is part of the partition key of relation "partitioned" ALTER TABLE partitioned ALTER COLUMN b TYPE char(5); -ERROR: cannot alter type of column referenced in partition key expression +ERROR: cannot alter column "b" because it is part of the partition key of relation "partitioned" -- partitioned table cannot participate in regular inheritance CREATE TABLE nonpartitioned ( a int, @@ -3945,9 +3945,9 @@ ERROR: cannot change inheritance of a partition -- partitioned tables; for example, part_5, which is list_parted2's -- partition, is partitioned on b; ALTER TABLE list_parted2 DROP COLUMN b; -ERROR: cannot drop column named in partition key +ERROR: cannot drop column "b" because it is part of the partition key of relation "part_5" ALTER TABLE list_parted2 ALTER COLUMN b TYPE text; -ERROR: cannot alter type of column named in partition key +ERROR: cannot alter column "b" because it is part of the partition key of relation "part_5" -- dropping non-partition key columns should be allowed on the parent table. ALTER TABLE list_parted DROP COLUMN b; SELECT * FROM list_parted; diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out index 262abf2bfb..3c302713dc 100644 --- a/src/test/regress/expected/create_table.out +++ b/src/test/regress/expected/create_table.out @@ -501,6 +501,42 @@ Partition of: partitioned2 FOR VALUES FROM ('-1', 'aaaaa') TO (100, 'ccccc') 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)))) DROP TABLE partitioned, partitioned2; +-- check that dependencies of partition columns are handled correctly +create domain intdom1 as int; +create table partitioned ( + a intdom1, + b text +) partition by range (a); +alter table partitioned drop column a; -- fail +ERROR: cannot drop column "a" because it is part of the partition key of relation "partitioned" +drop domain intdom1; -- fail, requires cascade +ERROR: cannot drop type intdom1 because other objects depend on it +DETAIL: table partitioned depends on type intdom1 +HINT: Use DROP ... CASCADE to drop the dependent objects too. +drop domain intdom1 cascade; +NOTICE: drop cascades to table partitioned +table partitioned; -- gone +ERROR: relation "partitioned" does not exist +LINE 1: table partitioned; + ^ +-- likewise for columns used in partition expressions +create domain intdom1 as int; +create table partitioned ( + a intdom1, + b text +) partition by range (plusone(a)); +alter table partitioned drop column a; -- fail +ERROR: cannot drop column "a" because it is part of the partition key of relation "partitioned" +drop domain intdom1; -- fail, requires cascade +ERROR: cannot drop type intdom1 because other objects depend on it +DETAIL: table partitioned depends on type intdom1 +HINT: Use DROP ... CASCADE to drop the dependent objects too. +drop domain intdom1 cascade; +NOTICE: drop cascades to table partitioned +table partitioned; -- gone +ERROR: relation "partitioned" does not exist +LINE 1: table partitioned; + ^ -- -- Partitions -- diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql index 9c6d86a0bf..144eeb480d 100644 --- a/src/test/regress/sql/create_table.sql +++ b/src/test/regress/sql/create_table.sql @@ -449,6 +449,39 @@ CREATE TABLE part2_1 PARTITION OF partitioned2 FOR VALUES FROM (-1, 'aaaaa') TO DROP TABLE partitioned, partitioned2; +-- check that dependencies of partition columns are handled correctly +create domain intdom1 as int; + +create table partitioned ( + a intdom1, + b text +) partition by range (a); + +alter table partitioned drop column a; -- fail + +drop domain intdom1; -- fail, requires cascade + +drop domain intdom1 cascade; + +table partitioned; -- gone + +-- likewise for columns used in partition expressions +create domain intdom1 as int; + +create table partitioned ( + a intdom1, + b text +) partition by range (plusone(a)); + +alter table partitioned drop column a; -- fail + +drop domain intdom1; -- fail, requires cascade + +drop domain intdom1 cascade; + +table partitioned; -- gone + + -- -- Partitions --