Re: BUG #18492: Adding a toasted column to a table with an inherited temp table fails with Assert

The following bug has been logged on the website:

Bug reference: 18492
Logged by: Alexander Lakhin
Email address: exclusion(at)gmail(dot)com
PostgreSQL version: 17beta1
Operating system: Ubuntu 22.04
Description:

The following script:
echo "CREATE TABLE t (a int);" | psql

echo "
CREATE TEMP TABLE tt() INHERITS (t);
select pg_sleep(1);
" | psql &

echo "
select pg_sleep(0.1);
ALTER TABLE t ADD COLUMN b text;
" | psql

triggers an assertion failure with the following stack trace:
TRAP: failed Assert("isTempOrTempToastNamespace(relnamespace)"), File:
"relcache.c", Line: 3619, PID: 1339672
...
#5 0x0000563cd5a466f0 in ExceptionalCondition (conditionName=0x563cd5c9eac8
"isTempOrTempToastNamespace(relnamespace)", fileName=0x563cd5c9e0c8
"relcache.c", lineNumber=3619) at assert.c:66
#6 0x0000563cd5a35131 in RelationBuildLocalRelation (relname=0x7ffc531aff20
"pg_toast_16390", relnamespace=99, tupDesc=0x563cd7622258, relid=16395,
accessmtd=2, relfilenumber=16395, reltablespace=0, shared_relation=false,
mapped_relation=false, relpersistence=116 't', relkind=116 't') at
relcache.c:3619
#7 0x0000563cd53bcfc3 in heap_create (relname=0x7ffc531aff20
"pg_toast_16390", relnamespace=99, reltablespace=0, relid=16395,
relfilenumber=16395, accessmtd=2, tupDesc=0x563cd7622258, relkind=116 't',
relpersistence=116 't',
shared_relation=false, mapped_relation=false,
allow_system_table_mods=true, relfrozenxid=0x7ffc531afd7c,
relminmxid=0x7ffc531afd80, create_storage=true) at heap.c:367
#8 0x0000563cd53beedc in heap_create_with_catalog (relname=0x7ffc531aff20
"pg_toast_16390", relnamespace=99, reltablespace=0, relid=16395,
reltypeid=0, reloftypeid=0, ownerid=10, accessmtd=2, tupdesc=0x563cd7622258,

cooked_constraints=0x0, relkind=116 't', relpersistence=116 't',
shared_relation=false, mapped_relation=false, oncommit=ONCOMMIT_NOOP,
reloptions=0, use_user_acl=false, allow_system_table_mods=true,
is_internal=true, relrewrite=0,
typaddress=0x0) at heap.c:1288
#9 0x0000563cd5400bfa in create_toast_table (rel=0x7f4c7090c878,
toastOid=0, toastIndexOid=0, reloptions=0, lockmode=8, check=true,
OIDOldToast=0) at toasting.c:246
#10 0x0000563cd54006e2 in CheckAndCreateToastTable (relOid=16390,
reloptions=0, lockmode=8, check=true, OIDOldToast=0) at toasting.c:85
#11 0x0000563cd540060f in AlterTableCreateToastTable (relOid=16390,
reloptions=0, lockmode=8) at toasting.c:59
#12 0x0000563cd550a8d9 in ATRewriteCatalogs (wqueue=0x7ffc531b0108,
lockmode=8, context=0x7ffc531b0300) at tablecmds.c:5180
#13 0x0000563cd5509910 in ATController (parsetree=0x563cd752a140,
rel=0x7f4c70909218, cmds=0x563cd752a0f0, recurse=true, lockmode=8,
context=0x7ffc531b0300) at tablecmds.c:4709
#14 0x0000563cd5509500 in AlterTable (stmt=0x563cd752a140, lockmode=8,
context=0x7ffc531b0300) at tablecmds.c:4347
#15 0x0000563cd5850f94 in ProcessUtilitySlow (pstate=0x563cd7553910,
pstmt=0x563cd752a1f0, queryString=0x563cd75294d0 "ALTER TABLE t ADD COLUMN b
text;", context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0,
dest=0x563cd752a5b0,
qc=0x7ffc531b0960) at utility.c:1318
#16 0x0000563cd5850852 in standard_ProcessUtility (pstmt=0x563cd752a1f0,
queryString=0x563cd75294d0 "ALTER TABLE t ADD COLUMN b text;",
readOnlyTree=false, context=PROCESS_UTILITY_TOPLEVEL, params=0x0,
queryEnv=0x0,
dest=0x563cd752a5b0, qc=0x7ffc531b0960) at utility.c:1067
#17 0x0000563cd584f752 in ProcessUtility (pstmt=0x563cd752a1f0,
queryString=0x563cd75294d0 "ALTER TABLE t ADD COLUMN b text;",
readOnlyTree=false, context=PROCESS_UTILITY_TOPLEVEL, params=0x0,
queryEnv=0x0, dest=0x563cd752a5b0,
qc=0x7ffc531b0960) at utility.c:523
...

Without asserts enabled, the ALTER TABLE ends up with:
ERROR: could not open file "base/16384/16396": No such file or directory

PG Bug reporting form <noreply(at)postgresql(dot)org> writes:
> The following script:
> echo "CREATE TABLE t (a int);" | psql

> echo "
> CREATE TEMP TABLE tt() INHERITS (t);
> select pg_sleep(1);
> " | psql &

> echo "
> select pg_sleep(0.1);
> ALTER TABLE t ADD COLUMN b text;
> " | psql

> triggers an assertion failure with the following stack trace:
> TRAP: failed Assert("isTempOrTempToastNamespace(relnamespace)"), File:
> "relcache.c", Line: 3619, PID: 1339672

I'm inclined to think that we should reject any ALTER TABLE on another
session's temp table. It could theoretically work in cases that don't
require touching the temp table's contents, but it has to fail in all
cases that do require that, and I don't really want that distinction
to be semantically visible. It's too implementation-dependent and
would be likely to act differently in different PG versions.

This example shows that the prohibition would also have to include
failing if an ALTER recurses to a child table that is another
session's temp table; but the same error occurs if you just try to
modify the other temp table directly. I did

Session 1:
regression=# create temp table mytemptable(f1 int);
CREATE TABLE

Session 2:
regression=# \d *.mytemptable
Table "pg_temp_60.mytemptable"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
f1 | integer | | |

regression=# alter table pg_temp_60.mytemptable add column f2 int;
ALTER TABLE
regression=# alter table pg_temp_60.mytemptable add column f3 text;
server closed the connection unexpectedly

Even though the "add column f2 int" step went through, I think it's
too scary to allow that. I think we have, or might have in future,
optimizations that assume that a session's temp tables aren't modified
by other sessions.

(Note that this would have failed anyway if I weren't doing it as
superuser, because I wouldn't have had lookup permission in
pg_temp_60.)

regards, tom lane

On 2024-06-03 13:50:22 -0400, Tom Lane wrote:
> I'm inclined to think that we should reject any ALTER TABLE on another
> session's temp table.

+1

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2024-06-03 13:50:22 -0400, Tom Lane wrote:
>> I'm inclined to think that we should reject any ALTER TABLE on another
>> session's temp table.

> +1

The attached seems to do the trick. I initially thought of adding
the check to CheckTableNotInUse, but that is problematic because it
would keep us from cleaning out a temp schema that had belonged to
some other backend. So I added YA wrapper routine.

I've gone through all the other callers of CheckTableNotInUse, and
they appear to have checks of RELATION_IS_OTHER_TEMP where necessary,
so there don't seem to be any related holes. With a different
factorization we could perhaps merge those other checks, but it
would be more invasive and we'd not gain all that much.

We could set up a test of this error path, but it'd require an
isolation or TAP script, and I'm unconvinced that it's worth the
trouble. The most likely breakage is for someone to forget to
make this check in some new code path, and a test using existing
features would not catch that.

regards, tom lane