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
Date: Thu, 06 Jan 2022 15:51:46 -0500
Message-ID: <1226576.1641502306@sss.pgh.pa.us> (raw)
In-Reply-To: <3057471.1641072425@sss.pgh.pa.us>
References: <17351-6db1870f3f4f612a@postgresql.org>
<3057471.1641072425@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
Attachments:prevent-alter-column-type-in-partitioned-table.patch (text/x-diff, 4.0K) downloaddiff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.cindex 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.outindex 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.sqlindex 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);
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, exclusion@gmail.com, 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: <1226576.1641502306@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox