Now that we've rearranged relation open to get a lock before touching

the rel, it's easy to get rid of the narrow race-condition window that
used to exist in VACUUM and CLUSTER.  Did some minor code-beautification
work in the same area, too.

4 files changed, 65 insertions, 58 deletions

diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 34a38b20e29..2930eacb503 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/commands/analyze.c,v 1.96 2006/07/14 14:52:18 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/commands/analyze.c,v 1.97 2006/08/18 16:09:08 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -129,20 +129,16 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt)
 	CHECK_FOR_INTERRUPTS();
 
 	/*
-	 * Race condition -- if the pg_class tuple has gone away since the last
-	 * time we saw it, we don't need to process it.
+	 * Open the relation, getting only a read lock on it.  If the rel has
+	 * been dropped since we last saw it, we don't need to process it.
 	 */
-	if (!SearchSysCacheExists(RELOID,
-							  ObjectIdGetDatum(relid),
-							  0, 0, 0))
+	onerel = try_relation_open(relid, AccessShareLock);
+	if (!onerel)
 		return;
 
 	/*
-	 * Open the class, getting only a read lock on it, and check permissions.
-	 * Permissions check should match vacuum's check!
+	 * Check permissions --- this should match vacuum's check!
 	 */
-	onerel = relation_open(relid, AccessShareLock);
-
 	if (!(pg_class_ownercheck(RelationGetRelid(onerel), GetUserId()) ||
 		  (pg_database_ownercheck(MyDatabaseId, GetUserId()) && !onerel->rd_rel->relisshared)))
 	{
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index f0cbaefd60e..bd2301dd62e 100644
--- 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.152 2006/07/31 20:09:00 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/commands/cluster.c,v 1.153 2006/08/18 16:09:08 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -240,6 +240,18 @@ cluster_rel(RelToCluster *rvtc, bool recheck)
 	CHECK_FOR_INTERRUPTS();
 
 	/*
+	 * We grab exclusive access to the target rel and index for the duration
+	 * of the transaction.	(This is redundant for the single-transaction
+	 * case, since cluster() already did it.)  The index lock is taken inside
+	 * check_index_is_clusterable.
+	 */
+	OldHeap = try_relation_open(rvtc->tableOid, AccessExclusiveLock);
+
+	/* If the table has gone away, we can skip processing it */
+	if (!OldHeap)
+		return;
+
+	/*
 	 * Since we may open a new transaction for each relation, we have to check
 	 * that the relation still is what we think it is.
 	 *
@@ -252,20 +264,23 @@ cluster_rel(RelToCluster *rvtc, bool recheck)
 		HeapTuple	tuple;
 		Form_pg_index indexForm;
 
+		/* Check that the user still owns the relation */
+		if (!pg_class_ownercheck(rvtc->tableOid, GetUserId()))
+		{
+			relation_close(OldHeap, AccessExclusiveLock);
+			return;
+		}
+
 		/*
-		 * Check if the relation and index still exist before opening them
+		 * Check that the index still exists
 		 */
 		if (!SearchSysCacheExists(RELOID,
-								  ObjectIdGetDatum(rvtc->tableOid),
-								  0, 0, 0) ||
-			!SearchSysCacheExists(RELOID,
 								  ObjectIdGetDatum(rvtc->indexOid),
 								  0, 0, 0))
+		{
+			relation_close(OldHeap, AccessExclusiveLock);
 			return;
-
-		/* Check that the user still owns the relation */
-		if (!pg_class_ownercheck(rvtc->tableOid, GetUserId()))
-			return;
+		}
 
 		/*
 		 * Check that the index is still the one with indisclustered set.
@@ -273,25 +288,21 @@ cluster_rel(RelToCluster *rvtc, bool recheck)
 		tuple = SearchSysCache(INDEXRELID,
 							   ObjectIdGetDatum(rvtc->indexOid),
 							   0, 0, 0);
-		if (!HeapTupleIsValid(tuple))
-			return;				/* could have gone away... */
+		if (!HeapTupleIsValid(tuple))		/* probably can't happen */
+		{
+			relation_close(OldHeap, AccessExclusiveLock);
+			return;
+		}
 		indexForm = (Form_pg_index) GETSTRUCT(tuple);
 		if (!indexForm->indisclustered)
 		{
 			ReleaseSysCache(tuple);
+			relation_close(OldHeap, AccessExclusiveLock);
 			return;
 		}
 		ReleaseSysCache(tuple);
 	}
 
-	/*
-	 * We grab exclusive access to the target rel and index for the duration
-	 * of the transaction.	(This is redundant for the single- transaction
-	 * case, since cluster() already did it.)  The index lock is taken inside
-	 * check_index_is_clusterable.
-	 */
-	OldHeap = heap_open(rvtc->tableOid, AccessExclusiveLock);
-
 	/* Check index is valid to cluster on */
 	check_index_is_clusterable(OldHeap, rvtc->indexOid, recheck);
 
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index a84b1b2cded..a5e10b793e6 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/commands/lockcmds.c,v 1.14 2006/03/05 15:58:24 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/commands/lockcmds.c,v 1.15 2006/08/18 16:09:08 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -59,7 +59,10 @@ LockTableCommand(LockStmt *lockstmt)
 			aclcheck_error(aclresult, ACL_KIND_CLASS,
 						   get_rel_name(reloid));
 
-		rel = conditional_relation_open(reloid, lockstmt->mode, lockstmt->nowait);
+		if (lockstmt->nowait)
+			rel = relation_open_nowait(reloid, lockstmt->mode);
+		else
+			rel = relation_open(reloid, lockstmt->mode);
 
 		/* Currently, we only allow plain tables to be locked */
 		if (rel->rd_rel->relkind != RELKIND_RELATION)
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index c21be2f4788..8c1df23d9b0 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -13,7 +13,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/commands/vacuum.c,v 1.337 2006/07/31 20:09:00 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/commands/vacuum.c,v 1.338 2006/08/18 16:09:08 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1042,31 +1042,12 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, char expected_relkind)
 	}
 
 	/*
-	 * Tell the cache replacement strategy that vacuum is causing all
-	 * following IO
-	 */
-	StrategyHintVacuum(true);
-
-	/*
 	 * Check for user-requested abort.	Note we want this to be inside a
 	 * transaction, so xact.c doesn't issue useless WARNING.
 	 */
 	CHECK_FOR_INTERRUPTS();
 
 	/*
-	 * Race condition -- if the pg_class tuple has gone away since the last
-	 * time we saw it, we don't need to vacuum it.
-	 */
-	if (!SearchSysCacheExists(RELOID,
-							  ObjectIdGetDatum(relid),
-							  0, 0, 0))
-	{
-		StrategyHintVacuum(false);
-		CommitTransactionCommand();
-		return;
-	}
-
-	/*
 	 * Determine the type of lock we want --- hard exclusive lock for a FULL
 	 * vacuum, but just ShareUpdateExclusiveLock for concurrent vacuum. Either
 	 * way, we can be sure that no other backend is vacuuming the same table.
@@ -1074,7 +1055,21 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, char expected_relkind)
 	lmode = vacstmt->full ? AccessExclusiveLock : ShareUpdateExclusiveLock;
 
 	/*
-	 * Open the class, get an appropriate lock on it, and check permissions.
+	 * Open the relation and get the appropriate lock on it.
+	 *
+	 * There's a race condition here: the rel may have gone away since
+	 * the last time we saw it.  If so, we don't need to vacuum it.
+	 */
+	onerel = try_relation_open(relid, lmode);
+
+	if (!onerel)
+	{
+		CommitTransactionCommand();
+		return;
+	}
+
+	/*
+	 * Check permissions.
 	 *
 	 * We allow the user to vacuum a table if he is superuser, the table
 	 * owner, or the database owner (but in the latter case, only if it's not
@@ -1083,8 +1078,6 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, char expected_relkind)
 	 * Note we choose to treat permissions failure as a WARNING and keep
 	 * trying to vacuum the rest of the DB --- is this appropriate?
 	 */
-	onerel = relation_open(relid, lmode);
-
 	if (!(pg_class_ownercheck(RelationGetRelid(onerel), GetUserId()) ||
 		  (pg_database_ownercheck(MyDatabaseId, GetUserId()) && !onerel->rd_rel->relisshared)))
 	{
@@ -1092,7 +1085,6 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, char expected_relkind)
 				(errmsg("skipping \"%s\" --- only table or database owner can vacuum it",
 						RelationGetRelationName(onerel))));
 		relation_close(onerel, lmode);
-		StrategyHintVacuum(false);
 		CommitTransactionCommand();
 		return;
 	}
@@ -1107,7 +1099,6 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, char expected_relkind)
 				(errmsg("skipping \"%s\" --- cannot vacuum indexes, views, or special system tables",
 						RelationGetRelationName(onerel))));
 		relation_close(onerel, lmode);
-		StrategyHintVacuum(false);
 		CommitTransactionCommand();
 		return;
 	}
@@ -1122,7 +1113,6 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, char expected_relkind)
 	if (isOtherTempNamespace(RelationGetNamespace(onerel)))
 	{
 		relation_close(onerel, lmode);
-		StrategyHintVacuum(false);
 		CommitTransactionCommand();
 		return;			/* assume no long-lived data in temp tables */
 	}
@@ -1146,6 +1136,12 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, char expected_relkind)
 	toast_relid = onerel->rd_rel->reltoastrelid;
 
 	/*
+	 * Tell the cache replacement strategy that vacuum is causing all
+	 * following IO
+	 */
+	StrategyHintVacuum(true);
+
+	/*
 	 * Do the actual work --- either FULL or "lazy" vacuum
 	 */
 	if (vacstmt->full)
@@ -1153,13 +1149,14 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, char expected_relkind)
 	else
 		lazy_vacuum_rel(onerel, vacstmt);
 
+	StrategyHintVacuum(false);
+
 	/* all done with this class, but hold lock until commit */
 	relation_close(onerel, NoLock);
 
 	/*
 	 * Complete the transaction and free all temporary memory used.
 	 */
-	StrategyHintVacuum(false);
 	CommitTransactionCommand();
 
 	/*