Rearrange order of operations in heap_drop_with_catalog and index_drop

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
index 1dbe77a8f53bc2fcabebab33ee7dc9b8dc6827ec..6f5ef2247bcd2549164cafaaf53aa5d9b9f0049b 100644 (file)
--- 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
index 9cb3c56110faa8cec0d6609b660f22256bd4bd6a..09fbca2a59d4c9fb09968fa8bfb68c5d41e9d74f 100644 (file)
--- 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
index a7eac55f2527d15147370c908a01a2987d038b69..6a7a8a3017e72ef5c40c212f8cc930d116ed9a10 100644 (file)
--- 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
index 06f0f4da3a4a2ab012bcecc8f61e99163b398df8..9f9479dcb87f51589d3b05202f197261641d0bd4 100644 (file)
--- 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);