Code review of CLUSTER patch. Clean up problems with relcache getting

confused, toasted data getting lost, etc.

doc/src/sgml/release.sgml

 <!--
-$Header: /cvsroot/pgsql/doc/src/sgml/release.sgml,v 1.145 2002/08/02 18:15:04 tgl Exp $
+$Header: /cvsroot/pgsql/doc/src/sgml/release.sgml,v 1.146 2002/08/11 21:17:34 tgl Exp $
 -->
 
 <appendix id="release">
@@ -24,6 +24,7 @@ CDATA means the content is "SGML-free", so you can write without
 worries about funny characters.
 -->
 <literallayout><![CDATA[
+CLUSTER is no longer hazardous to your schema
 COPY accepts a list of columns to copy
 ALTER TABLE DROP COLUMN
 CREATE OPERATOR CLASS/DROP OPERATOR CLASS

src/backend/catalog/heap.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/catalog/heap.c,v 1.219 2002/08/06 02:36:33 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/catalog/heap.c,v 1.220 2002/08/11 21:17:34 tgl Exp $
  *
  *
  * INTERFACE ROUTINES
@@ -198,7 +198,8 @@ SystemAttributeByName(const char *attname, bool relhasoids)
  *		Remove the system relation specific code to elsewhere eventually.
  *
  * If storage_create is TRUE then heap_storage_create is called here,
- * else caller must call heap_storage_create later.
+ * else caller must call heap_storage_create later (or not at all,
+ * if the relation doesn't need physical storage).
  * ----------------------------------------------------------------
  */
 Relation
@@ -291,7 +292,7 @@ heap_create(const char *relname,
 									 nailme);
 
 	/*
-	 * have the storage manager create the relation.
+	 * have the storage manager create the relation's disk file, if wanted.
 	 */
 	if (storage_create)
 		heap_storage_create(rel);
@@ -684,20 +685,21 @@ heap_create_with_catalog(const char *relname,
 	tupdesc->tdhasoid = BoolToHasOid(relhasoids);
 	
 	/*
-	 * Tell heap_create not to create a physical file; we'll do that below
-	 * after all our catalog updates are done.	(This isn't really
-	 * necessary anymore, but we may as well avoid the cycles of creating
-	 * and deleting the file in case we fail.)
+	 * Create the relcache entry (mostly dummy at this point) and the
+	 * physical disk file.  (If we fail further down, it's the smgr's
+	 * responsibility to remove the disk file again.)
+	 *
+	 * NB: create a physical file only if it's not a view.
 	 */
 	new_rel_desc = heap_create(relname,
 							   relnamespace,
 							   tupdesc,
 							   shared_relation,
-							   false,
+							   (relkind != RELKIND_VIEW),
 							   allow_system_table_mods);
 
 	/* Fetch the relation OID assigned by heap_create */
-	new_rel_oid = new_rel_desc->rd_att->attrs[0]->attrelid;
+	new_rel_oid = RelationGetRelid(new_rel_desc);
 
 	/* Assign an OID for the relation's tuple type */
 	new_type_oid = newoid();
@@ -761,12 +763,6 @@ heap_create_with_catalog(const char *relname,
 	 */
 	StoreConstraints(new_rel_desc, tupdesc);
 
-	/*
-	 * We create the disk file for this relation here
-	 */
-	if (relkind != RELKIND_VIEW)
-		heap_storage_create(new_rel_desc);
-
 	/*
 	 * ok, the relation has been cataloged, so close our relations and
 	 * return the oid of the newly created relation.

src/backend/catalog/index.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/catalog/index.c,v 1.188 2002/08/05 03:29:16 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/catalog/index.c,v 1.189 2002/08/11 21:17:34 tgl Exp $
  *
  *
  * INTERFACE ROUTINES
@@ -578,14 +578,18 @@ index_create(Oid heapRelationId,
 
 	indexTupDesc->tdhasoid = WITHOUTOID;
 	/*
-	 * create the index relation (but don't create storage yet)
+	 * create the index relation's relcache entry and physical disk file.
+	 * (If we fail further down, it's the smgr's responsibility to remove
+	 * the disk file again.)
 	 */
 	indexRelation = heap_create(indexRelationName,
 								namespaceId,
 								indexTupDesc,
 								shared_relation,
-								false,
+								true,
 								allow_system_table_mods);
+
+	/* Fetch the relation OID assigned by heap_create */
 	indexoid = RelationGetRelid(indexRelation);
 
 	/*
@@ -611,11 +615,6 @@ index_create(Oid heapRelationId,
 	 */
 	UpdateRelationRelation(indexRelation);
 
-	/*
-	 * We create the disk file for this relation here
-	 */
-	heap_storage_create(indexRelation);
-
 	/*
 	 * now update the object id's of all the attribute tuple forms in the
 	 * index relation's tuple descriptor

src/backend/catalog/pg_depend.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/catalog/pg_depend.c,v 1.4 2002/08/05 03:29:16 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/catalog/pg_depend.c,v 1.5 2002/08/11 21:17:34 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -121,15 +121,16 @@ recordMultipleDependencies(const ObjectAddress *depender,
 
 /*
  * deleteDependencyRecordsFor -- delete all records with given depender
- * classId/objectId.
+ * classId/objectId.  Returns the number of records deleted.
  *
  * This is used when redefining an existing object.  Links leading to the
  * object do not change, and links leading from it will be recreated
  * (possibly with some differences from before).
  */
-void
+long
 deleteDependencyRecordsFor(Oid classId, Oid objectId)
 {
+	long			count = 0;
 	Relation		depRel;
 	ScanKeyData		key[2];
 	SysScanDesc		scan;
@@ -150,11 +151,14 @@ deleteDependencyRecordsFor(Oid classId, Oid objectId)
 	while (HeapTupleIsValid(tup = systable_getnext(scan)))
 	{
 		simple_heap_delete(depRel, &tup->t_self);
+		count++;
 	}
 
 	systable_endscan(scan);
 
 	heap_close(depRel, RowExclusiveLock);
+
+	return count;
 }
 
 /*

src/backend/storage/buffer/bufmgr.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/storage/buffer/bufmgr.c,v 1.128 2002/08/06 02:36:34 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/storage/buffer/bufmgr.c,v 1.129 2002/08/11 21:17:34 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1038,11 +1038,6 @@ BufferReplace(BufferDesc *bufHdr)
  * RelationGetNumberOfBlocks
  *		Determines the current number of pages in the relation.
  *		Side effect: relation->rd_nblocks is updated.
- *
- * Note:
- *		XXX may fail for huge relations.
- *		XXX should be elsewhere.
- *		XXX maybe should be hidden
  */
 BlockNumber
 RelationGetNumberOfBlocks(Relation relation)
@@ -1061,6 +1056,23 @@ RelationGetNumberOfBlocks(Relation relation)
 	return relation->rd_nblocks;
 }
 
+/*
+ * RelationUpdateNumberOfBlocks
+ *		Forcibly update relation->rd_nblocks.
+ *
+ * If the relcache drops an entry for a temp relation, it must call this
+ * routine after recreating the relcache entry, so that rd_nblocks is
+ * re-sync'd with reality.  See RelationGetNumberOfBlocks.
+ */
+void
+RelationUpdateNumberOfBlocks(Relation relation)
+{
+	if (relation->rd_rel->relkind == RELKIND_VIEW)
+		relation->rd_nblocks = 0;
+	else
+		relation->rd_nblocks = smgrnblocks(DEFAULT_SMGR, relation);
+}
+
 /* ---------------------------------------------------------------------
  *		DropRelationBuffers
  *

src/backend/utils/cache/relcache.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/utils/cache/relcache.c,v 1.171 2002/08/06 02:36:35 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/utils/cache/relcache.c,v 1.172 2002/08/11 21:17:35 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -860,11 +860,12 @@ RelationBuildDesc(RelationBuildDescInfo buildinfo,
 
 	/*
 	 * normal relations are not nailed into the cache; nor can a pre-existing
-	 * relation be new or temp.
+	 * relation be new.  It could be temp though.  (Actually, it could be new
+	 * too, but it's okay to forget that fact if forced to flush the entry.)
 	 */
 	relation->rd_isnailed = false;
 	relation->rd_isnew = false;
-	relation->rd_istemp = false;
+	relation->rd_istemp = isTempNamespace(relation->rd_rel->relnamespace);
 
 	/*
 	 * initialize the tuple descriptor (relation->rd_att).
@@ -909,13 +910,23 @@ RelationBuildDesc(RelationBuildDescInfo buildinfo,
 	relation->rd_fd = -1;
 
 	/*
-	 * insert newly created relation into proper relcaches, restore memory
-	 * context and return the new reldesc.
+	 * Insert newly created relation into relcache hash tables.
 	 */
 	oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
 	RelationCacheInsert(relation);
 	MemoryContextSwitchTo(oldcxt);
 
+	/*
+	 * If it's a temp rel, RelationGetNumberOfBlocks will assume that
+	 * rd_nblocks is correct.  Must forcibly update the block count when
+	 * creating the relcache entry.  But if we are doing a rebuild, don't
+	 * do this yet; leave it to RelationClearRelation to do at the end.
+	 * (Otherwise, an elog in RelationUpdateNumberOfBlocks would leave us
+	 * with inconsistent relcache state.)
+	 */
+	if (relation->rd_istemp && oldrelation == NULL)
+		RelationUpdateNumberOfBlocks(relation);
+
 	return relation;
 }
 
@@ -1601,8 +1612,7 @@ RelationClose(Relation relation)
 
 #ifdef RELCACHE_FORCE_RELEASE
 	if (RelationHasReferenceCountZero(relation) &&
-		!relation->rd_isnew &&
-		!relation->rd_istemp)
+		!relation->rd_isnew)
 		RelationClearRelation(relation, false);
 #endif
 }
@@ -1733,19 +1743,15 @@ RelationClearRelation(Relation relation, bool rebuild)
 	{
 		/*
 		 * When rebuilding an open relcache entry, must preserve ref count
-		 * and new/temp flags.  Also attempt to preserve the tupledesc,
-		 * rewrite rules, and trigger substructures in place. Furthermore
-		 * we save/restore rd_nblocks (in case it is a new/temp relation)
-		 * *and* call RelationGetNumberOfBlocks (in case it isn't).
+		 * and rd_isnew flag.  Also attempt to preserve the tupledesc,
+		 * rewrite rules, and trigger substructures in place.
 		 */
 		int			old_refcnt = relation->rd_refcnt;
 		bool		old_isnew = relation->rd_isnew;
-		bool		old_istemp = relation->rd_istemp;
 		TupleDesc	old_att = relation->rd_att;
 		RuleLock   *old_rules = relation->rd_rules;
 		MemoryContext old_rulescxt = relation->rd_rulescxt;
 		TriggerDesc *old_trigdesc = relation->trigdesc;
-		BlockNumber old_nblocks = relation->rd_nblocks;
 		RelationBuildDescInfo buildinfo;
 
 		buildinfo.infotype = INFO_RELID;
@@ -1764,7 +1770,6 @@ RelationClearRelation(Relation relation, bool rebuild)
 		}
 		RelationSetReferenceCount(relation, old_refcnt);
 		relation->rd_isnew = old_isnew;
-		relation->rd_istemp = old_istemp;
 		if (equalTupleDescs(old_att, relation->rd_att))
 		{
 			FreeTupleDesc(relation->rd_att);
@@ -1791,13 +1796,14 @@ RelationClearRelation(Relation relation, bool rebuild)
 		}
 		else
 			FreeTriggerDesc(old_trigdesc);
-		relation->rd_nblocks = old_nblocks;
 
 		/*
-		 * this is kind of expensive, but I think we must do it in case
-		 * relation has been truncated...
+		 * Update rd_nblocks.  This is kind of expensive, but I think we must
+		 * do it in case relation has been truncated... we definitely must
+		 * do it if the rel is new or temp, since RelationGetNumberOfBlocks
+		 * will subsequently assume that the block count is correct.
 		 */
-		relation->rd_nblocks = RelationGetNumberOfBlocks(relation);
+		RelationUpdateNumberOfBlocks(relation);
 	}
 }
 
@@ -1811,18 +1817,19 @@ RelationFlushRelation(Relation relation)
 {
 	bool		rebuild;
 
-	if (relation->rd_isnew || relation->rd_istemp)
+	if (relation->rd_isnew)
 	{
 		/*
-		 * New and temp relcache entries must always be rebuilt, not
-		 * flushed; else we'd forget those two important status bits.
+		 * New relcache entries are always rebuilt, not flushed; else we'd
+		 * forget the "new" status of the relation, which is a useful
+		 * optimization to have.
 		 */
 		rebuild = true;
 	}
 	else
 	{
 		/*
-		 * Nonlocal rels can be dropped from the relcache if not open.
+		 * Pre-existing rels can be dropped from the relcache if not open.
 		 */
 		rebuild = !RelationHasReferenceCountZero(relation);
 	}
@@ -1921,7 +1928,7 @@ RelationCacheInvalidate(void)
 
 		relcacheInvalsReceived++;
 
-		if (RelationHasReferenceCountZero(relation) && !relation->rd_istemp)
+		if (RelationHasReferenceCountZero(relation))
 		{
 			/* Delete this entry immediately */
 			RelationClearRelation(relation, false);
@@ -1965,7 +1972,10 @@ AtEOXact_RelationCache(bool commit)
 		 *
 		 * During commit, reset the flag to false, since we are now out of the
 		 * creating transaction.  During abort, simply delete the relcache
-		 * entry --- it isn't interesting any longer.
+		 * entry --- it isn't interesting any longer.  (NOTE: if we have
+		 * forgotten the isnew state of a new relation due to a forced cache
+		 * flush, the entry will get deleted anyway by shared-cache-inval
+		 * processing of the aborted pg_class insertion.)
 		 */
 		if (relation->rd_isnew)
 		{

src/include/catalog/dependency.h

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $Id: dependency.h,v 1.3 2002/07/16 22:12:20 tgl Exp $
+ * $Id: dependency.h,v 1.4 2002/08/11 21:17:35 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -98,6 +98,6 @@ extern void recordMultipleDependencies(const ObjectAddress *depender,
 									   int nreferenced,
 									   DependencyType behavior);
 
-extern void deleteDependencyRecordsFor(Oid classId, Oid objectId);
+extern long deleteDependencyRecordsFor(Oid classId, Oid objectId);
 
 #endif   /* DEPENDENCY_H */

src/include/storage/bufmgr.h

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $Id: bufmgr.h,v 1.62 2002/08/06 02:36:35 tgl Exp $
+ * $Id: bufmgr.h,v 1.63 2002/08/11 21:17:35 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -161,6 +161,7 @@ extern void AtEOXact_Buffers(bool isCommit);
 extern void FlushBufferPool(void);
 extern BlockNumber BufferGetBlockNumber(Buffer buffer);
 extern BlockNumber RelationGetNumberOfBlocks(Relation relation);
+extern void RelationUpdateNumberOfBlocks(Relation relation);
 extern int	FlushRelationBuffers(Relation rel, BlockNumber firstDelBlock);
 extern void DropRelationBuffers(Relation rel);
 extern void DropRelFileNodeBuffers(RelFileNode rnode, bool istemp);

src/include/utils/rel.h

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $Id: rel.h,v 1.61 2002/08/06 02:36:35 tgl Exp $
+ * $Id: rel.h,v 1.62 2002/08/11 21:17:35 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -113,6 +113,10 @@ typedef struct RelationData
 								 * InvalidBlockNumber */
 	int			rd_refcnt;		/* reference count */
 	bool		rd_isnew;		/* rel was created in current xact */
+	/*
+	 * NOTE: rd_isnew should be relied on only for optimization purposes;
+	 * it is possible for new-ness to be "forgotten" (eg, after CLUSTER).
+	 */
 	bool		rd_istemp;		/* rel uses the local buffer mgr */
 	bool		rd_isnailed;	/* rel is nailed in cache */
 	bool		rd_indexfound;	/* true if rd_indexlist is valid */

src/test/regress/sql/cluster.sql

@@ -8,6 +8,7 @@ CREATE TABLE clstr_tst_s (rf_a SERIAL PRIMARY KEY,
 CREATE TABLE clstr_tst (a SERIAL PRIMARY KEY,
 	b INT,
 	c TEXT,
+	d TEXT,
 	CONSTRAINT clstr_tst_con FOREIGN KEY (b) REFERENCES clstr_tst_s);
 
 CREATE INDEX clstr_tst_b ON clstr_tst (b);
@@ -55,24 +56,26 @@ INSERT INTO clstr_tst (b, c) VALUES (15, 'quince');
 INSERT INTO clstr_tst (b, c) VALUES (7, 'siete');
 INSERT INTO clstr_tst (b, c) VALUES (16, 'dieciseis');
 INSERT INTO clstr_tst (b, c) VALUES (8, 'ocho');
-INSERT INTO clstr_tst (b, c) VALUES (6, 'seis');
+-- This entry is needed to test that TOASTED values are copied correctly.
+INSERT INTO clstr_tst (b, c, d) VALUES (6, 'seis', repeat('xyzzy', 100000));
 
 CLUSTER clstr_tst_c ON clstr_tst;
 
-SELECT * from clstr_tst;
-SELECT * from clstr_tst ORDER BY a;
-SELECT * from clstr_tst ORDER BY b;
-SELECT * from clstr_tst ORDER BY c;
+SELECT a,b,c,substring(d for 30), length(d) from clstr_tst;
+SELECT a,b,c,substring(d for 30), length(d) from clstr_tst ORDER BY a;
+SELECT a,b,c,substring(d for 30), length(d) from clstr_tst ORDER BY b;
+SELECT a,b,c,substring(d for 30), length(d) from clstr_tst ORDER BY c;
 
 -- Verify that inheritance link still works
 INSERT INTO clstr_tst_inh VALUES (0, 100, 'in child table');
-SELECT * from clstr_tst;
+SELECT a,b,c,substring(d for 30), length(d) from clstr_tst;
 
 -- Verify that foreign key link still works
 INSERT INTO clstr_tst (b, c) VALUES (1111, 'this should fail');
 
-SELECT conname FROM pg_constraint WHERE conrelid=(SELECT oid FROM pg_class
-	WHERE relname='clstr_tst');
+SELECT conname FROM pg_constraint WHERE conrelid = 'clstr_tst'::regclass;
 
 
-SELECT relname FROM pg_class WHERE relname LIKE 'clstr_tst%' ORDER BY relname;
+SELECT relname, relkind,
+    EXISTS(SELECT 1 FROM pg_class WHERE oid = c.reltoastrelid) AS hastoast
+FROM pg_class c WHERE relname LIKE 'clstr_tst%' ORDER BY relname;