Forbid ALTER TABLE and CLUSTER when there are pending AFTER-trigger events

tglsfdc · tglsfdc · commit 20e862155f10 · 2008-01-02T23:34:42.000Z
in the current backend for the target table.  These operations move tuples
around and would thus invalidate the TIDs stored in the trigger event records.
(We need not worry about events in other backends, since acquiring exclusive
lock should be enough to ensure there aren't any.)  It might be sufficient
to forbid only the table-rewriting variants of ALTER TABLE, but in the absence
of any compelling use-case, let's just be safe and simple.  Per follow-on
investigation of bug #3847, though this is not actually the same problem
reported therein.

Possibly this should be back-patched, but since the case has never been
reported from the field, I didn't bother.
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
@@ -11,7 +11,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/commands/cluster.c,v 1.166 2008/01/01 19:45:48 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/commands/cluster.c,v 1.167 2008/01/02 23:34:42 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -30,6 +30,7 @@
 #include "catalog/namespace.h"
 #include "catalog/toasting.h"
 #include "commands/cluster.h"
+#include "commands/trigger.h"
 #include "commands/vacuum.h"
 #include "miscadmin.h"
 #include "storage/procarray.h"
@@ -457,6 +458,17 @@ check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck)
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 			   errmsg("cannot cluster temporary tables of other sessions")));
 
+	/*
+	 * Also check for pending AFTER trigger events on the relation.  We can't
+	 * just leave those be, since they will try to fetch tuples that the
+	 * CLUSTER has moved to new TIDs.
+	 */
+	if (AfterTriggerPendingOnRel(RelationGetRelid(OldHeap)))
+		ereport(ERROR,
+				(errcode(ERRCODE_OBJECT_IN_USE),
+				 errmsg("cannot cluster table \"%s\" because it has pending trigger events",
+						RelationGetRelationName(OldHeap))));
+
 	/* Drop relcache refcnt on OldIndex, but keep lock */
 	index_close(OldIndex, NoLock);
 }
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.238 2008/01/01 19:45:49 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.239 2008/01/02 23:34:42 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -194,6 +194,7 @@ static void validateForeignKeyConstraint(FkConstraint *fkconstraint,
 							 Relation rel, Relation pkrel, Oid constraintOid);
 static void createForeignKeyTriggers(Relation rel, FkConstraint *fkconstraint,
 						 Oid constraintOid);
+static void CheckTableNotInUse(Relation rel);
 static void ATController(Relation rel, List *cmds, bool recurse);
 static void ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
 		  bool recurse, bool recursing);
@@ -598,13 +599,6 @@ ExecuteTruncate(TruncateStmt *stmt)
 		heap_truncate_check_FKs(rels, false);
 #endif
 
-	/*
-	 * Also check for pending AFTER trigger events on the target relations. We
-	 * can't just leave those be, since they will try to fetch tuples that the
-	 * TRUNCATE removes.
-	 */
-	AfterTriggerCheckTruncate(relids);
-
 	/*
 	 * OK, truncate each table.
 	 */
@@ -685,6 +679,17 @@ truncate_check_rel(Relation rel)
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 			  errmsg("cannot truncate temporary tables of other sessions")));
+
+	/*
+	 * Also check for pending AFTER trigger events on the relation.  We can't
+	 * just leave those be, since they will try to fetch tuples that the
+	 * TRUNCATE removes.
+	 */
+	if (AfterTriggerPendingOnRel(RelationGetRelid(rel)))
+		ereport(ERROR,
+				(errcode(ERRCODE_OBJECT_IN_USE),
+				 errmsg("cannot truncate table \"%s\" because it has pending trigger events",
+						RelationGetRelationName(rel))));
 }
 
 /*----------
@@ -1749,28 +1754,47 @@ void
 AlterTable(AlterTableStmt *stmt)
 {
 	Relation	rel = relation_openrv(stmt->relation, AccessExclusiveLock);
+
+	CheckTableNotInUse(rel);
+
+	ATController(rel, stmt->cmds, interpretInhOption(stmt->relation->inhOpt));
+}
+
+/*
+ * Disallow ALTER TABLE when the current backend has any open reference to
+ * it besides the one we just got (such as an open cursor or active plan);
+ * our AccessExclusiveLock doesn't protect us against stomping on our own
+ * foot, only other people's feet!
+ *
+ * Note: the only case known to cause serious trouble is ALTER COLUMN TYPE,
+ * and some changes are obviously pretty benign, so this could possibly be
+ * relaxed to only error out for certain types of alterations.  But the
+ * use-case for allowing any of these things is not obvious, so we won't
+ * work hard at it for now.
+ *
+ * We also reject ALTER TABLE if there are any pending AFTER trigger events
+ * for the rel.  This is certainly necessary for the rewriting variants of
+ * ALTER TABLE, because they don't preserve tuple TIDs and so the pending
+ * events would try to fetch the wrong tuples.  It might be overly cautious
+ * in other cases, but again it seems better to err on the side of paranoia.
+ */
+static void
+CheckTableNotInUse(Relation rel)
+{
 	int			expected_refcnt;
 
-	/*
-	 * Disallow ALTER TABLE when the current backend has any open reference to
-	 * it besides the one we just got (such as an open cursor or active plan);
-	 * our AccessExclusiveLock doesn't protect us against stomping on our own
-	 * foot, only other people's feet!
-	 *
-	 * Note: the only case known to cause serious trouble is ALTER COLUMN
-	 * TYPE, and some changes are obviously pretty benign, so this could
-	 * possibly be relaxed to only error out for certain types of alterations.
-	 * But the use-case for allowing any of these things is not obvious, so we
-	 * won't work hard at it for now.
-	 */
 	expected_refcnt = rel->rd_isnailed ? 2 : 1;
 	if (rel->rd_refcnt != expected_refcnt)
 		ereport(ERROR,
 				(errcode(ERRCODE_OBJECT_IN_USE),
 				 errmsg("relation \"%s\" is being used by active queries in this session",
 						RelationGetRelationName(rel))));
 
-	ATController(rel, stmt->cmds, interpretInhOption(stmt->relation->inhOpt));
+	if (AfterTriggerPendingOnRel(RelationGetRelid(rel)))
+		ereport(ERROR,
+				(errcode(ERRCODE_OBJECT_IN_USE),
+				 errmsg("cannot alter table \"%s\" because it has pending trigger events",
+						RelationGetRelationName(rel))));
 }
 
 /*
@@ -1781,7 +1805,8 @@ AlterTable(AlterTableStmt *stmt)
  * We do not reject if the relation is already open, because it's quite
  * likely that one or more layers of caller have it open.  That means it
  * is unsafe to use this entry point for alterations that could break
- * existing query plans.
+ * existing query plans.  On the assumption it's not used for such, we
+ * don't have to reject pending AFTER triggers, either.
  */
 void
 AlterTableInternal(Oid relid, List *cmds, bool recurse)
@@ -2784,12 +2809,7 @@ ATSimpleRecursion(List **wqueue, Relation rel,
 			if (childrelid == relid)
 				continue;
 			childrel = relation_open(childrelid, AccessExclusiveLock);
-			/* check for child relation in use in this session */
-			if (childrel->rd_refcnt != 1)
-				ereport(ERROR,
-						(errcode(ERRCODE_OBJECT_IN_USE),
-						 errmsg("relation \"%s\" is being used by active queries in this session",
-								RelationGetRelationName(childrel))));
+			CheckTableNotInUse(childrel);
 			ATPrepCmd(wqueue, childrel, cmd, false, true);
 			relation_close(childrel, NoLock);
 		}
@@ -2821,12 +2841,7 @@ ATOneLevelRecursion(List **wqueue, Relation rel,
 		Relation	childrel;
 
 		childrel = relation_open(childrelid, AccessExclusiveLock);
-		/* check for child relation in use in this session */
-		if (childrel->rd_refcnt != 1)
-			ereport(ERROR,
-					(errcode(ERRCODE_OBJECT_IN_USE),
-					 errmsg("relation \"%s\" is being used by active queries in this session",
-							RelationGetRelationName(childrel))));
+		CheckTableNotInUse(childrel);
 		ATPrepCmd(wqueue, childrel, cmd, true, true);
 		relation_close(childrel, NoLock);
 	}
@@ -3655,12 +3670,7 @@ ATExecDropColumn(Relation rel, const char *colName,
 			Form_pg_attribute childatt;
 
 			childrel = heap_open(childrelid, AccessExclusiveLock);
-			/* check for child relation in use in this session */
-			if (childrel->rd_refcnt != 1)
-				ereport(ERROR,
-						(errcode(ERRCODE_OBJECT_IN_USE),
-						 errmsg("relation \"%s\" is being used by active queries in this session",
-								RelationGetRelationName(childrel))));
+			CheckTableNotInUse(childrel);
 
 			tuple = SearchSysCacheCopyAttName(childrelid, colName);
 			if (!HeapTupleIsValid(tuple))		/* shouldn't happen */
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/commands/trigger.c,v 1.226 2008/01/01 19:45:49 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/commands/trigger.c,v 1.227 2008/01/02 23:34:42 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -3478,28 +3478,29 @@ AfterTriggerSetState(ConstraintsSetStmt *stmt)
 }
 
 /* ----------
- * AfterTriggerCheckTruncate()
- *		Test deferred-trigger status to see if a TRUNCATE is OK.
+ * AfterTriggerPendingOnRel()
+ *		Test to see if there are any pending after-trigger events for rel.
  *
- * The argument is a list of OIDs of relations due to be truncated.
- * We raise error if there are any pending after-trigger events for them.
+ * This is used by TRUNCATE, CLUSTER, ALTER TABLE, etc to detect whether
+ * it is unsafe to perform major surgery on a relation.  Note that only
+ * local pending events are examined.  We assume that having exclusive lock
+ * on a rel guarantees there are no unserviced events in other backends ---
+ * but having a lock does not prevent there being such events in our own.
  *
  * In some scenarios it'd be reasonable to remove pending events (more
  * specifically, mark them DONE by the current subxact) but without a lot
  * of knowledge of the trigger semantics we can't do this in general.
  * ----------
  */
-void
-AfterTriggerCheckTruncate(List *relids)
+bool
+AfterTriggerPendingOnRel(Oid relid)
 {
 	AfterTriggerEvent event;
 	int			depth;
 
-	/*
-	 * Ignore call if we aren't in a transaction.  (Shouldn't happen?)
-	 */
+	/* No-op if we aren't in a transaction.  (Shouldn't happen?) */
 	if (afterTriggers == NULL)
-		return;
+		return false;
 
 	/* Scan queued events */
 	for (event = afterTriggers->events.head;
@@ -3509,21 +3510,18 @@ AfterTriggerCheckTruncate(List *relids)
 		/*
 		 * We can ignore completed events.	(Even if a DONE flag is rolled
 		 * back by subxact abort, it's OK because the effects of the TRUNCATE
-		 * must get rolled back too.)
+		 * or whatever must get rolled back too.)
 		 */
 		if (event->ate_event & AFTER_TRIGGER_DONE)
 			continue;
 
-		if (list_member_oid(relids, event->ate_relid))
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("cannot truncate table \"%s\" because it has pending trigger events",
-							get_rel_name(event->ate_relid))));
+		if (event->ate_relid == relid)
+			return true;
 	}
 
 	/*
 	 * Also scan events queued by incomplete queries.  This could only matter
-	 * if a TRUNCATE is executed by a function or trigger within an updating
+	 * if TRUNCATE/etc is executed by a function or trigger within an updating
 	 * query on the same relation, which is pretty perverse, but let's check.
 	 */
 	for (depth = 0; depth <= afterTriggers->query_depth; depth++)
@@ -3535,13 +3533,12 @@ AfterTriggerCheckTruncate(List *relids)
 			if (event->ate_event & AFTER_TRIGGER_DONE)
 				continue;
 
-			if (list_member_oid(relids, event->ate_relid))
-				ereport(ERROR,
-						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-						 errmsg("cannot truncate table \"%s\" because it has pending trigger events",
-								get_rel_name(event->ate_relid))));
+			if (event->ate_relid == relid)
+				return true;
 		}
 	}
+
+	return false;
 }
 
 
diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h
@@ -6,7 +6,7 @@
  * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/commands/trigger.h,v 1.65 2008/01/01 19:45:57 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/commands/trigger.h,v 1.66 2008/01/02 23:34:42 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -149,7 +149,7 @@ extern void AfterTriggerEndXact(bool isCommit);
 extern void AfterTriggerBeginSubXact(void);
 extern void AfterTriggerEndSubXact(bool isCommit);
 extern void AfterTriggerSetState(ConstraintsSetStmt *stmt);
-extern void AfterTriggerCheckTruncate(List *relids);
+extern bool AfterTriggerPendingOnRel(Oid relid);
 
 
 /*
