Jeff Davis <pgsql@j-davis.com> writes:
> This patch tracks memory usage (at the block level) for all memory
> contexts. Individual palloc()s aren't tracked; only the new blocks
> allocated to the memory context with malloc().
> ...
> My general answer to the performance concerns is that they aren't a
> reason to block this patch, unless someone has a suggestion about how to
> fix them. Adding one field to a struct and a few arithmetic operations
> for each malloc() or realloc() seems reasonable to me.
Maybe, but this here is a pretty weak argument:
> The current state, where HashAgg just blows up the memory, is just not
> reasonable, and we need to track the memory to fix that problem.
Meh. HashAgg could track its memory usage without loading the entire
system with a penalty. Moreover, this is about fourth or fifth down the
list of the implementation problems with spilling hash aggregation to
disk. It would be good to see credible solutions for the bigger issues
before we buy into adding overhead for a mechanism with no uses in core.
> Others have also mentioned that we might want to use this mechanism to
> track memory for other operators, like Sort or HashJoin, which might be
> simpler and more accurate.
That would imply redefining the meaning of work_mem for those operations;
it would suddenly include a lot more overhead space than it used to,
causing them to spill to disk much more quickly than before, unless one
changes the work_mem setting to compensate. Not sure that people would
like such a change.
An even bigger problem is that it would pretty much break the logic around
LACKMEM() in tuplesort.c, which supposes that spilling individual tuples
to disk is a reliable and linear way to decrease the accounted-for memory.
Individual pfree's would typically not decrease the accounted total in
this implementation. Depending on what you have in mind for the
spill-to-disk behavior, this issue could be a fail for HashAgg as well,
which is another reason not to commit this patch in advance of seeing the
use-case.
regards, tom lane
