Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 88e6ad3

Browse files
anarazelanarazel
committed
Fix two memory leaks around force-storing tuples in slots.
As reported by Tom, when ExecStoreMinimalTuple() had to perform a conversion to store the minimal tuple in the slot, it forgot to respect the shouldFree flag, and leaked the tuple into the current memory context if true. Fix that by freeing the tuple in that case. Looking at the relevant code made me (Andres) realize that not having the shouldFree parameter to ExecForceStoreHeapTuple() was a bad idea. Some callers had to locally implement the necessary logic, and in one case it was missing, creating a potential per-group leak in non-hashed aggregation. The choice to not free the tuple in ExecComputeStoredGenerated() is not pretty, but not introduced by this commit - I'll start a separate discussion about it. Reported-By: Tom Lane Discussion: https://postgr.es/m/366.1555382816@sss.pgh.pa.us
1 parent 4d5840c commit 88e6ad3

File tree

7 files changed

+41
-20
lines changed

7 files changed

+41
-20
lines changed

‎contrib/postgres_fdw/postgres_fdw.c

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3711,9 +3711,7 @@ store_returning_result(PgFdwModifyState *fmstate,
37113711
* The returning slot will not necessarily be suitable to store
37123712
* heaptuples directly, so allow for conversion.
37133713
*/
3714-
ExecForceStoreHeapTuple(newtup, slot);
3715-
ExecMaterializeSlot(slot);
3716-
pfree(newtup);
3714+
ExecForceStoreHeapTuple(newtup, slot, true);
37173715
}
37183716
PG_CATCH();
37193717
{

‎src/backend/commands/trigger.c

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2586,7 +2586,7 @@ ExecBRInsertTriggers(EState *estate, ResultRelInfo *relinfo,
25862586
}
25872587
else if (newtuple != oldtuple)
25882588
{
2589-
ExecForceStoreHeapTuple(newtuple, slot);
2589+
ExecForceStoreHeapTuple(newtuple, slot, false);
25902590

25912591
if (should_free)
25922592
heap_freetuple(oldtuple);
@@ -2668,7 +2668,7 @@ ExecIRInsertTriggers(EState *estate, ResultRelInfo *relinfo,
26682668
}
26692669
else if (newtuple != oldtuple)
26702670
{
2671-
ExecForceStoreHeapTuple(newtuple, slot);
2671+
ExecForceStoreHeapTuple(newtuple, slot, false);
26722672

26732673
if (should_free)
26742674
heap_freetuple(oldtuple);
@@ -2797,7 +2797,7 @@ ExecBRDeleteTriggers(EState *estate, EPQState *epqstate,
27972797
else
27982798
{
27992799
trigtuple = fdw_trigtuple;
2800-
ExecForceStoreHeapTuple(trigtuple, slot);
2800+
ExecForceStoreHeapTuple(trigtuple, slot, false);
28012801
}
28022802

28032803
LocTriggerData.type = T_TriggerData;
@@ -2869,7 +2869,7 @@ ExecARDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
28692869
slot,
28702870
NULL);
28712871
else
2872-
ExecForceStoreHeapTuple(fdw_trigtuple, slot);
2872+
ExecForceStoreHeapTuple(fdw_trigtuple, slot, false);
28732873

28742874
AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_DELETE,
28752875
true, slot, NULL, NIL, NULL,
@@ -2898,7 +2898,7 @@ ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
28982898
LocTriggerData.tg_oldtable = NULL;
28992899
LocTriggerData.tg_newtable = NULL;
29002900

2901-
ExecForceStoreHeapTuple(trigtuple, slot);
2901+
ExecForceStoreHeapTuple(trigtuple, slot, false);
29022902

29032903
for (i = 0; i < trigdesc->numtriggers; i++)
29042904
{
@@ -3057,7 +3057,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
30573057
}
30583058
else
30593059
{
3060-
ExecForceStoreHeapTuple(fdw_trigtuple, oldslot);
3060+
ExecForceStoreHeapTuple(fdw_trigtuple, oldslot, false);
30613061
trigtuple = fdw_trigtuple;
30623062
}
30633063

@@ -3107,7 +3107,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
31073107
}
31083108
else if (newtuple != oldtuple)
31093109
{
3110-
ExecForceStoreHeapTuple(newtuple, newslot);
3110+
ExecForceStoreHeapTuple(newtuple, newslot, false);
31113111

31123112
/*
31133113
* If the tuple returned by the trigger / being stored, is the old
@@ -3164,7 +3164,7 @@ ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
31643164
oldslot,
31653165
NULL);
31663166
else if (fdw_trigtuple != NULL)
3167-
ExecForceStoreHeapTuple(fdw_trigtuple, oldslot);
3167+
ExecForceStoreHeapTuple(fdw_trigtuple, oldslot, false);
31683168

31693169
AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_UPDATE,
31703170
true, oldslot, newslot, recheckIndexes,
@@ -3192,7 +3192,7 @@ ExecIRUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
31923192
LocTriggerData.tg_oldtable = NULL;
31933193
LocTriggerData.tg_newtable = NULL;
31943194

3195-
ExecForceStoreHeapTuple(trigtuple, oldslot);
3195+
ExecForceStoreHeapTuple(trigtuple, oldslot, false);
31963196

31973197
for (i = 0; i < trigdesc->numtriggers; i++)
31983198
{
@@ -3228,7 +3228,7 @@ ExecIRUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
32283228
}
32293229
else if (newtuple != oldtuple)
32303230
{
3231-
ExecForceStoreHeapTuple(newtuple, newslot);
3231+
ExecForceStoreHeapTuple(newtuple, newslot, false);
32323232

32333233
if (should_free)
32343234
heap_freetuple(oldtuple);

‎src/backend/executor/execTuples.c

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1430,11 +1430,12 @@ ExecStoreMinimalTuple(MinimalTuple mtup,
14301430
*/
14311431
void
14321432
ExecForceStoreHeapTuple(HeapTuple tuple,
1433-
TupleTableSlot *slot)
1433+
TupleTableSlot *slot,
1434+
bool shouldFree)
14341435
{
14351436
if (TTS_IS_HEAPTUPLE(slot))
14361437
{
1437-
ExecStoreHeapTuple(tuple, slot, false);
1438+
ExecStoreHeapTuple(tuple, slot, shouldFree);
14381439
}
14391440
else if (TTS_IS_BUFFERTUPLE(slot))
14401441
{
@@ -1447,13 +1448,22 @@ ExecForceStoreHeapTuple(HeapTuple tuple,
14471448
oldContext = MemoryContextSwitchTo(slot->tts_mcxt);
14481449
bslot->base.tuple = heap_copytuple(tuple);
14491450
MemoryContextSwitchTo(oldContext);
1451+
1452+
if (shouldFree)
1453+
pfree(tuple);
14501454
}
14511455
else
14521456
{
14531457
ExecClearTuple(slot);
14541458
heap_deform_tuple(tuple, slot->tts_tupleDescriptor,
14551459
slot->tts_values, slot->tts_isnull);
14561460
ExecStoreVirtualTuple(slot);
1461+
1462+
if (shouldFree)
1463+
{
1464+
ExecMaterializeSlot(slot);
1465+
pfree(tuple);
1466+
}
14571467
}
14581468
}
14591469

@@ -1481,6 +1491,12 @@ ExecForceStoreMinimalTuple(MinimalTuple mtup,
14811491
heap_deform_tuple(&htup, slot->tts_tupleDescriptor,
14821492
slot->tts_values, slot->tts_isnull);
14831493
ExecStoreVirtualTuple(slot);
1494+
1495+
if (shouldFree)
1496+
{
1497+
ExecMaterializeSlot(slot);
1498+
pfree(mtup);
1499+
}
14841500
}
14851501
}
14861502

‎src/backend/executor/nodeAgg.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1806,7 +1806,7 @@ agg_retrieve_direct(AggState *aggstate)
18061806
* cleared from the slot.
18071807
*/
18081808
ExecForceStoreHeapTuple(aggstate->grp_firstTuple,
1809-
firstSlot);
1809+
firstSlot, true);
18101810
aggstate->grp_firstTuple = NULL; /* don't keep two pointers */
18111811

18121812
/* set up for first advance_aggregates call */

‎src/backend/executor/nodeIndexonlyscan.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,7 @@ IndexOnlyNext(IndexOnlyScanState *node)
205205
*/
206206
Assert(slot->tts_tupleDescriptor->natts ==
207207
scandesc->xs_hitupdesc->natts);
208-
ExecForceStoreHeapTuple(scandesc->xs_hitup, slot);
208+
ExecForceStoreHeapTuple(scandesc->xs_hitup, slot, false);
209209
}
210210
else if (scandesc->xs_itup)
211211
StoreIndexTuple(slot, scandesc->xs_itup, scandesc->xs_itupdesc);

‎src/backend/executor/nodeModifyTable.c

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -317,7 +317,12 @@ ExecComputeStoredGenerated(EState *estate, TupleTableSlot *slot)
317317

318318
oldtuple = ExecFetchSlotHeapTuple(slot, true, &should_free);
319319
newtuple = heap_modify_tuple(oldtuple, tupdesc, values, nulls, replaces);
320-
ExecForceStoreHeapTuple(newtuple, slot);
320+
/*
321+
* The tuple will be freed by way of the memory context - the slot might
322+
* only be cleared after the context is reset, and we'd thus potentially
323+
* double free.
324+
*/
325+
ExecForceStoreHeapTuple(newtuple, slot, false);
321326
if (should_free)
322327
heap_freetuple(oldtuple);
323328

@@ -979,7 +984,7 @@ ldelete:;
979984
slot = ExecGetReturningSlot(estate, resultRelInfo);
980985
if (oldtuple != NULL)
981986
{
982-
ExecForceStoreHeapTuple(oldtuple, slot);
987+
ExecForceStoreHeapTuple(oldtuple, slot, false);
983988
}
984989
else
985990
{

‎src/include/executor/tuptable.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -306,7 +306,9 @@ extern void ExecSetSlotDescriptor(TupleTableSlot *slot, TupleDesc tupdesc);
306306
extern TupleTableSlot *ExecStoreHeapTuple(HeapTuple tuple,
307307
TupleTableSlot *slot,
308308
bool shouldFree);
309-
extern void ExecForceStoreHeapTuple(HeapTuple tuple, TupleTableSlot *slot);
309+
extern void ExecForceStoreHeapTuple(HeapTuple tuple,
310+
TupleTableSlot *slot,
311+
bool shouldFree);
310312
extern TupleTableSlot *ExecStoreBufferHeapTuple(HeapTuple tuple,
311313
TupleTableSlot *slot,
312314
Buffer buffer);

0 commit comments

Comments
 (0)