public inbox for [email protected]  

help / color / mirror / Atom feed

From: Andres Freund <[email protected]>
To: Tom Lane <[email protected]>
Cc: Tomas Vondra <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: memory leak in trigger handling (since PG12)
Date: Tue, 23 May 2023 11:01:13 -0700
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>

Hi,

On 2023-05-23 13:28:30 -0400, Tom Lane wrote:
> Andres Freund <[email protected]> writes:
> > 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.

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?


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

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

view thread (5+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: memory leak in trigger handling (since PG12)
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox