Relcache leak when row triggers on partitions are fired by COPY

Started by Thomas Munroover 8 years ago5 messages
#1Thomas Munro
thomas.munro@enterprisedb.com
1 attachment(s)

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);
 
 	/*
#2Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Thomas Munro (#1)
Re: Relcache leak when row triggers on partitions are fired by COPY

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

#3Thomas Munro
thomas.munro@enterprisedb.com
In reply to: Amit Langote (#2)
1 attachment(s)
Re: Relcache leak when row triggers on partitions are fired by COPY

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;
#4Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Thomas Munro (#3)
Re: Relcache leak when row triggers on partitions are fired by COPY

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

#5Robert Haas
robertmhaas@gmail.com
In reply to: Amit Langote (#4)
Re: Relcache leak when row triggers on partitions are fired by COPY

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