Fix two serious bugs introduced into hash indexes by the 8.4 patch that made

hash indexes keep entries sorted by hash value.  First, the original plans for
concurrency assumed that insertions would happen only at the end of a page,
which is no longer true; this could cause scans to transiently fail to find
index entries in the presence of concurrent insertions.  We can compensate
by teaching scans to re-find their position after re-acquiring read locks.
Second, neither the bucket split nor the bucket compaction logic had been
fixed to preserve hashvalue ordering, so application of either of those
processes could lead to permanent corruption of an index, in the sense
that searches might fail to find entries that are present.

This patch fixes the split and compaction logic to preserve hashvalue
ordering, but it cannot do anything about pre-existing corruption.  We will
need to recommend reindexing all hash indexes in the 8.4.2 release notes.

To buy back the performance loss hereby induced in split and compaction,
fix them to use PageIndexMultiDelete instead of retail PageIndexDelete
operations.  We might later want to do something with qsort'ing the
page contents rather than doing a binary search for each insertion,
but that seemed more invasive than I cared to risk in a back-patch.

Per bug #5157 from Jeff Janes and subsequent investigation.

Author: tglsfdc

src/backend/access/hash/README

@@ -1,14 +1,15 @@
-$PostgreSQL: pgsql/src/backend/access/hash/README,v 1.8 2008/03/20 17:55:14 momjian Exp $
+$PostgreSQL: pgsql/src/backend/access/hash/README,v 1.9 2009/11/01 21:25:25 tgl Exp $
 
 Hash Indexing
 =============
 
-This directory contains an implementation of hash indexing for Postgres.  Most
-of the core ideas are taken from Margo Seltzer and Ozan Yigit, A New Hashing
-Package for UNIX, Proceedings of the Winter USENIX Conference, January 1991.
-(Our in-memory hashtable implementation, src/backend/utils/hash/dynahash.c,
-also relies on some of the same concepts; it is derived from code written by
-Esmond Pitt and later improved by Margo among others.)
+This directory contains an implementation of hash indexing for Postgres.
+Most of the core ideas are taken from Margo Seltzer and Ozan Yigit,
+A New Hashing Package for UNIX, Proceedings of the Winter USENIX Conference,
+January 1991.  (Our in-memory hashtable implementation,
+src/backend/utils/hash/dynahash.c, also relies on some of the same concepts;
+it is derived from code written by Esmond Pitt and later improved by Margo
+among others.)
 
 A hash index consists of two or more "buckets", into which tuples are
 placed whenever their hash key maps to the bucket number.  The
@@ -32,6 +33,14 @@ rebuilding it with REINDEX.  Overflow pages can be recycled for reuse
 in other buckets, but we never give them back to the operating system.
 There is no provision for reducing the number of buckets, either.
 
+As of PostgreSQL 8.4, hash index entries store only the hash code, not the
+actual data value, for each indexed item.  This makes the index entries
+smaller (perhaps very substantially so) and speeds up various operations.
+In particular, we can speed searches by keeping the index entries in any
+one index page sorted by hash code, thus allowing binary search to be used
+within an index page.  Note however that there is *no* assumption about the
+relative ordering of hash codes across different index pages of a bucket.
+
 
 Page Addressing
 ---------------
@@ -205,8 +214,18 @@ the reader ensures that the target bucket calculation is valid (otherwise
 the bucket might be split before the reader arrives at it, and the target
 entries might go into the new bucket).  Holding the bucket sharelock for
 the remainder of the scan prevents the reader's current-tuple pointer from
-being invalidated by other processes.  Notice though that the reader need
-not prevent other buckets from being split or compacted.
+being invalidated by splits or compactions.  Notice that the reader's lock
+does not prevent other buckets from being split or compacted.
+
+To keep concurrency reasonably good, we require readers to cope with
+concurrent insertions, which means that they have to be able to re-find
+their current scan position after re-acquiring the page sharelock.  Since
+deletion is not possible while a reader holds the bucket sharelock, and
+we assume that heap tuple TIDs are unique, this can be implemented by
+searching for the same heap tuple TID previously returned.  Insertion does
+not move index entries across pages, so the previously-returned index entry
+should always be on the same page, at the same or higher offset number,
+as it was before.
 
 The insertion algorithm is rather similar:
 
@@ -220,19 +239,20 @@ The insertion algorithm is rather similar:
 	read/exclusive-lock current page of bucket
 	if full, release, read/exclusive-lock next page; repeat as needed
 	>> see below if no space in any page of bucket
-	insert tuple
+	insert tuple at appropriate place in page
 	write/release current page
 	release bucket share-lock
 	read/exclusive-lock meta page
 	increment tuple count, decide if split needed
 	write/release meta page
 	done if no split needed, else enter Split algorithm below
 
-It is okay for an insertion to take place in a bucket that is being
-actively scanned, because it does not change the position of any existing
-item in the bucket, so scan states are not invalidated.  We only need the
-short-term buffer locks to ensure that readers do not see a
-partially-updated page.
+To speed searches, the index entries within any individual index page are
+kept sorted by hash code; the insertion code must take care to insert new
+entries in the right place.  It is okay for an insertion to take place in a
+bucket that is being actively scanned, because readers can cope with this
+as explained above.  We only need the short-term buffer locks to ensure
+that readers do not see a partially-updated page.
 
 It is clearly impossible for readers and inserters to deadlock, and in
 fact this algorithm allows them a very high degree of concurrency.

src/backend/access/hash/hash.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/hash/hash.c,v 1.113 2009/07/29 20:56:18 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/hash/hash.c,v 1.114 2009/11/01 21:25:25 tgl Exp $
  *
  * NOTES
  *	  This file contains only the public interface routines.
@@ -206,8 +206,10 @@ hashgettuple(PG_FUNCTION_ARGS)
 	ScanDirection dir = (ScanDirection) PG_GETARG_INT32(1);
 	HashScanOpaque so = (HashScanOpaque) scan->opaque;
 	Relation	rel = scan->indexRelation;
+	Buffer		buf;
 	Page		page;
 	OffsetNumber offnum;
+	ItemPointer current;
 	bool		res;
 
 	/* Hash indexes are always lossy since we store only the hash code */
@@ -225,8 +227,38 @@ hashgettuple(PG_FUNCTION_ARGS)
 	 * appropriate direction.  If we haven't done so yet, we call a routine to
 	 * get the first item in the scan.
 	 */
-	if (ItemPointerIsValid(&(so->hashso_curpos)))
+	current = &(so->hashso_curpos);
+	if (ItemPointerIsValid(current))
 	{
+		/*
+		 * An insertion into the current index page could have happened while
+		 * we didn't have read lock on it.  Re-find our position by looking
+		 * for the TID we previously returned.  (Because we hold share lock on
+		 * the bucket, no deletions or splits could have occurred; therefore
+		 * we can expect that the TID still exists in the current index page,
+		 * at an offset >= where we were.)
+		 */
+		OffsetNumber maxoffnum;
+
+		buf = so->hashso_curbuf;
+		Assert(BufferIsValid(buf));
+		page = BufferGetPage(buf);
+		maxoffnum = PageGetMaxOffsetNumber(page);
+		for (offnum = ItemPointerGetOffsetNumber(current);
+			 offnum <= maxoffnum;
+			 offnum = OffsetNumberNext(offnum))
+		{
+			IndexTuple	itup;
+
+			itup = (IndexTuple) PageGetItem(page, PageGetItemId(page, offnum));
+			if (ItemPointerEquals(&scan->xs_ctup.t_self, &itup->t_tid))
+				break;
+		}
+		if (offnum > maxoffnum)
+			elog(ERROR, "failed to re-find scan position within index \"%s\"",
+				 RelationGetRelationName(rel));
+		ItemPointerSetOffsetNumber(current, offnum);
+
 		/*
 		 * Check to see if we should kill the previously-fetched tuple.
 		 */
@@ -235,16 +267,14 @@ hashgettuple(PG_FUNCTION_ARGS)
 			/*
 			 * Yes, so mark it by setting the LP_DEAD state in the item flags.
 			 */
-			offnum = ItemPointerGetOffsetNumber(&(so->hashso_curpos));
-			page = BufferGetPage(so->hashso_curbuf);
 			ItemIdMarkDead(PageGetItemId(page, offnum));
 
 			/*
 			 * Since this can be redone later if needed, it's treated the same
 			 * as a commit-hint-bit status update for heap tuples: we mark the
 			 * buffer dirty but don't make a WAL log entry.
 			 */
-			SetBufferCommitInfoNeedsSave(so->hashso_curbuf);
+			SetBufferCommitInfoNeedsSave(buf);
 		}
 
 		/*
@@ -262,7 +292,7 @@ hashgettuple(PG_FUNCTION_ARGS)
 	{
 		while (res)
 		{
-			offnum = ItemPointerGetOffsetNumber(&(so->hashso_curpos));
+			offnum = ItemPointerGetOffsetNumber(current);
 			page = BufferGetPage(so->hashso_curbuf);
 			if (!ItemIdIsDead(PageGetItemId(page, offnum)))
 				break;
@@ -517,7 +547,8 @@ hashbulkdelete(PG_FUNCTION_ARGS)
 			HashPageOpaque opaque;
 			OffsetNumber offno;
 			OffsetNumber maxoffno;
-			bool		page_dirty = false;
+			OffsetNumber deletable[MaxOffsetNumber];
+			int			ndeletable = 0;
 
 			vacuum_delay_point();
 
@@ -529,9 +560,10 @@ hashbulkdelete(PG_FUNCTION_ARGS)
 			Assert(opaque->hasho_bucket == cur_bucket);
 
 			/* Scan each tuple in page */
-			offno = FirstOffsetNumber;
 			maxoffno = PageGetMaxOffsetNumber(page);
-			while (offno <= maxoffno)
+			for (offno = FirstOffsetNumber;
+				 offno <= maxoffno;
+				 offno = OffsetNumberNext(offno))
 			{
 				IndexTuple	itup;
 				ItemPointer htup;
@@ -541,30 +573,25 @@ hashbulkdelete(PG_FUNCTION_ARGS)
 				htup = &(itup->t_tid);
 				if (callback(htup, callback_state))
 				{
-					/* delete the item from the page */
-					PageIndexTupleDelete(page, offno);
-					bucket_dirty = page_dirty = true;
-
-					/* don't increment offno, instead decrement maxoffno */
-					maxoffno = OffsetNumberPrev(maxoffno);
-
+					/* mark the item for deletion */
+					deletable[ndeletable++] = offno;
 					tuples_removed += 1;
 				}
 				else
-				{
-					offno = OffsetNumberNext(offno);
-
 					num_index_tuples += 1;
-				}
 			}
 
 			/*
-			 * Write page if needed, advance to next page.
+			 * Apply deletions and write page if needed, advance to next page.
 			 */
 			blkno = opaque->hasho_nextblkno;
 
-			if (page_dirty)
+			if (ndeletable > 0)
+			{
+				PageIndexMultiDelete(page, deletable, ndeletable);
 				_hash_wrtbuf(rel, buf);
+				bucket_dirty = true;
+			}
 			else
 				_hash_relbuf(rel, buf);
 		}

src/backend/access/hash/hashinsert.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/hash/hashinsert.c,v 1.52 2009/01/01 17:23:35 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/hash/hashinsert.c,v 1.53 2009/11/01 21:25:25 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -20,10 +20,6 @@
 #include "utils/rel.h"
 
 
-static OffsetNumber _hash_pgaddtup(Relation rel, Buffer buf,
-			   Size itemsize, IndexTuple itup);
-
-
 /*
  *	_hash_doinsert() -- Handle insertion of a single index tuple.
  *
@@ -180,15 +176,16 @@ _hash_doinsert(Relation rel, IndexTuple itup)
 /*
  *	_hash_pgaddtup() -- add a tuple to a particular page in the index.
  *
- *		This routine adds the tuple to the page as requested; it does
- *		not write out the page.  It is an error to call pgaddtup() without
- *		a write lock and pin.
+ * This routine adds the tuple to the page as requested; it does not write out
+ * the page.  It is an error to call pgaddtup() without pin and write lock on
+ * the target buffer.
+ *
+ * Returns the offset number at which the tuple was inserted.  This function
+ * is responsible for preserving the condition that tuples in a hash index
+ * page are sorted by hashkey value.
  */
-static OffsetNumber
-_hash_pgaddtup(Relation rel,
-			   Buffer buf,
-			   Size itemsize,
-			   IndexTuple itup)
+OffsetNumber
+_hash_pgaddtup(Relation rel, Buffer buf, Size itemsize, IndexTuple itup)
 {
 	OffsetNumber itup_off;
 	Page		page;