Re: Broken defenses against dropping a partitioning column - Mailing list pgsql-hackers

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
 --