Re: BUG #17351: Altering a composite type created for a partitioned table can lead to a crash - Mailing list pgsql-bugs

I wrote:
> PG Bug reporting form <noreply@postgresql.org> writes:
>> When executing the following queries:
>> create table pt (a int, b int) partition by list (b);
>> create table t(a pt, check (a = '(1, 2)'::pt));
>> alter table pt alter column a type char(4);
>> \d+ t
>> The server crashes with the following stack:

> Hmm.  We really ought to reject the ALTER TABLE.  We do if "pt"
> is a plain table:

So the problem is that ATRewriteTables is supposed to make this check
(by calling find_composite_type_dependencies), but first it does

        /* Relations without storage may be ignored here */
        if (!RELKIND_HAS_STORAGE(tab->relkind))
            continue;

so the test is skipped for a partitioned table.  There is a separate
check in ATPrepAlterColumnType:

    if (tab->relkind == RELKIND_COMPOSITE_TYPE ||
        tab->relkind == RELKIND_FOREIGN_TABLE)
    {
        /*
         * For composite types and foreign tables, do this check now.  Regular
         * tables will check it later when the table is being rewritten.
         */
        find_composite_type_dependencies(rel->rd_rel->reltype, rel, NULL);
    }

The best fix seems to be to make that check use the inverse condition.
I also experimented with moving the "Relations without storage" early
exit down past the find_composite_type_dependencies step, in hopes of
getting rid of the check in ATPrepAlterColumnType.  But that fails to
cover all cases, because we might not set the rewrite flag.

> I think we're also failing to worry about the rowtype of the
> constant in t's check constraint; it seems like that has to
> be complained of as well, even if the underyling columns
> aren't of type "pt".

This seems to be all right, but I thought it'd be prudent to add
a test case covering it.

            regards, tom lane

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 89bc865e28..23364d3c7e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -12183,12 +12183,11 @@ ATPrepAlterColumnType(List **wqueue,
                  errmsg("\"%s\" is not a table",
                         RelationGetRelationName(rel))));

-    if (tab->relkind == RELKIND_COMPOSITE_TYPE ||
-        tab->relkind == RELKIND_FOREIGN_TABLE)
+    if (!RELKIND_HAS_STORAGE(tab->relkind))
     {
         /*
-         * For composite types and foreign tables, do this check now.  Regular
-         * tables will check it later when the table is being rewritten.
+         * For relations without storage, do this check now.  Regular tables
+         * will check it later when the table is being rewritten.
          */
         find_composite_type_dependencies(rel->rd_rel->reltype, rel, NULL);
     }
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 24d1c7cd28..16e0475663 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2061,11 +2061,24 @@ begin;
 create table skip_wal_skip_rewrite_index (c varchar(10) primary key);
 alter table skip_wal_skip_rewrite_index alter c type varchar(20);
 commit;
--- table's row type
-create table tab1 (a int, b text);
-create table tab2 (x int, y tab1);
-alter table tab1 alter column b type varchar; -- fails
-ERROR:  cannot alter table "tab1" because column "tab2.y" uses its row type
+-- We disallow changing table's row type if it's used for storage
+create table at_tab1 (a int, b text);
+create table at_tab2 (x int, y at_tab1);
+alter table at_tab1 alter column b type varchar; -- fails
+ERROR:  cannot alter table "at_tab1" because column "at_tab2.y" uses its row type
+drop table at_tab2;
+-- Use of row type in an expression is defended differently
+create table at_tab2 (x int, y text, check((x,y)::at_tab1 = (1,'42')::at_tab1));
+alter table at_tab1 alter column b type varchar; -- allowed, but ...
+insert into at_tab2 values(1,'42'); -- ... this will fail
+ERROR:  ROW() column has type text instead of type character varying
+drop table at_tab1, at_tab2;
+-- Check it for a partitioned table, too
+create table at_tab1 (a int, b text) partition by list(a);
+create table at_tab2 (x int, y at_tab1);
+alter table at_tab1 alter column b type varchar; -- fails
+ERROR:  cannot alter table "at_tab1" because column "at_tab2.y" uses its row type
+drop table at_tab1, at_tab2;
 -- Alter column type that's part of a partitioned index
 create table at_partitioned (a int, b text) partition by range (a);
 create table at_part_1 partition of at_partitioned for values from (0) to (1000);
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 5fac2585d9..ac894c0602 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1420,10 +1420,21 @@ create table skip_wal_skip_rewrite_index (c varchar(10) primary key);
 alter table skip_wal_skip_rewrite_index alter c type varchar(20);
 commit;

--- table's row type
-create table tab1 (a int, b text);
-create table tab2 (x int, y tab1);
-alter table tab1 alter column b type varchar; -- fails
+-- We disallow changing table's row type if it's used for storage
+create table at_tab1 (a int, b text);
+create table at_tab2 (x int, y at_tab1);
+alter table at_tab1 alter column b type varchar; -- fails
+drop table at_tab2;
+-- Use of row type in an expression is defended differently
+create table at_tab2 (x int, y text, check((x,y)::at_tab1 = (1,'42')::at_tab1));
+alter table at_tab1 alter column b type varchar; -- allowed, but ...
+insert into at_tab2 values(1,'42'); -- ... this will fail
+drop table at_tab1, at_tab2;
+-- Check it for a partitioned table, too
+create table at_tab1 (a int, b text) partition by list(a);
+create table at_tab2 (x int, y at_tab1);
+alter table at_tab1 alter column b type varchar; -- fails
+drop table at_tab1, at_tab2;

 -- Alter column type that's part of a partitioned index
 create table at_partitioned (a int, b text) partition by range (a);