*** a/src/backend/catalog/pg_inherits.c --- b/src/backend/catalog/pg_inherits.c *************** *** 158,164 **** List * find_all_inheritors(Oid parentrelId, LOCKMODE lockmode) { List *rels_list; ! ListCell *l; /* * We build a list starting with the given rel and adding all direct and --- 158,212 ---- find_all_inheritors(Oid parentrelId, LOCKMODE lockmode) { List *rels_list; ! ! find_all_inheritors_with_inhcount(parentrelId, lockmode, ! &rels_list, NULL); ! ! return rels_list; ! } ! ! /* ! * find_all_inheritors_with_inhcount ! * ! * It returns a list of relations OIDs including the given rel plus ! * all relations that inherit from it, directly or indirectly. ! * In addition, it also returns the expected inhcount for them. ! * ! * The expected inhcount means the number of times a certain relation being ! * inherited from any other relations which were inherited from the root ! * of inheritance tree directly or indirectly. ! * ! * It is used to control some of ALTER TABLE operations on columns inherited ! * from a relation. For example, an inherited column must has same name ! * within the series of inheritance. So, we need to prevent renaming when ! * the column to be renamed is delivered from multiple independent relations. ! * ! * 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 ! * then no locks are acquired, but caller must beware of race conditions ! * against possible DROPs of child relations. ! * ! * T2 For example, T2 and T3 are inherited from T1, T4 is inherited ! * / \ from T2 and T3. And, T5 is inherited from T3 and S1. ! * T1 T4 When we try to rename a column within T1, it needs to rename ! * \ / inherited column in the child relations also. ! * T3-T5 If attinhcount of the column is larger than its expected inhcount, ! * / it means the column is delivered from the different origin, so ! * S1 we have to prevent this operations to keep database integrity. ! * ! * In this example, the expected inhcount is 1 in T2, T3 and T5, and 2 in T4. ! * The column inherited from T1 has attinhcount = 2 in T4, so it does not ! * prevent renaming. ! * ! * If the column inherited from T1 has attinhcount = 2 in T5, it means the ! * column was also inherited from S1, so we can prevent renaming it. ! */ ! void ! find_all_inheritors_with_inhcount(Oid parentrelId, LOCKMODE lockmode, ! List **child_oids, List **child_inhs) ! { ! List *rels_oids, *rels_inhs; ! ListCell *cell; /* * We build a list starting with the given rel and adding all direct and *************** *** 166,196 **** find_all_inheritors(Oid parentrelId, LOCKMODE lockmode) * already-found rels and the agenda of rels yet to be scanned for more * children. This is a bit tricky but works because the foreach() macro * doesn't fetch the next list element until the bottom of the loop. */ ! rels_list = list_make1_oid(parentrelId); ! foreach(l, rels_list) { ! Oid currentrel = lfirst_oid(l); ! List *currentchildren; /* Get the direct children of this rel */ ! currentchildren = find_inheritance_children(currentrel, lockmode); ! /* ! * Add to the queue only those children not already seen. This avoids ! * making duplicate entries in case of multiple inheritance paths from ! * the same parent. (It'll also keep us from getting into an infinite ! * loop, though theoretically there can't be any cycles in the ! * inheritance graph anyway.) ! */ ! rels_list = list_concat_unique_oid(rels_list, currentchildren); } ! return rels_list; } - /* * has_subclass - does this relation have any children? * --- 214,274 ---- * already-found rels and the agenda of rels yet to be scanned for more * children. This is a bit tricky but works because the foreach() macro * doesn't fetch the next list element until the bottom of the loop. + * + * The root of the inheritance tree always has 0 as an expected inhcount, + * because its parent, even if exists, it is never contained within the + * inheritance tree originally delivered from the root. */ ! rels_oids = list_make1_oid(parentrelId); ! rels_inhs = list_make1_int(0); ! foreach (cell, rels_oids) { ! Oid cur_relid = lfirst_oid(cell); ! Oid temp_relid; ! List *temp; ! ListCell *lt, *lo, *li; /* Get the direct children of this rel */ ! temp = find_inheritance_children(cur_relid, lockmode); ! foreach (lt, temp) ! { ! Oid temp_relid = lfirst_oid(lt); ! bool found = false; ! ! /* ! * Add to the queue only those children not already seen. ! * This avoids making duplicate entries in case of multiple ! * inheritance paths from the same parent. (It'll also keep ! * us from getting into an infinite loop, though theoretically ! * there can't be any cycles in the inheritance graph anyway.) ! */ ! forboth (lo, rels_oids, li, rels_inhs) ! { ! if (lfirst_oid(lo) == temp_relid) ! { ! /* increment its expected inhcount */ ! lfirst_int(li)++; ! found = true; ! break; ! } ! } ! ! if (!found) ! { ! rels_oids = lappend_oid(rels_oids, temp_relid); ! rels_inhs = lappend_int(rels_inhs, 1); ! } ! } } ! if (child_oids) ! *child_oids = rels_oids; ! if (child_inhs) ! *child_inhs = rels_inhs; } /* * has_subclass - does this relation have any children? * *** a/src/backend/commands/alter.c --- b/src/backend/commands/alter.c *************** *** 125,130 **** ExecRenameStmt(RenameStmt *stmt) --- 125,131 ---- renameatt(relid, stmt->subname, /* old att name */ stmt->newname, /* new att name */ + 0, /* expected inhcount */ interpretInhOption(stmt->relation->inhOpt), /* recursive? */ false); /* recursing already? */ break; *** a/src/backend/commands/tablecmds.c --- b/src/backend/commands/tablecmds.c *************** *** 1906,1911 **** void --- 1906,1912 ---- renameatt(Oid myrelid, const char *oldattname, const char *newattname, + int expected_inhcount, bool recurse, bool recursing) { *************** *** 1946,1969 **** renameatt(Oid myrelid, */ if (recurse) { ! ListCell *child; ! List *children; ! ! children = find_all_inheritors(myrelid, AccessExclusiveLock); /* ! * 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) { ! Oid childrelid = lfirst_oid(child); ! if (childrelid == myrelid) continue; /* note we need not recurse again */ ! renameatt(childrelid, oldattname, newattname, false, true); } } else --- 1947,1978 ---- */ if (recurse) { ! List *child_oids, *child_inhs; ! ListCell *lo, *li; /* ! * It is same as find_all_inheritors, except for it returns the ! * list of expected inhcount for each child relations. ! * It is a value to be set on pg_attribute.inhcount when the column ! * to be renamed is not merged with any other column which have ! * different origin. ! * If pg_attribute.inhcount is larger than the expected inhcount, ! * it means the column is merged from different serieses. */ ! find_all_inheritors_with_inhcount(myrelid, AccessExclusiveLock, ! &child_oids, &child_inhs); ! ! forboth (lo, child_oids, li, child_inhs) { ! Oid child_relid = lfirst_oid(lo); ! int child_inhcount = lfirst_int(li); ! if (child_relid == myrelid) continue; + /* note we need not recurse again */ ! renameatt(child_relid, oldattname, newattname, ! child_inhcount, false, true); } } else *************** *** 2007,2012 **** renameatt(Oid myrelid, --- 2016,2032 ---- errmsg("cannot rename inherited column \"%s\"", oldattname))); + /* + * if the attribute is multiple inherited, forbid the renaming, + * even if we are already inside a recursive rename, because we + * have no reasonable way to keep its integrity. + */ + if (attform->attinhcount > expected_inhcount) + ereport(ERROR, + (errcode(ERRCODE_INVALID_TABLE_DEFINITION), + (errmsg("cannot rename multiple inherited column \"%s\"", + oldattname)))); + /* new name should not already exist */ /* this test is deliberately not attisdropped-aware */ *************** *** 5772,5782 **** ATPrepAlterColumnType(List **wqueue, colName))); /* Don't alter inherited columns */ ! if (attTup->attinhcount > 0 && !recursing) ! ereport(ERROR, ! (errcode(ERRCODE_INVALID_TABLE_DEFINITION), ! errmsg("cannot alter inherited column \"%s\"", ! colName))); /* Look up the target type */ targettype = typenameTypeId(NULL, typeName, &targettypmod); --- 5792,5820 ---- colName))); /* Don't alter inherited columns */ ! if (!recursing) ! { ! if (attTup->attinhcount > 0) ! ereport(ERROR, ! (errcode(ERRCODE_INVALID_TABLE_DEFINITION), ! errmsg("cannot alter inherited column \"%s\"", ! colName))); ! } ! else ! { ! /* ! * The TypeName->location is not used in this code path, ! * so we utilize it to store an enpected inhconunt for each ! * child relations. ! */ ! int expected_inhcount = typeName->location; ! ! if (attTup->attinhcount > expected_inhcount) ! ereport(ERROR, ! (errcode(ERRCODE_INVALID_TABLE_DEFINITION), ! errmsg("cannot alter multiple inherited column \"%s\"", ! colName))); ! } /* Look up the target type */ targettype = typenameTypeId(NULL, typeName, &targettypmod); *************** *** 5858,5869 **** ATPrepAlterColumnType(List **wqueue, ReleaseSysCache(tuple); /* ! * The recursion case is handled by ATSimpleRecursion. However, if we are ! * told not to recurse, there had better not be any child tables; else the ! * alter would put them out of step. */ if (recurse) ! ATSimpleRecursion(wqueue, rel, cmd, recurse); else if (!recursing && find_inheritance_children(RelationGetRelid(rel), NoLock) != NIL) ereport(ERROR, --- 5896,5943 ---- ReleaseSysCache(tuple); /* ! * The recursion case is not handled by ATSimpleRecursion, because we need ! * to compute an expected inhcount for each child relations. This value is ! * delivered to the recursive invocation to check unexpected type changes. ! * If we are told not to recurse, there had better not be any child tables; ! * else the alter would put them out of step. */ if (recurse) ! { ! Relation child_rel; ! List *child_oids; ! List *child_inhs; ! ListCell *lo; ! ListCell *li; ! ! find_all_inheritors_with_inhcount(RelationGetRelid(rel), ! AccessExclusiveLock, ! &child_oids, &child_inhs); ! forboth(lo, child_oids, li, child_inhs) ! { ! AlterTableCmd *child_cmd; ! Relation child_rel; ! Oid child_relid = lfirst_oid(lo); ! int child_inhcount = lfirst_int(li); ! ! if (child_relid == RelationGetRelid(rel)) ! continue; ! ! /* ! * The TypeName->location is not used in this code path, ! * so we utilize it to store an expected inhcount for each ! * child relation. ! */ ! child_cmd = copyObject(cmd); ! ((TypeName *)child_cmd->def)->location = child_inhcount; ! ! /* find_all_inheritors_with_inhcount already got lock */ ! child_rel = relation_open(child_relid, NoLock); ! CheckTableNotInUse(child_rel, "ALTER TABLE"); ! ATPrepCmd(wqueue, child_rel, child_cmd, false, true); ! relation_close(child_rel, NoLock); ! } ! } else if (!recursing && find_inheritance_children(RelationGetRelid(rel), NoLock) != NIL) ereport(ERROR, *** a/src/include/catalog/pg_inherits_fn.h --- b/src/include/catalog/pg_inherits_fn.h *************** *** 19,24 **** --- 19,28 ---- extern List *find_inheritance_children(Oid parentrelId, LOCKMODE lockmode); extern List *find_all_inheritors(Oid parentrelId, LOCKMODE lockmode); + extern void find_all_inheritors_with_inhcount(Oid parentrelId, + LOCKMODE lockmode, + List **child_oids, + List **child_inhs); extern bool has_subclass(Oid relationId); extern bool typeInheritsFrom(Oid subclassTypeId, Oid superclassTypeId); *** a/src/include/commands/tablecmds.h --- b/src/include/commands/tablecmds.h *************** *** 42,47 **** extern void ExecuteTruncate(TruncateStmt *stmt); --- 42,48 ---- extern void renameatt(Oid myrelid, const char *oldattname, const char *newattname, + int expected_inhcount, bool recurse, bool recursing); *** a/src/test/regress/expected/inherit.out --- b/src/test/regress/expected/inherit.out *************** *** 1053,1055 **** NOTICE: merging column "a" with inherited definition --- 1053,1158 ---- 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 and type changes (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 s1 ALTER b TYPE float; -- to be failed + ERROR: cannot alter multiple inherited column "b" + ALTER TABLE s1 ALTER c TYPE float; + ALTER TABLE ts ALTER c TYPE text; -- to be failed + ERROR: cannot alter inherited column "c" + ALTER TABLE ts ALTER d TYPE text; + ALTER TABLE t1 RENAME a TO aa; + ALTER TABLE t1 RENAME b TO bb; -- to be failed + ERROR: cannot rename multiple 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 | double precision | | plain | + dd | text | | extended | + Inherits: t1, + s1 + Has OIDs: no + + DROP TABLE ts; + -- Test for renaming and type changes (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 ALTER aa TYPE float; + ALTER TABLE t1 RENAME aa TO aaa; + \d+ t4 + Table "public.t4" + Column | Type | Modifiers | Storage | Description + --------+------------------+-----------+---------+------------- + aaa | double precision | | 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 ALTER aaa TYPE text; + ALTER TABLE t1 ALTER b TYPE text; -- to be failed + ERROR: cannot alter multiple inherited column "b" + ALTER TABLE t1 RENAME aaa TO aaaa; + ALTER TABLE t1 RENAME b TO bb; -- to be failed + ERROR: cannot rename multiple inherited column "b" + \d+ ts + Table "public.ts" + Column | Type | Modifiers | Storage | Description + --------+------------------+-----------+----------+------------- + aaaa | text | | extended | + b | integer | | plain | + x | integer | | plain | + c | double precision | | 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 attrelid::regclass, attname, attinhcount, r.expected + FROM (SELECT inhrelid, count(*) AS expected FROM pg_inherits + WHERE inhparent IN (SELECT inhrelid FROM r) GROUP BY inhrelid) r + JOIN pg_attribute a ON r.inhrelid = a.attrelid WHERE attislocal = false; + attrelid | attname | attinhcount | expected + ----------+---------+-------------+---------- + t2 | b | 1 | 1 + t3 | b | 1 | 1 + t4 | b | 2 | 2 + t4 | x | 1 | 2 + t4 | y | 1 | 2 + t2 | aaaa | 1 | 1 + t3 | aaaa | 1 | 1 + t4 | aaaa | 2 | 2 + ts | b | 2 | 1 + ts | x | 1 | 1 + ts | c | 1 | 1 + ts | aaaa | 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 *** a/src/test/regress/sql/inherit.sql --- b/src/test/regress/sql/inherit.sql *************** *** 334,336 **** CREATE TABLE inh_error1 () INHERITS (t1, t4); --- 334,382 ---- 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 and type changes (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 s1 ALTER b TYPE float; -- to be failed + ALTER TABLE s1 ALTER c TYPE float; + ALTER TABLE ts ALTER c TYPE text; -- to be failed + ALTER TABLE ts ALTER d TYPE text; + + 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 and type changes (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 ALTER aa TYPE float; + ALTER TABLE t1 RENAME aa TO aaa; + \d+ t4 + + CREATE TABLE ts (d int) INHERITS (t2, s1); + ALTER TABLE t1 ALTER aaa TYPE text; + ALTER TABLE t1 ALTER b TYPE text; -- to be failed + 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 attrelid::regclass, attname, attinhcount, r.expected + FROM (SELECT inhrelid, count(*) AS expected FROM pg_inherits + WHERE inhparent IN (SELECT inhrelid FROM r) GROUP BY inhrelid) r + JOIN pg_attribute a ON r.inhrelid = a.attrelid WHERE attislocal = false; + + DROP TABLE t1, s1 CASCADE;