Thread: BUG #18492: Adding a toasted column to a table with an inherited temp table fails with Assert
BUG #18492: Adding a toasted column to a table with an inherited temp table fails with Assert
From
PG Bug reporting form
Date:
The following bug has been logged on the website:
Bug reference: 18492
Logged by: Alexander Lakhin
Email address: exclusion@gmail.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
Re: BUG #18492: Adding a toasted column to a table with an inherited temp table fails with Assert
From
Tom Lane
Date:
PG Bug reporting form <noreply@postgresql.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
Re: BUG #18492: Adding a toasted column to a table with an inherited temp table fails with Assert
From
Andres Freund
Date:
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
Re: BUG #18492: Adding a toasted column to a table with an inherited temp table fails with Assert
From
Tom Lane
Date:
Andres Freund <andres@anarazel.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
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7b6c69b7a5..6adfd87614 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -399,6 +399,7 @@ static CoercionPathType findFkeyCast(Oid targetTypeId, Oid sourceTypeId,
static void validateForeignKeyConstraint(char *conname,
Relation rel, Relation pkrel,
Oid pkindOid, Oid constraintOid);
+static void CheckAlterTableIsSafe(Relation rel);
static void ATController(AlterTableStmt *parsetree,
Relation rel, List *cmds, bool recurse, LOCKMODE lockmode,
AlterTableUtilityContext *context);
@@ -4269,6 +4270,37 @@ CheckTableNotInUse(Relation rel, const char *stmt)
stmt, RelationGetRelationName(rel))));
}
+/*
+ * CheckAlterTableIsSafe
+ * Verify that it's safe to allow ALTER TABLE on this relation.
+ *
+ * This consists of CheckTableNotInUse() plus a check that the relation
+ * isn't another session's temp table. We must split out the temp-table
+ * check because there are callers of CheckTableNotInUse() that don't want
+ * that, notably DROP TABLE. (We must allow DROP or we couldn't clean out
+ * an orphaned temp schema.) Compare truncate_check_activity().
+ */
+static void
+CheckAlterTableIsSafe(Relation rel)
+{
+ /*
+ * Don't allow ALTER on temp tables of other backends. Their local buffer
+ * manager is not going to cope if we need to change the table's contents.
+ * Even if we don't, there may be optimizations that assume temp tables
+ * aren't subject to such interference.
+ */
+ if (RELATION_IS_OTHER_TEMP(rel))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot alter temporary tables of other sessions")));
+
+ /*
+ * Also check for active uses of the relation in the current transaction,
+ * including open scans and pending AFTER trigger events.
+ */
+ CheckTableNotInUse(rel, "ALTER TABLE");
+}
+
/*
* AlterTableLookupRelation
* Look up, and lock, the OID for the relation named by an alter table
@@ -4342,7 +4374,7 @@ AlterTable(AlterTableStmt *stmt, LOCKMODE lockmode,
/* Caller is required to provide an adequate lock. */
rel = relation_open(context->relid, NoLock);
- CheckTableNotInUse(rel, "ALTER TABLE");
+ CheckAlterTableIsSafe(rel);
ATController(stmt, rel, stmt->cmds, stmt->relation->inh, lockmode, context);
}
@@ -5748,7 +5780,9 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
/*
* Don't allow rewrite on temp tables of other backends ... their
- * local buffer manager is not going to cope.
+ * local buffer manager is not going to cope. (This is redundant
+ * with the check in CheckAlterTableIsSafe, but for safety we'll
+ * check here too.)
*/
if (RELATION_IS_OTHER_TEMP(OldHeap))
ereport(ERROR,
@@ -6619,7 +6653,7 @@ ATSimpleRecursion(List **wqueue, Relation rel,
continue;
/* find_all_inheritors already got lock */
childrel = relation_open(childrelid, NoLock);
- CheckTableNotInUse(childrel, "ALTER TABLE");
+ CheckAlterTableIsSafe(childrel);
ATPrepCmd(wqueue, childrel, cmd, false, true, lockmode, context);
relation_close(childrel, NoLock);
}
@@ -6628,7 +6662,7 @@ ATSimpleRecursion(List **wqueue, Relation rel,
/*
* Obtain list of partitions of the given table, locking them all at the given
- * lockmode and ensuring that they all pass CheckTableNotInUse.
+ * lockmode and ensuring that they all pass CheckAlterTableIsSafe.
*
* This function is a no-op if the given relation is not a partitioned table;
* in particular, nothing is done if it's a legacy inheritance parent.
@@ -6649,7 +6683,7 @@ ATCheckPartitionsNotInUse(Relation rel, LOCKMODE lockmode)
/* find_all_inheritors already got lock */
childrel = table_open(lfirst_oid(cell), NoLock);
- CheckTableNotInUse(childrel, "ALTER TABLE");
+ CheckAlterTableIsSafe(childrel);
table_close(childrel, NoLock);
}
list_free(inh);
@@ -6682,7 +6716,7 @@ ATTypedTableRecursion(List **wqueue, Relation rel, AlterTableCmd *cmd,
Relation childrel;
childrel = relation_open(childrelid, lockmode);
- CheckTableNotInUse(childrel, "ALTER TABLE");
+ CheckAlterTableIsSafe(childrel);
ATPrepCmd(wqueue, childrel, cmd, true, true, lockmode, context);
relation_close(childrel, NoLock);
}
@@ -7354,7 +7388,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
/* find_inheritance_children already got lock */
childrel = table_open(childrelid, NoLock);
- CheckTableNotInUse(childrel, "ALTER TABLE");
+ CheckAlterTableIsSafe(childrel);
/* Find or create work queue entry for this table */
childtab = ATGetQueueEntry(wqueue, childrel);
@@ -9031,7 +9065,7 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
/* find_inheritance_children already got lock */
childrel = table_open(childrelid, NoLock);
- CheckTableNotInUse(childrel, "ALTER TABLE");
+ CheckAlterTableIsSafe(childrel);
tuple = SearchSysCacheCopyAttName(childrelid, colName);
if (!HeapTupleIsValid(tuple)) /* shouldn't happen */
@@ -9514,7 +9548,7 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
/* find_inheritance_children already got lock */
childrel = table_open(childrelid, NoLock);
- CheckTableNotInUse(childrel, "ALTER TABLE");
+ CheckAlterTableIsSafe(childrel);
/* Find or create work queue entry for this table */
childtab = ATGetQueueEntry(wqueue, childrel);
@@ -10343,7 +10377,7 @@ addFkRecurseReferencing(List **wqueue, Constraint *fkconstraint, Relation rel,
referenced;
ListCell *cell;
- CheckTableNotInUse(partition, "ALTER TABLE");
+ CheckAlterTableIsSafe(partition);
attmap = build_attrmap_by_name(RelationGetDescr(partition),
RelationGetDescr(rel),
@@ -12460,7 +12494,7 @@ ATExecDropConstraint(Relation rel, const char *constrName,
/* Must match lock taken by RemoveTriggerById: */
frel = table_open(con->confrelid, AccessExclusiveLock);
- CheckTableNotInUse(frel, "ALTER TABLE");
+ CheckAlterTableIsSafe(frel);
table_close(frel, NoLock);
}
@@ -12537,7 +12571,7 @@ ATExecDropConstraint(Relation rel, const char *constrName,
/* find_inheritance_children already got lock */
childrel = table_open(childrelid, NoLock);
- CheckTableNotInUse(childrel, "ALTER TABLE");
+ CheckAlterTableIsSafe(childrel);
ScanKeyInit(&skey[0],
Anum_pg_constraint_conrelid,
@@ -12840,7 +12874,7 @@ ATPrepAlterColumnType(List **wqueue,
/* find_all_inheritors already got lock */
childrel = relation_open(childrelid, NoLock);
- CheckTableNotInUse(childrel, "ALTER TABLE");
+ CheckAlterTableIsSafe(childrel);
/*
* Verify that the child doesn't have any inherited definitions of