Content-Type: multipart/mixed; boundary="------------4PHEJAioxGAjMqNODFO03M1K"
Message-ID: <b3c8da23-8850-7697-2aaf-947bb2a4d0a3@enterprisedb.com>
Date: Thu, 25 May 2023 16:41:14 +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)
Content-Language: en-US
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Andres Freund <andres@anarazel.de>
Cc: Tom Lane <tgl@sss.pgh.pa.us>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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>
In-Reply-To: <c3ff31ce-622c-81a9-9dcc-dbb27c5eaf9e@enterprisedb.com>
Archived-At: 
 <https://www.postgresql.org/message-id/b3c8da23-8850-7697-2aaf-947bb2a4d0a3%40enterprisedb.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------4PHEJAioxGAjMqNODFO03M1K
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


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
--------------4PHEJAioxGAjMqNODFO03M1K
Content-Type: text/x-patch; charset=UTF-8;
 name="0001-Use-per-tuple-context-in-ExecGetAllUpdatedCols.patch"
Content-Disposition: attachment;
 filename*0="0001-Use-per-tuple-context-in-ExecGetAllUpdatedCols.patch"
Content-Transfer-Encoding: base64

RnJvbSA5YTZlY2NlYmFlYTk2OWViNWQyZTE1ZGU5OWFhYWJiNDBiMDI0OTljIE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBUb21hcyBWb25kcmEgPHRvbWFzLnZvbmRyYUBwb3N0
Z3Jlc3FsLm9yZz4KRGF0ZTogVHVlLCAyMyBNYXkgMjAyMyAxNzo0NTo0NyArMDIwMApTdWJq
ZWN0OiBbUEFUQ0hdIFVzZSBwZXItdHVwbGUgY29udGV4dCBpbiBFeGVjR2V0QWxsVXBkYXRl
ZENvbHMKCi0tLQogc3JjL2JhY2tlbmQvZXhlY3V0b3IvZXhlY1V0aWxzLmMgfCAxMiArKysr
KysrKysrLS0KIDEgZmlsZSBjaGFuZ2VkLCAxMCBpbnNlcnRpb25zKCspLCAyIGRlbGV0aW9u
cygtKQoKZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2V4ZWN1dG9yL2V4ZWNVdGlscy5jIGIv
c3JjL2JhY2tlbmQvZXhlY3V0b3IvZXhlY1V0aWxzLmMKaW5kZXggMjVhYmE5YTI0M2EuLjM3
ZjJmYTE3M2Y2IDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9leGVjdXRvci9leGVjVXRpbHMu
YworKysgYi9zcmMvYmFja2VuZC9leGVjdXRvci9leGVjVXRpbHMuYwpAQCAtMTM0OSw4ICsx
MzQ5LDE2IEBAIEV4ZWNHZXRFeHRyYVVwZGF0ZWRDb2xzKFJlc3VsdFJlbEluZm8gKnJlbGlu
Zm8sIEVTdGF0ZSAqZXN0YXRlKQogQml0bWFwc2V0ICoKIEV4ZWNHZXRBbGxVcGRhdGVkQ29s
cyhSZXN1bHRSZWxJbmZvICpyZWxpbmZvLCBFU3RhdGUgKmVzdGF0ZSkKIHsKLQlyZXR1cm4g
Ym1zX3VuaW9uKEV4ZWNHZXRVcGRhdGVkQ29scyhyZWxpbmZvLCBlc3RhdGUpLAotCQkJCQkg
RXhlY0dldEV4dHJhVXBkYXRlZENvbHMocmVsaW5mbywgZXN0YXRlKSk7CisJQml0bWFwc2V0
CSAgICpyZXQ7CisJTWVtb3J5Q29udGV4dAlvbGRjeHQKKwkJPSBNZW1vcnlDb250ZXh0U3dp
dGNoVG8oR2V0UGVyVHVwbGVNZW1vcnlDb250ZXh0KGVzdGF0ZSkpOworCisJcmV0ID0gYm1z
X3VuaW9uKEV4ZWNHZXRVcGRhdGVkQ29scyhyZWxpbmZvLCBlc3RhdGUpLAorCQkJCQlFeGVj
R2V0RXh0cmFVcGRhdGVkQ29scyhyZWxpbmZvLCBlc3RhdGUpKTsKKworCU1lbW9yeUNvbnRl
eHRTd2l0Y2hUbyhvbGRjeHQpOworCisJcmV0dXJuIHJldDsKIH0KIAogLyoKLS0gCjIuNDAu
MQoK

--------------4PHEJAioxGAjMqNODFO03M1K--