I wrote:
> So I think we're probably stuck with the approach of adding new internal
> dependencies. If we go that route, then our options for the released
> branches are (1) do nothing, or (2) back-patch the code that adds such
> dependencies, but without a catversion bump. That would mean that only
> tables created after the next minor releases would have protection against
> this problem. That's not ideal but maybe it's okay, considering that we
> haven't seen actual field reports of trouble of this kind.
Here's a proposed patch for that. It's mostly pretty straightforward,
except I had to add some recursion defenses in findDependentObjects that
weren't there before. But those seem like a good idea anyway to prevent
infinite recursion in case of bogus entries in pg_depend.
I also took the liberty of improving some related error messages that
I thought were unnecessarily vague and not up to project standards.
Per above, I'm envisioning applying this to HEAD and v12 with a catversion
bump, and to v11 and v10 with no catversion bump.
regards, tom lane
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 6315fc4..3356461 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:
@@ -855,6 +871,16 @@ findDependentObjects(const ObjectAddress *object,
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.
*/
AcquireDeletionLock(&otherObject, 0);
@@ -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 032fab9..b7bcdd9 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 fc1c4df..8bdd0f0 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);
@@ -10256,14 +10257,10 @@ ATPrepAlterColumnType(List **wqueue,
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 */
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index ef9c868..bee9b3f 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 3578b80..e5407bb 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 262abf2..3c30271 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 9c6d86a..144eeb4 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
--
