Thread: BUG #17351: Altering a composite type created for a partitioned table can lead to a crash

The following bug has been logged on the website:

Bug reference:      17351
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.com
PostgreSQL version: 14.1
Operating system:   Ubuntu 20.04
Description:

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:
Core was generated by `postgres: law regression [local] SELECT
                        '.
Program terminated with signal SIGABRT, Aborted.
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
50      ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt 
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007f8af00f3859 in __GI_abort () at abort.c:79
#2  0x000055be552e0cc3 in ExceptionalCondition (conditionName=0x55be5536aaad
"true", 
    errorType=0x55be5536aa93 "unrecognized TOAST vartag",
fileName=0x55be5536aa00 "heaptuple.c", lineNumber=1320)
    at assert.c:69
#3  0x000055be54bc28b9 in heap_deform_tuple (tuple=0x7fff462dbae0,
tupleDesc=0x7f8ae2c518b0, values=0x55be55d89650, 
    isnull=0x55be55d93fd8) at heaptuple.c:1320
#4  0x000055be552410bf in record_out (fcinfo=0x7fff462dbb70) at
rowtypes.c:364
#5  0x000055be552ec16f in FunctionCall1Coll (flinfo=0x7fff462dbbf0,
collation=0, arg1=94275972402416) at fmgr.c:1138
#6  0x000055be552ed72d in OutputFunctionCall (flinfo=0x7fff462dbbf0,
val=94275972402416) at fmgr.c:1575
#7  0x000055be552ed9e4 in OidOutputFunctionCall (functionId=2291,
val=94275972402416) at fmgr.c:1658
#8  0x000055be5525dece in get_const_expr (constval=0x55be55d88c98,
context=0x7fff462dc300, showtype=0)
    at ruleutils.c:10261
#9  0x000055be55258add in get_rule_expr (node=0x55be55d88c98,
context=0x7fff462dc300, showimplicit=true)
    at ruleutils.c:8348
#10 0x000055be552589dc in get_rule_expr_paren (node=0x55be55d88c98,
context=0x7fff462dc300, showimplicit=true, 
    parentNode=0x55be55d88b90) at ruleutils.c:8301
#11 0x000055be5525bec8 in get_oper_expr (expr=0x55be55d88b90,
context=0x7fff462dc300) at ruleutils.c:9612
#12 0x000055be55258dc8 in get_rule_expr (node=0x55be55d88b90,
context=0x7fff462dc300, showimplicit=false)
    at ruleutils.c:8453
#13 0x000055be5524d6af in deparse_expression_pretty (expr=0x55be55d88b90,
dpcontext=0x55be55d89170, forceprefix=false, 
    showimplicit=false, prettyFlags=7, startIndent=0) at ruleutils.c:3547
#14 0x000055be5524af4e in pg_get_constraintdef_worker (constraintId=16391,
fullCommand=false, prettyFlags=7, 
    missing_ok=true) at ruleutils.c:2419
#15 0x000055be5524a1ed in pg_get_constraintdef_ext (fcinfo=0x55be55d25290)
at ruleutils.c:2089
#16 0x000055be54e9bf85 in ExecInterpExpr (state=0x55be55d24990,
econtext=0x55be55d24478, isnull=0x7fff462dc79f)
    at execExprInterp.c:749
#17 0x000055be54e9e33c in ExecInterpExprStillValid (state=0x55be55d24990,
econtext=0x55be55d24478, 
    isNull=0x7fff462dc79f) at execExprInterp.c:1824
#18 0x000055be54eb8858 in ExecEvalExprSwitchContext (state=0x55be55d24990,
econtext=0x55be55d24478, 
    isNull=0x7fff462dc79f) at ../../../src/include/executor/executor.h:339
#19 0x000055be54eb88d0 in ExecProject (projInfo=0x55be55d24988) at
../../../src/include/executor/executor.h:373
#20 0x000055be54eb8daf in ExecScan (node=0x55be55d24360,
accessMtd=0x55be54efaf88 <SeqNext>, 
    recheckMtd=0x55be54efb031 <SeqRecheck>) at execScan.c:238
#21 0x000055be54efb08a in ExecSeqScan (pstate=0x55be55d24360) at
nodeSeqscan.c:112
#22 0x000055be54eb491d in ExecProcNodeFirst (node=0x55be55d24360) at
execProcnode.c:463
#23 0x000055be54efc75e in ExecProcNode (node=0x55be55d24360) at
../../../src/include/executor/executor.h:257
#24 0x000055be54efc8b1 in ExecSort (pstate=0x55be55d24148) at
nodeSort.c:108
#25 0x000055be54eb491d in ExecProcNodeFirst (node=0x55be55d24148) at
execProcnode.c:463
#26 0x000055be54ea8409 in ExecProcNode (node=0x55be55d24148) at
../../../src/include/executor/executor.h:257
#27 0x000055be54eab027 in ExecutePlan (estate=0x55be55d23f10,
planstate=0x55be55d24148, use_parallel_mode=false, 
    operation=CMD_SELECT, sendTuples=true, numberTuples=0,
direction=ForwardScanDirection, dest=0x55be55d8ad08, 
    execute_once=true) at execMain.c:1551
#28 0x000055be54ea8b40 in standard_ExecutorRun (queryDesc=0x55be55d14dc0,
direction=ForwardScanDirection, count=0, 
    execute_once=true) at execMain.c:361
#29 0x000055be54ea892b in ExecutorRun (queryDesc=0x55be55d14dc0,
direction=ForwardScanDirection, count=0, 
    execute_once=true) at execMain.c:305
#30 0x000055be55125a1e in PortalRunSelect (portal=0x55be55c8d7a0,
forward=true, count=0, dest=0x55be55d8ad08)
    at pquery.c:921
#31 0x000055be55125642 in PortalRun (portal=0x55be55c8d7a0,
count=9223372036854775807, isTopLevel=true, run_once=true, 
    dest=0x55be55d8ad08, altdest=0x55be55d8ad08, qc=0x7fff462dcc80) at
pquery.c:765
#32 0x000055be5511e52c in exec_simple_query (
    query_string=0x55be55c1e8a0 "SELECT r.conname,
pg_catalog.pg_get_constraintdef(r.oid, true)\nFROM pg_catalog.pg_constraint
r\nWHERE r.conrelid = '16388' AND r.contype = 'c'\nORDER BY 1;") at
postgres.c:1214
#33 0x000055be5512341b in PostgresMain (argc=1, argv=0x7fff462dcea0,
dbname=0x55be55c4a9f8 "regression", 
    username=0x55be55c4a9d8 "law") at postgres.c:4486
#34 0x000055be55047d64 in BackendRun (port=0x55be55c42110) at
postmaster.c:4530
#35 0x000055be550475bf in BackendStartup (port=0x55be55c42110) at
postmaster.c:4252
#36 0x000055be550433b4 in ServerLoop () at postmaster.c:1745
#37 0x000055be55042b11 in PostmasterMain (argc=3, argv=0x55be55c18910) at
postmaster.c:1417
#38 0x000055be54f322fc in main (argc=3, argv=0x55be55c18910) at main.c:209

The outcome varies depending on the constant in the check.
Without "partition by ..." I get "cannot alter table "pt" because column
"t.a" uses its row type" error.

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:

regression=# create table pt (a int, b int);
CREATE TABLE
regression=# create table t(a pt);
CREATE TABLE
regression=# alter table pt alter column a type char(4);
ERROR:  cannot alter table "pt" because column "t.a" uses its row type

So something is mistakenly skipping that check for partitioned
tables.

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".

            regards, tom lane

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