Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hoXOY-000872-35 for pgsql-hackers@arkaria.postgresql.org; Fri, 19 Jul 2019 18:14:14 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1hoXOW-0002I3-QH for pgsql-hackers@arkaria.postgresql.org; Fri, 19 Jul 2019 18:14:12 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hoXOW-0002Fp-ER for pgsql-hackers@lists.postgresql.org; Fri, 19 Jul 2019 18:14:12 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hoXOT-0005MY-1b for pgsql-hackers@lists.postgresql.org; Fri, 19 Jul 2019 18:14:11 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.14.4/8.14.4) with ESMTP id x6JIE3Sh026100; Fri, 19 Jul 2019 14:14:03 -0400 From: Tom Lane To: Alvaro Herrera cc: pgsql-hackers@lists.postgresql.org, Manuel Rigger Subject: Re: Broken defenses against dropping a partitioning column In-reply-to: <30041.1562597936@sss.pgh.pa.us> References: <20190708143153.GA5929@alvherre.pgsql> <30041.1562597936@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Mon, 08 Jul 2019 10:58:56 -0400" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <26075.1563560002.0@sss.pgh.pa.us> Date: Fri, 19 Jul 2019 14:14:03 -0400 Message-ID: <26099.1563560043@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <26075.1563560002.1@sss.pgh.pa.us> 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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="add-dependencies-for-partitioning-columns-1.patch"; charset="us-ascii" Content-ID: <26075.1563560002.2@sss.pgh.pa.us> Content-Description: add-dependencies-for-partitioning-columns-1.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/depend= ency.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 !=3D 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 =3D 3; } else + { + /* Consider dependencies of this object and any sub-objects it has */ nkeys =3D 2; + } = scan =3D systable_beginscan(*depRel, DependDependerIndexId, true, NULL, nkeys, key); @@ -567,6 +571,18 @@ findDependentObjects(const ObjectAddress *object, otherObject.objectId =3D foundDep->refobjid; otherObject.objectSubId =3D 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 =3D=3D object->classId && + otherObject.objectId =3D=3D object->objectId && + object->objectSubId =3D=3D 0) + continue; + switch (foundDep->deptype) { case DEPENDENCY_NORMAL: @@ -855,6 +871,16 @@ findDependentObjects(const ObjectAddress *object, otherObject.objectSubId =3D 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 =3D=3D object->classId && + otherObject.objectId =3D=3D object->objectId && + object->objectSubId =3D=3D 0) + continue; + + /* * Must lock the dependent object before recursing to it. */ AcquireDeletionLock(&otherObject, 0); @@ -1588,8 +1614,10 @@ recordDependencyOnExpr(const ObjectAddress *depende= r, * As above, but only one relation is expected to be referenced (with * varno =3D 1 and varlevelsup =3D 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 partic= ular, @@ -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 !=3D self_behavior && context.addrs->numrefs > 0) + if ((behavior !=3D self_behavior || reverse_self) && + context.addrs->numrefs > 0) { ObjectAddresses *self_addrs; ObjectAddress *outobj; @@ -1657,11 +1686,23 @@ recordDependencyOnSingleRelExpr(const ObjectAddres= s *depender, } context.addrs->numrefs =3D 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 =3D 0; selfref < self_addrs->numrefs; selfref++) + { + ObjectAddress *thisobj =3D 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 separa= te - * 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 bulletpro= of + * so we need the dependencies too.) + */ + for (i =3D 0; i < partnatts; i++) + { + if (partattrs[i] =3D=3D 0) + continue; /* ignore expressions here */ + + referenced.classId =3D RelationRelationId; + referenced.objectId =3D RelationGetRelid(rel); + referenced.objectSubId =3D 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 partitioni= ng + * 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/table= cmds.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, cons= t char *colName, errmsg("cannot drop system column \"%s\"", colName))); = - /* Don't drop inherited columns */ + /* + * Don't drop inherited columns, unless recursing (presumably from a dro= p + * 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 thi= s + * 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 partitio= n 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 exp= ression"))); + ereport(ERROR, + (errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("cannot alter column \"%s\" because it is part of the partiti= on key of relation \"%s\"", + colName, RelationGetRelationName(rel)))); } = /* Look up the target type */ diff --git a/src/include/catalog/dependency.h b/src/include/catalog/depend= ency.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 Obje= ctAddress *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 o= f 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 expressio= n +ERROR: cannot alter column "b" because it is part of the partition key o= f 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 o= f relation "part_5" -- dropping non-partition key columns should be allowed on the parent tab= le. 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', 'aa= aaa') 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) =3D '-1'::integer) AND (= substr(b, 1, 5) >=3D 'aaaaa'::text))) AND (((a + 1) < 100) OR (((a + 1) =3D= 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 VA= LUES 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 -- ------- =_aaaaaaaaaa0--