Re: Memory leak from ExecutorState context?

On Sun, May 14, 2023 at 12:10:00AM +0200, Tomas Vondra wrote:
> On 5/12/23 23:36, Melanie Plageman wrote:
> > Thanks for continuing to work on this.
> > 
> > Are you planning to modify what is displayed for memory usage in
> > EXPLAIN ANALYZE?
> > 
> 
> We could do that, but we can do that separately - it's a separate and
> independent improvement, I think.
> 
> Also, do you have a proposal how to change the explain output? In
> principle we already have the number of batches, so people can calculate
> the "peak" amount of memory (assuming they realize what it means).

I don't know that we can expect people looking at the EXPLAIN output to
know how much space the different file buffers are taking up. Not to
mention that it is different for parallel hash join.

I like Jean-Guillaume's idea in his email responding to this point:

> We could add the batch memory consumption with the number of batches. Eg.:

>  Buckets: 4096 (originally 4096) 
>  Batches: 32768 (originally 8192) using 256MB
>  Memory Usage: 192kB

However, I think we can discuss this for 17.

> I think the main problem with adding this info to EXPLAIN is that I'm
> not sure it's very useful in practice. I've only really heard about this
> memory explosion issue when the query dies with OOM or takes forever,
> but EXPLAIN ANALYZE requires the query to complete.
> 
> A separate memory context (which  pg_log_backend_memory_contexts can
> dump to server log) is more valuable, I think.

Yes, I'm satisfied with scoping this to only the patch with the
dedicated memory context for now.

> > Also, since that won't help a user who OOMs, I wondered if the spillCxt
> > is helpful on its own or if we need some kind of logging message for
> > users to discover that this is what led them to running out of memory.
> > 
> 
> I think the separate memory context is definitely an improvement,
> helpful on it's own it makes it clear *what* allocated the memory. It
> requires having the memory context stats, but we may already dump them
> into the server log if malloc returns NULL. Granted, it depends on how
> the system is configured and it won't help when OOM killer hits :-(

Right. I suppose if someone had an OOM and the OOM killer ran, they may
be motivated to disable vm overcommit and then perhaps the memory
context name will show up somewhere in an error message or log?

> I wouldn't object to having some sort of log message, but when exactly
> would we emit it? Presumably after exceeding some amount of memory, but
> what would it be? It can't be too low (not to trigger it too often) or
> too high (failing to report the issue). Also, do you think it should go
> to the user or just to the server log?

I think where the log is delivered is dependent on under what conditions
we log -- if it is fairly preemptive, then doing so in the server log is
enough.

However, I think we can discuss this in the future. You are right that
the dedicated memory context by itself is an improvement.
Determining when to emit the log message seems like it will be too
difficult to accomplish in a day or so.

- Melanie