src/backend/catalog/pg_inherits.c | 85 ++++++++++++++++++++++++ src/backend/commands/alter.c | 1 + src/backend/commands/tablecmds.c | 114 +++++++++++++++++++++++++++------ src/include/catalog/pg_inherits_fn.h | 4 + src/include/commands/tablecmds.h | 1 + src/test/regress/expected/inherit.out | 78 ++++++++++++++++++++++ src/test/regress/sql/inherit.sql | 36 ++++++++++ 7 files changed, 299 insertions(+), 20 deletions(-) diff --git a/src/backend/catalog/pg_inherits.c b/src/backend/catalog/pg_inherits.c index e256f6f..4fb8a25 100644 --- a/src/backend/catalog/pg_inherits.c +++ b/src/backend/catalog/pg_inherits.c @@ -190,6 +190,91 @@ find_all_inheritors(Oid parentrelId, LOCKMODE lockmode) return rels_list; } +/* + * find_all_inheritors_with_inhcount + * + * It is same as find_all_inheritors, except for it returns an excepted + * inhcount for each child relations. + * The expected inhcount means the number of times a relation is inherited + * from a certain origin. It shall be 1 in normal inheritance tree cases. + * However, if a relation has 'diamond-inheritance' structure, it may be + * more than 1. + * + * T2 + * / \ + * T1 T4 + * \ / + * T3-T5 + * / + * S1 + * + * In this case, T2 and T3 are inherited from T1, and T4 is inherited from + * T2 and T3. In T4, all the columns inherited from T1 are merged. + * The expected of T1, T2 and T3 are 1, but 2 for T4. The pg_attribute.inhcount + * of T4 will be 2 (if no corruption or bugs). It is not over the expected + * inhcount, so we can alter the column inherited from T1. + * + * If T5 is inherited from T3 and S1, the pg_attribute.inhcount of T5 will + * be 2. However, its expected inhcount when we try to alter T1 and its child + * relation is 1, because it does not have any diamond-inheritance from the + * series of T1. In this case, its inhcount will over the expected one, so + * we can prevent incorrect ALTER TABLE. + */ +void +find_all_inheritors_with_inhcount(Oid parentrelId, LOCKMODE lockmode, + List **child_oids, List **child_inhs) +{ + List *children, *rels_oids, *rels_inhs; + ListCell *lc, *lo, *li; + + /* + * The root of the inheritance tree always has 1 as an expected + * inhcount. + */ + rels_oids = list_make1_oid(parentrelId); + rels_inhs = list_make1_int(1); + + children = find_inheritance_children(parentrelId, lockmode); + foreach (lc, children) + { + Oid cur_relid = lfirst_oid(lc); + List *cur_children; + bool found = false; + + forboth (lo, rels_oids, li, rels_inhs) + { + /* + * If we found a duplicated relation within a certain inheritance + * tree, it means a diamond-inheritance. So, we increment its + * expected inhcount without appending an element to the list. + */ + if (lfirst_oid(lo) == cur_relid) + { + lfirst_int(li)++; + found = true; + break; + } + } + + if (!found) + { + rels_oids = lappend_oid(rels_oids, cur_relid); + rels_inhs = lappend_int(rels_inhs, 1); + } + + /* + * Unlike find_all_inheritors(), we need to walk on child relations + * that have diamond inheritance tree, because this function has to + * return correct expected inhecount to the caller. + */ + cur_children = find_inheritance_children(cur_relid, lockmode); + + children = list_concat(children, cur_children); + } + + *child_oids = rels_oids; + *child_inhs = rels_inhs; +} /* * has_subclass - does this relation have any children? diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c index 8c81eaa..fa3279a 100644 --- a/src/backend/commands/alter.c +++ b/src/backend/commands/alter.c @@ -125,6 +125,7 @@ ExecRenameStmt(RenameStmt *stmt) renameatt(relid, stmt->subname, /* old att name */ stmt->newname, /* new att name */ + 1, /* expected inhcount */ interpretInhOption(stmt->relation->inhOpt), /* recursive? */ false); /* recursing already? */ break; diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 2db97dd..60d7687 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -1906,6 +1906,7 @@ void renameatt(Oid myrelid, const char *oldattname, const char *newattname, + int expected_inhcount, bool recurse, bool recursing) { @@ -1946,24 +1947,32 @@ renameatt(Oid myrelid, */ if (recurse) { - ListCell *child; - List *children; - - children = find_all_inheritors(myrelid, AccessExclusiveLock); + List *child_oids, *child_inhs; + ListCell *lo, *li; /* - * 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. + * 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. */ - foreach(child, children) + find_all_inheritors_with_inhcount(myrelid, AccessExclusiveLock, + &child_oids, &child_inhs); + + forboth (lo, child_oids, li, child_inhs) { - Oid childrelid = lfirst_oid(child); + Oid child_relid = lfirst_oid(lo); + int child_inhcount = lfirst_int(li); - if (childrelid == myrelid) + if (child_relid == myrelid) continue; + /* note we need not recurse again */ - renameatt(childrelid, oldattname, newattname, false, true); + renameatt(child_relid, oldattname, newattname, + child_inhcount, false, true); } } else @@ -2007,6 +2016,17 @@ renameatt(Oid myrelid, 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 */ @@ -5771,11 +5791,29 @@ 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))); + 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); @@ -5857,12 +5895,48 @@ 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. + * 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) - ATSimpleRecursion(wqueue, rel, cmd, 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, diff --git a/src/include/catalog/pg_inherits_fn.h b/src/include/catalog/pg_inherits_fn.h index 91baec3..e750fcd 100644 --- a/src/include/catalog/pg_inherits_fn.h +++ b/src/include/catalog/pg_inherits_fn.h @@ -19,6 +19,10 @@ 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); diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index 8bc716a..7f90306 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -42,6 +42,7 @@ extern void ExecuteTruncate(TruncateStmt *stmt); extern void renameatt(Oid myrelid, const char *oldattname, const char *newattname, + int expected_inhcount, bool recurse, bool recursing); diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out index 9c83a32..8cb3db8 100644 --- a/src/test/regress/expected/inherit.out +++ b/src/test/regress/expected/inherit.out @@ -1053,3 +1053,81 @@ 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 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 + +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..76fe589 100644 --- a/src/test/regress/sql/inherit.sql +++ b/src/test/regress/sql/inherit.sql @@ -334,3 +334,39 @@ 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 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 + +DROP TABLE t1, s1 CASCADE;