rdelaage
/
postgresql
mirror of https://git.postgresql.org/git/postgresql.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Install dependencies to prevent dropping partition key columns. 

The logic in ATExecDropColumn that rejects dropping partition key
columns is quite an inadequate defense, because it doesn't execute
in cases where a column needs to be dropped due to cascade from
something that only the column, not the whole partitioned table,
depends on.  That leaves us with a badly broken partitioned table;
even an attempt to load its relcache entry will fail.

We really need to have explicit pg_depend entries that show that the
column can't be dropped without dropping the whole table.  Hence,
add those entries.  In v12 and HEAD, bump catversion to ensure that
partitioned tables will have such entries.  We can't do that in
released branches of course, so in v10 and v11 this patch affords
protection only to partitioned tables created after the patch is
installed.  Given the lack of field complaints (this bug was found
by fuzz-testing not by end users), that's probably good enough.

In passing, fix ATExecDropColumn and ATPrepAlterColumnType
messages to be more specific about which partition key column
they're complaining about.

Per report from Manuel Rigger.  Back-patch to v10 where partitioned
tables were added.

Discussion: https://postgr.es/m/CA+u7OA4JKCPFrdrAbOs7XBiCyD61XJxeNav4LefkSmBLQ-Vobg@mail.gmail.com
Discussion: https://postgr.es/m/31920.1562526703@sss.pgh.pa.us
This commit is contained in:
Tom Lane 2019-07-22 14:55:22 -04:00
parent 7961886580
commit a0555ddab9
9 changed files with 174 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

53
src/backend/catalog/dependency.c
View File

@ -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);
}

28
src/backend/catalog/heap.c
View File

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

39
src/backend/commands/tablecmds.c
View File

@ -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);

10
src/bin/pg_dump/pg_dump_sort.c
View File

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

2
src/include/catalog/catversion.h
View File

@ -53,6 +53,6 @@
*/
/* yyyymmddN */
#define CATALOG_VERSION_NO 201907142
#define CATALOG_VERSION_NO 201907222
#endif

2
src/include/catalog/dependency.h
View File

@ -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);

12
src/test/regress/expected/alter_table.out
View File

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

36
src/test/regress/expected/create_table.out
View File

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

33
src/test/regress/sql/create_table.sql
View File

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