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

Commit0e1deaa

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 parent8a4fa29 commit0e1deaa

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
@@ -526,14 +526,18 @@ findDependentObjects(const ObjectAddress *object,
526526
ObjectIdGetDatum(object->objectId));
527527
if (object->objectSubId!=0)
528528
{
529+
/* Consider only dependencies of this sub-object */
529530
ScanKeyInit(&key[2],
530531
Anum_pg_depend_objsubid,
531532
BTEqualStrategyNumber,F_INT4EQ,
532533
Int32GetDatum(object->objectSubId));
533534
nkeys=3;
534535
}
535536
else
537+
{
538+
/* Consider dependencies of this object and any sub-objects it has */
536539
nkeys=2;
540+
}
537541

538542
scan=systable_beginscan(*depRel,DependDependerIndexId, true,
539543
NULL,nkeys,key);
@@ -546,6 +550,18 @@ findDependentObjects(const ObjectAddress *object,
546550
otherObject.objectId=foundDep->refobjid;
547551
otherObject.objectSubId=foundDep->refobjsubid;
548552

553+
/*
554+
* When scanning dependencies of a whole object, we may find rows
555+
* linking sub-objects of the object to the object itself. (Normally,
556+
* such a dependency is implicit, but we must make explicit ones in
557+
* some cases involving partitioning.) We must ignore such rows to
558+
* avoid infinite recursion.
559+
*/
560+
if (otherObject.classId==object->classId&&
561+
otherObject.objectId==object->objectId&&
562+
object->objectSubId==0)
563+
continue;
564+
549565
switch (foundDep->deptype)
550566
{
551567
caseDEPENDENCY_NORMAL:
@@ -732,6 +748,16 @@ findDependentObjects(const ObjectAddress *object,
732748
otherObject.objectId=foundDep->objid;
733749
otherObject.objectSubId=foundDep->objsubid;
734750

751+
/*
752+
* If what we found is a sub-object of the current object, just ignore
753+
* it. (Normally, such a dependency is implicit, but we must make
754+
* explicit ones in some cases involving partitioning.)
755+
*/
756+
if (otherObject.classId==object->classId&&
757+
otherObject.objectId==object->objectId&&
758+
object->objectSubId==0)
759+
continue;
760+
735761
/*
736762
* Must lock the dependent object before recursing to it.
737763
*/
@@ -1379,8 +1405,10 @@ recordDependencyOnExpr(const ObjectAddress *depender,
13791405
* As above, but only one relation is expected to be referenced (with
13801406
* varno = 1 and varlevelsup = 0). Pass the relation OID instead of a
13811407
* range table. An additional frammish is that dependencies on that
1382-
* relation (or its component columns) will be marked with 'self_behavior',
1383-
* whereas 'behavior' is used for everything else.
1408+
* relation's component columns will be marked with 'self_behavior',
1409+
* whereas 'behavior' is used for everything else; also, if 'reverse_self'
1410+
* is true, those dependencies are reversed so that the columns are made
1411+
* to depend on the table not vice versa.
13841412
*
13851413
* NOTE: the caller should ensure that a whole-table dependency on the
13861414
* specified relation is created separately, if one is needed. In particular,
@@ -1393,7 +1421,7 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
13931421
Node*expr,OidrelId,
13941422
DependencyTypebehavior,
13951423
DependencyTypeself_behavior,
1396-
boolignore_self)
1424+
boolreverse_self)
13971425
{
13981426
find_expr_references_contextcontext;
13991427
RangeTblEntryrte;
@@ -1416,7 +1444,8 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
14161444
eliminate_duplicate_dependencies(context.addrs);
14171445

14181446
/* Separate self-dependencies if necessary */
1419-
if (behavior!=self_behavior&&context.addrs->numrefs>0)
1447+
if ((behavior!=self_behavior||reverse_self)&&
1448+
context.addrs->numrefs>0)
14201449
{
14211450
ObjectAddresses*self_addrs;
14221451
ObjectAddress*outobj;
@@ -1447,11 +1476,23 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
14471476
}
14481477
context.addrs->numrefs=outrefs;
14491478

1450-
/* Record the self-dependencies */
1451-
if (!ignore_self)
1479+
/* Record the self-dependencieswith the appropriate direction*/
1480+
if (!reverse_self)
14521481
recordMultipleDependencies(depender,
14531482
self_addrs->refs,self_addrs->numrefs,
14541483
self_behavior);
1484+
else
1485+
{
1486+
/* Can't use recordMultipleDependencies, so do it the hard way */
1487+
intselfref;
1488+
1489+
for (selfref=0;selfref<self_addrs->numrefs;selfref++)
1490+
{
1491+
ObjectAddress*thisobj=self_addrs->refs+selfref;
1492+
1493+
recordDependencyOn(thisobj,depender,self_behavior);
1494+
}
1495+
}
14551496

14561497
free_object_addresses(self_addrs);
14571498
}

‎src/backend/catalog/heap.c

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

31723172
/*
3173-
* Anything mentioned in the expressions. We must ignore the column
3174-
* references, which will depend on the table itself; there is no separate
3175-
* partition key object.
3173+
* The partitioning columns are made internally dependent on the table,
3174+
* because we cannot drop any of them without dropping the whole table.
3175+
* (ATExecDropColumn independently enforces that, but it's not bulletproof
3176+
* so we need the dependencies too.)
3177+
*/
3178+
for (i=0;i<partnatts;i++)
3179+
{
3180+
if (partattrs[i]==0)
3181+
continue;/* ignore expressions here */
3182+
3183+
referenced.classId=RelationRelationId;
3184+
referenced.objectId=RelationGetRelid(rel);
3185+
referenced.objectSubId=partattrs[i];
3186+
3187+
recordDependencyOn(&referenced,&myself,DEPENDENCY_INTERNAL);
3188+
}
3189+
3190+
/*
3191+
* Also consider anything mentioned in partition expressions. External
3192+
* references (e.g. functions) get NORMAL dependencies. Table columns
3193+
* mentioned in the expressions are handled the same as plain partitioning
3194+
* columns, i.e. they become internally dependent on the whole table.
31763195
*/
31773196
if (partexprs)
31783197
recordDependencyOnSingleRelExpr(&myself,
31793198
(Node*)partexprs,
31803199
RelationGetRelid(rel),
31813200
DEPENDENCY_NORMAL,
3182-
DEPENDENCY_AUTO, true);
3201+
DEPENDENCY_INTERNAL,
3202+
true/* reverse the self-deps */ );
31833203

31843204
/*
31853205
* 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
@@ -6549,25 +6549,26 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
65496549
errmsg("cannot drop system column \"%s\"",
65506550
colName)));
65516551

6552-
/* Don't drop inherited columns */
6552+
/*
6553+
* Don't drop inherited columns, unless recursing (presumably from a drop
6554+
* of the parent column)
6555+
*/
65536556
if (targetatt->attinhcount>0&& !recursing)
65546557
ereport(ERROR,
65556558
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
65566559
errmsg("cannot drop inherited column \"%s\"",
65576560
colName)));
65586561

6559-
/* Don't drop columns used in the partition key */
6562+
/*
6563+
* Don't drop columns used in the partition key, either. (If we let this
6564+
* go through, the key column's dependencies would cause a cascaded drop
6565+
* of the whole table, which is surely not what the user expected.)
6566+
*/
65606567
if (is_partition_attr(rel,attnum,&is_expr))
6561-
{
6562-
if (!is_expr)
6563-
ereport(ERROR,
6564-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
6565-
errmsg("cannot drop column named in partition key")));
6566-
else
6567-
ereport(ERROR,
6568-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
6569-
errmsg("cannot drop column referenced in partition key expression")));
6570-
}
6568+
ereport(ERROR,
6569+
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
6570+
errmsg("cannot drop column \"%s\" because it is part of the partition key of relation \"%s\"",
6571+
colName,RelationGetRelationName(rel))));
65716572

65726573
ReleaseSysCache(tuple);
65736574

@@ -8776,16 +8777,10 @@ ATPrepAlterColumnType(List **wqueue,
87768777

87778778
/* Don't alter columns used in the partition key */
87788779
if (is_partition_attr(rel,attnum,&is_expr))
8779-
{
8780-
if (!is_expr)
8781-
ereport(ERROR,
8782-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
8783-
errmsg("cannot alter type of column named in partition key")));
8784-
else
8785-
ereport(ERROR,
8786-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
8787-
errmsg("cannot alter type of column referenced in partition key expression")));
8788-
}
8780+
ereport(ERROR,
8781+
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
8782+
errmsg("cannot alter column \"%s\" because it is part of the partition key of relation \"%s\"",
8783+
colName,RelationGetRelationName(rel))));
87898784

87908785
/* Look up the target type */
87918786
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
@@ -1173,6 +1173,24 @@ repairDependencyLoop(DumpableObject **loop,
11731173
}
11741174
}
11751175

1176+
/*
1177+
* Loop of table with itself --- just ignore it.
1178+
*
1179+
* (Actually, what this arises from is a dependency of a table column on
1180+
* another column, which happens with generated columns; or a dependency
1181+
* of a table column on the whole table, which happens with partitioning.
1182+
* But we didn't pay attention to sub-object IDs while collecting the
1183+
* dependency data, so we can't see that here.)
1184+
*/
1185+
if (nLoop==1)
1186+
{
1187+
if (loop[0]->objType==DO_TABLE)
1188+
{
1189+
removeObjectDependency(loop[0],loop[0]->dumpId);
1190+
return;
1191+
}
1192+
}
1193+
11761194
/*
11771195
* If all the objects are TABLE_DATA items, what we must have is a
11781196
* 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
@@ -194,7 +194,7 @@ extern void recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
194194
Node*expr,OidrelId,
195195
DependencyTypebehavior,
196196
DependencyTypeself_behavior,
197-
boolignore_self);
197+
boolreverse_self);
198198

199199
externObjectClassgetObjectClass(constObjectAddress*object);
200200

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3309,13 +3309,13 @@ LINE 1: ALTER TABLE partitioned ADD EXCLUDE USING gist (a WITH &&);
33093309
^
33103310
-- cannot drop column that is part of the partition key
33113311
ALTER TABLE partitioned DROP COLUMN a;
3312-
ERROR: cannot drop columnnamed inpartition key
3312+
ERROR: cannot drop column"a" because it is part of thepartition key of relation "partitioned"
33133313
ALTER TABLE partitioned ALTER COLUMN a TYPE char(5);
3314-
ERROR: cannot altertype of column named inpartition key
3314+
ERROR: cannot altercolumn "a" because it is part of thepartition key of relation "partitioned"
33153315
ALTER TABLE partitioned DROP COLUMN b;
3316-
ERROR: cannot drop columnreferenced inpartition keyexpression
3316+
ERROR: cannot drop column"b" because it is part of thepartition keyof relation "partitioned"
33173317
ALTER TABLE partitioned ALTER COLUMN b TYPE char(5);
3318-
ERROR: cannot altertype of column referenced inpartition keyexpression
3318+
ERROR: cannot altercolumn "b" because it is part of thepartition keyof relation "partitioned"
33193319
-- partitioned table cannot participate in regular inheritance
33203320
CREATE TABLE nonpartitioned (
33213321
a int,
@@ -3697,9 +3697,9 @@ ERROR: cannot change inheritance of a partition
36973697
-- partitioned tables; for example, part_5, which is list_parted2's
36983698
-- partition, is partitioned on b;
36993699
ALTER TABLE list_parted2 DROP COLUMN b;
3700-
ERROR: cannot drop columnnamed inpartition key
3700+
ERROR: cannot drop column"b" because it is part of thepartition key of relation "part_5"
37013701
ALTER TABLE list_parted2 ALTER COLUMN b TYPE text;
3702-
ERROR: cannot altertype of column named inpartition key
3702+
ERROR: cannot altercolumn "b" because it is part of thepartition key of relation "part_5"
37033703
-- cleanup
37043704
DROP TABLE list_parted, list_parted2, range_parted;
37053705
-- more tests for certain multi-level partitioning scenarios

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

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

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

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

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

434434
DROPTABLE partitioned, partitioned2;
435435

436+
-- check that dependencies of partition columns are handled correctly
437+
createdomainintdom1asint;
438+
439+
createtablepartitioned (
440+
a intdom1,
441+
btext
442+
) partition by range (a);
443+
444+
altertable partitioned drop column a;-- fail
445+
446+
dropdomain intdom1;-- fail, requires cascade
447+
448+
dropdomain intdom1 cascade;
449+
450+
table partitioned;-- gone
451+
452+
-- likewise for columns used in partition expressions
453+
createdomainintdom1asint;
454+
455+
createtablepartitioned (
456+
a intdom1,
457+
btext
458+
) partition by range (plusone(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+
436469
--
437470
-- Partitions
438471
--

0 commit comments

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