From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: pgsql-hackers@lists.postgresql.org, Manuel Rigger <rigger.manuel@gmail.com>
Subject: Re: Broken defenses against dropping a partitioning column
Date: Fri, 19 Jul 2019 14:14:03 -0400
Message-ID: <26099.1563560043@sss.pgh.pa.us> (raw)
In-Reply-To: <30041.1562597936@sss.pgh.pa.us>
References: <20190708143153.GA5929@alvherre.pgsql>
<30041.1562597936@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
Attachments:add-dependencies-for-partitioning-columns-1.patch (text/x-diff, 13.9K) downloaddiff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.cindex 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.cindex 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.cindex 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.hindex 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.outindex 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.outindex 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.sqlindex 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
--
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, alvherre@2ndquadrant.com, rigger.manuel@gmail.com
Subject: Re: Broken defenses against dropping a partitioning column
In-Reply-To: <26099.1563560043@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox