Re: BUG #18492: Adding a toasted column to a table with an inherited temp table fails with Assert - Mailing list pgsql-bugs

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