From: Tom Lane <tgl@sss.pgh.pa.us>
To: exclusion@gmail.com
cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17351: Altering a composite type created for a partitioned
 table can lead to a crash
In-reply-to: <3057471.1641072425@sss.pgh.pa.us>
References: <17351-6db1870f3f4f612a@postgresql.org>
 <3057471.1641072425@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Sat, 01 Jan 2022 16:27:05 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1226475.1641502213.0@sss.pgh.pa.us>
Date: Thu, 06 Jan 2022 15:51:46 -0500
Message-ID: <1226576.1641502306@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1226576.1641502306%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1226475.1641502213.1@sss.pgh.pa.us>

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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="prevent-alter-column-type-in-partitioned-table.patch";
	charset="us-ascii"
Content-ID: <1226475.1641502213.2@sss.pgh.pa.us>
Content-Description: prevent-alter-column-type-in-partitioned-table.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/table=
cmds.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 =3D=3D RELKIND_COMPOSITE_TYPE ||
-		tab->relkind =3D=3D 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 ty=
pe
+-- 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 =3D (1,'42')::a=
t_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/a=
lter_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 varcha=
r(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 =3D (1,'42')::a=
t_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);

------- =_aaaaaaaaaa0--