Subject: Re: memory leak in trigger handling (since PG12)

From: Tomas Vondra <[email-masked]>
Date: Tue, 23 May 2023 23:26:42 +0200

On 5/23/23 19:14, Andres Freund wrote:
> Hi,
> 
> On 2023-05-23 18:23:00 +0200, Tomas Vondra wrote:
>> This means that for an UPDATE with triggers, we may end up calling this
>> for each row, possibly multiple bitmaps. And those bitmaps are allocated
>> in ExecutorState, so won't be freed until the end of the query :-(
> 
> Ugh.
> 
> 
> I've wondered about some form of instrumentation to detect such issues
> before. It's obviously a problem that we can have fairly large leaks, like the
> one you just discovered, without detecting it for a couple years. It's kinda
> made worse by the memory context infrastructure, because it hides such issues.
> 
> Could it help to have a mode where the executor shutdown hook checks how much
> memory is allocated in ExecutorState and warns if its too much? There's IIRC a
> few places that allocate large things directly in it, but most of those
> probably should be dedicated contexts anyway.  Something similar could be
> useful for some other long-lived contexts.
> 

Not sure such simple instrumentation would help much, unfortunately :-(

We only discovered this because the user reported OOM crashes, which
means the executor didn't get to the shutdown hook at all. Yeah, maybe
if we had such instrumentation it'd get triggered for milder cases, but
I'd bet the amount of noise is going to be significant.

For example, there's a nearby thread about hashjoin allocating buffiles
etc. in ExecutorState - we ended up moving that to a separate context,
but surely there are more such cases (not just for ExecutorState).

The really hard thing was determining what causes the memory leak - the
simple instrumentation doesn't help with that at all. It tells you there
might be a leak, but you don't know where did the allocations came from.

What I ended up doing is a simple gdb script that sets breakpoints on
all palloc/pfree variants, and prints info (including the backtrace) for
each call on ExecutorState. And then a script that aggregate those to
identify which backtraces allocated most chunks that were not freed.

This was super slow (definitely useless outside development environment)
but it made it super obvious where the root cause is.

I experimented with instrumenting the code a bit (as alternative to gdb
breakpoints) - still slow, but much faster than gdb. But perhaps useful
for special (valgrind-like) testing mode ...

Would require testing with more data, though. I doubt we'd find much
with our regression tests, which have tiny data sets.

>  ...
>
>> Attached is a patch, restoring the pre-12 behavior for me.
> 
> Hm. Somehow this doesn't seem quite right. Shouldn't we try to use a shorter
> lived memory context instead? Otherwise we'll just end up with the same
> problem in a few years.
>

I agree using a shorter lived memory context would be more elegant, and
more in line with how we do things. But it's not clear to me why we'd
end up with the same problem in a few years with what the patch does.

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Tom Lane <[email-masked]>
Date: Tue, 23 May 2023 17:39:44 -0400

> identify which backtraces allocated most chunks that were not freed.

FWIW, I've had some success localizing palloc memory leaks with valgrind's
leak detection mode.  The trick is to ask it for a report before the
context gets destroyed.  Beats writing your own infrastructure ...

> with our regression tests, which have tiny data sets.

Yeah, it's not clear whether we could make the still-hypothetical check
sensitive enough to find leaks using small test cases without getting an
unworkable number of false positives.  Still, might be worth trying.
It might be an acceptable tradeoff to have stricter rules for what can
be allocated in ExecutorState in order to make this sort of problem
more easily detectable.

			regards, tom lane

---

From: Tomas Vondra <[email-masked]>
Date: Wed, 24 May 2023 10:49:14 +0200

> context gets destroyed.  Beats writing your own infrastructure ...

I haven't tried valgrind, so can't compare.

Would it be possible to filter which memory contexts to track? Say we
know the leak is in ExecutorState, so we don't need to track allocations
in other contexts. That was a huge speedup for me, maybe it'd help
valgrind too.

Also, how did you ask for the report before context gets destroyed?

> unworkable number of false positives.  Still, might be worth trying.

I'm not against experimenting with that. Were you thinking about
something that'd be cheap enough to just be enabled always/everywhere,
or something we'd enable during testing?

This reminded me a strangeloop talk [1] [2] about the Scalene memory
profiler from UMass. That's for Python, but they did some smart tricks
to reduce the cost of profiling - maybe we could do something similar,
possibly by extending the memory contexts a bit.

[1] https://youtu.be/vVUnCXKuNOg?t=1405
[2] https://youtu.be/vVUnCXKuNOg?t=1706

> be allocated in ExecutorState in order to make this sort of problem

Would these rules be just customary, or defined/enforced in the code
somehow? I can't quite imagine how would that work, TBH.

regards

---

From: Tom Lane <[email-masked]>
Date: Wed, 24 May 2023 11:57:24 -0400

> in other contexts. That was a huge speedup for me, maybe it'd help

I don't think valgrind has a way to do that, but this'd be something you
set up specially in any case.

> Also, how did you ask for the report before context gets destroyed?

There are several valgrind client requests that could be helpful:

/* Do a full memory leak check (like --leak-check=full) mid-execution. */
#define VALGRIND_DO_LEAK_CHECK                                   \
    VALGRIND_DO_CLIENT_REQUEST_STMT(VG_USERREQ__DO_LEAK_CHECK,   \
                                    0, 0, 0, 0, 0)

/* Same as VALGRIND_DO_LEAK_CHECK but only showing the entries for
   which there was an increase in leaked bytes or leaked nr of blocks
   since the previous leak search. */
#define VALGRIND_DO_ADDED_LEAK_CHECK                            \
    VALGRIND_DO_CLIENT_REQUEST_STMT(VG_USERREQ__DO_LEAK_CHECK,  \
                                    0, 1, 0, 0, 0)

/* Return number of leaked, dubious, reachable and suppressed bytes found by
   all previous leak checks.  They must be lvalues.  */
#define VALGRIND_COUNT_LEAK_BLOCKS(leaked, dubious, reachable, suppressed) \

Putting VALGRIND_DO_ADDED_LEAK_CHECK someplace in the executor loop would
help narrow things down pretty quickly, assuming you had a self-contained
example demonstrating the leak.  I don't recall exactly how I used these
but it was something along that line.

> or something we'd enable during testing?

We seem to have already paid the overhead of counting all palloc
allocations, so I don't think it'd be too expensive to occasionally check
the ExecutorState's mem_allocated and see if it's growing (especially
if we only do so in assert-enabled builds).  The trick is to define the
rules for what's worth reporting.

> somehow? I can't quite imagine how would that work, TBH.

If the check bleated "WARNING: possible executor memory leak" during
regression testing, people would soon become conditioned to doing
whatever they have to do to avoid it ;-)

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Wed, 24 May 2023 11:14:08 -0700

Hi,

> but surely there are more such cases (not just for ExecutorState).

Yes, that's why I said that we would have to more of those into dedicated
contexts - which is a good idea independent of this issue.

> identify which backtraces allocated most chunks that were not freed.

FWIW, for things like this I found "heaptrack" to be extremely helpful.

E.g. for a reproducer of the problem here, it gave me the attach "flame graph"
of the peak memory usage - after attaching to a running backend and running an
UPDATE triggering the leak..

Because the view I screenshotted shows the stacks contributing to peak memory
usage, it works nicely to find "temporary leaks", even if memory is actually
freed after all etc.

> end up with the same problem in a few years with what the patch does.

Because it sets up the pattern of manual memory management and continues to
run the relevant code within a query-lifetime context.

Greetings,

Andres Freund

---

From: Tomas Vondra <[email-masked]>
Date: Wed, 24 May 2023 21:56:22 +0200

> contexts - which is a good idea independent of this issue.

Yeah, I think that's a good idea in principle.

> usage, it works nicely to find "temporary leaks", even if memory is actually

That's a nice visualization, but isn't that useful only once you
determine there's a memory leak? Which I think is the hard problem.

> run the relevant code within a query-lifetime context.

Oh, you mean someone might add new allocations to this code (or into one
of the functions executed from it), and that'd leak again? Yeah, true.

regards

---

From: Andres Freund <[email-masked]>
Date: Wed, 24 May 2023 13:22:09 -0700

Hi,

> determine there's a memory leak? Which I think is the hard problem.

So is your gdb approach, unless I am misunderstanding? The view I
screenshotted shows the "peak" allocated memory, if you have a potential leak,
you can see where most of the allocated memory was allocated. Which at least
provides you with a good idea of where to look for a problem in more detail.

> of the functions executed from it), and that'd leak again? Yeah, true.

Yes. It's certainly not obvious this far down that we are called in a
semi-long-lived memory context.

Greetings,

Andres Freund

---

From: Tomas Vondra <[email-masked]>
Date: Thu, 25 May 2023 16:27:16 +0200

> provides you with a good idea of where to look for a problem in more detail.

Right, it wasn't my ambition to detect memory leaks but to source of the
leak if there's one. I got a bit distracted by the discussion detecting
leaks etc.

> semi-long-lived memory context.

That's true, but I don't see how adding a ExecGetAllUpdatedCols()
variant that allocates stuff in a short-lived context improves this.
That'll only cover memory allocated in ExecGetAllUpdatedCols() and
nothing else.

regards