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
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index f22d0a07987..2fc1f9bbe8b 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2315,6 +2315,7 @@ CopyFrom(CopyState cstate)
HeapTuple *bufferedTuples = NULL; /* initialize to silence warning */
Size bufferedTuplesSize = 0;
int firstBufferedLineNo = 0;
+ ListCell *l;
Assert(cstate->rel);
@@ -2773,6 +2774,15 @@ CopyFrom(CopyState cstate)
ExecDropSingleTupleTableSlot(cstate->partition_tuple_slot);
}
+ /* Close any trigger target relations */
+ foreach(l, estate->es_trig_target_relations)
+ {
+ resultRelInfo = (ResultRelInfo *) lfirst(l);
+ /* Close indices and then the relation itself */
+ ExecCloseIndices(resultRelInfo);
+ heap_close(resultRelInfo->ri_RelationDesc, NoLock);
+ }
+
FreeExecutorState(estate);
/*
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
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index f22d0a07987..137b1ef42d9 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2773,6 +2773,9 @@ CopyFrom(CopyState cstate)
ExecDropSingleTupleTableSlot(cstate->partition_tuple_slot);
}
+ /* Close any trigger target relations */
+ ExecCleanUpTriggerState(estate);
+
FreeExecutorState(estate);
/*
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 819395a9678..1566fb46074 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -4110,16 +4110,7 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events,
if (local_estate)
{
- ListCell *l;
-
- foreach(l, estate->es_trig_target_relations)
- {
- ResultRelInfo *resultRelInfo = (ResultRelInfo *) lfirst(l);
-
- /* Close indices and then the relation itself */
- ExecCloseIndices(resultRelInfo);
- heap_close(resultRelInfo->ri_RelationDesc, NoLock);
- }
+ ExecCleanUpTriggerState(estate);
FreeExecutorState(estate);
}
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 2535d2ee695..fb2ba3302c0 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1447,6 +1447,24 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
}
/*
+ * Close any relations that have been opened by ExecGetTriggerResultRel().
+ */
+void
+ExecCleanUpTriggerState(EState *estate)
+{
+ ListCell *l;
+
+ foreach(l, estate->es_trig_target_relations)
+ {
+ ResultRelInfo *resultRelInfo = (ResultRelInfo *) lfirst(l);
+
+ /* Close indices and then the relation itself */
+ ExecCloseIndices(resultRelInfo);
+ heap_close(resultRelInfo->ri_RelationDesc, NoLock);
+ }
+}
+
+/*
* ExecContextForcesOids
*
* This is pretty grotty: when doing INSERT, UPDATE, or CREATE TABLE AS,
@@ -1610,16 +1628,8 @@ ExecEndPlan(PlanState *planstate, EState *estate)
resultRelInfo++;
}
- /*
- * likewise close any trigger target relations
- */
- foreach(l, estate->es_trig_target_relations)
- {
- resultRelInfo = (ResultRelInfo *) lfirst(l);
- /* Close indices and then the relation itself */
- ExecCloseIndices(resultRelInfo);
- heap_close(resultRelInfo->ri_RelationDesc, NoLock);
- }
+ /* likewise close any trigger target relations */
+ ExecCleanUpTriggerState(estate);
/*
* close any relations selected FOR [KEY] UPDATE/SHARE, again keeping
@@ -3173,14 +3183,7 @@ EvalPlanQualEnd(EPQState *epqstate)
ExecResetTupleTable(estate->es_tupleTable, false);
/* close any trigger target relations attached to this EState */
- foreach(l, estate->es_trig_target_relations)
- {
- ResultRelInfo *resultRelInfo = (ResultRelInfo *) lfirst(l);
-
- /* Close indices and then the relation itself */
- ExecCloseIndices(resultRelInfo);
- heap_close(resultRelInfo->ri_RelationDesc, NoLock);
- }
+ ExecCleanUpTriggerState(estate);
MemoryContextSwitchTo(oldcontext);
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 3107cf5b89e..4f19579ee0b 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -184,6 +184,7 @@ extern void InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation partition_root,
int instrument_options);
extern ResultRelInfo *ExecGetTriggerResultRel(EState *estate, Oid relid);
+extern void ExecCleanUpTriggerState(EState *estate);
extern bool ExecContextForcesOids(PlanState *planstate, bool *hasoids);
extern void ExecConstraints(ResultRelInfo *resultRelInfo,
TupleTableSlot *slot, EState *estate);
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index c300449f3aa..0d560fb3eed 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -1882,4 +1882,14 @@ NOTICE: trigger on parted2_stmt_trig AFTER UPDATE for STATEMENT
delete from parted_stmt_trig;
NOTICE: trigger on parted_stmt_trig BEFORE DELETE for STATEMENT
NOTICE: trigger on parted_stmt_trig AFTER DELETE for STATEMENT
+-- insert via copy on the parent
+copy parted_stmt_trig(a) from stdin;
+NOTICE: trigger on parted_stmt_trig BEFORE INSERT for STATEMENT
+NOTICE: trigger on parted_stmt_trig1 BEFORE INSERT for ROW
+NOTICE: trigger on parted_stmt_trig1 AFTER INSERT for ROW
+NOTICE: trigger on parted_stmt_trig AFTER INSERT for STATEMENT
+-- insert via copy on the first partition
+copy parted_stmt_trig1(a) from stdin;
+NOTICE: trigger on parted_stmt_trig1 BEFORE INSERT for ROW
+NOTICE: trigger on parted_stmt_trig1 AFTER INSERT for ROW
drop table parted_stmt_trig, parted2_stmt_trig;
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index e5dbcaeea36..5581fcb1648 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1347,4 +1347,16 @@ with upd as (
) update parted_stmt_trig set a = a;
delete from parted_stmt_trig;
+
+-- insert via copy on the parent
+copy parted_stmt_trig(a) from stdin;
+1
+2
+\.
+
+-- insert via copy on the first partition
+copy parted_stmt_trig1(a) from stdin;
+1
+\.
+
drop table parted_stmt_trig, parted2_stmt_trig;
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