Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit6cdefc8

Browse files
committed
Install dependencies to prevent dropping partition key columns.
The logic in ATExecDropColumn that rejects dropping partition keycolumns is quite an inadequate defense, because it doesn't executein cases where a column needs to be dropped due to cascade fromsomething 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 thecolumn can't be dropped without dropping the whole table. Hence,add those entries. In v12 and HEAD, bump catversion to ensure thatpartitioned tables will have such entries. We can't do that inreleased branches of course, so in v10 and v11 this patch affordsprotection only to partitioned tables created after the patch isinstalled. Given the lack of field complaints (this bug was foundby fuzz-testing not by end users), that's probably good enough.In passing, fix ATExecDropColumn and ATPrepAlterColumnTypemessages to be more specific about which partition key columnthey're complaining about.Per report from Manuel Rigger. Back-patch to v10 where partitionedtables were added.Discussion:https://postgr.es/m/CA+u7OA4JKCPFrdrAbOs7XBiCyD61XJxeNav4LefkSmBLQ-Vobg@mail.gmail.comDiscussion:https://postgr.es/m/31920.1562526703@sss.pgh.pa.us
1 parent577c880 commit6cdefc8

File tree

8 files changed

+182
-39
lines changed

8 files changed

+182
-39
lines changed

‎src/backend/catalog/dependency.c

Lines changed: 47 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -523,14 +523,18 @@ findDependentObjects(const ObjectAddress *object,
523523
ObjectIdGetDatum(object->objectId));
524524
if (object->objectSubId!=0)
525525
{
526+
/* Consider only dependencies of this sub-object */
526527
ScanKeyInit(&key[2],
527528
Anum_pg_depend_objsubid,
528529
BTEqualStrategyNumber,F_INT4EQ,
529530
Int32GetDatum(object->objectSubId));
530531
nkeys=3;
531532
}
532533
else
534+
{
535+
/* Consider dependencies of this object and any sub-objects it has */
533536
nkeys=2;
537+
}
534538

535539
scan=systable_beginscan(*depRel,DependDependerIndexId, true,
536540
NULL,nkeys,key);
@@ -543,6 +547,18 @@ findDependentObjects(const ObjectAddress *object,
543547
otherObject.objectId=foundDep->refobjid;
544548
otherObject.objectSubId=foundDep->refobjsubid;
545549

550+
/*
551+
* When scanning dependencies of a whole object, we may find rows
552+
* linking sub-objects of the object to the object itself. (Normally,
553+
* such a dependency is implicit, but we must make explicit ones in
554+
* some cases involving partitioning.) We must ignore such rows to
555+
* avoid infinite recursion.
556+
*/
557+
if (otherObject.classId==object->classId&&
558+
otherObject.objectId==object->objectId&&
559+
object->objectSubId==0)
560+
continue;
561+
546562
switch (foundDep->deptype)
547563
{
548564
caseDEPENDENCY_NORMAL:
@@ -739,6 +755,16 @@ findDependentObjects(const ObjectAddress *object,
739755
otherObject.objectId=foundDep->objid;
740756
otherObject.objectSubId=foundDep->objsubid;
741757

758+
/*
759+
* If what we found is a sub-object of the current object, just ignore
760+
* it. (Normally, such a dependency is implicit, but we must make
761+
* explicit ones in some cases involving partitioning.)
762+
*/
763+
if (otherObject.classId==object->classId&&
764+
otherObject.objectId==object->objectId&&
765+
object->objectSubId==0)
766+
continue;
767+
742768
/*
743769
* Must lock the dependent object before recursing to it.
744770
*/
@@ -1388,8 +1414,10 @@ recordDependencyOnExpr(const ObjectAddress *depender,
13881414
* As above, but only one relation is expected to be referenced (with
13891415
* varno = 1 and varlevelsup = 0). Pass the relation OID instead of a
13901416
* range table. An additional frammish is that dependencies on that
1391-
* relation (or its component columns) will be marked with 'self_behavior',
1392-
* whereas 'behavior' is used for everything else.
1417+
* relation's component columns will be marked with 'self_behavior',
1418+
* whereas 'behavior' is used for everything else; also, if 'reverse_self'
1419+
* is true, those dependencies are reversed so that the columns are made
1420+
* to depend on the table not vice versa.
13931421
*
13941422
* NOTE: the caller should ensure that a whole-table dependency on the
13951423
* specified relation is created separately, if one is needed. In particular,
@@ -1402,7 +1430,7 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
14021430
Node*expr,OidrelId,
14031431
DependencyTypebehavior,
14041432
DependencyTypeself_behavior,
1405-
boolignore_self)
1433+
boolreverse_self)
14061434
{
14071435
find_expr_references_contextcontext;
14081436
RangeTblEntryrte;
@@ -1425,7 +1453,8 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
14251453
eliminate_duplicate_dependencies(context.addrs);
14261454

14271455
/* Separate self-dependencies if necessary */
1428-
if (behavior!=self_behavior&&context.addrs->numrefs>0)
1456+
if ((behavior!=self_behavior||reverse_self)&&
1457+
context.addrs->numrefs>0)
14291458
{
14301459
ObjectAddresses*self_addrs;
14311460
ObjectAddress*outobj;
@@ -1456,11 +1485,23 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
14561485
}
14571486
context.addrs->numrefs=outrefs;
14581487

1459-
/* Record the self-dependencies */
1460-
if (!ignore_self)
1488+
/* Record the self-dependencieswith the appropriate direction*/
1489+
if (!reverse_self)
14611490
recordMultipleDependencies(depender,
14621491
self_addrs->refs,self_addrs->numrefs,
14631492
self_behavior);
1493+
else
1494+
{
1495+
/* Can't use recordMultipleDependencies, so do it the hard way */
1496+
intselfref;
1497+
1498+
for (selfref=0;selfref<self_addrs->numrefs;selfref++)
1499+
{
1500+
ObjectAddress*thisobj=self_addrs->refs+selfref;
1501+
1502+
recordDependencyOn(thisobj,depender,self_behavior);
1503+
}
1504+
}
14641505

14651506
free_object_addresses(self_addrs);
14661507
}

‎src/backend/catalog/heap.c

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3444,16 +3444,36 @@ StorePartitionKey(Relation rel,
34443444
}
34453445

34463446
/*
3447-
* Anything mentioned in the expressions. We must ignore the column
3448-
* references, which will depend on the table itself; there is no separate
3449-
* partition key object.
3447+
* The partitioning columns are made internally dependent on the table,
3448+
* because we cannot drop any of them without dropping the whole table.
3449+
* (ATExecDropColumn independently enforces that, but it's not bulletproof
3450+
* so we need the dependencies too.)
3451+
*/
3452+
for (i=0;i<partnatts;i++)
3453+
{
3454+
if (partattrs[i]==0)
3455+
continue;/* ignore expressions here */
3456+
3457+
referenced.classId=RelationRelationId;
3458+
referenced.objectId=RelationGetRelid(rel);
3459+
referenced.objectSubId=partattrs[i];
3460+
3461+
recordDependencyOn(&referenced,&myself,DEPENDENCY_INTERNAL);
3462+
}
3463+
3464+
/*
3465+
* Also consider anything mentioned in partition expressions. External
3466+
* references (e.g. functions) get NORMAL dependencies. Table columns
3467+
* mentioned in the expressions are handled the same as plain partitioning
3468+
* columns, i.e. they become internally dependent on the whole table.
34503469
*/
34513470
if (partexprs)
34523471
recordDependencyOnSingleRelExpr(&myself,
34533472
(Node*)partexprs,
34543473
RelationGetRelid(rel),
34553474
DEPENDENCY_NORMAL,
3456-
DEPENDENCY_AUTO, true);
3475+
DEPENDENCY_INTERNAL,
3476+
true/* reverse the self-deps */ );
34573477

34583478
/*
34593479
* We must invalidate the relcache so that the next

‎src/backend/commands/tablecmds.c

Lines changed: 17 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -6825,27 +6825,28 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
68256825
errmsg("cannot drop system column \"%s\"",
68266826
colName)));
68276827

6828-
/* Don't drop inherited columns */
6828+
/*
6829+
* Don't drop inherited columns, unless recursing (presumably from a drop
6830+
* of the parent column)
6831+
*/
68296832
if (targetatt->attinhcount>0&& !recursing)
68306833
ereport(ERROR,
68316834
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
68326835
errmsg("cannot drop inherited column \"%s\"",
68336836
colName)));
68346837

6835-
/* Don't drop columns used in the partition key */
6838+
/*
6839+
* Don't drop columns used in the partition key, either. (If we let this
6840+
* go through, the key column's dependencies would cause a cascaded drop
6841+
* of the whole table, which is surely not what the user expected.)
6842+
*/
68366843
if (has_partition_attrs(rel,
68376844
bms_make_singleton(attnum-FirstLowInvalidHeapAttributeNumber),
68386845
&is_expr))
6839-
{
6840-
if (!is_expr)
6841-
ereport(ERROR,
6842-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
6843-
errmsg("cannot drop column named in partition key")));
6844-
else
6845-
ereport(ERROR,
6846-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
6847-
errmsg("cannot drop column referenced in partition key expression")));
6848-
}
6846+
ereport(ERROR,
6847+
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
6848+
errmsg("cannot drop column \"%s\" because it is part of the partition key of relation \"%s\"",
6849+
colName,RelationGetRelationName(rel))));
68496850

68506851
ReleaseSysCache(tuple);
68516852

@@ -9554,16 +9555,10 @@ ATPrepAlterColumnType(List **wqueue,
95549555
if (has_partition_attrs(rel,
95559556
bms_make_singleton(attnum-FirstLowInvalidHeapAttributeNumber),
95569557
&is_expr))
9557-
{
9558-
if (!is_expr)
9559-
ereport(ERROR,
9560-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
9561-
errmsg("cannot alter type of column named in partition key")));
9562-
else
9563-
ereport(ERROR,
9564-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
9565-
errmsg("cannot alter type of column referenced in partition key expression")));
9566-
}
9558+
ereport(ERROR,
9559+
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
9560+
errmsg("cannot alter column \"%s\" because it is part of the partition key of relation \"%s\"",
9561+
colName,RelationGetRelationName(rel))));
95679562

95689563
/* Look up the target type */
95699564
typenameTypeIdAndMod(NULL,typeName,&targettype,&targettypmod);

‎src/bin/pg_dump/pg_dump_sort.c

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1202,6 +1202,24 @@ repairDependencyLoop(DumpableObject **loop,
12021202
}
12031203
}
12041204

1205+
/*
1206+
* Loop of table with itself --- just ignore it.
1207+
*
1208+
* (Actually, what this arises from is a dependency of a table column on
1209+
* another column, which happens with generated columns; or a dependency
1210+
* of a table column on the whole table, which happens with partitioning.
1211+
* But we didn't pay attention to sub-object IDs while collecting the
1212+
* dependency data, so we can't see that here.)
1213+
*/
1214+
if (nLoop==1)
1215+
{
1216+
if (loop[0]->objType==DO_TABLE)
1217+
{
1218+
removeObjectDependency(loop[0],loop[0]->dumpId);
1219+
return;
1220+
}
1221+
}
1222+
12051223
/*
12061224
* If all the objects are TABLE_DATA items, what we must have is a
12071225
* circular set of foreign key constraints (or a single self-referential

‎src/include/catalog/dependency.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,7 @@ extern void recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
209209
Node*expr,OidrelId,
210210
DependencyTypebehavior,
211211
DependencyTypeself_behavior,
212-
boolignore_self);
212+
boolreverse_self);
213213

214214
externObjectClassgetObjectClass(constObjectAddress*object);
215215

‎src/test/regress/expected/alter_table.out

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3538,13 +3538,13 @@ LINE 1: ALTER TABLE partitioned ADD EXCLUDE USING gist (a WITH &&);
35383538
^
35393539
-- cannot drop column that is part of the partition key
35403540
ALTER TABLE partitioned DROP COLUMN a;
3541-
ERROR: cannot drop columnnamed inpartition key
3541+
ERROR: cannot drop column"a" because it is part of thepartition key of relation "partitioned"
35423542
ALTER TABLE partitioned ALTER COLUMN a TYPE char(5);
3543-
ERROR: cannot altertype of column named inpartition key
3543+
ERROR: cannot altercolumn "a" because it is part of thepartition key of relation "partitioned"
35443544
ALTER TABLE partitioned DROP COLUMN b;
3545-
ERROR: cannot drop columnreferenced inpartition keyexpression
3545+
ERROR: cannot drop column"b" because it is part of thepartition keyof relation "partitioned"
35463546
ALTER TABLE partitioned ALTER COLUMN b TYPE char(5);
3547-
ERROR: cannot altertype of column referenced inpartition keyexpression
3547+
ERROR: cannot altercolumn "b" because it is part of thepartition keyof relation "partitioned"
35483548
-- partitioned table cannot participate in regular inheritance
35493549
CREATE TABLE nonpartitioned (
35503550
a int,
@@ -4046,9 +4046,9 @@ ERROR: cannot change inheritance of a partition
40464046
-- partitioned tables; for example, part_5, which is list_parted2's
40474047
-- partition, is partitioned on b;
40484048
ALTER TABLE list_parted2 DROP COLUMN b;
4049-
ERROR: cannot drop columnnamed inpartition key
4049+
ERROR: cannot drop column"b" because it is part of thepartition key of relation "part_5"
40504050
ALTER TABLE list_parted2 ALTER COLUMN b TYPE text;
4051-
ERROR: cannot altertype of column named inpartition key
4051+
ERROR: cannot altercolumn "b" because it is part of thepartition key of relation "part_5"
40524052
-- dropping non-partition key columns should be allowed on the parent table.
40534053
ALTER TABLE list_parted DROP COLUMN b;
40544054
SELECT * FROM list_parted;

‎src/test/regress/expected/create_table.out

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -456,6 +456,42 @@ Partition of: partitioned2 FOR VALUES FROM ('-1', 'aaaaa') TO (100, 'ccccc')
456456
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))))
457457

458458
DROP TABLE partitioned, partitioned2;
459+
-- check that dependencies of partition columns are handled correctly
460+
create domain intdom1 as int;
461+
create table partitioned (
462+
a intdom1,
463+
b text
464+
) partition by range (a);
465+
alter table partitioned drop column a; -- fail
466+
ERROR: cannot drop column "a" because it is part of the partition key of relation "partitioned"
467+
drop domain intdom1; -- fail, requires cascade
468+
ERROR: cannot drop type intdom1 because other objects depend on it
469+
DETAIL: table partitioned depends on type intdom1
470+
HINT: Use DROP ... CASCADE to drop the dependent objects too.
471+
drop domain intdom1 cascade;
472+
NOTICE: drop cascades to table partitioned
473+
table partitioned; -- gone
474+
ERROR: relation "partitioned" does not exist
475+
LINE 1: table partitioned;
476+
^
477+
-- likewise for columns used in partition expressions
478+
create domain intdom1 as int;
479+
create table partitioned (
480+
a intdom1,
481+
b text
482+
) partition by range (plusone(a));
483+
alter table partitioned drop column a; -- fail
484+
ERROR: cannot drop column "a" because it is part of the partition key of relation "partitioned"
485+
drop domain intdom1; -- fail, requires cascade
486+
ERROR: cannot drop type intdom1 because other objects depend on it
487+
DETAIL: table partitioned depends on type intdom1
488+
HINT: Use DROP ... CASCADE to drop the dependent objects too.
489+
drop domain intdom1 cascade;
490+
NOTICE: drop cascades to table partitioned
491+
table partitioned; -- gone
492+
ERROR: relation "partitioned" does not exist
493+
LINE 1: table partitioned;
494+
^
459495
--
460496
-- Partitions
461497
--

‎src/test/regress/sql/create_table.sql

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -431,6 +431,39 @@ CREATE TABLE part2_1 PARTITION OF partitioned2 FOR VALUES FROM (-1, 'aaaaa') TO
431431

432432
DROPTABLE partitioned, partitioned2;
433433

434+
-- check that dependencies of partition columns are handled correctly
435+
createdomainintdom1asint;
436+
437+
createtablepartitioned (
438+
a intdom1,
439+
btext
440+
) partition by range (a);
441+
442+
altertable partitioned drop column a;-- fail
443+
444+
dropdomain intdom1;-- fail, requires cascade
445+
446+
dropdomain intdom1 cascade;
447+
448+
table partitioned;-- gone
449+
450+
-- likewise for columns used in partition expressions
451+
createdomainintdom1asint;
452+
453+
createtablepartitioned (
454+
a intdom1,
455+
btext
456+
) partition by range (plusone(a));
457+
458+
altertable partitioned drop column a;-- fail
459+
460+
dropdomain intdom1;-- fail, requires cascade
461+
462+
dropdomain intdom1 cascade;
463+
464+
table partitioned;-- gone
465+
466+
434467
--
435468
-- Partitions
436469
--

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp