On 5/23/23 22:57, Tomas Vondra wrote:
>
>
> On 5/23/23 18:39, Tom Lane wrote:
>> Tomas Vondra <tomas.von...@enterprisedb.com> writes:
>>> it seems there's a fairly annoying memory leak in trigger code,
>>> introduced by
>>> ...
>>> Attached is a patch, restoring the pre-12 behavior for me.
>>
>>> While looking for other places allocating stuff in ExecutorState (for
>>> the UPDATE case) and leaving it there, I found two more cases:
>>
>>> 1) copy_plpgsql_datums
>>
>>> 2) make_expanded_record_from_tupdesc
>>> make_expanded_record_from_exprecord
>>
>>> All of this is calls from plpgsql_exec_trigger.
>>
>> Not sure about the expanded-record case, but both of your other two
>> fixes feel like poor substitutes for pushing the memory into a
>> shorter-lived context. In particular I'm quite surprised that
>> plpgsql isn't already allocating that workspace in the "procedure"
>> memory context.
>>
>
> I don't disagree, but which memory context should this use and
> when/where should we switch to it?
>
> I haven't seen any obvious memory context candidate in the code
> calling ExecGetAllUpdatedCols, so I guess we'd have to pass it from
> above. Is that a good idea for backbranches ...
>
I looked at this again, and I think GetPerTupleMemoryContext(estate)
might do the trick, see the 0002 part. Unfortunately it's not much
smaller/simpler than just freeing the chunks, because we end up doing
oldcxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
updatedCols = ExecGetAllUpdatedCols(relinfo, estate);
MemoryContextSwitchTo(oldcxt);
and then have to pass updatedCols elsewhere. It's tricky to just switch
to the context (e.g. in ExecASUpdateTriggers/ExecARUpdateTriggers), as
AfterTriggerSaveEvent allocates other bits of memory too (in a longer
lived context). So we'd have to do another switch again. Not sure how
backpatch-friendly would that be.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL CompanyFrom 85439e2587f035040c82e7303b96b887e650b01f Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas.von...@postgresql.org>
Date: Tue, 23 May 2023 17:45:47 +0200
Subject: [PATCH 1/3] Free updatedCols bitmaps
---
src/backend/commands/trigger.c | 22 ++++++++++++++++++++--
src/backend/executor/execMain.c | 9 +++++++--
2 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 4b295f8da5e..fb11203c473 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2916,6 +2916,8 @@ ExecBSUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
(errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
errmsg("BEFORE STATEMENT trigger cannot return a value")));
}
+
+ bms_free(updatedCols);
}
void
@@ -2928,12 +2930,18 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
Assert(relinfo->ri_RootResultRelInfo == NULL);
if (trigdesc && trigdesc->trig_update_after_statement)
+ {
+ Bitmapset *updatedCols = ExecGetAllUpdatedCols(relinfo, estate);
+
AfterTriggerSaveEvent(estate, relinfo, NULL, NULL,
TRIGGER_EVENT_UPDATE,
false, NULL, NULL, NIL,
- ExecGetAllUpdatedCols(relinfo, estate),
+ updatedCols,
transition_capture,
false);
+
+ bms_free(updatedCols);
+ }
}
bool
@@ -3043,6 +3051,9 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
heap_freetuple(trigtuple);
if (should_free_new)
heap_freetuple(oldtuple);
+
+ bms_free(updatedCols);
+
return false; /* "do nothing" */
}
else if (newtuple != oldtuple)
@@ -3068,6 +3079,8 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
if (should_free_trig)
heap_freetuple(trigtuple);
+ bms_free(updatedCols);
+
return true;
}
@@ -3107,6 +3120,7 @@ ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
*/
TupleTableSlot *oldslot;
ResultRelInfo *tupsrc;
+ Bitmapset *updatedCols;
Assert((src_partinfo != NULL && dst_partinfo != NULL) ||
!is_crosspart_update);
@@ -3129,14 +3143,18 @@ ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
else
ExecClearTuple(oldslot);
+ updatedCols = ExecGetAllUpdatedCols(relinfo, estate);
+
AfterTriggerSaveEvent(estate, relinfo,
src_partinfo, dst_partinfo,
TRIGGER_EVENT_UPDATE,
true,
oldslot, newslot, recheckIndexes,
- ExecGetAllUpdatedCols(relinfo, estate),
+ updatedCols,
transition_capture,
is_crosspart_update);
+
+ bms_free(updatedCols);
}
}
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index c76fdf59ec4..98502b956c2 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -2367,6 +2367,7 @@ ExecUpdateLockMode(EState *estate, ResultRelInfo *relinfo)
{
Bitmapset *keyCols;
Bitmapset *updatedCols;
+ LockTupleMode lockMode;
/*
* Compute lock mode to use. If columns that are part of the key have not
@@ -2378,9 +2379,13 @@ ExecUpdateLockMode(EState *estate, ResultRelInfo *relinfo)
INDEX_ATTR_BITMAP_KEY);
if (bms_overlap(keyCols, updatedCols))
- return LockTupleExclusive;
+ lockMode = LockTupleExclusive;
+ else
+ lockMode = LockTupleNoKeyExclusive;
+
+ bms_free(updatedCols);
- return LockTupleNoKeyExclusive;
+ return lockMode;
}
/*
--
2.40.1
From 99246f4de6668d7a3f6054f672e67e66a1982edf Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas.von...@postgresql.org>
Date: Wed, 24 May 2023 13:12:03 +0200
Subject: [PATCH 2/3] Switch to short-lived MemoryContext
---
src/backend/commands/trigger.c | 39 ++++++++++++++++++++++-----------
src/backend/executor/execMain.c | 14 ++++++------
2 files changed, 33 insertions(+), 20 deletions(-)
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index fb11203c473..f5872ca6da2 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2867,6 +2867,7 @@ ExecBSUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
int i;
TriggerData LocTriggerData = {0};
Bitmapset *updatedCols;
+ MemoryContext oldcxt;
trigdesc = relinfo->ri_TrigDesc;
@@ -2883,8 +2884,12 @@ ExecBSUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
/* statement-level triggers operate on the parent table */
Assert(relinfo->ri_RootResultRelInfo == NULL);
+ oldcxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
+
updatedCols = ExecGetAllUpdatedCols(relinfo, estate);
+ MemoryContextSwitchTo(oldcxt);
+
LocTriggerData.type = T_TriggerData;
LocTriggerData.tg_event = TRIGGER_EVENT_UPDATE |
TRIGGER_EVENT_BEFORE;
@@ -2916,8 +2921,6 @@ ExecBSUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
(errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
errmsg("BEFORE STATEMENT trigger cannot return a value")));
}
-
- bms_free(updatedCols);
}
void
@@ -2931,7 +2934,14 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
if (trigdesc && trigdesc->trig_update_after_statement)
{
- Bitmapset *updatedCols = ExecGetAllUpdatedCols(relinfo, estate);
+ MemoryContext oldcxt;
+ Bitmapset *updatedCols;
+
+ oldcxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
+
+ updatedCols = ExecGetAllUpdatedCols(relinfo, estate);
+
+ MemoryContextSwitchTo(oldcxt);
AfterTriggerSaveEvent(estate, relinfo, NULL, NULL,
TRIGGER_EVENT_UPDATE,
@@ -2939,8 +2949,6 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
updatedCols,
transition_capture,
false);
-
- bms_free(updatedCols);
}
}
@@ -2962,6 +2970,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
TriggerData LocTriggerData = {0};
int i;
Bitmapset *updatedCols;
+ MemoryContext oldcxt;
LockTupleMode lockmode;
/* Determine lock mode to use */
@@ -3015,7 +3024,13 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
TRIGGER_EVENT_ROW |
TRIGGER_EVENT_BEFORE;
LocTriggerData.tg_relation = relinfo->ri_RelationDesc;
+
+ oldcxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
+
updatedCols = ExecGetAllUpdatedCols(relinfo, estate);
+
+ MemoryContextSwitchTo(oldcxt);
+
LocTriggerData.tg_updatedcols = updatedCols;
for (i = 0; i < trigdesc->numtriggers; i++)
{
@@ -3051,9 +3066,6 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
heap_freetuple(trigtuple);
if (should_free_new)
heap_freetuple(oldtuple);
-
- bms_free(updatedCols);
-
return false; /* "do nothing" */
}
else if (newtuple != oldtuple)
@@ -3079,8 +3091,6 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
if (should_free_trig)
heap_freetuple(trigtuple);
- bms_free(updatedCols);
-
return true;
}
@@ -3120,7 +3130,8 @@ ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
*/
TupleTableSlot *oldslot;
ResultRelInfo *tupsrc;
- Bitmapset *updatedCols;
+ MemoryContext oldcxt;
+ Bitmapset *updatedCols;
Assert((src_partinfo != NULL && dst_partinfo != NULL) ||
!is_crosspart_update);
@@ -3143,8 +3154,12 @@ ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
else
ExecClearTuple(oldslot);
+ oldcxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
+
updatedCols = ExecGetAllUpdatedCols(relinfo, estate);
+ MemoryContextSwitchTo(oldcxt);
+
AfterTriggerSaveEvent(estate, relinfo,
src_partinfo, dst_partinfo,
TRIGGER_EVENT_UPDATE,
@@ -3153,8 +3168,6 @@ ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
updatedCols,
transition_capture,
is_crosspart_update);
-
- bms_free(updatedCols);
}
}
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 98502b956c2..71e93486c42 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -2367,7 +2367,9 @@ ExecUpdateLockMode(EState *estate, ResultRelInfo *relinfo)
{
Bitmapset *keyCols;
Bitmapset *updatedCols;
- LockTupleMode lockMode;
+ MemoryContext oldcxt;
+
+ oldcxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
/*
* Compute lock mode to use. If columns that are part of the key have not
@@ -2378,14 +2380,12 @@ ExecUpdateLockMode(EState *estate, ResultRelInfo *relinfo)
keyCols = RelationGetIndexAttrBitmap(relinfo->ri_RelationDesc,
INDEX_ATTR_BITMAP_KEY);
- if (bms_overlap(keyCols, updatedCols))
- lockMode = LockTupleExclusive;
- else
- lockMode = LockTupleNoKeyExclusive;
+ MemoryContextSwitchTo(oldcxt);
- bms_free(updatedCols);
+ if (bms_overlap(keyCols, updatedCols))
+ return LockTupleExclusive;
- return lockMode;
+ return LockTupleNoKeyExclusive;
}
/*
--
2.40.1
From 4375046f8b1f376e404b6ed65a358ee6d55d3b41 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <to...@2ndquadrant.com>
Date: Sat, 13 May 2023 13:19:49 +0200
Subject: [PATCH 3/3] Free space allocated by copy_plpgsql_datums
During PL/pgSQL execution, we allocate a local copy for calls. When
executing triggers, this gets allocated in ExecutorState, so it'd be
kept until query completes.
Free it explicitly, as part of cleanup.
---
src/pl/plpgsql/src/pl_exec.c | 39 +++++++++++++++++++++++++++++++-----
1 file changed, 34 insertions(+), 5 deletions(-)
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 4b76f7699a6..99283d354f4 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -264,6 +264,8 @@ static void coerce_function_result_tuple(PLpgSQL_execstate *estate,
static void plpgsql_exec_error_callback(void *arg);
static void copy_plpgsql_datums(PLpgSQL_execstate *estate,
PLpgSQL_function *func);
+static void free_plpgsql_datums(PLpgSQL_execstate *estate,
+ PLpgSQL_function *func);
static void plpgsql_fulfill_promise(PLpgSQL_execstate *estate,
PLpgSQL_var *var);
static MemoryContext get_stmt_mcontext(PLpgSQL_execstate *estate);
@@ -788,6 +790,8 @@ plpgsql_exec_function(PLpgSQL_function *func, FunctionCallInfo fcinfo,
exec_eval_cleanup(&estate);
/* stmt_mcontext will be destroyed when function's main context is */
+ free_plpgsql_datums(&estate, func);
+
/*
* Pop the error context stack
*/
@@ -1142,6 +1146,8 @@ plpgsql_exec_trigger(PLpgSQL_function *func,
exec_eval_cleanup(&estate);
/* stmt_mcontext will be destroyed when function's main context is */
+ free_plpgsql_datums(&estate, func);
+
/*
* Pop the error context stack
*/
@@ -1217,6 +1223,8 @@ plpgsql_exec_event_trigger(PLpgSQL_function *func, EventTriggerData *trigdata)
exec_eval_cleanup(&estate);
/* stmt_mcontext will be destroyed when function's main context is */
+ free_plpgsql_datums(&estate, func);
+
/*
* Pop the error context stack
*/
@@ -1304,15 +1312,20 @@ copy_plpgsql_datums(PLpgSQL_execstate *estate,
char *ws_next;
int i;
- /* Allocate local datum-pointer array */
- estate->datums = (PLpgSQL_datum **)
- palloc(sizeof(PLpgSQL_datum *) * ndatums);
-
/*
+ * Allocate local datum-pointer array, followed by space for the values.
+ *
* To reduce palloc overhead, we make a single palloc request for all the
* space needed for locally-instantiated datums.
*/
- workspace = palloc(func->copiable_size);
+ estate->datums = (PLpgSQL_datum **)
+ palloc(MAXALIGN(sizeof(PLpgSQL_datum *) * ndatums) + func->copiable_size);
+
+ /*
+ * Space for values starts after the datum-pointer array, but we need to make
+ * sure it's aligned properly.
+ */
+ workspace = (char *) estate->datums + MAXALIGN(sizeof(PLpgSQL_datum *) * ndatums);
ws_next = workspace;
/* Fill datum-pointer array, copying datums into workspace as needed */
@@ -1362,6 +1375,22 @@ copy_plpgsql_datums(PLpgSQL_execstate *estate,
Assert(ws_next == workspace + func->copiable_size);
}
+/*
+ * Free the local execution variables. We've allocated all of the space at once
+ * so we can simply free the main pointer.
+ */
+static void
+free_plpgsql_datums(PLpgSQL_execstate *estate,
+ PLpgSQL_function *func)
+{
+ if (estate->datums != NULL)
+ {
+ pfree(estate->datums);
+ estate->datums = NULL;
+ }
+
+}
+
/*
* If the variable has an armed "promise", compute the promised value
* and assign it to the variable.
--
2.40.1
