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

Commita0555dd

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

File tree

9 files changed

+174
-41
lines changed

9 files changed

+174
-41
lines changed

‎src/backend/catalog/dependency.c

Lines changed: 47 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -543,14 +543,18 @@ findDependentObjects(const ObjectAddress *object,
543543
ObjectIdGetDatum(object->objectId));
544544
if (object->objectSubId!=0)
545545
{
546+
/* Consider only dependencies of this sub-object */
546547
ScanKeyInit(&key[2],
547548
Anum_pg_depend_objsubid,
548549
BTEqualStrategyNumber,F_INT4EQ,
549550
Int32GetDatum(object->objectSubId));
550551
nkeys=3;
551552
}
552553
else
554+
{
555+
/* Consider dependencies of this object and any sub-objects it has */
553556
nkeys=2;
557+
}
554558

555559
scan=systable_beginscan(*depRel,DependDependerIndexId, true,
556560
NULL,nkeys,key);
@@ -567,6 +571,18 @@ findDependentObjects(const ObjectAddress *object,
567571
otherObject.objectId=foundDep->refobjid;
568572
otherObject.objectSubId=foundDep->refobjsubid;
569573

574+
/*
575+
* When scanning dependencies of a whole object, we may find rows
576+
* linking sub-objects of the object to the object itself. (Normally,
577+
* such a dependency is implicit, but we must make explicit ones in
578+
* some cases involving partitioning.) We must ignore such rows to
579+
* avoid infinite recursion.
580+
*/
581+
if (otherObject.classId==object->classId&&
582+
otherObject.objectId==object->objectId&&
583+
object->objectSubId==0)
584+
continue;
585+
570586
switch (foundDep->deptype)
571587
{
572588
caseDEPENDENCY_NORMAL:
@@ -854,6 +870,16 @@ findDependentObjects(const ObjectAddress *object,
854870
otherObject.objectId=foundDep->objid;
855871
otherObject.objectSubId=foundDep->objsubid;
856872

873+
/*
874+
* If what we found is a sub-object of the current object, just ignore
875+
* it. (Normally, such a dependency is implicit, but we must make
876+
* explicit ones in some cases involving partitioning.)
877+
*/
878+
if (otherObject.classId==object->classId&&
879+
otherObject.objectId==object->objectId&&
880+
object->objectSubId==0)
881+
continue;
882+
857883
/*
858884
* Must lock the dependent object before recursing to it.
859885
*/
@@ -1588,8 +1614,10 @@ recordDependencyOnExpr(const ObjectAddress *depender,
15881614
* As above, but only one relation is expected to be referenced (with
15891615
* varno = 1 and varlevelsup = 0). Pass the relation OID instead of a
15901616
* range table. An additional frammish is that dependencies on that
1591-
* relation (or its component columns) will be marked with 'self_behavior',
1592-
* whereas 'behavior' is used for everything else.
1617+
* relation's component columns will be marked with 'self_behavior',
1618+
* whereas 'behavior' is used for everything else; also, if 'reverse_self'
1619+
* is true, those dependencies are reversed so that the columns are made
1620+
* to depend on the table not vice versa.
15931621
*
15941622
* NOTE: the caller should ensure that a whole-table dependency on the
15951623
* specified relation is created separately, if one is needed. In particular,
@@ -1602,7 +1630,7 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
16021630
Node*expr,OidrelId,
16031631
DependencyTypebehavior,
16041632
DependencyTypeself_behavior,
1605-
boolignore_self)
1633+
boolreverse_self)
16061634
{
16071635
find_expr_references_contextcontext;
16081636
RangeTblEntryrte;
@@ -1626,7 +1654,8 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
16261654
eliminate_duplicate_dependencies(context.addrs);
16271655

16281656
/* Separate self-dependencies if necessary */
1629-
if (behavior!=self_behavior&&context.addrs->numrefs>0)
1657+
if ((behavior!=self_behavior||reverse_self)&&
1658+
context.addrs->numrefs>0)
16301659
{
16311660
ObjectAddresses*self_addrs;
16321661
ObjectAddress*outobj;
@@ -1657,11 +1686,23 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
16571686
}
16581687
context.addrs->numrefs=outrefs;
16591688

1660-
/* Record the self-dependencies */
1661-
if (!ignore_self)
1689+
/* Record the self-dependencieswith the appropriate direction*/
1690+
if (!reverse_self)
16621691
recordMultipleDependencies(depender,
16631692
self_addrs->refs,self_addrs->numrefs,
16641693
self_behavior);
1694+
else
1695+
{
1696+
/* Can't use recordMultipleDependencies, so do it the hard way */
1697+
intselfref;
1698+
1699+
for (selfref=0;selfref<self_addrs->numrefs;selfref++)
1700+
{
1701+
ObjectAddress*thisobj=self_addrs->refs+selfref;
1702+
1703+
recordDependencyOn(thisobj,depender,self_behavior);
1704+
}
1705+
}
16651706

16661707
free_object_addresses(self_addrs);
16671708
}

‎src/backend/catalog/heap.c

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

34933493
/*
3494-
* Anything mentioned in the expressions. We must ignore the column
3495-
* references, which will depend on the table itself; there is no separate
3496-
* partition key object.
3494+
* The partitioning columns are made internally dependent on the table,
3495+
* because we cannot drop any of them without dropping the whole table.
3496+
* (ATExecDropColumn independently enforces that, but it's not bulletproof
3497+
* so we need the dependencies too.)
3498+
*/
3499+
for (i=0;i<partnatts;i++)
3500+
{
3501+
if (partattrs[i]==0)
3502+
continue;/* ignore expressions here */
3503+
3504+
referenced.classId=RelationRelationId;
3505+
referenced.objectId=RelationGetRelid(rel);
3506+
referenced.objectSubId=partattrs[i];
3507+
3508+
recordDependencyOn(&referenced,&myself,DEPENDENCY_INTERNAL);
3509+
}
3510+
3511+
/*
3512+
* Also consider anything mentioned in partition expressions. External
3513+
* references (e.g. functions) get NORMAL dependencies. Table columns
3514+
* mentioned in the expressions are handled the same as plain partitioning
3515+
* columns, i.e. they become internally dependent on the whole table.
34973516
*/
34983517
if (partexprs)
34993518
recordDependencyOnSingleRelExpr(&myself,
35003519
(Node*)partexprs,
35013520
RelationGetRelid(rel),
35023521
DEPENDENCY_NORMAL,
3503-
DEPENDENCY_AUTO, true);
3522+
DEPENDENCY_INTERNAL,
3523+
true/* reverse the self-deps */ );
35043524

35053525
/*
35063526
* 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
@@ -7021,27 +7021,28 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
70217021
errmsg("cannot drop system column \"%s\"",
70227022
colName)));
70237023

7024-
/* Don't drop inherited columns */
7024+
/*
7025+
* Don't drop inherited columns, unless recursing (presumably from a drop
7026+
* of the parent column)
7027+
*/
70257028
if (targetatt->attinhcount > 0 && !recursing)
70267029
ereport(ERROR,
70277030
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
70287031
errmsg("cannot drop inherited column \"%s\"",
70297032
colName)));
70307033

7031-
/* Don't drop columns used in the partition key */
7034+
/*
7035+
* Don't drop columns used in the partition key, either. (If we let this
7036+
* go through, the key column's dependencies would cause a cascaded drop
7037+
* of the whole table, which is surely not what the user expected.)
7038+
*/
70327039
if (has_partition_attrs(rel,
70337040
bms_make_singleton(attnum - FirstLowInvalidHeapAttributeNumber),
70347041
&is_expr))
7035-
{
7036-
if (!is_expr)
7037-
ereport(ERROR,
7038-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
7039-
errmsg("cannot drop column named in partition key")));
7040-
else
7041-
ereport(ERROR,
7042-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
7043-
errmsg("cannot drop column referenced in partition key expression")));
7044-
}
7042+
ereport(ERROR,
7043+
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
7044+
errmsg("cannot drop column \"%s\" because it is part of the partition key of relation \"%s\"",
7045+
colName, RelationGetRelationName(rel))));
70457046

70467047
ReleaseSysCache(tuple);
70477048

@@ -10255,16 +10256,10 @@ ATPrepAlterColumnType(List **wqueue,
1025510256
if (has_partition_attrs(rel,
1025610257
bms_make_singleton(attnum - FirstLowInvalidHeapAttributeNumber),
1025710258
&is_expr))
10258-
{
10259-
if (!is_expr)
10260-
ereport(ERROR,
10261-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
10262-
errmsg("cannot alter type of column named in partition key")));
10263-
else
10264-
ereport(ERROR,
10265-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
10266-
errmsg("cannot alter type of column referenced in partition key expression")));
10267-
}
10259+
ereport(ERROR,
10260+
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
10261+
errmsg("cannot alter column \"%s\" because it is part of the partition key of relation \"%s\"",
10262+
colName, RelationGetRelationName(rel))));
1026810263

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

‎src/bin/pg_dump/pg_dump_sort.c

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1104,7 +1104,15 @@ repairDependencyLoop(DumpableObject **loop,
11041104
}
11051105
}
11061106

1107-
/* Loop of table with itself, happens with generated columns */
1107+
/*
1108+
* Loop of table with itself --- just ignore it.
1109+
*
1110+
* (Actually, what this arises from is a dependency of a table column on
1111+
* another column, which happens with generated columns; or a dependency
1112+
* of a table column on the whole table, which happens with partitioning.
1113+
* But we didn't pay attention to sub-object IDs while collecting the
1114+
* dependency data, so we can't see that here.)
1115+
*/
11081116
if (nLoop==1)
11091117
{
11101118
if (loop[0]->objType==DO_TABLE)

‎src/include/catalog/catversion.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,6 @@
5353
*/
5454

5555
/*yyyymmddN */
56-
#defineCATALOG_VERSION_NO201907142
56+
#defineCATALOG_VERSION_NO201907222
5757

5858
#endif

‎src/include/catalog/dependency.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ extern void recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
156156
Node*expr,OidrelId,
157157
DependencyTypebehavior,
158158
DependencyTypeself_behavior,
159-
boolignore_self);
159+
boolreverse_self);
160160

161161
externObjectClassgetObjectClass(constObjectAddress*object);
162162

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3446,13 +3446,13 @@ LINE 1: ALTER TABLE partitioned ADD EXCLUDE USING gist (a WITH &&);
34463446
^
34473447
-- cannot drop column that is part of the partition key
34483448
ALTER TABLE partitioned DROP COLUMN a;
3449-
ERROR: cannot drop columnnamed inpartition key
3449+
ERROR: cannot drop column"a" because it is part of thepartition key of relation "partitioned"
34503450
ALTER TABLE partitioned ALTER COLUMN a TYPE char(5);
3451-
ERROR: cannot altertype of column named inpartition key
3451+
ERROR: cannot altercolumn "a" because it is part of thepartition key of relation "partitioned"
34523452
ALTER TABLE partitioned DROP COLUMN b;
3453-
ERROR: cannot drop columnreferenced inpartition keyexpression
3453+
ERROR: cannot drop column"b" because it is part of thepartition keyof relation "partitioned"
34543454
ALTER TABLE partitioned ALTER COLUMN b TYPE char(5);
3455-
ERROR: cannot altertype of column referenced inpartition keyexpression
3455+
ERROR: cannot altercolumn "b" because it is part of thepartition keyof relation "partitioned"
34563456
-- partitioned table cannot participate in regular inheritance
34573457
CREATE TABLE nonpartitioned (
34583458
a int,
@@ -3945,9 +3945,9 @@ ERROR: cannot change inheritance of a partition
39453945
-- partitioned tables; for example, part_5, which is list_parted2's
39463946
-- partition, is partitioned on b;
39473947
ALTER TABLE list_parted2 DROP COLUMN b;
3948-
ERROR: cannot drop columnnamed inpartition key
3948+
ERROR: cannot drop column"b" because it is part of thepartition key of relation "part_5"
39493949
ALTER TABLE list_parted2 ALTER COLUMN b TYPE text;
3950-
ERROR: cannot altertype of column named inpartition key
3950+
ERROR: cannot altercolumn "b" because it is part of thepartition key of relation "part_5"
39513951
-- dropping non-partition key columns should be allowed on the parent table.
39523952
ALTER TABLE list_parted DROP COLUMN b;
39533953
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
@@ -501,6 +501,42 @@ Partition of: partitioned2 FOR VALUES FROM ('-1', 'aaaaa') TO (100, 'ccccc')
501501
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))))
502502

503503
DROP TABLE partitioned, partitioned2;
504+
-- check that dependencies of partition columns are handled correctly
505+
create domain intdom1 as int;
506+
create table partitioned (
507+
a intdom1,
508+
b text
509+
) partition by range (a);
510+
alter table partitioned drop column a; -- fail
511+
ERROR: cannot drop column "a" because it is part of the partition key of relation "partitioned"
512+
drop domain intdom1; -- fail, requires cascade
513+
ERROR: cannot drop type intdom1 because other objects depend on it
514+
DETAIL: table partitioned depends on type intdom1
515+
HINT: Use DROP ... CASCADE to drop the dependent objects too.
516+
drop domain intdom1 cascade;
517+
NOTICE: drop cascades to table partitioned
518+
table partitioned; -- gone
519+
ERROR: relation "partitioned" does not exist
520+
LINE 1: table partitioned;
521+
^
522+
-- likewise for columns used in partition expressions
523+
create domain intdom1 as int;
524+
create table partitioned (
525+
a intdom1,
526+
b text
527+
) partition by range (plusone(a));
528+
alter table partitioned drop column a; -- fail
529+
ERROR: cannot drop column "a" because it is part of the partition key of relation "partitioned"
530+
drop domain intdom1; -- fail, requires cascade
531+
ERROR: cannot drop type intdom1 because other objects depend on it
532+
DETAIL: table partitioned depends on type intdom1
533+
HINT: Use DROP ... CASCADE to drop the dependent objects too.
534+
drop domain intdom1 cascade;
535+
NOTICE: drop cascades to table partitioned
536+
table partitioned; -- gone
537+
ERROR: relation "partitioned" does not exist
538+
LINE 1: table partitioned;
539+
^
504540
--
505541
-- Partitions
506542
--

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

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

450450
DROPTABLE partitioned, partitioned2;
451451

452+
-- check that dependencies of partition columns are handled correctly
453+
createdomainintdom1asint;
454+
455+
createtablepartitioned (
456+
a intdom1,
457+
btext
458+
) partition by range (a);
459+
460+
altertable partitioned drop column a;-- fail
461+
462+
dropdomain intdom1;-- fail, requires cascade
463+
464+
dropdomain intdom1 cascade;
465+
466+
table partitioned;-- gone
467+
468+
-- likewise for columns used in partition expressions
469+
createdomainintdom1asint;
470+
471+
createtablepartitioned (
472+
a intdom1,
473+
btext
474+
) partition by range (plusone(a));
475+
476+
altertable partitioned drop column a;-- fail
477+
478+
dropdomain intdom1;-- fail, requires cascade
479+
480+
dropdomain intdom1 cascade;
481+
482+
table partitioned;-- gone
483+
484+
452485
--
453486
-- Partitions
454487
--

0 commit comments

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