Fix building of large (bigger than shared_buffers) hash indexes.

tglsfdc · tglsfdc · commit 07f69137b15e · 2016-06-24T16:57:36.000-04:00
When the index is predicted to need more than NBuffers buckets, CREATE INDEX attempts to sort the index entries by hash key before insertion, so as to reduce thrashing. This code path got broken by commit 9f03ca9, which overlooked that _hash_form_tuple() is not just an alias for index_form_tuple(). The index got built anyway, but with garbage data, so that searches for pre-existing tuples always failed. Fix by refactoring to separate construction of the indexable data from calling index_form_tuple(). Per bug #14210 from Daniel Newman. Back-patch to 9.5 where the bug was introduced. Report: <20160623162507.17237.39471@wrigleys.postgresql.org>
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
@@ -140,19 +140,25 @@ hashbuildCallback(Relation index,
 				  void *state)
 {
 	HashBuildState *buildstate = (HashBuildState *) state;
+	Datum		index_values[1];
+	bool		index_isnull[1];
 	IndexTuple	itup;
 
-	/* Hash indexes don't index nulls, see notes in hashinsert */
-	if (isnull[0])
+	/* convert data to a hash key; on failure, do not insert anything */
+	if (!_hash_convert_tuple(index,
+							 values, isnull,
+							 index_values, index_isnull))
 		return;
 
 	/* Either spool the tuple for sorting, or just put it into the index */
 	if (buildstate->spool)
-		_h_spool(buildstate->spool, &htup->t_self, values, isnull);
+		_h_spool(buildstate->spool, &htup->t_self,
+				 index_values, index_isnull);
 	else
 	{
 		/* form an index tuple and point it at the heap tuple */
-		itup = _hash_form_tuple(index, values, isnull);
+		itup = index_form_tuple(RelationGetDescr(index),
+								index_values, index_isnull);
 		itup->t_tid = htup->t_self;
 		_hash_doinsert(index, itup);
 		pfree(itup);
@@ -179,22 +185,18 @@ hashinsert(PG_FUNCTION_ARGS)
 	Relation	heapRel = (Relation) PG_GETARG_POINTER(4);
 	IndexUniqueCheck checkUnique = (IndexUniqueCheck) PG_GETARG_INT32(5);
 #endif
+	Datum		index_values[1];
+	bool		index_isnull[1];
 	IndexTuple	itup;
 
-	/*
-	 * If the single index key is null, we don't insert it into the index.
-	 * Hash tables support scans on '='. Relational algebra says that A = B
-	 * returns null if either A or B is null.  This means that no
-	 * qualification used in an index scan could ever return true on a null
-	 * attribute.  It also means that indices can't be used by ISNULL or
-	 * NOTNULL scans, but that's an artifact of the strategy map architecture
-	 * chosen in 1986, not of the way nulls are handled here.
-	 */
-	if (isnull[0])
+	/* convert data to a hash key; on failure, do not insert anything */
+	if (!_hash_convert_tuple(rel,
+							 values, isnull,
+							 index_values, index_isnull))
 		PG_RETURN_BOOL(false);
 
-	/* generate an index tuple */
-	itup = _hash_form_tuple(rel, values, isnull);
+	/* form an index tuple and point it at the heap tuple */
+	itup = index_form_tuple(RelationGetDescr(rel), index_values, index_isnull);
 	itup->t_tid = *ht_ctid;
 
 	_hash_doinsert(rel, itup);
diff --git a/src/backend/access/hash/hashutil.c b/src/backend/access/hash/hashutil.c
@@ -248,27 +248,37 @@ _hash_get_indextuple_hashkey(IndexTuple itup)
 }
 
 /*
- * _hash_form_tuple - form an index tuple containing hash code only
+ * _hash_convert_tuple - convert raw index data to hash key
+ *
+ * Inputs: values and isnull arrays for the user data column(s)
+ * Outputs: values and isnull arrays for the index tuple, suitable for
+ *		passing to index_form_tuple().
+ *
+ * Returns true if successful, false if not (because there are null values).
+ * On a false result, the given data need not be indexed.
+ *
+ * Note: callers know that the index-column arrays are always of length 1.
+ * In principle, there could be more than one input column, though we do not
+ * currently support that.
  */
-IndexTuple
-_hash_form_tuple(Relation index, Datum *values, bool *isnull)
+bool
+_hash_convert_tuple(Relation index,
+					Datum *user_values, bool *user_isnull,
+					Datum *index_values, bool *index_isnull)
 {
-	IndexTuple	itup;
 	uint32		hashkey;
-	Datum		hashkeydatum;
-	TupleDesc	hashdesc;
 
-	if (isnull[0])
-		hashkeydatum = (Datum) 0;
-	else
-	{
-		hashkey = _hash_datum2hashkey(index, values[0]);
-		hashkeydatum = UInt32GetDatum(hashkey);
-	}
-	hashdesc = RelationGetDescr(index);
-	Assert(hashdesc->natts == 1);
-	itup = index_form_tuple(hashdesc, &hashkeydatum, isnull);
-	return itup;
+	/*
+	 * We do not insert null values into hash indexes.  This is okay because
+	 * the only supported search operator is '=', and we assume it is strict.
+	 */
+	if (user_isnull[0])
+		return false;
+
+	hashkey = _hash_datum2hashkey(index, user_values[0]);
+	index_values[0] = UInt32GetDatum(hashkey);
+	index_isnull[0] = false;
+	return true;
 }
 
 /*
diff --git a/src/include/access/hash.h b/src/include/access/hash.h
@@ -350,8 +350,9 @@ extern Bucket _hash_hashkey2bucket(uint32 hashkey, uint32 maxbucket,
 extern uint32 _hash_log2(uint32 num);
 extern void _hash_checkpage(Relation rel, Buffer buf, int flags);
 extern uint32 _hash_get_indextuple_hashkey(IndexTuple itup);
-extern IndexTuple _hash_form_tuple(Relation index,
-				 Datum *values, bool *isnull);
+extern bool _hash_convert_tuple(Relation index,
+					Datum *user_values, bool *user_isnull,
+					Datum *index_values, bool *index_isnull);
 extern OffsetNumber _hash_binsearch(Page page, uint32 hash_value);
 extern OffsetNumber _hash_binsearch_last(Page page, uint32 hash_value);
 
