pgsql-fix-inherit-rename.5.patch

 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;