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.
This commit is contained in:
Robert Haas
parent
1526d4e38f
commit
63f9282f6e
|
|
@ -13,7 +13,7 @@
|
|||
*
|
||||
*
|
||||
* IDENTIFICATION
|
||||
* $PostgreSQL: pgsql/src/backend/catalog/pg_inherits.c,v 1.5 2010/01/02 16:57:36 momjian Exp $
|
||||
* $PostgreSQL: pgsql/src/backend/catalog/pg_inherits.c,v 1.6 2010/02/01 19:28:56 rhaas Exp $
|
||||
*
|
||||
*-------------------------------------------------------------------------
|
||||
*/
|
||||
|
|
@ -148,6 +148,9 @@ find_inheritance_children(Oid parentrelId, LOCKMODE lockmode)
|
|||
* find_all_inheritors -
|
||||
* Returns a list of relation OIDs including the given rel plus
|
||||
* all relations that inherit from it, directly or indirectly.
|
||||
* Optionally, it also returns the number of parents found for
|
||||
* each such relation within the inheritance tree rooted at the
|
||||
* given rel.
|
||||
*
|
||||
* The specified lock type is acquired on all child relations (but not on the
|
||||
* given rel; caller should already have locked it). If lockmode is NoLock
|
||||
|
|
@ -155,9 +158,9 @@ find_inheritance_children(Oid parentrelId, LOCKMODE lockmode)
|
|||
* against possible DROPs of child relations.
|
||||
*/
|
||||
List *
|
||||
find_all_inheritors(Oid parentrelId, LOCKMODE lockmode)
|
||||
find_all_inheritors(Oid parentrelId, LOCKMODE lockmode, List **numparents)
|
||||
{
|
||||
List *rels_list;
|
||||
List *rels_list, *rel_numparents;
|
||||
ListCell *l;
|
||||
|
||||
/*
|
||||
|
|
@ -168,11 +171,13 @@ find_all_inheritors(Oid parentrelId, LOCKMODE lockmode)
|
|||
* doesn't fetch the next list element until the bottom of the loop.
|
||||
*/
|
||||
rels_list = list_make1_oid(parentrelId);
|
||||
rel_numparents = list_make1_int(0);
|
||||
|
||||
foreach(l, rels_list)
|
||||
{
|
||||
Oid currentrel = lfirst_oid(l);
|
||||
List *currentchildren;
|
||||
ListCell *lc;
|
||||
|
||||
/* Get the direct children of this rel */
|
||||
currentchildren = find_inheritance_children(currentrel, lockmode);
|
||||
|
|
@ -184,9 +189,37 @@ find_all_inheritors(Oid parentrelId, LOCKMODE lockmode)
|
|||
* loop, though theoretically there can't be any cycles in the
|
||||
* inheritance graph anyway.)
|
||||
*/
|
||||
rels_list = list_concat_unique_oid(rels_list, currentchildren);
|
||||
foreach(lc, currentchildren)
|
||||
{
|
||||
Oid child_oid = lfirst_oid(lc);
|
||||
bool found = false;
|
||||
ListCell *lo;
|
||||
ListCell *li;
|
||||
|
||||
/* if the rel is already there, bump number-of-parents counter */
|
||||
forboth(lo, rels_list, li, rel_numparents)
|
||||
{
|
||||
if (lfirst_oid(lo) == child_oid)
|
||||
{
|
||||
lfirst_int(li)++;
|
||||
found = true;
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
/* if it's not there, add it. expect 1 parent, initially. */
|
||||
if (!found)
|
||||
{
|
||||
rels_list = lappend_oid(rels_list, child_oid);
|
||||
rel_numparents = lappend_int(rel_numparents, 1);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if (numparents)
|
||||
*numparents = rel_numparents;
|
||||
else
|
||||
list_free(rel_numparents);
|
||||
return rels_list;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -8,7 +8,7 @@
|
|||
*
|
||||
*
|
||||
* IDENTIFICATION
|
||||
* $PostgreSQL: pgsql/src/backend/commands/alter.c,v 1.33 2010/01/02 16:57:36 momjian Exp $
|
||||
* $PostgreSQL: pgsql/src/backend/commands/alter.c,v 1.34 2010/02/01 19:28:56 rhaas Exp $
|
||||
*
|
||||
*-------------------------------------------------------------------------
|
||||
*/
|
||||
|
|
@ -126,7 +126,7 @@ ExecRenameStmt(RenameStmt *stmt)
|
|||
stmt->subname, /* old att name */
|
||||
stmt->newname, /* new att name */
|
||||
interpretInhOption(stmt->relation->inhOpt), /* recursive? */
|
||||
false); /* recursing already? */
|
||||
0); /* expected inhcount */
|
||||
break;
|
||||
case OBJECT_TRIGGER:
|
||||
renametrig(relid,
|
||||
|
|
|
|||
|
|
@ -8,7 +8,7 @@
|
|||
*
|
||||
*
|
||||
* IDENTIFICATION
|
||||
* $PostgreSQL: pgsql/src/backend/commands/analyze.c,v 1.148 2010/01/22 16:40:18 rhaas Exp $
|
||||
* $PostgreSQL: pgsql/src/backend/commands/analyze.c,v 1.149 2010/02/01 19:28:56 rhaas Exp $
|
||||
*
|
||||
*-------------------------------------------------------------------------
|
||||
*/
|
||||
|
|
@ -1390,7 +1390,8 @@ acquire_inherited_sample_rows(Relation onerel, HeapTuple *rows, int targrows,
|
|||
* Find all members of inheritance set. We only need AccessShareLock on
|
||||
* the children.
|
||||
*/
|
||||
tableOIDs = find_all_inheritors(RelationGetRelid(onerel), AccessShareLock);
|
||||
tableOIDs =
|
||||
find_all_inheritors(RelationGetRelid(onerel), AccessShareLock, NULL);
|
||||
|
||||
/*
|
||||
* Check that there's at least one descendant, else fail. This could
|
||||
|
|
|
|||
|
|
@ -8,7 +8,7 @@
|
|||
*
|
||||
*
|
||||
* IDENTIFICATION
|
||||
* $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.320 2010/01/28 23:21:11 petere Exp $
|
||||
* $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.321 2010/02/01 19:28:56 rhaas Exp $
|
||||
*
|
||||
*-------------------------------------------------------------------------
|
||||
*/
|
||||
|
|
@ -824,7 +824,7 @@ ExecuteTruncate(TruncateStmt *stmt)
|
|||
ListCell *child;
|
||||
List *children;
|
||||
|
||||
children = find_all_inheritors(myrelid, AccessExclusiveLock);
|
||||
children = find_all_inheritors(myrelid, AccessExclusiveLock, NULL);
|
||||
|
||||
foreach(child, children)
|
||||
{
|
||||
|
|
@ -1943,7 +1943,7 @@ renameatt(Oid myrelid,
|
|||
const char *oldattname,
|
||||
const char *newattname,
|
||||
bool recurse,
|
||||
bool recursing)
|
||||
int expected_parents)
|
||||
{
|
||||
Relation targetrelation;
|
||||
Relation attrelation;
|
||||
|
|
@ -1987,24 +1987,31 @@ renameatt(Oid myrelid,
|
|||
*/
|
||||
if (recurse)
|
||||
{
|
||||
ListCell *child;
|
||||
List *children;
|
||||
List *child_oids, *child_numparents;
|
||||
ListCell *lo, *li;
|
||||
|
||||
children = find_all_inheritors(myrelid, AccessExclusiveLock);
|
||||
/*
|
||||
* we need the number of parents for each child so that the recursive
|
||||
* calls to renameatt() can determine whether there are any parents
|
||||
* outside the inheritance hierarchy being processed.
|
||||
*/
|
||||
child_oids = find_all_inheritors(myrelid, AccessExclusiveLock,
|
||||
&child_numparents);
|
||||
|
||||
/*
|
||||
* find_all_inheritors does the recursive search of the inheritance
|
||||
* hierarchy, so all we have to do is process all of the relids in the
|
||||
* list that it returns.
|
||||
*/
|
||||
foreach(child, children)
|
||||
forboth(lo, child_oids, li, child_numparents)
|
||||
{
|
||||
Oid childrelid = lfirst_oid(child);
|
||||
Oid childrelid = lfirst_oid(lo);
|
||||
int numparents = lfirst_int(li);
|
||||
|
||||
if (childrelid == myrelid)
|
||||
continue;
|
||||
/* note we need not recurse again */
|
||||
renameatt(childrelid, oldattname, newattname, false, true);
|
||||
renameatt(childrelid, oldattname, newattname, false, numparents);
|
||||
}
|
||||
}
|
||||
else
|
||||
|
|
@ -2012,8 +2019,10 @@ renameatt(Oid myrelid,
|
|||
/*
|
||||
* If we are told not to recurse, there had better not be any child
|
||||
* tables; else the rename would put them out of step.
|
||||
*
|
||||
* expected_parents will only be 0 if we are not already recursing.
|
||||
*/
|
||||
if (!recursing &&
|
||||
if (expected_parents == 0 &&
|
||||
find_inheritance_children(myrelid, NoLock) != NIL)
|
||||
ereport(ERROR,
|
||||
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
|
||||
|
|
@ -2039,10 +2048,15 @@ renameatt(Oid myrelid,
|
|||
oldattname)));
|
||||
|
||||
/*
|
||||
* if the attribute is inherited, forbid the renaming, unless we are
|
||||
* already inside a recursive rename.
|
||||
* if the attribute is inherited, forbid the renaming. if this is a
|
||||
* top-level call to renameatt(), then expected_parents will be 0, so the
|
||||
* effect of this code will be to prohibit the renaming if the attribute
|
||||
* is inherited at all. if this is a recursive call to renameatt(),
|
||||
* expected_parents will be the number of parents the current relation has
|
||||
* within the inheritance hierarchy being processed, so we'll prohibit
|
||||
* the renaming only if there are additional parents from elsewhere.
|
||||
*/
|
||||
if (attform->attinhcount > 0 && !recursing)
|
||||
if (attform->attinhcount > expected_parents)
|
||||
ereport(ERROR,
|
||||
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
|
||||
errmsg("cannot rename inherited column \"%s\"",
|
||||
|
|
@ -3410,7 +3424,7 @@ ATSimpleRecursion(List **wqueue, Relation rel,
|
|||
ListCell *child;
|
||||
List *children;
|
||||
|
||||
children = find_all_inheritors(relid, AccessExclusiveLock);
|
||||
children = find_all_inheritors(relid, AccessExclusiveLock, NULL);
|
||||
|
||||
/*
|
||||
* find_all_inheritors does the recursive search of the inheritance
|
||||
|
|
@ -7233,7 +7247,7 @@ ATExecAddInherit(Relation child_rel, RangeVar *parent)
|
|||
* We use weakest lock we can on child's children, namely AccessShareLock.
|
||||
*/
|
||||
children = find_all_inheritors(RelationGetRelid(child_rel),
|
||||
AccessShareLock);
|
||||
AccessShareLock, NULL);
|
||||
|
||||
if (list_member_oid(children, RelationGetRelid(parent_rel)))
|
||||
ereport(ERROR,
|
||||
|
|
|
|||
|
|
@ -22,7 +22,7 @@
|
|||
*
|
||||
*
|
||||
* IDENTIFICATION
|
||||
* $PostgreSQL: pgsql/src/backend/optimizer/prep/prepunion.c,v 1.179 2010/01/02 16:57:47 momjian Exp $
|
||||
* $PostgreSQL: pgsql/src/backend/optimizer/prep/prepunion.c,v 1.180 2010/02/01 19:28:56 rhaas Exp $
|
||||
*
|
||||
*-------------------------------------------------------------------------
|
||||
*/
|
||||
|
|
@ -1180,7 +1180,7 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
|
|||
lockmode = AccessShareLock;
|
||||
|
||||
/* Scan for all members of inheritance set, acquire needed locks */
|
||||
inhOIDs = find_all_inheritors(parentOID, lockmode);
|
||||
inhOIDs = find_all_inheritors(parentOID, lockmode, NULL);
|
||||
|
||||
/*
|
||||
* Check that there's at least one descendant, else treat as no-child
|
||||
|
|
|
|||
|
|
@ -7,7 +7,7 @@
|
|||
* Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group
|
||||
* Portions Copyright (c) 1994, Regents of the University of California
|
||||
*
|
||||
* $PostgreSQL: pgsql/src/include/catalog/pg_inherits_fn.h,v 1.2 2010/01/02 16:58:01 momjian Exp $
|
||||
* $PostgreSQL: pgsql/src/include/catalog/pg_inherits_fn.h,v 1.3 2010/02/01 19:28:56 rhaas Exp $
|
||||
*
|
||||
*-------------------------------------------------------------------------
|
||||
*/
|
||||
|
|
@ -18,7 +18,8 @@
|
|||
#include "storage/lock.h"
|
||||
|
||||
extern List *find_inheritance_children(Oid parentrelId, LOCKMODE lockmode);
|
||||
extern List *find_all_inheritors(Oid parentrelId, LOCKMODE lockmode);
|
||||
extern List *find_all_inheritors(Oid parentrelId, LOCKMODE lockmode,
|
||||
List **parents);
|
||||
extern bool has_subclass(Oid relationId);
|
||||
extern bool typeInheritsFrom(Oid subclassTypeId, Oid superclassTypeId);
|
||||
|
||||
|
|
|
|||
|
|
@ -7,7 +7,7 @@
|
|||
* Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group
|
||||
* Portions Copyright (c) 1994, Regents of the University of California
|
||||
*
|
||||
* $PostgreSQL: pgsql/src/include/commands/tablecmds.h,v 1.45 2010/01/02 16:58:03 momjian Exp $
|
||||
* $PostgreSQL: pgsql/src/include/commands/tablecmds.h,v 1.46 2010/02/01 19:28:56 rhaas Exp $
|
||||
*
|
||||
*-------------------------------------------------------------------------
|
||||
*/
|
||||
|
|
@ -43,7 +43,7 @@ extern void renameatt(Oid myrelid,
|
|||
const char *oldattname,
|
||||
const char *newattname,
|
||||
bool recurse,
|
||||
bool recursing);
|
||||
int expected_parents);
|
||||
|
||||
extern void RenameRelation(Oid myrelid,
|
||||
const char *newrelname,
|
||||
|
|
|
|||
|
|
@ -1053,3 +1053,97 @@ NOTICE: merging column "a" with inherited definition
|
|||
ERROR: column "a" has a storage parameter conflict
|
||||
DETAIL: MAIN versus EXTENDED
|
||||
DROP TABLE t1, t2, t3, t4, t12_storage, t12_comments, t1_inh, t13_inh, t13_like, t_all;
|
||||
-- Test for renaming in simple multiple inheritance
|
||||
CREATE TABLE t1 (a int, b int);
|
||||
CREATE TABLE s1 (b int, c int);
|
||||
CREATE TABLE ts (d int) INHERITS (t1, s1);
|
||||
NOTICE: merging multiple inherited definitions of column "b"
|
||||
ALTER TABLE t1 RENAME a TO aa;
|
||||
ALTER TABLE t1 RENAME b TO bb; -- to be failed
|
||||
ERROR: cannot rename inherited column "b"
|
||||
ALTER TABLE ts RENAME aa TO aaa; -- to be failed
|
||||
ERROR: cannot rename inherited column "aa"
|
||||
ALTER TABLE ts RENAME d TO dd;
|
||||
\d+ ts
|
||||
Table "public.ts"
|
||||
Column | Type | Modifiers | Storage | Description
|
||||
--------+---------+-----------+---------+-------------
|
||||
aa | integer | | plain |
|
||||
b | integer | | plain |
|
||||
c | integer | | plain |
|
||||
dd | integer | | plain |
|
||||
Inherits: t1,
|
||||
s1
|
||||
Has OIDs: no
|
||||
|
||||
DROP TABLE ts;
|
||||
-- Test for renaming in diamond inheritance
|
||||
CREATE TABLE t2 (x int) INHERITS (t1);
|
||||
CREATE TABLE t3 (y int) INHERITS (t1);
|
||||
CREATE TABLE t4 (z int) INHERITS (t2, t3);
|
||||
NOTICE: merging multiple inherited definitions of column "aa"
|
||||
NOTICE: merging multiple inherited definitions of column "b"
|
||||
ALTER TABLE t1 RENAME aa TO aaa;
|
||||
\d+ t4
|
||||
Table "public.t4"
|
||||
Column | Type | Modifiers | Storage | Description
|
||||
--------+---------+-----------+---------+-------------
|
||||
aaa | integer | | plain |
|
||||
b | integer | | plain |
|
||||
x | integer | | plain |
|
||||
y | integer | | plain |
|
||||
z | integer | | plain |
|
||||
Inherits: t2,
|
||||
t3
|
||||
Has OIDs: no
|
||||
|
||||
CREATE TABLE ts (d int) INHERITS (t2, s1);
|
||||
NOTICE: merging multiple inherited definitions of column "b"
|
||||
ALTER TABLE t1 RENAME aaa TO aaaa;
|
||||
ALTER TABLE t1 RENAME b TO bb; -- to be failed
|
||||
ERROR: cannot rename inherited column "b"
|
||||
\d+ ts
|
||||
Table "public.ts"
|
||||
Column | Type | Modifiers | Storage | Description
|
||||
--------+---------+-----------+---------+-------------
|
||||
aaaa | integer | | plain |
|
||||
b | integer | | plain |
|
||||
x | integer | | plain |
|
||||
c | integer | | plain |
|
||||
d | integer | | plain |
|
||||
Inherits: t2,
|
||||
s1
|
||||
Has OIDs: no
|
||||
|
||||
WITH RECURSIVE r AS (
|
||||
SELECT 't1'::regclass AS inhrelid
|
||||
UNION ALL
|
||||
SELECT c.inhrelid FROM pg_inherits c, r WHERE r.inhrelid = c.inhparent
|
||||
)
|
||||
SELECT a.attrelid::regclass, a.attname, a.attinhcount, e.expected
|
||||
FROM (SELECT inhrelid, count(*) AS expected FROM pg_inherits
|
||||
WHERE inhparent IN (SELECT inhrelid FROM r) GROUP BY inhrelid) e
|
||||
JOIN pg_attribute a ON e.inhrelid = a.attrelid WHERE NOT attislocal
|
||||
ORDER BY a.attrelid::regclass::text, a.attnum;
|
||||
attrelid | attname | attinhcount | expected
|
||||
----------+---------+-------------+----------
|
||||
t2 | aaaa | 1 | 1
|
||||
t2 | b | 1 | 1
|
||||
t3 | aaaa | 1 | 1
|
||||
t3 | b | 1 | 1
|
||||
t4 | aaaa | 2 | 2
|
||||
t4 | b | 2 | 2
|
||||
t4 | x | 1 | 2
|
||||
t4 | y | 1 | 2
|
||||
ts | aaaa | 1 | 1
|
||||
ts | b | 2 | 1
|
||||
ts | x | 1 | 1
|
||||
ts | c | 1 | 1
|
||||
(12 rows)
|
||||
|
||||
DROP TABLE t1, s1 CASCADE;
|
||||
NOTICE: drop cascades to 4 other objects
|
||||
DETAIL: drop cascades to table t2
|
||||
drop cascades to table ts
|
||||
drop cascades to table t3
|
||||
drop cascades to table t4
|
||||
|
|
|
|||
|
|
@ -334,3 +334,42 @@ CREATE TABLE inh_error1 () INHERITS (t1, t4);
|
|||
CREATE TABLE inh_error2 (LIKE t4 INCLUDING STORAGE) INHERITS (t1);
|
||||
|
||||
DROP TABLE t1, t2, t3, t4, t12_storage, t12_comments, t1_inh, t13_inh, t13_like, t_all;
|
||||
|
||||
-- Test for renaming in simple multiple inheritance
|
||||
CREATE TABLE t1 (a int, b int);
|
||||
CREATE TABLE s1 (b int, c int);
|
||||
CREATE TABLE ts (d int) INHERITS (t1, s1);
|
||||
|
||||
ALTER TABLE t1 RENAME a TO aa;
|
||||
ALTER TABLE t1 RENAME b TO bb; -- to be failed
|
||||
ALTER TABLE ts RENAME aa TO aaa; -- to be failed
|
||||
ALTER TABLE ts RENAME d TO dd;
|
||||
\d+ ts
|
||||
|
||||
DROP TABLE ts;
|
||||
|
||||
-- Test for renaming in diamond inheritance
|
||||
CREATE TABLE t2 (x int) INHERITS (t1);
|
||||
CREATE TABLE t3 (y int) INHERITS (t1);
|
||||
CREATE TABLE t4 (z int) INHERITS (t2, t3);
|
||||
|
||||
ALTER TABLE t1 RENAME aa TO aaa;
|
||||
\d+ t4
|
||||
|
||||
CREATE TABLE ts (d int) INHERITS (t2, s1);
|
||||
ALTER TABLE t1 RENAME aaa TO aaaa;
|
||||
ALTER TABLE t1 RENAME b TO bb; -- to be failed
|
||||
\d+ ts
|
||||
|
||||
WITH RECURSIVE r AS (
|
||||
SELECT 't1'::regclass AS inhrelid
|
||||
UNION ALL
|
||||
SELECT c.inhrelid FROM pg_inherits c, r WHERE r.inhrelid = c.inhparent
|
||||
)
|
||||
SELECT a.attrelid::regclass, a.attname, a.attinhcount, e.expected
|
||||
FROM (SELECT inhrelid, count(*) AS expected FROM pg_inherits
|
||||
WHERE inhparent IN (SELECT inhrelid FROM r) GROUP BY inhrelid) e
|
||||
JOIN pg_attribute a ON e.inhrelid = a.attrelid WHERE NOT attislocal
|
||||
ORDER BY a.attrelid::regclass::text, a.attnum;
|
||||
|
||||
DROP TABLE t1, s1 CASCADE;
|
||||
|
|
|
|||
Loading…
Reference in New Issue