From 448eb0837f7a8db270481475e8803e1e8a19a37e Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sat, 28 Aug 2004 21:05:26 +0000
Subject: [PATCH] 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.
---
src/backend/catalog/heap.c | 85 +++++++++++++++-----------------
src/backend/catalog/index.c | 42 +++++++++-------
src/backend/commands/tablecmds.c | 4 +-
src/include/catalog/heap.h | 6 +--
4 files changed, 68 insertions(+), 69 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 1dbe77a8f53..6f5ef2247bc 100644
--- 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 9cb3c56110f..09fbca2a59d 100644
--- 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 a7eac55f252..6a7a8a3017e 100644
--- 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 06f0f4da3a4..9f9479dcb87 100644
--- 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);
--
GitLab