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
Attachments:
fix-relcache-leak-in-copy-with-triggers.patchapplication/octet-stream; name=fix-relcache-leak-in-copy-with-triggers.patchDownload+10-0
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
fix-relcache-leak-in-copy-with-triggers-v2.patchapplication/octet-stream; name=fix-relcache-leak-in-copy-with-triggers-v2.patchDownload+48-28
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers