Re: memory leak in trigger handling (since PG12)

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.

> The bitmaps are typically fairly small (a couple bytes), but for wider
> tables it can be a couple dozen bytes. But it's primarily driven by
> number of updated rows.

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

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

Greetings,

Andres Freund