Thread: [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
Attachment
Re: [HACKERS] Relcache leak when row triggers on partitions are firedby COPY
From
Amit Langote
Date:
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
Re: [HACKERS] Relcache leak when row triggers on partitions are firedby COPY
From
Thomas Munro
Date:
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
Attachment
Re: [HACKERS] Relcache leak when row triggers on partitions are firedby COPY
From
Amit Langote
Date:
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