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

From: Tom Lane <[email-masked]>
Date: Tue, 23 May 2023 13:28:30 -0400

Andres Freund <[email-masked]> writes:
> I've wondered about some form of instrumentation to detect such issues
> before.

Yeah.

> 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?

It'd be very hard to set a limit for what's "too much", since the amount
of stuff created initially will depend on the plan size.  In any case
I think that the important issue is not how much absolute space, but is
there per-row leakage.  I wonder if we could do something involving
checking for continued growth after the first retrieved tuple, or
something like that.

> Random aside: I've been wondering whether it'd be worth introducing an
> in-place representation of Bitmap (e.g. if the low bit is set, the low 63 bits
> are in-place, if unset, it's a pointer).

Why?  Unlike Lists, those things are already a single palloc chunk.

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Tue, 23 May 2023 11:01:13 -0700

Hi,

On 2023-05-23 13:28:30 -0400, Tom Lane wrote:
> of stuff created initially will depend on the plan size.

I was thinking of some limit that should really never be reached outside of a
leak or work_mem based allocations, say 2GB or so.

> In any case I think that the important issue is not how much absolute space,
> but is there per-row leakage.  I wonder if we could do something involving
> checking for continued growth after the first retrieved tuple, or something
> like that.

As-is I some nodes do large allocations the query context, but are not
guaranteed to be reached when gathering the first row. So we would still have
to move such large allocations out of ExecutorState.

I think it might be best to go for a combination of these two
heuristics. Store the size of es_query_context after standard_ExecutorStart(),
that would include the allocation of the executor tree itself. Then in
standard_ExecutorEnd(), if the difference in size of ExecutorState is bigger
than some constant *and* is bigger than the initial size by some factor, emit
a warning.

The constant size difference avoids spurious warnings in case of a small plan
that just grows due to a few fmgr lookups or such, the factor takes care of
the plan complexity?

> Why?  Unlike Lists, those things are already a single palloc chunk.

We do a fair amount of 8 byte allocations - they have quite a bit of overhead,
even after c6e0fe1f2a0. Not needing allocations for the common case of
bitmapsets with a max member < 63 seems like it could be worth it.

Greetings,

Andres Freund

---

From: Jakub Wartak <[email-masked]>
Date: Wed, 24 May 2023 10:19:26 +0200

Hi, just two cents:

> leak or work_mem based allocations, say 2GB or so.

RE: instrumentation subthread:
if that helps then below technique can work somewhat good on normal
binaries for end users (given there are debug symbols installed), so
maybe we don't need that much infrastructure added in to see the hot
code path:

perf probe -x /path/to/postgres 'palloc' 'size=%di:u64' # RDI on
x86_64(palloc size arg0)
perf record -avg --call-graph dwarf -e probe_postgres:palloc -aR -p
<pid> sleep 3 # cannot be longer, huge overhead (~3s=~2GB)

it produces:
    50.27%  (563d0e380670) size=24
            |
            ---palloc
               bms_copy
               ExecUpdateLockMode
               ExecBRUpdateTriggers
               ExecUpdate
[..]

    49.73%  (563d0e380670) size=16
            |
            ---palloc
               bms_copy
               RelationGetIndexAttrBitmap
               ExecUpdateLockMode
               ExecBRUpdateTriggers
               ExecUpdate
[..]

Now we know that those small palloc() are guilty, but we didn't know
at the time with Tomas. The problem here is that we do not know in
palloc() - via its own arguments for which MemoryContext this is going
to be allocated for. This is problematic for perf, because on RHEL8, I
was not able to generate an uprobe that was capable of reaching a
global variable (CurrentMemoryContext) at that time.

Additionally what was even more frustrating on diagnosing that case on
the customer end system, was that such OOMs were crashing other
PostgreSQL clusters on the same OS. Even knowing the exact guilty
statement it was impossible to limit RSS memory usage of that
particular backend. So, what you are proposing makes a lot of sense.
Also it got me thinking of implementing safety-memory-net-GUC
debug_query_limit_backend_memory=X MB that would inject
setrlimit(RLIMIT_DATA) through external extension via hook(s) and
un-set it later, but the man page states it works for mmap() only
after Linux 4.7+ so it is future proof but won't work e.g. on RHEL7 -
maybe that's still good enough?; Or, well maybe try to hack a palloc()
a little, but that has probably too big overhead, right? (just
thinking loud).

-Jakub Wartak.

---

From: Tomas Vondra <[email-masked]>
Date: Wed, 24 May 2023 15:17:52 +0200

> global variable (CurrentMemoryContext) at that time.

I think there are a couple even bigger issues:

(a) There are other methods that allocate memory - repalloc, palloc0,
MemoryContextAlloc, ... and so on. But presumably we can trace all of
them at once? We'd have to trace reset/deletes too.

(b) This doesn't say if/when the allocated chunks get freed - even with
a fix, we'll still do exactly the same number of allocations, except
that we'll free the memory shortly after that. I wonder if we could
print a bit more info for each probe, to match the alloc/free requests.

> a little, but that has probably too big overhead, right? (just

Not sure about setting a hard limit - that seems pretty tricky and may
easily backfire. It's already possible to set such memory limit using
e.g. cgroups, or even better use VMs to isolate the instances.

I certainly agree it's annoying that when OOM hits, we end up with no
information about what used the memory. Maybe we could have a threshold
triggering a call to MemoryContextStats? So that we have at least some
memory usage info in the log in case the OOM killer intervenes.

regards

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