Rearrange order of operations in heap_drop_with_catalog and index_drop

tglsfdc · tglsfdc · commit 448eb0837f7a · 2004-08-28T21:05:26.000Z
so that we close and flush the doomed relation's relcache entry before
we start to delete the underlying catalog rows, rather than afterwards.
For awhile yesterday I thought that an unexpected relcache entry rebuild
partway through this sequence might explain the infrequent parallel
regression failures we were chasing.  It doesn't, mainly because there's
no CommandCounterIncrement in the sequence and so the deletions aren't
"really" done yet.  But it sure seems like trouble waiting to happen.
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/catalog/heap.c,v 1.272 2004/07/11 19:52:48 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/catalog/heap.c,v 1.273 2004/08/28 21:05:26 tgl Exp $
  *
  *
  * INTERFACE ROUTINES
@@ -71,7 +71,7 @@ static void AddNewRelationType(const char *typeName,
 				   Oid new_rel_oid,
 				   char new_rel_kind,
 				   Oid new_type_oid);
-static void RelationRemoveInheritance(Relation relation);
+static void RelationRemoveInheritance(Oid relid);
 static void StoreRelCheck(Relation rel, char *ccname, char *ccbin);
 static void StoreConstraints(Relation rel, TupleDesc tupdesc);
 static void SetRelationNumChecks(Relation rel, int numchecks);
@@ -836,7 +836,7 @@ heap_create_with_catalog(const char *relname,
  * linking this relation to its parent(s).
  */
 static void
-RelationRemoveInheritance(Relation relation)
+RelationRemoveInheritance(Oid relid)
 {
 	Relation	catalogRelation;
 	SysScanDesc scan;
@@ -848,7 +848,7 @@ RelationRemoveInheritance(Relation relation)
 	ScanKeyInit(&key,
 				Anum_pg_inherits_inhrelid,
 				BTEqualStrategyNumber, F_OIDEQ,
-				ObjectIdGetDatum(RelationGetRelid(relation)));
+				ObjectIdGetDatum(relid));
 
 	scan = systable_beginscan(catalogRelation, InheritsRelidSeqnoIndex, true,
 							  SnapshotNow, 1, &key);
@@ -1015,7 +1015,7 @@ RemoveAttributeById(Oid relid, AttrNumber attnum)
 	heap_close(attr_rel, RowExclusiveLock);
 
 	if (attnum > 0)
-		RemoveStatistics(rel, attnum);
+		RemoveStatistics(relid, attnum);
 
 	relation_close(rel, NoLock);
 }
@@ -1147,33 +1147,24 @@ RemoveAttrDefaultById(Oid attrdefId)
 	relation_close(myrel, NoLock);
 }
 
-/* ----------------------------------------------------------------
- *		heap_drop_with_catalog	- removes specified relation from catalogs
- *
- *		1)	open relation, acquire exclusive lock.
- *		2)	flush relation buffers from bufmgr
- *		3)	remove inheritance information
- *		4)	remove pg_statistic tuples
- *		5)	remove pg_attribute tuples
- *		6)	remove pg_class tuple
- *		7)	unlink relation file
+/*
+ * heap_drop_with_catalog	- removes specified relation from catalogs
  *
  * Note that this routine is not responsible for dropping objects that are
  * linked to the pg_class entry via dependencies (for example, indexes and
  * constraints).  Those are deleted by the dependency-tracing logic in
  * dependency.c before control gets here.  In general, therefore, this routine
  * should never be called directly; go through performDeletion() instead.
- * ----------------------------------------------------------------
  */
 void
-heap_drop_with_catalog(Oid rid)
+heap_drop_with_catalog(Oid relid)
 {
 	Relation	rel;
 
 	/*
 	 * Open and lock the relation.
 	 */
-	rel = relation_open(rid, AccessExclusiveLock);
+	rel = relation_open(relid, AccessExclusiveLock);
 
 	/*
 	 * Release all buffers that belong to this relation, after writing any
@@ -1182,53 +1173,57 @@ heap_drop_with_catalog(Oid rid)
 	FlushRelationBuffers(rel, (BlockNumber) 0);
 
 	/*
-	 * remove inheritance information
+	 * Schedule unlinking of the relation's physical file at commit.
 	 */
-	RelationRemoveInheritance(rel);
+	if (rel->rd_rel->relkind != RELKIND_VIEW &&
+		rel->rd_rel->relkind != RELKIND_COMPOSITE_TYPE)
+	{
+		if (rel->rd_smgr == NULL)
+			rel->rd_smgr = smgropen(rel->rd_node);
+		smgrscheduleunlink(rel->rd_smgr, rel->rd_istemp);
+		rel->rd_smgr = NULL;
+	}
 
 	/*
-	 * delete statistics
+	 * Close relcache entry, but *keep* AccessExclusiveLock on the
+	 * relation until transaction commit.  This ensures no one else will
+	 * try to do something with the doomed relation.
 	 */
-	RemoveStatistics(rel, 0);
+	relation_close(rel, NoLock);
 
 	/*
-	 * delete attribute tuples
+	 * Forget any ON COMMIT action for the rel
 	 */
-	DeleteAttributeTuples(RelationGetRelid(rel));
+	remove_on_commit_action(relid);
 
 	/*
-	 * delete relation tuple
+	 * Flush the relation from the relcache.  We want to do this before
+	 * starting to remove catalog entries, just to be certain that no
+	 * relcache entry rebuild will happen partway through.  (That should
+	 * not really matter, since we don't do CommandCounterIncrement here,
+	 * but let's be safe.)
 	 */
-	DeleteRelationTuple(RelationGetRelid(rel));
+	RelationForgetRelation(relid);
 
 	/*
-	 * forget any ON COMMIT action for the rel
+	 * remove inheritance information
 	 */
-	remove_on_commit_action(rid);
+	RelationRemoveInheritance(relid);
 
 	/*
-	 * unlink the relation's physical file and finish up.
+	 * delete statistics
 	 */
-	if (rel->rd_rel->relkind != RELKIND_VIEW &&
-		rel->rd_rel->relkind != RELKIND_COMPOSITE_TYPE)
-	{
-		if (rel->rd_smgr == NULL)
-			rel->rd_smgr = smgropen(rel->rd_node);
-		smgrscheduleunlink(rel->rd_smgr, rel->rd_istemp);
-		rel->rd_smgr = NULL;
-	}
+	RemoveStatistics(relid, 0);
 
 	/*
-	 * Close relcache entry, but *keep* AccessExclusiveLock on the
-	 * relation until transaction commit.  This ensures no one else will
-	 * try to do something with the doomed relation.
+	 * delete attribute tuples
 	 */
-	relation_close(rel, NoLock);
+	DeleteAttributeTuples(relid);
 
 	/*
-	 * flush the relation from the relcache
+	 * delete relation tuple
 	 */
-	RelationForgetRelation(rid);
+	DeleteRelationTuple(relid);
 }
 
 
@@ -1884,7 +1879,7 @@ RemoveRelConstraints(Relation rel, const char *constrName,
  * for that column.
  */
 void
-RemoveStatistics(Relation rel, AttrNumber attnum)
+RemoveStatistics(Oid relid, AttrNumber attnum)
 {
 	Relation	pgstatistic;
 	SysScanDesc scan;
@@ -1897,7 +1892,7 @@ RemoveStatistics(Relation rel, AttrNumber attnum)
 	ScanKeyInit(&key[0],
 				Anum_pg_statistic_starelid,
 				BTEqualStrategyNumber, F_OIDEQ,
-				ObjectIdGetDatum(RelationGetRelid(rel)));
+				ObjectIdGetDatum(relid));
 
 	if (attnum == 0)
 		nkeys = 1;
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/catalog/index.c,v 1.235 2004/08/01 17:32:14 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/catalog/index.c,v 1.236 2004/08/28 21:05:26 tgl Exp $
  *
  *
  * INTERFACE ROUTINES
@@ -769,8 +769,6 @@ index_drop(Oid indexId)
 	HeapTuple	tuple;
 	bool		hasexprs;
 
-	Assert(OidIsValid(indexId));
-
 	/*
 	 * To drop an index safely, we must grab exclusive lock on its parent
 	 * table; otherwise there could be other backends using the index!
@@ -790,14 +788,24 @@ index_drop(Oid indexId)
 	LockRelation(userIndexRelation, AccessExclusiveLock);
 
 	/*
-	 * fix RELATION relation
+	 * flush buffer cache and schedule physical removal of the file
 	 */
-	DeleteRelationTuple(indexId);
+	FlushRelationBuffers(userIndexRelation, (BlockNumber) 0);
+
+	if (userIndexRelation->rd_smgr == NULL)
+		userIndexRelation->rd_smgr = smgropen(userIndexRelation->rd_node);
+	smgrscheduleunlink(userIndexRelation->rd_smgr,
+					   userIndexRelation->rd_istemp);
+	userIndexRelation->rd_smgr = NULL;
 
 	/*
-	 * fix ATTRIBUTE relation
+	 * Close and flush the index's relcache entry, to ensure relcache
+	 * doesn't try to rebuild it while we're deleting catalog entries.
+	 * We keep the lock though.
 	 */
-	DeleteAttributeTuples(indexId);
+	index_close(userIndexRelation);
+
+	RelationForgetRelation(indexId);
 
 	/*
 	 * fix INDEX relation, and check for expressional index
@@ -822,18 +830,17 @@ index_drop(Oid indexId)
 	 * statistics about them.
 	 */
 	if (hasexprs)
-		RemoveStatistics(userIndexRelation, 0);
+		RemoveStatistics(indexId, 0);
 
 	/*
-	 * flush buffer cache and physically remove the file
+	 * fix ATTRIBUTE relation
 	 */
-	FlushRelationBuffers(userIndexRelation, (BlockNumber) 0);
+	DeleteAttributeTuples(indexId);
 
-	if (userIndexRelation->rd_smgr == NULL)
-		userIndexRelation->rd_smgr = smgropen(userIndexRelation->rd_node);
-	smgrscheduleunlink(userIndexRelation->rd_smgr,
-					   userIndexRelation->rd_istemp);
-	userIndexRelation->rd_smgr = NULL;
+	/*
+	 * fix RELATION relation
+	 */
+	DeleteRelationTuple(indexId);
 
 	/*
 	 * We are presently too lazy to attempt to compute the new correct
@@ -846,12 +853,9 @@ index_drop(Oid indexId)
 	CacheInvalidateRelcache(userHeapRelation);
 
 	/*
-	 * Close rels, but keep locks
+	 * Close owning rel, but keep lock
 	 */
-	index_close(userIndexRelation);
 	heap_close(userHeapRelation, NoLock);
-
-	RelationForgetRelation(indexId);
 }
 
 /* ----------------------------------------------------------------
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.126 2004/08/15 23:44:46 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.127 2004/08/28 21:05:26 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -4924,7 +4924,7 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
 	add_column_datatype_dependency(RelationGetRelid(rel), attnum, targettype);
 
 	/* Drop any pg_statistic entry for the column, since it's now wrong type */
-	RemoveStatistics(rel, attnum);
+	RemoveStatistics(RelationGetRelid(rel), attnum);
 
 	/*
 	 * Update the default, if present, by brute force --- remove and re-add
diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2003, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/catalog/heap.h,v 1.68 2004/07/11 19:52:51 tgl Exp $
+ * $PostgreSQL: pgsql/src/include/catalog/heap.h,v 1.69 2004/08/28 21:05:26 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -54,7 +54,7 @@ extern Oid heap_create_with_catalog(const char *relname,
 						 OnCommitAction oncommit,
 						 bool allow_system_table_mods);
 
-extern void heap_drop_with_catalog(Oid rid);
+extern void heap_drop_with_catalog(Oid relid);
 
 extern void heap_truncate(Oid rid);
 
@@ -81,7 +81,7 @@ extern void RemoveAttributeById(Oid relid, AttrNumber attnum);
 extern void RemoveAttrDefault(Oid relid, AttrNumber attnum,
 				  DropBehavior behavior, bool complain);
 extern void RemoveAttrDefaultById(Oid attrdefId);
-extern void RemoveStatistics(Relation rel, AttrNumber attnum);
+extern void RemoveStatistics(Oid relid, AttrNumber attnum);
 
 extern Form_pg_attribute SystemAttributeDefinition(AttrNumber attno,
 						  bool relhasoids);

Original file line number	Diff line number	Diff line change
`@@ -8,7 +8,7 @@`
`8`	`8`	`*`
`9`	`9`	`*`
`10`	`10`	`* IDENTIFICATION`
`11`		`- * $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.126 2004/08/15 23:44:46 tgl Exp $`
	`11`	`+ * $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.127 2004/08/28 21:05:26 tgl Exp $`
`12`	`12`	`*`
`13`	`13`	`*-------------------------------------------------------------------------`
`14`	`14`	`*/`
`@@ -4924,7 +4924,7 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,`
`4924`	`4924`	`add_column_datatype_dependency(RelationGetRelid(rel), attnum, targettype);`
`4925`	`4925`
`4926`	`4926`	`/* Drop any pg_statistic entry for the column, since it's now wrong type */`
`4927`		`- RemoveStatistics(rel, attnum);`
	`4927`	`+ RemoveStatistics(RelationGetRelid(rel), attnum);`
`4928`	`4928`
`4929`	`4929`	`/*`
`4930`	`4930`	`* Update the default, if present, by brute force --- remove and re-add`