Horton

Re: WIP: [[Parallel] Shared] Hash

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>

View thread View on postgr.es
From: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
To: Thomas Munro <thomas.munro@enterprisedb.com>
Cc: Peter Geoghegan <pg@heroku.com>, Haribabu Kommi <kommi.haribabu@gmail.com>, Pg Hackers <pgsql-hackers@postgresql.org>
Date: 2017-01-31T13:10:58Z
Lists: pgsql-hackers 
>
> 0003-hj-refactor-memory-accounting-v4.patch:
>
> Modify the existing hash join code to work in terms of chunks when
> estimating and later tracking memory usage.  This is probably more
> accurate than the current tuple-based approach, because it tries to
> take into account the space used by chunk headers and the wasted space
> in chunks.  In practice the difference is probably small, but it's
> arguably more accurate;  I did this because I need chunk-based
> accounting the later patches.  Also, make HASH_CHUNK_SIZE the actual
> size of allocated chunks (ie the header information is included in
> that size so we allocate exactly 32KB, not 32KB + a bit, for the
> benefit of the dsa allocator which otherwise finishes up allocating
> 36KB).
>
I looked at this patch. I agree that it accounts the memory usage more
accurately. Here are few comments.

spaceUsed is defined with comment
    Size        spaceUsed;        /* memory space currently used by tuples */

In ExecHashTableCreate(), although the space is allocated for buckets, no
tuples are yet inserted, so no space is used by the tuples, so going strictly
by the comment, spaceUsed should be 0 in that function. But I think the patch
is accounting the spaceUsed more accurately. Without this patch, the actual
allocation might cross spaceAllowed without being noticed. With this patch
that's not possible. Probably we should change the comment to say memory space
currently allocated. However, ExecHashIncreaseNumBatches() may change the
number of buckets; the patch does not seem to account for spaceUsed changes
because of that.

Without this patch ExecHashTableInsert() used to account for the space used by
a single tuple inserted. The patch moves this calculation in dense_alloc() and
accounts for out-of-bound allocation for larger tuples. That's good.

The change in ExecChooseHashTableSize() too looks fine.

In ExecHashTableReset(), do we want to update spacePeak while setting
spaceUsed.

While this patch tracks space usage more accurately, I am afraid we might be
overdoing it; a reason why we don't track space usage accurately now. But I
think I will leave it to be judged by someone who is more familiar with the
code and possibly has historical perspective.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Commits

  1. Add shared tuplestores.

  2. Remove BufFile's isTemp flag.

  3. Provide DSM segment to ExecXXXInitializeWorker functions.

  4. Optimize joins when the inner relation can be proven unique.

  5. Fix failure to use clamp_row_est() for parallel joins.

  6. Document lack of validation when attaching foreign partitions.

  7. Fix inclusions of c.h from .h files.

  8. Fix inclusions of postgres_fe.h from .h files.

  9. Bring plpgsql into line with header inclusion policy.

  10. Document intentional violations of header inclusion policy.

  11. Shut down Gather's children before shutting down Gather itself.

  12. btree: Support parallel index scans.

  13. Add explicit ORDER BY to a few tests that exercise hash-join code.

  14. Revise hash join code so that we can increase the number of batches

  15. Rewrite hash join to use simple linked lists instead of a