pgsql-fix-inherit-rename.6.patch

*** 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;