8000 Prevent memory leaks from accumulating across printtup() calls. · kariya/postgres@da2c6a9 · GitHub
[go: up one dir, main page]
Skip to content

Commit da2c6a9

Browse files
tglsfdctglsfdc
committed
Prevent memory leaks from accumulating across printtup() calls.
Historically, printtup() has assumed that it could prevent memory leakage by pfree'ing the string result of each output function and manually managing detoasting of toasted values. This amounts to assuming that datatype output functions never leak any memory internally; an assumption we've already decided to be bogus elsewhere, for example in COPY OUT. range_out in particular is known to leak multiple kilobytes per call, as noted in bug #8573 from Godfried Vanluffelen. While we could go in and fix that leak, it wouldn't be very notationally convenient, and in any case there have been and undoubtedly will again be other leaks in other output functions. So what seems like the best solution is to run the output functions in a temporary memory context that can be reset after each row, as we're doing in COPY OUT. Some quick experimentation suggests this is actually a tad faster than the retail pfree's anyway. This patch fixes all the variants of printtup, except for debugtup() which is used in standalone mode. It doesn't seem worth worrying about query-lifespan leaks in standalone mode, and fixing that case would be a bit tedious since debugtup() doesn't currently have any startup or shutdown functions. While at it, remove manual detoast management from several other output-function call sites that had copied it from printtup(). This doesn't make a lot of difference right now, but in view of recent discussions about supporting "non-flattened" Datums, we're going to want that code gone eventually anyway. Back-patch to 9.2 where range_out was introduced. We might eventually decide to back-patch this further, but in the absence of known major leaks in older output functions, I'll refrain for now.
1 parent c142a1a commit da2c6a9

File tree

4 files changed

+62
-125
lines changed

4 files changed

+62
-125
lines changed

‎src/backend/access/common/printtup.c

Lines changed: 49 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#include "libpq/pqformat.h"
2121
#include "tcop/pquery.h"
2222
#include "utils/lsyscache.h"
23+
#include "utils/memutils.h"
2324

2425

2526
static void printtup_startup(DestReceiver *self, int operation,
@@ -60,6 +61,7 @@ typedef struct
6061
TupleDesc attrinfo; /* The attr info we are set up for */
6162
int nattrs;
6263
PrinttupAttrInfo *myinfo; /* Cached info about each attr */
64+
MemoryContext tmpcontext; /* Memory context for per-row workspace */
6365
} DR_printtup;
6466

6567
/* ----------------
@@ -86,6 +88,7 @@ printtup_create_DR(CommandDest dest)
8688
self->attrinfo = NULL;
8789
self->nattrs = 0;
8890
self->myinfo = NULL;
91+
self->tmpcontext = NULL;
8992

9093
return (DestReceiver *) self;
9194
}
@@ -123,6 +126,18 @@ printtup_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
123126
DR_printtup *myState = (DR_printtup *) self;
124127
Portal portal = myState->portal;
125128

129+
/*
130+
* Create a temporary memory context that we can reset once per row to
131+
* recover palloc'd memory. This avoids any problems with leaks inside
132+
* datatype output routines, and should be faster than retail pfree's
133+
* anyway.
134+
*/
135+
myState->tmpcontext = AllocSetContextCreate(CurrentMemoryContext,
136+
"printtup",
137+
ALLOCSET_DEFAULT_MINSIZE,
138+
ALLOCSET_DEFAULT_INITSIZE,
139+
ALLOCSET_DEFAULT_MAXSIZE);
140+
126141
if (PG_PROTOCOL_MAJOR(FrontendProtocol) < 3)
127142
{
128143
/*
@@ -288,6 +303,7 @@ printtup(TupleTableSlot *slot, DestReceiver *self)
288303
{
289304
TupleDesc typeinfo = slot->tts_tupleDescriptor;
290305
DR_printtup *myState = (DR_printtup *) self;
306+
MemoryContext oldcontext;
291307
StringInfoData buf;
292308
int natts = typeinfo->natts;
293309
int i;
@@ -299,8 +315,11 @@ printtup(TupleTableSlot *slot, DestReceiver *self)
299315
/* Make sure the tuple is fully deconstructed */
300316
slot_getallattrs(slot);
301317

318+
/* Switch into per-row context so we can recover memory below */
319+
oldcontext = MemoryContextSwitchTo(myState->tmpcontext);
320+
302321
/*
303-
* Prepare a DataRow message
322+
* Prepare a DataRow message (note buffer is in per-row context)
304323
*/
305324
pq_beginmessage(&buf, 'D');
306325

@@ -312,32 +331,21 @@ printtup(TupleTableSlot *slot, DestReceiver *self)
312331
for (i = 0; i < natts; ++i)
313332
{
314333
PrinttupAttrInfo *thisState = myState->myinfo + i;
315-
Datum origattr = slot->tts_values[i],
316-
attr;
334+
Datum attr = slot->tts_values[i];
317335

318336
if (slot->tts_isnull[i])
319337
{
320338
pq_sendint(&buf, -1, 4);
321339
continue;
322340
}
323341

324-
/*
325-
* If we have a toasted datum, forcibly detoast it here to avoid
326-
* memory leakage inside the type's output routine.
327-
*/
328-
if (thisState->typisvarlena)
329-
attr = PointerGetDatum(PG_DETOAST_DATUM(origattr));
330-
else
331-
attr = origattr;
332-
333342
if (thisState->format == 0)
334343
{
335344
/* Text output */
336345
char *outputstr;
337346

338347
outputstr = OutputFunctionCall(&thisState->finfo, attr);
339348
pq_sendcountedtext(&buf, outputstr, strlen(outputstr), false);
340-
pfree(outputstr);
341349
}
342350
else
343351
{
@@ -348,15 +356,14 @@ printtup(TupleTableSlot *slot, DestReceiver *self)
348356
pq_sendint(&buf, VARSIZE(outputbytes) - VARHDRSZ, 4);
349357
pq_sendbytes(&buf, VARDATA(outputbytes),
350358
VARSIZE(outputbytes) - VARHDRSZ);
351-
pfree(outputbytes);
352359
}
353-
354-
/* Clean up detoasted copy, if any */
355-
if (DatumGetPointer(attr) != DatumGetPointer(origattr))
356-
pfree(DatumGetPointer(attr));
357360
}
358361

359362
pq_endmessage(&buf);
363+
364+
/* Return to caller's context, and flush row's temporary memory */
365+
MemoryContextSwitchTo(oldcontext);
366+
MemoryContextReset(myState->tmpcontext);
360367
}
361368

362369
/* ----------------
@@ -368,6 +375,7 @@ printtup_20(TupleTableSlot *slot, DestReceiver *self)
368375
{
369376
TupleDesc typeinfo = slot->tts_tupleDescriptor;
370377
DR_printtup *myState = (DR_printtup *) self;
378+
MemoryContext oldcontext;
371379
StringInfoData buf;
372380
int natts = typeinfo->natts;
373381
int i,
@@ -381,6 +389,9 @@ printtup_20(TupleTableSlot *slot, DestReceiver *self)
381389
/* Make sure the tuple is fully deconstructed */
382390
slot_getallattrs(slot);
383391

392+
/* Switch into per-row context so we can recover memory below */
393+
oldcontext = MemoryContextSwitchTo(myState->tmpcontext);
394+
384395
/*
385396
* tell the frontend to expect new tuple data (in ASCII style)
386397
*/
@@ -412,34 +423,23 @@ printtup_20(TupleTableSlot *slot, DestReceiver *self)
412423
for (i = 0; i < natts; ++i)
413424
{
414425
PrinttupAttrInfo *thisState = myState->myinfo + i;
415-
Datum origattr = slot->tts_values[i],
416-
attr;
426+
Datum attr = slot->tts_values[i];
417427
char *outputstr;
418428

419429
if (slot->tts_isnull[i])
420430
continue;
421431

422432
Assert(thisState->format == 0);
423433

424-
/*
425-
* If we have a toasted datum, forcibly detoast it here to avoid
426-
* memory leakage inside the type's output routine.
427-
*/
428-
if (thisState->typisvarlena)
429-
attr = PointerGetDatum(PG_DETOAST_DATUM(origattr));
430-
else
431-
attr = origattr;
432-
433434
outputstr = OutputFunctionCall(&thisState->finfo, attr);
434435
pq_sendcountedtext(&buf, outputstr, strlen(outputstr), true);
435-
pfree(outputstr);
436-
437-
/* Clean up detoasted copy, if any */
438-
if (DatumGetPointer(attr) != DatumGetPointer(origattr))
439-
pfree(DatumGetPointer(attr));
440436
}
441437

442438
pq_endmessage(&buf);
439+
440+
/* Return to caller's context, and flush row's temporary memory */
441+
MemoryContextSwitchTo(oldcontext);
442+
MemoryContextReset(myState->tmpcontext);
443443
}
444444

445445
/* ----------------
@@ -456,6 +456,10 @@ printtup_shutdown(DestReceiver *self)
456456
myState->myinfo = NULL;
457457

458458
myState->attrinfo = NULL;
459+
460+
if (myState->tmpcontext)
461+
MemoryContextDelete(myState->tmpcontext);
462+
myState->tmpcontext = NULL;
459463
}
460464

461465
/* ----------------
@@ -518,39 +522,23 @@ debugtup(TupleTableSlot *slot, DestReceiver *self)
518522
TupleDesc typeinfo = slot->tts_tupleDescriptor;
519523
int natts = typeinfo->natts;
520524
int i;
521-
Datum origattr,
522-
attr;
525+
Datum attr;
523526
char *value;
524527
bool isnull;
525528
Oid typoutput;
526529
bool typisvarlena;
527530

528531
for (i = 0; i < natts; ++i)
529532
{
530-
origattr = slot_getattr(slot, i + 1, &isnull);
533+
attr = slot_getattr(slot, i + 1, &isnull);
531534
if (isnull)
532535
continue;
533536
getTypeOutputInfo(typeinfo->attrs[i]->atttypid,
534537
&typoutput, &typisvarlena);
535538

536-
/*
537-
* If we have a toasted datum, forcibly detoast it here to avoid
538-
* memory leakage inside the type's output routine.
539-
*/
540-
if (typisvarlena)
541-
attr = PointerGetDatum(PG_DETOAST_DATUM(origattr));
542-
else
543-
attr = origattr;
544-
545539
value = OidOutputFunctionCall(typoutput, attr);
546540

547541
printatt((unsigned) i + 1, typeinfo->attrs[i], value);
548-
549-
pfree(value);
550-
551-
/* Clean up detoasted copy, if any */
552-
if (DatumGetPointer(attr) != DatumGetPointer(origattr))
553-
pfree(DatumGetPointer(attr));
554542
}
555543
printf("\t----\n");
556544
}
@@ -569,6 +557,7 @@ printtup_internal_20(TupleTableSlot *slot, DestReceiver *self)
569557
{
570558
TupleDesc typeinfo = slot->tts_tupleDescriptor;
571559
DR_printtup *myState = (DR_printtup *) self;
560+
MemoryContext oldcontext;
572561
StringInfoData buf;
573562
int natts = typeinfo->natts;
574563
int i,
@@ -582,6 +571,9 @@ printtup_internal_20(TupleTableSlot *slot, DestReceiver *self)
582571
/* Make sure the tuple is fully deconstructed */
583572
slot_getallattrs(slot);
584573

574+
/* Switch into per-row context so we can recover memory below */
575+
oldcontext = MemoryContextSwitchTo(myState->tmpcontext);
576+
585577
/*
586578
* tell the frontend to expect new tuple data (in binary style)
587579
*/
@@ -613,35 +605,23 @@ printtup_internal_20(TupleTableSlot *slot, DestReceiver *self)
613605
for (i = 0; i < natts; ++i)
614606
{
615607
PrinttupAttrInfo *thisState = myState->myinfo + i;
616-
Datum origattr = slot->tts_values[i],
617-
attr;
608+
Datum attr = slot->tts_values[i];
618609
bytea *outputbytes;
619610

620611
if (slot->tts_isnull[i])
621612
continue;
622613

623614
Assert(thisState->format == 1);
624615

625-
/*
626-
* If we have a toasted datum, forcibly detoast it here to avoid
627-
* memory leakage inside the type's output routine.
628-
*/
629-
if (thisState->typisvarlena)
630-
attr = PointerGetDatum(PG_DETOAST_DATUM(origattr));
631-
else
632-
attr = origattr;
633-
634616
outputbytes = SendFunctionCall(&thisState->finfo, attr);
635-
/* We assume the result will not have been toasted */
636617
pq_sendint(&buf, VARSIZE(outputbytes) - VARHDRSZ, 4);
637618
pq_sendbytes(&buf, VARDATA(outputbytes),
638619
VARSIZE(outputbytes) - VARHDRSZ);
639-
pfree(outputbytes);
640-
641-
/* Clean up detoasted copy, if any */
642-
if (DatumGetPointer(attr) != DatumGetPointer(origattr))
643-
pfree(DatumGetPointer(attr));
644620
}
645621

646622
pq_endmessage(&buf);
623+
624+
/* Return to caller's context, and flush row's temporary memory */
625+
MemoryContextSwitchTo(oldcontext);
626+
MemoryContextReset(myState->tmpcontext);
647627
}

‎src/backend/bootstrap/bootstrap.c

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -835,7 +835,6 @@ InsertOneValue(char *value, int i)
835835
Oid typioparam;
836836
Oid typinput;
837837
Oid typoutput;
838-
char *prt;
839838

840839
AssertArg(i >= 0 && i < MAXATTR);
841840

@@ -849,9 +848,14 @@ InsertOneValue(char *value, int i)
849848
&typinput, &typoutput);
850849

851850
values[i] = OidInputFunctionCall(typinput, value, typioparam, -1);
852-
prt = OidOutputFunctionCall(typoutput, values[i]);
853-
elog(DEBUG4, "inserted -> %s", prt);
854-
pfree(prt);
851+
852+
/*
853+
* We use ereport not elog here so that parameters aren't evaluated unless
854+
* the message is going to be printed, which generally it isn't
855+
*/
856+
ereport(DEBUG4,
857+
(errmsg_internal("inserted -> %s",
858+
OidOutputFunctionCall(typoutput, values[i]))));
855859
}
856860

857861
/* ----------------

‎src/backend/executor/spi.c

Lines changed: 3 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -869,9 +869,7 @@ SPI_fname(TupleDesc tupdesc, int fnumber)
869869
char *
870870
SPI_getvalue(HeapTuple tuple, TupleDesc tupdesc, int fnumber)
871871
{
872-
char *result;
873-
Datum origval,
874-
val;
872+
Datum val;
875873
bool isnull;
876874
Oid typoid,
877875
foutoid;
@@ -886,7 +884,7 @@ SPI_getvalue(HeapTuple tuple, TupleDesc tupdesc, int fnumber)
886884
return NULL;
887885
}
888886

889-
origval = heap_getattr(tuple, fnumber, tupdesc, &isnull);
887+
val = heap_getattr(tuple, fnumber, tupdesc, &isnull);
890888
if (isnull)
891889
return NULL;
892890

@@ -897,22 +895,7 @@ SPI_getvalue(HeapTuple tuple, TupleDesc tupdesc, int fnumber)
897895

898896
getTypeOutputInfo(typoid, &foutoid, &typisvarlena);
899897

900-
/*
901-
* If we have a toasted datum, forcibly detoast it here to avoid memory
902-
* leakage inside the type's output routine.
903-
*/
904-
if (typisvarlena)
905-
val = PointerGetDatum(PG_DETOAST_DATUM(origval));
906-
else
907-
val = origval;
908-
909-
result = OidOutputFunctionCall(foutoid, val);
910-
911-
/* Clean up detoasted copy, if any */
912-
if (val != origval)
913-
pfree(DatumGetPointer(val));
914-
915-
return result;
898+
return OidOutputFunctionCall(foutoid, val);
916899
}
917900

918901
Datum

0 commit comments

Comments
 (0)
0