2010/1/31 KaiGai Kohei <kai...@ak.jp.nec.com>:
> The attached patch modified find_all_inheritors() to return the list of
> expected inhcount, if List * pointer is given. And, it focuses on only
> the bugs in renameatt() case.
I have cleaned up and simplified this patch. Attached is the version
I intend to commit. Changes:
- make find_all_inheritors return the list of OIDs, as previously,
instead of using an out parameter
- undo some useless variable renamings
- undo some useless whitespace changes
- rework comments
- fix regression tests to avoid using the same alias twice in the same query
- add an ORDER BY clause to the regression tests so that they pass on my machine
- improve the names of some of the new variables
- instead of adding an additional argument to renameatt(), just
replace the existing 'bool recursing' with 'int expected_parents'.
This allows merging the two versions of the "cannot rename inherited
column" message together, which seems like a reasonably good idea to
me, though if someone has a better idea that's fine. I didn't think
the one additional word added to the message provided enough clarity
to make it worth creating another translatable string.
> Also, the ALTER COLUMN TYPE case should be also fixed in the 9.1 release
> (or 9.0.1?).
If the fix is something we could commit for 9.0.1, then we ought to do
it now before 9.0 is released. If you want to submit a follow-on
patch to address ALTER COLUMN TYPE once this is committed, then please
do so.
...Robert
diff --git a/src/backend/catalog/pg_inherits.c b/src/backend/catalog/pg_inherits.c
index e256f6f..b53f44d 100644
--- a/src/backend/catalog/pg_inherits.c
+++ b/src/backend/catalog/pg_inherits.c
@@ -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 **parents)
{
- List *rels_list;
+ List *rels_list, *rel_parents;
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_parents = 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,35 @@ 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;
+
+ forboth(lo, rels_list, li, rel_parents)
+ {
+ if (lfirst_oid(lo) == child_oid)
+ {
+ lfirst_int(li)++;
+ found = true;
+ break;
+ }
+ }
+
+ if (!found)
+ {
+ rels_list = lappend_oid(rels_list, child_oid);
+ rel_parents = lappend_int(rel_parents, 1);
+ }
+ }
}
+ if (parents)
+ *parents = rel_parents;
+ else
+ list_free(rel_parents);
return rels_list;
}
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index 8c81eaa..efbd088 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -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,
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 9062c15..c93b57f 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -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
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 50e4244..52f32fc 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -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_parents;
+ 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_parents);
/*
* 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_parents)
{
- 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,
diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c
index c7e9c85..50a6ab7 100644
--- a/src/backend/optimizer/prep/prepunion.c
+++ b/src/backend/optimizer/prep/prepunion.c
@@ -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
diff --git a/src/include/catalog/pg_inherits_fn.h b/src/include/catalog/pg_inherits_fn.h
index 91baec3..70a624b 100644
--- a/src/include/catalog/pg_inherits_fn.h
+++ b/src/include/catalog/pg_inherits_fn.h
@@ -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);
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index 8bc716a..f9269cc 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -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,
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 9c83a32..dc54e56 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -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, 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
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index 192e860..bbd5752 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -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, a.attnum;
+
+DROP TABLE t1, s1 CASCADE;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
