From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Alexander Pyhalov <a.pyhalov@postgrespro.ru>
Cc: 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, 22 Jun 2023 13:46:02 +0200
Message-ID: <36e564f7-6cde-927d-9089-19471a05e8f3@enterprisedb.com> (raw)
In-Reply-To: <acddb17c89b0d6cb940eaeda18c08bbe@postgrespro.ru>
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>
<b3c8da23-8850-7697-2aaf-947bb2a4d0a3@enterprisedb.com>
<acddb17c89b0d6cb940eaeda18c08bbe@postgrespro.ru>
On 6/22/23 13:07, Alexander Pyhalov wrote:
> Tomas Vondra писал 2023-05-25 17:41:
>
>> 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.
>>
>
> Hi.
> Unfortunately, this patch has broken trigdata->tg_updatedcols usage in
> AFTER UPDATE triggers.
> Should it be if not fixed, but at least mentioned in documentation?
>
That's clearly a bug, we can't just break stuff like that.
> Attaching sample code. After creating trigger, an issue can be
> reproduced as this:
>
> create table test (i int, j int);
> create function report_update_fields() RETURNS TRIGGER AS
> '/location/to/trig_test.so' language C;
> create trigger test_update after update ON test FOR EACH ROW EXECUTE
> FUNCTION report_update_fields();
> insert into test values (1, 12);
> update test set j=2;
>
I haven't tried the reproducer, but I think I see the issue - we store
the bitmap as part of the event to be executed later, but the bitmap is
in per-tuple context and gets reset. So I guess we need to copy it into
the proper long-lived context (e.g. AfterTriggerEvents).
I'll get that fixed.
Anyway, this probably hints we should not recalculate the bitmap over
and over, but calculate it once and keep it for all events. Not
something we can do in backbranches, though.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
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, a.pyhalov@postgrespro.ru, 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: <36e564f7-6cde-927d-9089-19471a05e8f3@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