From da2c6a919533e29623b03a2213b72ece69875bb3 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 3 Nov 2013 11:33:09 -0500 Subject: [PATCH] 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. --- src/backend/access/common/printtup.c | 118 +++++++++++---------------- src/backend/bootstrap/bootstrap.c | 12 ++- src/backend/executor/spi.c | 23 +----- src/backend/utils/adt/rowtypes.c | 34 +------- 4 files changed, 62 insertions(+), 125 deletions(-) diff --git a/src/backend/access/common/printtup.c b/src/backend/access/common/printtup.c index e87e6752b6..5286be5391 100644 --- a/src/backend/access/common/printtup.c +++ b/src/backend/access/common/printtup.c @@ -20,6 +20,7 @@ #include "libpq/pqformat.h" #include "tcop/pquery.h" #include "utils/lsyscache.h" +#include "utils/memutils.h" static void printtup_startup(DestReceiver *self, int operation, @@ -60,6 +61,7 @@ typedef struct TupleDesc attrinfo; /* The attr info we are set up for */ int nattrs; PrinttupAttrInfo *myinfo; /* Cached info about each attr */ + MemoryContext tmpcontext; /* Memory context for per-row workspace */ } DR_printtup; /* ---------------- @@ -86,6 +88,7 @@ printtup_create_DR(CommandDest dest) self->attrinfo = NULL; self->nattrs = 0; self->myinfo = NULL; + self->tmpcontext = NULL; return (DestReceiver *) self; } @@ -123,6 +126,18 @@ printtup_startup(DestReceiver *self, int operation, TupleDesc typeinfo) DR_printtup *myState = (DR_printtup *) self; Portal portal = myState->portal; + /* + * Create a temporary memory context that we can reset once per row to + * recover palloc'd memory. This avoids any problems with leaks inside + * datatype output routines, and should be faster than retail pfree's + * anyway. + */ + myState->tmpcontext = AllocSetContextCreate(CurrentMemoryContext, + "printtup", + ALLOCSET_DEFAULT_MINSIZE, + ALLOCSET_DEFAULT_INITSIZE, + ALLOCSET_DEFAULT_MAXSIZE); + if (PG_PROTOCOL_MAJOR(FrontendProtocol) < 3) { /* @@ -288,6 +303,7 @@ printtup(TupleTableSlot *slot, DestReceiver *self) { TupleDesc typeinfo = slot->tts_tupleDescriptor; DR_printtup *myState = (DR_printtup *) self; + MemoryContext oldcontext; StringInfoData buf; int natts = typeinfo->natts; int i; @@ -299,8 +315,11 @@ printtup(TupleTableSlot *slot, DestReceiver *self) /* Make sure the tuple is fully deconstructed */ slot_getallattrs(slot); + /* Switch into per-row context so we can recover memory below */ + oldcontext = MemoryContextSwitchTo(myState->tmpcontext); + /* - * Prepare a DataRow message + * Prepare a DataRow message (note buffer is in per-row context) */ pq_beginmessage(&buf, 'D'); @@ -312,8 +331,7 @@ printtup(TupleTableSlot *slot, DestReceiver *self) for (i = 0; i < natts; ++i) { PrinttupAttrInfo *thisState = myState->myinfo + i; - Datum origattr = slot->tts_values[i], - attr; + Datum attr = slot->tts_values[i]; if (slot->tts_isnull[i]) { @@ -321,15 +339,6 @@ printtup(TupleTableSlot *slot, DestReceiver *self) continue; } - /* - * If we have a toasted datum, forcibly detoast it here to avoid - * memory leakage inside the type's output routine. - */ - if (thisState->typisvarlena) - attr = PointerGetDatum(PG_DETOAST_DATUM(origattr)); - else - attr = origattr; - if (thisState->format == 0) { /* Text output */ @@ -337,7 +346,6 @@ printtup(TupleTableSlot *slot, DestReceiver *self) outputstr = OutputFunctionCall(&thisState->finfo, attr); pq_sendcountedtext(&buf, outputstr, strlen(outputstr), false); - pfree(outputstr); } else { @@ -348,15 +356,14 @@ printtup(TupleTableSlot *slot, DestReceiver *self) pq_sendint(&buf, VARSIZE(outputbytes) - VARHDRSZ, 4); pq_sendbytes(&buf, VARDATA(outputbytes), VARSIZE(outputbytes) - VARHDRSZ); - pfree(outputbytes); } - - /* Clean up detoasted copy, if any */ - if (DatumGetPointer(attr) != DatumGetPointer(origattr)) - pfree(DatumGetPointer(attr)); } pq_endmessage(&buf); + + /* Return to caller's context, and flush row's temporary memory */ + MemoryContextSwitchTo(oldcontext); + MemoryContextReset(myState->tmpcontext); } /* ---------------- @@ -368,6 +375,7 @@ printtup_20(TupleTableSlot *slot, DestReceiver *self) { TupleDesc typeinfo = slot->tts_tupleDescriptor; DR_printtup *myState = (DR_printtup *) self; + MemoryContext oldcontext; StringInfoData buf; int natts = typeinfo->natts; int i, @@ -381,6 +389,9 @@ printtup_20(TupleTableSlot *slot, DestReceiver *self) /* Make sure the tuple is fully deconstructed */ slot_getallattrs(slot); + /* Switch into per-row context so we can recover memory below */ + oldcontext = MemoryContextSwitchTo(myState->tmpcontext); + /* * tell the frontend to expect new tuple data (in ASCII style) */ @@ -412,8 +423,7 @@ printtup_20(TupleTableSlot *slot, DestReceiver *self) for (i = 0; i < natts; ++i) { PrinttupAttrInfo *thisState = myState->myinfo + i; - Datum origattr = slot->tts_values[i], - attr; + Datum attr = slot->tts_values[i]; char *outputstr; if (slot->tts_isnull[i]) @@ -421,25 +431,15 @@ printtup_20(TupleTableSlot *slot, DestReceiver *self) Assert(thisState->format == 0); - /* - * If we have a toasted datum, forcibly detoast it here to avoid - * memory leakage inside the type's output routine. - */ - if (thisState->typisvarlena) - attr = PointerGetDatum(PG_DETOAST_DATUM(origattr)); - else - attr = origattr; - outputstr = OutputFunctionCall(&thisState->finfo, attr); pq_sendcountedtext(&buf, outputstr, strlen(outputstr), true); - pfree(outputstr); - - /* Clean up detoasted copy, if any */ - if (DatumGetPointer(attr) != DatumGetPointer(origattr)) - pfree(DatumGetPointer(attr)); } pq_endmessage(&buf); + + /* Return to caller's context, and flush row's temporary memory */ + MemoryContextSwitchTo(oldcontext); + MemoryContextReset(myState->tmpcontext); } /* ---------------- @@ -456,6 +456,10 @@ printtup_shutdown(DestReceiver *self) myState->myinfo = NULL; myState->attrinfo = NULL; + + if (myState->tmpcontext) + MemoryContextDelete(myState->tmpcontext); + myState->tmpcontext = NULL; } /* ---------------- @@ -518,8 +522,7 @@ debugtup(TupleTableSlot *slot, DestReceiver *self) TupleDesc typeinfo = slot->tts_tupleDescriptor; int natts = typeinfo->natts; int i; - Datum origattr, - attr; + Datum attr; char *value; bool isnull; Oid typoutput; @@ -527,30 +530,15 @@ debugtup(TupleTableSlot *slot, DestReceiver *self) for (i = 0; i < natts; ++i) { - origattr = slot_getattr(slot, i + 1, &isnull); + attr = slot_getattr(slot, i + 1, &isnull); if (isnull) continue; getTypeOutputInfo(typeinfo->attrs[i]->atttypid, &typoutput, &typisvarlena); - /* - * If we have a toasted datum, forcibly detoast it here to avoid - * memory leakage inside the type's output routine. - */ - if (typisvarlena) - attr = PointerGetDatum(PG_DETOAST_DATUM(origattr)); - else - attr = origattr; - value = OidOutputFunctionCall(typoutput, attr); printatt((unsigned) i + 1, typeinfo->attrs[i], value); - - pfree(value); - - /* Clean up detoasted copy, if any */ - if (DatumGetPointer(attr) != DatumGetPointer(origattr)) - pfree(DatumGetPointer(attr)); } printf("\t----\n"); } @@ -569,6 +557,7 @@ printtup_internal_20(TupleTableSlot *slot, DestReceiver *self) { TupleDesc typeinfo = slot->tts_tupleDescriptor; DR_printtup *myState = (DR_printtup *) self; + MemoryContext oldcontext; StringInfoData buf; int natts = typeinfo->natts; int i, @@ -582,6 +571,9 @@ printtup_internal_20(TupleTableSlot *slot, DestReceiver *self) /* Make sure the tuple is fully deconstructed */ slot_getallattrs(slot); + /* Switch into per-row context so we can recover memory below */ + oldcontext = MemoryContextSwitchTo(myState->tmpcontext); + /* * tell the frontend to expect new tuple data (in binary style) */ @@ -613,8 +605,7 @@ printtup_internal_20(TupleTableSlot *slot, DestReceiver *self) for (i = 0; i < natts; ++i) { PrinttupAttrInfo *thisState = myState->myinfo + i; - Datum origattr = slot->tts_values[i], - attr; + Datum attr = slot->tts_values[i]; bytea *outputbytes; if (slot->tts_isnull[i]) @@ -622,26 +613,15 @@ printtup_internal_20(TupleTableSlot *slot, DestReceiver *self) Assert(thisState->format == 1); - /* - * If we have a toasted datum, forcibly detoast it here to avoid - * memory leakage inside the type's output routine. - */ - if (thisState->typisvarlena) - attr = PointerGetDatum(PG_DETOAST_DATUM(origattr)); - else - attr = origattr; - outputbytes = SendFunctionCall(&thisState->finfo, attr); - /* We assume the result will not have been toasted */ pq_sendint(&buf, VARSIZE(outputbytes) - VARHDRSZ, 4); pq_sendbytes(&buf, VARDATA(outputbytes), VARSIZE(outputbytes) - VARHDRSZ); - pfree(outputbytes); - - /* Clean up detoasted copy, if any */ - if (DatumGetPointer(attr) != DatumGetPointer(origattr)) - pfree(DatumGetPointer(attr)); } pq_endmessage(&buf); + + /* Return to caller's context, and flush row's temporary memory */ + MemoryContextSwitchTo(oldcontext); + MemoryContextReset(myState->tmpcontext); } diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c index 8905596c0b..9d1d8e37cc 100644 --- a/src/backend/bootstrap/bootstrap.c +++ b/src/backend/bootstrap/bootstrap.c @@ -835,7 +835,6 @@ InsertOneValue(char *value, int i) Oid typioparam; Oid typinput; Oid typoutput; - char *prt; AssertArg(i >= 0 && i < MAXATTR); @@ -849,9 +848,14 @@ InsertOneValue(char *value, int i) &typinput, &typoutput); values[i] = OidInputFunctionCall(typinput, value, typioparam, -1); - prt = OidOutputFunctionCall(typoutput, values[i]); - elog(DEBUG4, "inserted -> %s", prt); - pfree(prt); + + /* + * We use ereport not elog here so that parameters aren't evaluated unless + * the message is going to be printed, which generally it isn't + */ + ereport(DEBUG4, + (errmsg_internal("inserted -> %s", + OidOutputFunctionCall(typoutput, values[i])))); } /* ---------------- diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c index 8ba6e107a9..d6947c4e09 100644 --- a/src/backend/executor/spi.c +++ b/src/backend/executor/spi.c @@ -869,9 +869,7 @@ SPI_fname(TupleDesc tupdesc, int fnumber) char * SPI_getvalue(HeapTuple tuple, TupleDesc tupdesc, int fnumber) { - char *result; - Datum origval, - val; + Datum val; bool isnull; Oid typoid, foutoid; @@ -886,7 +884,7 @@ SPI_getvalue(HeapTuple tuple, TupleDesc tupdesc, int fnumber) return NULL; } - origval = heap_getattr(tuple, fnumber, tupdesc, &isnull); + val = heap_getattr(tuple, fnumber, tupdesc, &isnull); if (isnull) return NULL; @@ -897,22 +895,7 @@ SPI_getvalue(HeapTuple tuple, TupleDesc tupdesc, int fnumber) getTypeOutputInfo(typoid, &foutoid, &typisvarlena); - /* - * If we have a toasted datum, forcibly detoast it here to avoid memory - * leakage inside the type's output routine. - */ - if (typisvarlena) - val = PointerGetDatum(PG_DETOAST_DATUM(origval)); - else - val = origval; - - result = OidOutputFunctionCall(foutoid, val); - - /* Clean up detoasted copy, if any */ - if (val != origval) - pfree(DatumGetPointer(val)); - - return result; + return OidOutputFunctionCall(foutoid, val); } Datum diff --git a/src/backend/utils/adt/rowtypes.c b/src/backend/utils/adt/rowtypes.c index 1bd473af65..2d8b0c7886 100644 --- a/src/backend/utils/adt/rowtypes.c +++ b/src/backend/utils/adt/rowtypes.c @@ -397,15 +397,7 @@ record_out(PG_FUNCTION_ARGS) column_info->column_type = column_type; } - /* - * If we have a toasted datum, forcibly detoast it here to avoid - * memory leakage inside the type's output routine. - */ - if (column_info->typisvarlena) - attr = PointerGetDatum(PG_DETOAST_DATUM(values[i])); - else - attr = values[i]; - + attr = values[i]; value = OutputFunctionCall(&column_info->proc, attr); /* Detect whether we need double quotes for this value */ @@ -436,12 +428,6 @@ record_out(PG_FUNCTION_ARGS) } if (nq) appendStringInfoCharMacro(&buf, '"'); - - pfree(value); - - /* Clean up detoasted copy, if any */ - if (DatumGetPointer(attr) != DatumGetPointer(values[i])) - pfree(DatumGetPointer(attr)); } appendStringInfoChar(&buf, ')'); @@ -758,27 +744,11 @@ record_send(PG_FUNCTION_ARGS) column_info->column_type = column_type; } - /* - * If we have a toasted datum, forcibly detoast it here to avoid - * memory leakage inside the type's output routine. - */ - if (column_info->typisvarlena) - attr = PointerGetDatum(PG_DETOAST_DATUM(values[i])); - else - attr = values[i]; - + attr = values[i]; outputbytes = SendFunctionCall(&column_info->proc, attr); - - /* We assume the result will not have been toasted */ pq_sendint(&buf, VARSIZE(outputbytes) - VARHDRSZ, 4); pq_sendbytes(&buf, VARDATA(outputbytes), VARSIZE(outputbytes) - VARHDRSZ); - - pfree(outputbytes); - - /* Clean up detoasted copy, if any */ - if (DatumGetPointer(attr) != DatumGetPointer(values[i])) - pfree(DatumGetPointer(attr)); } pfree(values); -- 2.40.0