Disallow changing an inherited column's type if not all parents changed.

If a table inherits from multiple unrelated parents, we must disallow
changing the type of a column inherited from multiple such parents, else
it would be out of step with the other parents.  However, it's possible
for the column to ultimately be inherited from just one common ancestor,
in which case a change starting from that ancestor should still be
allowed.  (I would not be excited about preserving that option, were
it not that we have regression test cases exercising it already ...)

It's slightly annoying that this patch looks different from the logic
with the same end goal in renameatt(), and more annoying that it
requires an extra syscache lookup to make the test.  However, the
recursion logic is quite different in the two functions, and a
back-patched bug fix is no place to be trying to unify them.

Per report from Manuel Rigger.  Back-patch to 9.5.  The bug exists in
9.4 too (and doubtless much further back); but the way the recursion
is done in 9.4 is a good bit different, so that substantial refactoring
would be needed to fix it in 9.4.  I'm disinclined to do that, or risk
introducing new bugs, for a bug that has escaped notice for this long.

Discussion: https://postgr.es/m/CA+u7OA4qogDv9rz1HAb-ADxttXYPqQdUdPY_yd4kCzywNxRQXA@mail.gmail.com

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8a8095c8a00142b453ebfddfe936091fd1de6b29..b8d2e321014a08e840521e84ffb38db7e11c9973 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7937,7 +7937,11 @@ ATPrepAlterColumnType(List **wqueue,
                                 errmsg("cannot alter system column \"%s\"",
                                                colName)));
 
-       /* Don't alter inherited columns */
+       /*
+        * Don't alter inherited columns.  At outer level, there had better not be
+        * any inherited definition; when recursing, we assume this was checked at
+        * the parent level (see below).
+        */
        if (attTup->attinhcount > 0 && !recursing)
                ereport(ERROR,
                                (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
@@ -8053,20 +8057,26 @@ ATPrepAlterColumnType(List **wqueue,
        if (recurse)
        {
                Oid                     relid = RelationGetRelid(rel);
-               ListCell   *child;
-               List       *children;
+               List       *child_oids,
+                                  *child_numparents;
+               ListCell   *lo,
+                                  *li;
 
-               children = find_all_inheritors(relid, lockmode, NULL);
+               child_oids = find_all_inheritors(relid, lockmode,
+                                                                                &child_numparents);
 
                /*
                 * 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_numparents)
                {
-                       Oid                     childrelid = lfirst_oid(child);
+                       Oid                     childrelid = lfirst_oid(lo);
+                       int                     numparents = lfirst_int(li);
                        Relation        childrel;
+                       HeapTuple       childtuple;
+                       Form_pg_attribute childattTup;
 
                        if (childrelid == relid)
                                continue;
@@ -8075,6 +8085,29 @@ ATPrepAlterColumnType(List **wqueue,
                        childrel = relation_open(childrelid, NoLock);
                        CheckTableNotInUse(childrel, "ALTER TABLE");
 
+                       /*
+                        * Verify that the child doesn't have any inherited definitions of
+                        * this column that came from outside this inheritance hierarchy.
+                        * (renameatt makes a similar test, though in a different way
+                        * because of its different recursion mechanism.)
+                        */
+                       childtuple = SearchSysCacheAttName(RelationGetRelid(childrel),
+                                                                                          colName);
+                       if (!HeapTupleIsValid(childtuple))
+                               ereport(ERROR,
+                                               (errcode(ERRCODE_UNDEFINED_COLUMN),
+                                                errmsg("column \"%s\" of relation \"%s\" does not exist",
+                                                               colName, RelationGetRelationName(childrel))));
+                       childattTup = (Form_pg_attribute) GETSTRUCT(childtuple);
+
+                       if (childattTup->attinhcount > numparents)
+                               ereport(ERROR,
+                                               (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+                                                errmsg("cannot alter inherited column \"%s\" of relation \"%s\"",
+                                                               colName, RelationGetRelationName(childrel))));
+
+                       ReleaseSysCache(childtuple);
+
                        /*
                         * Remap the attribute numbers.  If no USING expression was
                         * specified, there is no need for this step.

diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 550842fbae2e635bdba6fce17f899f8b9b9b9e72..ac80b9a6ceca61a8ef69445c3b356a5707ad1197 100644 (file)
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -653,6 +653,16 @@ select * from d;
  32 | one | two | three
 (1 row)
 
+-- The above verified that we can change the type of a multiply-inherited
+-- column; but we should reject that if any definition was inherited from
+-- an unrelated parent.
+create temp table parent1(f1 int, f2 int);
+create temp table parent2(f1 int, f3 bigint);
+create temp table childtab(f4 int) inherits(parent1, parent2);
+NOTICE:  merging multiple inherited definitions of column "f1"
+alter table parent1 alter column f1 type bigint;  -- fail, conflict w/parent2
+ERROR:  cannot alter inherited column "f1" of relation "childtab"
+alter table parent1 alter column f2 type bigint;  -- ok
 -- check that oid column is handled properly during alter table inherit
 create table oid_parent (a int) with oids;
 create table oid_child () inherits (oid_parent);

diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index 6bfd333f9164e77c36454a79ca6a8b8688a7d63a..cac7a3f84e7f891bc8ef4acff45e7c00eb1d7d66 100644 (file)
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -160,6 +160,15 @@ insert into d values('test','one','two','three');
 alter table a alter column aa type integer using bit_length(aa);
 select * from d;
 
+-- The above verified that we can change the type of a multiply-inherited
+-- column; but we should reject that if any definition was inherited from
+-- an unrelated parent.
+create temp table parent1(f1 int, f2 int);
+create temp table parent2(f1 int, f3 bigint);
+create temp table childtab(f4 int) inherits(parent1, parent2);
+alter table parent1 alter column f1 type bigint;  -- fail, conflict w/parent2
+alter table parent1 alter column f2 type bigint;  -- ok
+
 -- check that oid column is handled properly during alter table inherit
 create table oid_parent (a int) with oids;