Add checks to TRUNCATE, CLUSTER, and REINDEX to prevent performing these

operations when the current transaction has any open references to the
target relation or index (implying it has an active query using the relation).
The need for this was previously recognized in connection with ALTER TABLE,
but anything that summarily eliminates tuples or moves them around would
confuse an active scan.

While this patch does not in itself fix bug #3883 (the deadlock would happen
before the new check fires), it will discourage people from attempting the
sequence of operations that creates a deadlock risk, so it's at least a
partial response to that problem.

In passing, add a previously-missing check to REINDEX to prevent trying to
reindex another backend's temp table.  This isn't a security problem since
only a superuser would get past the schema permission checks, but if we are
testing for this in other utility commands then surely REINDEX should too.

Author: tglsfdc

src/backend/catalog/index.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/catalog/index.c,v 1.291 2008/01/14 01:39:09 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/catalog/index.c,v 1.292 2008/01/30 19:46:48 tgl Exp $
  *
  *
  * INTERFACE ROUTINES
@@ -33,11 +33,13 @@
 #include "catalog/heap.h"
 #include "catalog/index.h"
 #include "catalog/indexing.h"
+#include "catalog/namespace.h"
 #include "catalog/pg_constraint.h"
 #include "catalog/pg_operator.h"
 #include "catalog/pg_opclass.h"
 #include "catalog/pg_tablespace.h"
 #include "catalog/pg_type.h"
+#include "commands/tablecmds.h"
 #include "executor/executor.h"
 #include "miscadmin.h"
 #include "optimizer/clauses.h"
@@ -2219,6 +2221,21 @@ reindex_index(Oid indexId)
 	 */
 	iRel = index_open(indexId, AccessExclusiveLock);
 
+	/*
+	 * Don't allow reindex on temp tables of other backends ... their local
+	 * buffer manager is not going to cope.
+	 */
+	if (isOtherTempNamespace(RelationGetNamespace(iRel)))
+		ereport(ERROR,
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+				 errmsg("cannot reindex temporary tables of other sessions")));
+
+	/*
+	 * Also check for active uses of the index in the current transaction;
+	 * we don't want to reindex underneath an open indexscan.
+	 */
+	CheckTableNotInUse(iRel, "REINDEX INDEX");
+
 	/*
 	 * If it's a shared index, we must do inplace processing (because we have
 	 * no way to update relfilenode in other databases).  Otherwise we can do

src/backend/commands/cluster.c

@@ -11,7 +11,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/commands/cluster.c,v 1.168 2008/01/15 21:20:28 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/commands/cluster.c,v 1.169 2008/01/30 19:46:48 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -30,6 +30,7 @@
 #include "catalog/namespace.h"
 #include "catalog/toasting.h"
 #include "commands/cluster.h"
+#include "commands/tablecmds.h"
 #include "commands/trigger.h"
 #include "commands/vacuum.h"
 #include "miscadmin.h"
@@ -459,15 +460,10 @@ check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck)
 			   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.
+	 * Also check for active uses of the relation in the current transaction,
+	 * including open scans and pending AFTER trigger events.
 	 */
-	if (AfterTriggerPendingOnRel(RelationGetRelid(OldHeap)))
-		ereport(ERROR,
-				(errcode(ERRCODE_OBJECT_IN_USE),
-				 errmsg("cannot cluster table \"%s\" because it has pending trigger events",
-						RelationGetRelationName(OldHeap))));
+	CheckTableNotInUse(OldHeap, "CLUSTER");
 
 	/* Drop relcache refcnt on OldIndex, but keep lock */
 	index_close(OldIndex, NoLock);

src/backend/commands/tablecmds.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.240 2008/01/17 18:56:54 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.241 2008/01/30 19:46:48 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -194,7 +194,6 @@ 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);
@@ -681,15 +680,10 @@ truncate_check_rel(Relation rel)
 			  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.
+	 * Also check for active uses of the relation in the current transaction,
+	 * including open scans and pending AFTER trigger events.
 	 */
-	if (AfterTriggerPendingOnRel(RelationGetRelid(rel)))
-		ereport(ERROR,
-				(errcode(ERRCODE_OBJECT_IN_USE),
-				 errmsg("cannot truncate table \"%s\" because it has pending trigger events",
-						RelationGetRelationName(rel))));
+	CheckTableNotInUse(rel, "TRUNCATE");
 }
 
 /*----------
@@ -1728,6 +1722,55 @@ renamerel(Oid myrelid, const char *newrelname, ObjectType reltype)
 	relation_close(targetrelation, NoLock);
 }
 
+/*
+ * Disallow ALTER TABLE (and similar commands) when the current backend has
+ * any open reference to the target table besides the one just acquired by
+ * the calling command; this implies there's an open cursor or active plan.
+ * We need this check because our AccessExclusiveLock doesn't protect us
+ * against stomping on our own foot, only other people's feet!
+ *
+ * For ALTER TABLE, 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 these commands 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.
+ *
+ * REINDEX calls this with "rel" referencing the index to be rebuilt; here
+ * we are worried about active indexscans on the index.  The trigger-event
+ * check can be skipped, since we are doing no damage to the parent table.
+ *
+ * The statement name (eg, "ALTER TABLE") is passed for use in error messages.
+ */
+void
+CheckTableNotInUse(Relation rel, const char *stmt)
+{
+	int			expected_refcnt;
+
+	expected_refcnt = rel->rd_isnailed ? 2 : 1;
+	if (rel->rd_refcnt != expected_refcnt)
+		ereport(ERROR,
+				(errcode(ERRCODE_OBJECT_IN_USE),
+				 /* translator: first %s is a SQL command, eg ALTER TABLE */
+				 errmsg("cannot %s \"%s\" because "
+						"it is being used by active queries in this session",
+						stmt, RelationGetRelationName(rel))));
+
+	if (rel->rd_rel->relkind != RELKIND_INDEX &&
+		AfterTriggerPendingOnRel(RelationGetRelid(rel)))
+		ereport(ERROR,
+				(errcode(ERRCODE_OBJECT_IN_USE),
+				 /* translator: first %s is a SQL command, eg ALTER TABLE */
+				 errmsg("cannot %s \"%s\" because "
+						"it has pending trigger events",
+						stmt, RelationGetRelationName(rel))));
+}
+
 /*
  * AlterTable
  *		Execute ALTER TABLE, which can be a list of subcommands
@@ -1766,48 +1809,11 @@ AlterTable(AlterTableStmt *stmt)
 {
 	Relation	rel = relation_openrv(stmt->relation, AccessExclusiveLock);
 
-	CheckTableNotInUse(rel);
+	CheckTableNotInUse(rel, "ALTER TABLE");
 
 	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;
-
-	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))));
-
-	if (AfterTriggerPendingOnRel(RelationGetRelid(rel)))
-		ereport(ERROR,
-				(errcode(ERRCODE_OBJECT_IN_USE),
-				 errmsg("cannot alter table \"%s\" because it has pending trigger events",
-						RelationGetRelationName(rel))));
-}
-
 /*
  * AlterTableInternal
  *
@@ -2820,7 +2826,7 @@ ATSimpleRecursion(List **wqueue, Relation rel,
 			if (childrelid == relid)
 				continue;
 			childrel = relation_open(childrelid, AccessExclusiveLock);
-			CheckTableNotInUse(childrel);
+			CheckTableNotInUse(childrel, "ALTER TABLE");
 			ATPrepCmd(wqueue, childrel, cmd, false, true);
 			relation_close(childrel, NoLock);
 		}
@@ -2852,7 +2858,7 @@ ATOneLevelRecursion(List **wqueue, Relation rel,
 		Relation	childrel;
 
 		childrel = relation_open(childrelid, AccessExclusiveLock);
-		CheckTableNotInUse(childrel);
+		CheckTableNotInUse(childrel, "ALTER TABLE");
 		ATPrepCmd(wqueue, childrel, cmd, true, true);
 		relation_close(childrel, NoLock);
 	}
@@ -3681,7 +3687,7 @@ ATExecDropColumn(Relation rel, const char *colName,
 			Form_pg_attribute childatt;
 
 			childrel = heap_open(childrelid, AccessExclusiveLock);
-			CheckTableNotInUse(childrel);
+			CheckTableNotInUse(childrel, "ALTER TABLE");
 
 			tuple = SearchSysCacheCopyAttName(childrelid, colName);
 			if (!HeapTupleIsValid(tuple))		/* shouldn't happen */

src/include/commands/tablecmds.h

@@ -7,7 +7,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/tablecmds.h,v 1.36 2008/01/01 19:45:57 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/commands/tablecmds.h,v 1.37 2008/01/30 19:46:48 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -34,6 +34,8 @@ extern void AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
 							   Oid oldNspOid, Oid newNspOid,
 							   bool hasDependEntry);
 
+extern void CheckTableNotInUse(Relation rel, const char *stmt);
+
 extern void ExecuteTruncate(TruncateStmt *stmt);
 
 extern void renameatt(Oid myrelid,