On Wed, May 15, 2019 at 06:19:38PM +0800, Hubert Zhang wrote: >Hi all, > >When we build hash table for a hash join node etc., we split tuples into >different hash buckets. Since tuples could not all be held in memory. >Postgres splits each bucket into batches, only the current batch of bucket >is in memory while other batches are written to disk. > >During ExecHashTableInsert(), if the memory cost exceeds the operator >allowed limit(hashtable->spaceAllowed), batches will be split on the fly by >calling ExecHashIncreaseNumBatches(). > >In past, if data is distributed unevenly, the split of batch may failed(All >the tuples falls into one split batch and the other batch is empty) Then >Postgres will set hashtable->growEnable to false. And never expand batch >number any more. > >If tuples become diverse in future, spliting batch is still valuable and >could avoid the current batch become too big and finally OOM. > >To fix this, we introduce a penalty on hashtable->spaceAllowed, which is >the threshold to determine whether to increase batch number. >If batch split failed, we increase the penalty instead of just turn off the >growEnable flag. > >Any comments? >
There's already another thread discussing various issues with how hashjoin increases the number of batches, including various issues with how/when we disable adding more batches.
In general I think you're right something like this is necessary, but I think we may need to rethink growEnable a bit more.
For example, the way you implemented it, after reaching the increased limit, we just increase the number of batches just like today, and then decide whether it actually helped. But that means we double the number of BufFile entries, which uses more and more memory (because each is 8kB and we need 1 per batch). I think in this case (after increasing the limit) we should check whether increasing batches makes sense or not. And only do it if it helps. Otherwise we'll double the amount of memory for BufFile(s) and also the work_mem. That's not a good idea.
But as I said, there are other issues discussed on the other thread. For example we only disable the growth when all rows fall into the same batch. But that's overly strict.