From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Andres Freund <andres@anarazel.de>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: memory leak in trigger handling (since PG12)
Date: Thu, 25 May 2023 16:41:14 +0200
Message-ID: <b3c8da23-8850-7697-2aaf-947bb2a4d0a3@enterprisedb.com> (raw)
In-Reply-To: <c3ff31ce-622c-81a9-9dcc-dbb27c5eaf9e@enterprisedb.com>
References: <222a3442-7f7d-246c-ed9b-a76209d19239@enterprisedb.com>
<960113.1684859980@sss.pgh.pa.us>
<a5d42a2c-0c2e-9fb8-793f-0675d85669be@enterprisedb.com>
<f706d912-b6fa-f1d7-5a39-dc1be5999183@enterprisedb.com>
<20230524185559.un4dqfem3u3ktons@awork3.anarazel.de>
<c3ff31ce-622c-81a9-9dcc-dbb27c5eaf9e@enterprisedb.com>
On 5/24/23 21:49, Tomas Vondra wrote:
>
>
> On 5/24/23 20:55, Andres Freund wrote:
>> Hi,
>>
>> On 2023-05-24 15:38:41 +0200, Tomas Vondra wrote:
>>> I looked at this again, and I think GetPerTupleMemoryContext(estate)
>>> might do the trick, see the 0002 part.
>>
>> Yea, that seems like the right thing here.
>>
>>
>>> Unfortunately it's not much
>>> smaller/simpler than just freeing the chunks, because we end up doing
>>>
>>> oldcxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
>>> updatedCols = ExecGetAllUpdatedCols(relinfo, estate);
>>> MemoryContextSwitchTo(oldcxt);
>>
>> We could add a variant of ExecGetAllUpdatedCols that switches the context.
>>
>
> Yeah, we could do that. I was thinking about backpatching, and modifying
> ExecGetAllUpdatedCols signature would be ABI break, but adding a
> variant should be fine.
>
>>
>>> and then have to pass updatedCols elsewhere. It's tricky to just switch
>>> to the context (e.g. in ExecASUpdateTriggers/ExecARUpdateTriggers), as
>>> AfterTriggerSaveEvent allocates other bits of memory too (in a longer
>>> lived context).
>>
>> Hm - on a quick look the allocations in trigger.c itself are done with
>> MemoryContextAlloc().
>>
>> I did find a problematic path, namely that ExecGetChildToRootMap() ends up
>> building resultRelInfo->ri_ChildToRootMap in CurrentMemoryContext.
>>
>> That seems like a flat out bug to me - we can't just store data in a
>> ResultRelInfo without ensuring the memory is lives long enough. Nearby places
>> like ExecGetRootToChildMap() do make sure to switch to es_query_cxt.
>>
>>
>> Did you see other bits of memory getting allocated in CurrentMemoryContext?
>>
>
> No, I simply tried to do the context switch and then gave up when it
> crashed on the ExecGetRootToChildMap info. I haven't looked much
> further, but you may be right it's the only bit.
>
> It didn't occur to me it might be a bug - I think the code simply
> assumes it gets called with suitable memory context, just like we do in
> various other places. Maybe it should document the assumption.
>
>>
>>> So we'd have to do another switch again. Not sure how
>>> backpatch-friendly would that be.
>>
>> Yea, that's a valid concern. I think it might be reasonable to use something
>> like ExecGetAllUpdatedColsCtx() in the backbranches, and switch to a
>> short-lived context for the trigger invocations in >= 16.
>>
>
The attached patch does this - I realized we actually have estate in
ExecGetAllUpdatedCols(), so we don't even need a variant with a
different signature. That makes the patch much simpler.
The question is whether we need the signature anyway. There might be a
caller expecting the result to be in CurrentMemoryContext (be it
ExecutorState or something else), and this change might break it. But
I'm not aware of any callers, so maybe that's fine.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:0001-Use-per-tuple-context-in-ExecGetAllUpdatedCols.patch (text/x-patch, 1.1K) download
From 9a6eccebaea969eb5d2e15de99aaabb40b02499c Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas.vondra@postgresql.org>
Date: Tue, 23 May 2023 17:45:47 +0200
Subject: [PATCH] Use per-tuple context in ExecGetAllUpdatedCols
---
src/backend/executor/execUtils.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.cindex 25aba9a243a..37f2fa173f6 100644--- a/src/backend/executor/execUtils.c+++ b/src/backend/executor/execUtils.c@@ -1349,8 +1349,16 @@ ExecGetExtraUpdatedCols(ResultRelInfo *relinfo, EState *estate)
Bitmapset *
ExecGetAllUpdatedCols(ResultRelInfo *relinfo, EState *estate)
{
- return bms_union(ExecGetUpdatedCols(relinfo, estate),- ExecGetExtraUpdatedCols(relinfo, estate));+ Bitmapset *ret;+ MemoryContext oldcxt+ = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));++ ret = bms_union(ExecGetUpdatedCols(relinfo, estate),+ ExecGetExtraUpdatedCols(relinfo, estate));++ MemoryContextSwitchTo(oldcxt);++ return ret;
}
/*
--
2.40.1
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: pgsql-hackers@postgresql.org
Cc: tomas.vondra@enterprisedb.com, andres@anarazel.de, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org
Subject: Re: memory leak in trigger handling (since PG12)
In-Reply-To: <b3c8da23-8850-7697-2aaf-947bb2a4d0a3@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox