Message-ID: <5479b188-17b1-ad6b-bf7d-fcefaf9d382d@enterprisedb.com>
Date: Thu, 25 May 2023 16:27:16 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: memory leak in trigger handling (since PG12)
To: Andres Freund <andres@anarazel.de>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <222a3442-7f7d-246c-ed9b-a76209d19239@enterprisedb.com>
 <20230523171433.earidmyzock7fnk4@awork3.anarazel.de>
 <b9f40ba0-6b8b-0a4a-ad10-891e6f41d882@enterprisedb.com>
 <20230524181408.wlgzhoe7he7h3tjk@awork3.anarazel.de>
 <ee89cce4-6b08-5c06-fd88-4fef5a204875@enterprisedb.com>
 <20230524202209.xdgmvz7hg23xy76d@awork3.anarazel.de>
Content-Language: en-US
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: <20230524202209.xdgmvz7hg23xy76d@awork3.anarazel.de>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/5479b188-17b1-ad6b-bf7d-fcefaf9d382d%40enterprisedb.com>
Precedence: bulk


On 5/24/23 22:22, Andres Freund wrote:
> Hi,
> 
> On 2023-05-24 21:56:22 +0200, Tomas Vondra wrote:
>>>> 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.
>>>
>>> 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.
>>>
>>
>> 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.
> 
> 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.
> 

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.

> 
>>>>> 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.
>>>
>>> Because it sets up the pattern of manual memory management and continues to
>>> 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.
> 
> Yes. It's certainly not obvious this far down that we are called in a
> 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

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