Re: Explanation for bug #13908: hash joins are badly broken

Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> writes:
> On 02/06/2016 08:39 PM, Andres Freund wrote:
>> FWIW, I've done that at some point. Noticeable speedups (that's what
>> I cared about), but a bit annoying to use. There's many random
>> pfree()s around, and then there's MemoryContextContains(),
>> GetMemoryChunkContext(), GetMemoryChunkSpace() - which all are
>> pretty fundamentally incompatible with such an allocator. I ended up
>> having a full header when assertions are enabled, to be able to
>> detect usage of these functions and assert out.
>>
>> I didn't concentrate on improving memory usage, but IIRC it was even
>> noticeable for some simpler things.

> I think the hassle is not that bad when most of the fragments have the
> same life cycle. With hashjoin that's almost exactly the case, except
> when we realize we need to increase the number of buckets - in that case
> we need to split the set of accumulated tuples in two.

Yeah, I think that a context type that just admits "we'll crash if you try
to pfree" would only be usable for allocations that are managed by just
a very small amount of code --- but the hashjoin tuple table qualifies,
and I think there would be other use-cases, perhaps tuplesort/tuplestore.

Andres' idea of adding a chunk header only in assert builds isn't a bad
one, perhaps; though I think the near-certainty of a core dump if you try
to use the header for anything might be good enough. pfree and repalloc
are an ironclad certainty to crash in a pretty obvious way, and we could
likely add some assert checks to MemoryContextContains and friends to make
them 99.99% certain to fail without paying the price of a chunk header.

regards, tom lane