Re: Use BumpContext contexts for TupleHashTables' tablecxt

I wrote:
> David Rowley <dgrowleyml(at)gmail(dot)com> writes:
>> I can't help wonder if we can improve the memory context parameter
>> names in BuildTupleHashTable(). Every time I see "tablecxt" I have to
>> remind myself that it's not for the bucket array, just the stuff we
>> have the buckets point to. Would "hashedtuplecxt" be better?

> I agree these names are not great. I think they might be leftovers
> from a time when there actually was a complete hash-table structure
> in that context.

Looking closer, it seems like a lot of the confusion here arose when
TupleHashTables were rebased onto simplehash.h. That patch seems to
have paid about zero attention to updating comments that it'd
falsified or at least made misleading. Here's a v2 that tries to
clean up some of the mess.

I'm also proposing to move the MemoryContextReset of that context
into ResetTupleHashTable instead of documenting that we expect the
caller to do that. The old way just seems like a bizarre artifact.

> Related to this, while I was chasing Jeff's complaint I realized that
> the none-too-small simplehash table for this is getting made in the
> query's ExecutorState. That's pretty awful from the standpoint of
> being able to blame memory consumption on the hash node.

I didn't do anything about this. I briefly considered having
BuildTupleHashTable construct a per-hashtable context, but backed
off after seeing that nodeAgg.c goes to some effort to use the same
metacxt and tuplescxt for several hashtables. I'm not sure if that
had measured performance benefits behind it, but it well might have.
I now think that if we care, the right thing is to make the remaining
callers construct extra memory contexts for their hash tables. That
could be left for another day.

regards, tom lane