Skip to content

Commit 63f9282

Browse files
robertmhaasrobertmhaas
committed
Tighten integrity checks on ALTER TABLE ... ALTER COLUMN ... RENAME.
When a column is renamed, we recursively rename the same column in all descendent tables. But if one of those tables also inherits that column from a table outside the inheritance hierarchy rooted at the named table, we must throw an error. The previous coding correctly prohibited the rename when the parent had inherited the column from elsewhere, but overlooked the case where the parent was OK but a child table also inherited the same column from a second, unrelated parent. For now, not backpatched due to lack of complaints from the field. KaiGai Kohei, with further changes by me. Reviewed by Bernd Helme and Tom Lane.
1 parent 1526d4e commit 63f9282

File tree

9 files changed

+211
-29
lines changed

9 files changed

+211
-29
lines changed

‎src/backend/catalog/pg_inherits.c

Lines changed: 37 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
*
1414
*
1515
* IDENTIFICATION
16-
* $PostgreSQL: pgsql/src/backend/catalog/pg_inherits.c,v 1.5 2010/01/02 16:57:36 momjian Exp $
16+
* $PostgreSQL: pgsql/src/backend/catalog/pg_inherits.c,v 1.6 2010/02/01 19:28:56 rhaas Exp $
1717
*
1818
*-------------------------------------------------------------------------
1919
*/
@@ -148,16 +148,19 @@ find_inheritance_children(Oid parentrelId, LOCKMODE lockmode)
148148
* find_all_inheritors -
149149
* Returns a list of relation OIDs including the given rel plus
150150
* all relations that inherit from it, directly or indirectly.
151+
* Optionally, it also returns the number of parents found for
152+
* each such relation within the inheritance tree rooted at the
153+
* given rel.
151154
*
152155
* The specified lock type is acquired on all child relations (but not on the
153156
* given rel; caller should already have locked it). If lockmode is NoLock
154157
* then no locks are acquired, but caller must beware of race conditions
155158
* against possible DROPs of child relations.
156159
*/
157160
List *
158-
find_all_inheritors(Oid parentrelId, LOCKMODE lockmode)
161+
find_all_inheritors(Oid parentrelId, LOCKMODE lockmode, List **numparents)
159162
{
160-
List *rels_list;
163+
List *rels_list, *rel_numparents;
161164
ListCell *l;
162165

163166
/*
@@ -168,11 +171,13 @@ find_all_inheritors(Oid parentrelId, LOCKMODE lockmode)
168171
* doesn't fetch the next list element until the bottom of the loop.
169172
*/
170173
rels_list = list_make1_oid(parentrelId);
174+
rel_numparents = list_make1_int(0);
171175

172176
foreach(l, rels_list)
173177
{
174178
Oid currentrel = lfirst_oid(l);
175179
List *currentchildren;
180+
ListCell *lc;
176181

177182
/* Get the direct children of this rel */
178183
currentchildren = find_inheritance_children(currentrel, lockmode);
@@ -184,9 +189,37 @@ find_all_inheritors(Oid parentrelId, LOCKMODE lockmode)
184189
* loop, though theoretically there can't be any cycles in the
185190
* inheritance graph anyway.)
186191
*/
187-
rels_list = list_concat_unique_oid(rels_list, currentchildren);
192+
foreach(lc, currentchildren)
193+
{
194+
Oid child_oid = lfirst_oid(lc);
195+
bool found = false;
196+
ListCell *lo;
197+
ListCell *li;
198+
199+
/* if the rel is already there, bump number-of-parents counter */
200+
forboth(lo, rels_list, li, rel_numparents)
201+
{
202+
if (lfirst_oid(lo) == child_oid)
203+
{
204+
lfirst_int(li)++;
205+
found = true;
206+
break;
207+
}
208+
}
209+
210+
/* if it's not there, add it. expect 1 parent, initially. */
211+
if (!found)
212+
{
213+
rels_list = lappend_oid(rels_list, child_oid);
214+
rel_numparents = lappend_int(rel_numparents, 1);
215+
}
216+
}
188217
}
189218

219+
if (numparents)
220+
*numparents = rel_numparents;
221+
else
222+
list_free(rel_numparents);
190223
return rels_list;
191224
}
192225

‎src/backend/commands/alter.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/commands/alter.c,v 1.33 2010/01/02 16:57:36 momjian Exp $
11+
* $PostgreSQL: pgsql/src/backend/commands/alter.c,v 1.34 2010/02/01 19:28:56 rhaas Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -126,7 +126,7 @@ ExecRenameStmt(RenameStmt *stmt)
126126
stmt->subname, /* old att name */
127127
stmt->newname, /* new att name */
128128
interpretInhOption(stmt->relation->inhOpt), /* recursive? */
129-
false); /* recursing already? */
129+
0); /* expected inhcount */
130130
break;
131131
case OBJECT_TRIGGER:
132132
renametrig(relid,

‎src/backend/commands/analyze.c

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/commands/analyze.c,v 1.148 2010/01/22 16:40:18 rhaas Exp $
11+
* $PostgreSQL: pgsql/src/backend/commands/analyze.c,v 1.149 2010/02/01 19:28:56 rhaas Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -1390,7 +1390,8 @@ acquire_inherited_sample_rows(Relation onerel, HeapTuple *rows, int targrows,
13901390
* Find all members of inheritance set. We only need AccessShareLock on
13911391
* the children.
13921392
*/
1393-
tableOIDs = find_all_inheritors(RelationGetRelid(onerel), AccessShareLock);
1393+
tableOIDs =
1394+
find_all_inheritors(RelationGetRelid(onerel), AccessShareLock, NULL);
13941395

13951396
/*
13961397
* Check that there's at least one descendant, else fail. This could

‎src/backend/commands/tablecmds.c

Lines changed: 29 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.320 2010/01/28 23:21:11 petere Exp $
11+
* $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.321 2010/02/01 19:28:56 rhaas Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -824,7 +824,7 @@ ExecuteTruncate(TruncateStmt *stmt)
824824
ListCell *child;
825825
List *children;
826826

827-
children = find_all_inheritors(myrelid, AccessExclusiveLock);
827+
children = find_all_inheritors(myrelid, AccessExclusiveLock, NULL);
828828

829829
foreach(child, children)
830830
{
@@ -1943,7 +1943,7 @@ renameatt(Oid myrelid,
19431943
const char *oldattname,
19441944
const char *newattname,
19451945
bool recurse,
1946-
bool recursing)
1946+
int expected_parents)
19471947
{
19481948
Relation targetrelation;
19491949
Relation attrelation;
@@ -1987,33 +1987,42 @@ renameatt(Oid myrelid,
19871987
*/
19881988
if (recurse)
19891989
{
1990-
ListCell *child;
1991-
List *children;
1990+
List *child_oids, *child_numparents;
1991+
ListCell *lo, *li;
19921992

1993-
children = find_all_inheritors(myrelid, AccessExclusiveLock);
1993+
/*
1994+
* we need the number of parents for each child so that the recursive
1995+
* calls to renameatt() can determine whether there are any parents
1996+
* outside the inheritance hierarchy being processed.
1997+
*/
1998+
child_oids = find_all_inheritors(myrelid, AccessExclusiveLock,
1999+
&child_numparents);
19942000

19952001
/*
19962002
* find_all_inheritors does the recursive search of the inheritance
19972003
* hierarchy, so all we have to do is process all of the relids in the
19982004
* list that it returns.
19992005
*/
2000-
foreach(child, children)
2006+
forboth(lo, child_oids, li, child_numparents)
20012007
{
2002-
Oid childrelid = lfirst_oid(child);
2008+
Oid childrelid = lfirst_oid(lo);
2009+
int numparents = lfirst_int(li);
20032010

20042011
if (childrelid == myrelid)
20052012
continue;
20062013
/* note we need not recurse again */
2007-
renameatt(childrelid, oldattname, newattname, false, true);
2014+
renameatt(childrelid, oldattname, newattname, false, numparents);
20082015
}
20092016
}
20102017
else
20112018
{
20122019
/*
20132020
* If we are told not to recurse, there had better not be any child
20142021
* tables; else the rename would put them out of step.
2022+
*
2023+
* expected_parents will only be 0 if we are not already recursing.
20152024
*/
2016-
if (!recursing &&
2025+
if (expected_parents == 0 &&
20172026
find_inheritance_children(myrelid, NoLock) != NIL)
20182027
ereport(ERROR,
20192028
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
@@ -2039,10 +2048,15 @@ renameatt(Oid myrelid,
20392048
oldattname)));
20402049

20412050
/*
2042-
* if the attribute is inherited, forbid the renaming, unless we are
2043-
* already inside a recursive rename.
2051+
* if the attribute is inherited, forbid the renaming. if this is a
2052+
* top-level call to renameatt(), then expected_parents will be 0, so the
2053+
* effect of this code will be to prohibit the renaming if the attribute
2054+
* is inherited at all. if this is a recursive call to renameatt(),
2055+
* expected_parents will be the number of parents the current relation has
2056+
* within the inheritance hierarchy being processed, so we'll prohibit
2057+
* the renaming only if there are additional parents from elsewhere.
20442058
*/
2045-
if (attform->attinhcount > 0 && !recursing)
2059+
if (attform->attinhcount > expected_parents)
20462060
ereport(ERROR,
20472061
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
20482062
errmsg("cannot rename inherited column \"%s\"",
@@ -3410,7 +3424,7 @@ ATSimpleRecursion(List **wqueue, Relation rel,
34103424
ListCell *child;
34113425
List *children;
34123426

3413-
children = find_all_inheritors(relid, AccessExclusiveLock);
3427+
children = find_all_inheritors(relid, AccessExclusiveLock, NULL);
34143428

34153429
/*
34163430
* find_all_inheritors does the recursive search of the inheritance
@@ -7233,7 +7247,7 @@ ATExecAddInherit(Relation child_rel, RangeVar *parent)
72337247
* We use weakest lock we can on child's children, namely AccessShareLock.
72347248
*/
72357249
children = find_all_inheritors(RelationGetRelid(child_rel),
7236-
AccessShareLock);
7250+
AccessShareLock, NULL);
72377251

72387252
if (list_member_oid(children, RelationGetRelid(parent_rel)))
72397253
ereport(ERROR,

‎src/backend/optimizer/prep/prepunion.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222
*
2323
*
2424
* IDENTIFICATION
25-
* $PostgreSQL: pgsql/src/backend/optimizer/prep/prepunion.c,v 1.179 2010/01/02 16:57:47 momjian Exp $
25+
* $PostgreSQL: pgsql/src/backend/optimizer/prep/prepunion.c,v 1.180 2010/02/01 19:28:56 rhaas Exp $
2626
*
2727
*-------------------------------------------------------------------------
2828
*/
@@ -1180,7 +1180,7 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
11801180
lockmode = AccessShareLock;
11811181

11821182
/* Scan for all members of inheritance set, acquire needed locks */
1183-
inhOIDs = find_all_inheritors(parentOID, lockmode);
1183+
inhOIDs = find_all_inheritors(parentOID, lockmode, NULL);
11841184

11851185
/*
11861186
* Check that there's at least one descendant, else treat as no-child

‎src/include/catalog/pg_inherits_fn.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group
88
* Portions Copyright (c) 1994, Regents of the University of California
99
*
10-
* $PostgreSQL: pgsql/src/include/catalog/pg_inherits_fn.h,v 1.2 2010/01/02 16:58:01 momjian Exp $
10+
* $PostgreSQL: pgsql/src/include/catalog/pg_inherits_fn.h,v 1.3 2010/02/01 19:28:56 rhaas Exp $
1111
*
1212
*-------------------------------------------------------------------------
1313
*/
@@ -18,7 +18,8 @@
1818
#include "storage/lock.h"
1919

2020
extern List *find_inheritance_children(Oid parentrelId, LOCKMODE lockmode);
21-
extern List *find_all_inheritors(Oid parentrelId, LOCKMODE lockmode);
21+
extern List *find_all_inheritors(Oid parentrelId, LOCKMODE lockmode,
22+
List **parents);
2223
extern bool has_subclass(Oid relationId);
2324
extern bool typeInheritsFrom(Oid subclassTypeId, Oid superclassTypeId);
2425

‎src/include/commands/tablecmds.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group
88
* Portions Copyright (c) 1994, Regents of the University of California
99
*
10-
* $PostgreSQL: pgsql/src/include/commands/tablecmds.h,v 1.45 2010/01/02 16:58:03 momjian Exp $
10+
* $PostgreSQL: pgsql/src/include/commands/tablecmds.h,v 1.46 2010/02/01 19:28:56 rhaas Exp $
1111
*
1212
*-------------------------------------------------------------------------
1313
*/
@@ -43,7 +43,7 @@ extern void renameatt(Oid myrelid,
4343
const char *oldattname,
4444
const char *newattname,
4545
bool recurse,
46-
bool recursing);
46+
int expected_parents);
4747

4848
extern void RenameRelation(Oid myrelid,
4949
const char *newrelname,

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

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1053,3 +1053,97 @@ NOTICE: merging column "a" with inherited definition
10531053
ERROR: column "a" has a storage parameter conflict
10541054
DETAIL: MAIN versus EXTENDED
10551055
DROP TABLE t1, t2, t3, t4, t12_storage, t12_comments, t1_inh, t13_inh, t13_like, t_all;
1056+
-- Test for renaming in simple multiple inheritance
1057+
CREATE TABLE t1 (a int, b int);
1058+
CREATE TABLE s1 (b int, c int);
1059+
CREATE TABLE ts (d int) INHERITS (t1, s1);
1060+
NOTICE: merging multiple inherited definitions of column "b"
1061+
ALTER TABLE t1 RENAME a TO aa;
1062+
ALTER TABLE t1 RENAME b TO bb; -- to be failed
1063+
ERROR: cannot rename inherited column "b"
1064+
ALTER TABLE ts RENAME aa TO aaa; -- to be failed
1065+
ERROR: cannot rename inherited column "aa"
1066+
ALTER TABLE ts RENAME d TO dd;
1067+
\d+ ts
1068+
Table "public.ts"
1069+
Column | Type | Modifiers | Storage | Description
1070+
--------+---------+-----------+---------+-------------
1071+
aa | integer | | plain |
1072+
b | integer | | plain |
1073+
c | integer | | plain |
1074+
dd | integer | | plain |
1075+
Inherits: t1,
1076+
s1
1077+
Has OIDs: no
1078+
1079+
DROP TABLE ts;
1080+
-- Test for renaming in diamond inheritance
1081+
CREATE TABLE t2 (x int) INHERITS (t1);
1082+
CREATE TABLE t3 (y int) INHERITS (t1);
1083+
CREATE TABLE t4 (z int) INHERITS (t2, t3);
1084+
NOTICE: merging multiple inherited definitions of column "aa"
1085+
NOTICE: merging multiple inherited definitions of column "b"
1086+
ALTER TABLE t1 RENAME aa TO aaa;
1087+
\d+ t4
1088+
Table "public.t4"
1089+
Column | Type | Modifiers | Storage | Description
1090+
--------+---------+-----------+---------+-------------
1091+
aaa | integer | | plain |
1092+
b | integer | | plain |
1093+
x | integer | | plain |
1094+
y | integer | | plain |
1095+
z | integer | | plain |
1096+
Inherits: t2,
1097+
t3
1098+
Has OIDs: no
1099+
1100+
CREATE TABLE ts (d int) INHERITS (t2, s1);
1101+
NOTICE: merging multiple inherited definitions of column "b"
1102+
ALTER TABLE t1 RENAME aaa TO aaaa;
1103+
ALTER TABLE t1 RENAME b TO bb; -- to be failed
1104+
ERROR: cannot rename inherited column "b"
1105+
\d+ ts
1106+
Table "public.ts"
1107+
Column | Type | Modifiers | Storage | Description
1108+
--------+---------+-----------+---------+-------------
1109+
aaaa | integer | | plain |
1110+
b | integer | | plain |
1111+
x | integer | | plain |
1112+
c | integer | | plain |
1113+
d | integer | | plain |
1114+
Inherits: t2,
1115+
s1
1116+
Has OIDs: no
1117+
1118+
WITH RECURSIVE r AS (
1119+
SELECT 't1'::regclass AS inhrelid
1120+
UNION ALL
1121+
SELECT c.inhrelid FROM pg_inherits c, r WHERE r.inhrelid = c.inhparent
1122+
)
1123+
SELECT a.attrelid::regclass, a.attname, a.attinhcount, e.expected
1124+
FROM (SELECT inhrelid, count(*) AS expected FROM pg_inherits
1125+
WHERE inhparent IN (SELECT inhrelid FROM r) GROUP BY inhrelid) e
1126+
JOIN pg_attribute a ON e.inhrelid = a.attrelid WHERE NOT attislocal
1127+
ORDER BY a.attrelid::regclass::text, a.attnum;
1128+
attrelid | attname | attinhcount | expected
1129+
----------+---------+-------------+----------
1130+
t2 | aaaa | 1 | 1
1131+
t2 | b | 1 | 1
1132+
t3 | aaaa | 1 | 1
1133+
t3 | b | 1 | 1
1134+
t4 | aaaa | 2 | 2
1135+
t4 | b | 2 | 2
1136+
t4 | x | 1 | 2
1137+
t4 | y | 1 | 2
1138+
ts | aaaa | 1 | 1
1139+
ts | b | 2 | 1
1140+
ts | x | 1 | 1
1141+
ts | c | 1 | 1
1142+
(12 rows)
1143+
1144+
DROP TABLE t1, s1 CASCADE;
1145+
NOTICE: drop cascades to 4 other objects
1146+
DETAIL: drop cascades to table t2
1147+
drop cascades to table ts
1148+
drop cascades to table t3
1149+
drop cascades to table t4

0 commit comments

Comments
 (0)