From 8cf739de850f596777be14aeb430359cf59cf47c Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 24 Jun 2016 16:57:36 -0400 Subject: [PATCH] Fix building of large (bigger than shared_buffers) hash indexes. 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 9f03ca915196dfc8, 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> --- src/backend/access/hash/hash.c | 34 ++++++++++++----------- src/backend/access/hash/hashutil.c | 44 ++++++++++++++++++------------ src/include/access/hash.h | 5 ++-- 3 files changed, 48 insertions(+), 35 deletions(-) diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c index 49a6c816aa..19695ee102 100644 --- a/src/backend/access/hash/hash.c +++ b/src/backend/access/hash/hash.c @@ -176,19 +176,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); @@ -208,22 +214,18 @@ hashinsert(Relation rel, Datum *values, bool *isnull, ItemPointer ht_ctid, Relation heapRel, IndexUniqueCheck checkUnique) { + 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)) return 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 index 456954b063..822862db7a 100644 --- a/src/backend/access/hash/hashutil.c +++ b/src/backend/access/hash/hashutil.c @@ -240,27 +240,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 index fa3f9b61ca..ce314180e6 100644 --- a/src/include/access/hash.h +++ b/src/include/access/hash.h @@ -359,8 +359,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);