Обсуждение: [HACKERS] Relcache leak when row triggers on partitions are fired by COPY

Hi hackers,

While testing the patch I'm writing for the transition table open
item, I noticed that we can leak Relation objects like this:

postgres=# create table parent (a text, b int) partition by list (a);
CREATE TABLE
postgres=# create table child partition of parent for values in ('AAA');
CREATE TABLE
postgres=# create or replace function my_trigger_func() returns
trigger language plpgsql as $$ begin raise notice 'hello'; return
null; end; $$;
CREATE FUNCTION
postgres=# create trigger child_trigger after insert or update or
delete on child for each row execute procedure my_trigger_func();
CREATE TRIGGER
postgres=# copy parent (a, b) from stdin;
Enter data to be copied followed by a newline.
End with a backslash and a period on a line by itself.
>> AAA 42
>> \.
NOTICE:  hello
WARNING:  relcache reference leak: relation "child" not closed
COPY 1

The leaked object is created by ExecGetTriggerResultRel, called by
afterTriggerInvokeEvents.  That function relies on someone, usually
ExecEndPlan or EvalPlanQualEnd, to clean up es_trig_target_relations.
Shouldn't copy.c do the same thing (see attached)?  Or perhaps there
should there be an ExecCleanupTriggerState(estate) used by all places?

-- 
Thomas Munro
http://www.enterprisedb.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi Thomas,

On 2017/05/16 9:12, Thomas Munro wrote:
> Hi hackers,
> 
> While testing the patch I'm writing for the transition table open
> item, I noticed that we can leak Relation objects like this:
> 
> postgres=# create table parent (a text, b int) partition by list (a);
> CREATE TABLE
> postgres=# create table child partition of parent for values in ('AAA');
> CREATE TABLE
> postgres=# create or replace function my_trigger_func() returns
> trigger language plpgsql as $$ begin raise notice 'hello'; return
> null; end; $$;
> CREATE FUNCTION
> postgres=# create trigger child_trigger after insert or update or
> delete on child for each row execute procedure my_trigger_func();
> CREATE TRIGGER
> postgres=# copy parent (a, b) from stdin;
> Enter data to be copied followed by a newline.
> End with a backslash and a period on a line by itself.
>>> AAA 42
>>> \.
> NOTICE:  hello
> WARNING:  relcache reference leak: relation "child" not closed
> COPY 1

Thanks for the test case and the patch.

> The leaked object is created by ExecGetTriggerResultRel, called by
> afterTriggerInvokeEvents.  That function relies on someone, usually
> ExecEndPlan or EvalPlanQualEnd, to clean up es_trig_target_relations.
> Shouldn't copy.c do the same thing (see attached)?  Or perhaps there
> should there be an ExecCleanupTriggerState(estate) used by all places?

I vote for ExecCleanupTriggerState(estate).  After your patch, there will
be 4 places, including afterTriggerInvokeEvents(), ExecEndPlan(), and
EvalPlanQualEnd(), that repeat the same block of code.

Thanks,
Amit

On Tue, May 16, 2017 at 12:32 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> I vote for ExecCleanupTriggerState(estate).  After your patch, there will
> be 4 places, including afterTriggerInvokeEvents(), ExecEndPlan(), and
> EvalPlanQualEnd(), that repeat the same block of code.

Ok, here's a patch like that.  The call to ExecCloseIndices() may
technically be redundant (we never opened them).

-- 
Thomas Munro
http://www.enterprisedb.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2017/05/16 10:03, Thomas Munro wrote:
> On Tue, May 16, 2017 at 12:32 PM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> I vote for ExecCleanupTriggerState(estate).  After your patch, there will
>> be 4 places, including afterTriggerInvokeEvents(), ExecEndPlan(), and
>> EvalPlanQualEnd(), that repeat the same block of code.
> 
> Ok, here's a patch like that.

Thanks, looks good to me.

>  The call to ExecCloseIndices() may
> technically be redundant (we never opened them).

Actually yes.  We never do ExecOpenIndices() on the ResultRelInfos
contained in es_trig_target_relations.

Thanks,
Amit

On Mon, May 15, 2017 at 9:17 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> Ok, here's a patch like that.
>
> Thanks, looks good to me.

Committed.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company