Re: [BUGS] BUG #8573: int4range memory consumption - Mailing list pgsql-hackers

Jim Nasby <decibel@decibel.org> writes:
> On Nov 1, 2013, at 2:08 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> While we could doubtless hack range_out to release those strings again,
>> it seems to me that that's just sticking a finger in the dike.  I'm
>> inclined to think that we really ought to solve this class of problems
>> once and for all by fixing printtup.c to run the output functions in a
>> temporary memory context,
>> ...
>> we're already using a reset-per-row approach to memory management of
>> output function calls in COPY OUT, and I know for a fact that we've
>> squeezed that code path as hard as we could.

> +1. COPY is actually the case I was worried about� if you're dealing with large amounts of data in other clients ISTM
thatother things will bottleneck before the extra memory context. 

Attached is a proposed patch for this.  It fixes most of the functions
in printtup.c to use a per-row memory context.  (I did not bother to
fix debugtup(), which is used only in standalone mode.  If you're doing
queries large enough for mem leaks to be problematic in standalone mode,
you're nuts.)  I also simplified some other places that had copied the
notion of forced detoasting before an output function call, as that seems
dubious at best, and wasn't being done uniformly anyway.

My original thought had been to get rid of all pfree's of output function
results, so as to make the world safe for returning constant strings when
such functions find it appropriate.  However, I ran into a showstopper
problem: SPI_getvalue(), which is little more than a wrapper around the
appropriate type output function, is documented as returning a pfree'able
string.  Some though not all of the callers in core and contrib take the
hint and pfree the result, and I'm sure there are more in third-party
extensions.  So if we want to allow output functions to return
non-palloc'd strings, it seems we have to either change SPI_getvalue()'s
API contract or insert a pstrdup() call in it.  Neither of these is
attractive, mainly because the vast majority of output function results
would still be palloc'd even if we made aggressive use of the option not
to.  That means that if we did the former, light testing wouldn't
necessarily show a problem if someone forgot to remove a pfree() after a
SPI_getvalue() call, and it also means that if we did the latter, the
pstrdup() would usually be a waste of cycles and space.

So I've abandoned that idea for now, and just recommend applying the
attached patch as far back as 9.2, where range_out was added.

Comments?

            regards, tom lane

diff --git a/src/backend/access/common/printtup.c b/src/backend/access/common/printtup.c
index 8daac9e..0e8c947 100644
*** a/src/backend/access/common/printtup.c
--- b/src/backend/access/common/printtup.c
***************
*** 21,26 ****
--- 21,27 ----
  #include "tcop/pquery.h"
  #include "utils/lsyscache.h"
  #include "utils/memdebug.h"
+ #include "utils/memutils.h"


  static void printtup_startup(DestReceiver *self, int operation,
*************** typedef struct
*** 61,66 ****
--- 62,68 ----
      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;

  /* ----------------
*************** printtup_create_DR(CommandDest dest)
*** 87,92 ****
--- 89,95 ----
      self->attrinfo = NULL;
      self->nattrs = 0;
      self->myinfo = NULL;
+     self->tmpcontext = NULL;

      return (DestReceiver *) self;
  }
*************** printtup_startup(DestReceiver *self, int
*** 124,129 ****
--- 127,144 ----
      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)
      {
          /*
*************** printtup(TupleTableSlot *slot, DestRecei
*** 289,294 ****
--- 304,310 ----
  {
      TupleDesc    typeinfo = slot->tts_tupleDescriptor;
      DR_printtup *myState = (DR_printtup *) self;
+     MemoryContext oldcontext;
      StringInfoData buf;
      int            natts = typeinfo->natts;
      int            i;
*************** printtup(TupleTableSlot *slot, DestRecei
*** 300,307 ****
      /* Make sure the tuple is fully deconstructed */
      slot_getallattrs(slot);

      /*
!      * Prepare a DataRow message
       */
      pq_beginmessage(&buf, 'D');

--- 316,326 ----
      /* 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 (note buffer is in per-row context)
       */
      pq_beginmessage(&buf, 'D');

*************** printtup(TupleTableSlot *slot, DestRecei
*** 313,320 ****
      for (i = 0; i < natts; ++i)
      {
          PrinttupAttrInfo *thisState = myState->myinfo + i;
!         Datum        origattr = slot->tts_values[i],
!                     attr;

          if (slot->tts_isnull[i])
          {
--- 332,338 ----
      for (i = 0; i < natts; ++i)
      {
          PrinttupAttrInfo *thisState = myState->myinfo + i;
!         Datum        attr = slot->tts_values[i];

          if (slot->tts_isnull[i])
          {
*************** printtup(TupleTableSlot *slot, DestRecei
*** 323,352 ****
          }

          /*
!          * If we have a toasted datum, forcibly detoast it here to avoid
!          * memory leakage inside the type's output routine.
!          *
!          * Here we catch undefined bytes in tuples that are returned to the
           * client without hitting disk; see comments at the related check in
!          * PageAddItem().  Whether to test before or after detoast is somewhat
!          * arbitrary, as is whether to test external/compressed data at all.
!          * Undefined bytes in the pre-toast datum will have triggered Valgrind
!          * errors in the compressor or toaster; any error detected here for
!          * such datums would indicate an (unlikely) bug in a type-independent
!          * facility.  Therefore, this test is most useful for uncompressed,
!          * non-external datums.
!          *
!          * We don't presently bother checking non-varlena datums for undefined
!          * data.  PageAddItem() does check them.
           */
          if (thisState->typisvarlena)
!         {
!             VALGRIND_CHECK_MEM_IS_DEFINED(origattr, VARSIZE_ANY(origattr));
!
!             attr = PointerGetDatum(PG_DETOAST_DATUM(origattr));
!         }
!         else
!             attr = origattr;

          if (thisState->format == 0)
          {
--- 341,355 ----
          }

          /*
!          * Here we catch undefined bytes in datums that are returned to the
           * client without hitting disk; see comments at the related check in
!          * PageAddItem().  This test is most useful for uncompressed,
!          * non-external datums, but we're quite likely to see such here when
!          * testing new C functions.
           */
          if (thisState->typisvarlena)
!             VALGRIND_CHECK_MEM_IS_DEFINED(DatumGetPointer(attr),
!                                           VARSIZE_ANY(attr));

          if (thisState->format == 0)
          {
*************** printtup(TupleTableSlot *slot, DestRecei
*** 355,361 ****

              outputstr = OutputFunctionCall(&thisState->finfo, attr);
              pq_sendcountedtext(&buf, outputstr, strlen(outputstr), false);
-             pfree(outputstr);
          }
          else
          {
--- 358,363 ----
*************** printtup(TupleTableSlot *slot, DestRecei
*** 366,380 ****
              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);
  }

  /* ----------------
--- 368,381 ----
              pq_sendint(&buf, VARSIZE(outputbytes) - VARHDRSZ, 4);
              pq_sendbytes(&buf, VARDATA(outputbytes),
                           VARSIZE(outputbytes) - VARHDRSZ);
          }
      }

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

  /* ----------------
*************** printtup_20(TupleTableSlot *slot, DestRe
*** 386,391 ****
--- 387,393 ----
  {
      TupleDesc    typeinfo = slot->tts_tupleDescriptor;
      DR_printtup *myState = (DR_printtup *) self;
+     MemoryContext oldcontext;
      StringInfoData buf;
      int            natts = typeinfo->natts;
      int            i,
*************** printtup_20(TupleTableSlot *slot, DestRe
*** 399,404 ****
--- 401,409 ----
      /* 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)
       */
*************** printtup_20(TupleTableSlot *slot, DestRe
*** 430,437 ****
      for (i = 0; i < natts; ++i)
      {
          PrinttupAttrInfo *thisState = myState->myinfo + i;
!         Datum        origattr = slot->tts_values[i],
!                     attr;
          char       *outputstr;

          if (slot->tts_isnull[i])
--- 435,441 ----
      for (i = 0; i < natts; ++i)
      {
          PrinttupAttrInfo *thisState = myState->myinfo + i;
!         Datum        attr = slot->tts_values[i];
          char       *outputstr;

          if (slot->tts_isnull[i])
*************** printtup_20(TupleTableSlot *slot, DestRe
*** 439,463 ****

          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);
  }

  /* ----------------
--- 443,457 ----

          Assert(thisState->format == 0);

          outputstr = OutputFunctionCall(&thisState->finfo, attr);
          pq_sendcountedtext(&buf, outputstr, strlen(outputstr), true);
      }

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

  /* ----------------
*************** printtup_shutdown(DestReceiver *self)
*** 474,479 ****
--- 468,477 ----
      myState->myinfo = NULL;

      myState->attrinfo = NULL;
+
+     if (myState->tmpcontext)
+         MemoryContextDelete(myState->tmpcontext);
+     myState->tmpcontext = NULL;
  }

  /* ----------------
*************** debugtup(TupleTableSlot *slot, DestRecei
*** 536,543 ****
      TupleDesc    typeinfo = slot->tts_tupleDescriptor;
      int            natts = typeinfo->natts;
      int            i;
!     Datum        origattr,
!                 attr;
      char       *value;
      bool        isnull;
      Oid            typoutput;
--- 534,540 ----
      TupleDesc    typeinfo = slot->tts_tupleDescriptor;
      int            natts = typeinfo->natts;
      int            i;
!     Datum        attr;
      char       *value;
      bool        isnull;
      Oid            typoutput;
*************** debugtup(TupleTableSlot *slot, DestRecei
*** 545,574 ****

      for (i = 0; i < natts; ++i)
      {
!         origattr = 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");
  }
--- 542,556 ----

      for (i = 0; i < natts; ++i)
      {
!         attr = slot_getattr(slot, i + 1, &isnull);
          if (isnull)
              continue;
          getTypeOutputInfo(typeinfo->attrs[i]->atttypid,
                            &typoutput, &typisvarlena);

          value = OidOutputFunctionCall(typoutput, attr);

          printatt((unsigned) i + 1, typeinfo->attrs[i], value);
      }
      printf("\t----\n");
  }
*************** printtup_internal_20(TupleTableSlot *slo
*** 587,592 ****
--- 569,575 ----
  {
      TupleDesc    typeinfo = slot->tts_tupleDescriptor;
      DR_printtup *myState = (DR_printtup *) self;
+     MemoryContext oldcontext;
      StringInfoData buf;
      int            natts = typeinfo->natts;
      int            i,
*************** printtup_internal_20(TupleTableSlot *slo
*** 600,605 ****
--- 583,591 ----
      /* 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)
       */
*************** printtup_internal_20(TupleTableSlot *slo
*** 631,638 ****
      for (i = 0; i < natts; ++i)
      {
          PrinttupAttrInfo *thisState = myState->myinfo + i;
!         Datum        origattr = slot->tts_values[i],
!                     attr;
          bytea       *outputbytes;

          if (slot->tts_isnull[i])
--- 617,623 ----
      for (i = 0; i < natts; ++i)
      {
          PrinttupAttrInfo *thisState = myState->myinfo + i;
!         Datum        attr = slot->tts_values[i];
          bytea       *outputbytes;

          if (slot->tts_isnull[i])
*************** printtup_internal_20(TupleTableSlot *slo
*** 640,665 ****

          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);
  }
--- 625,639 ----

          Assert(thisState->format == 1);

          outputbytes = SendFunctionCall(&thisState->finfo, attr);
          pq_sendint(&buf, VARSIZE(outputbytes) - VARHDRSZ, 4);
          pq_sendbytes(&buf, VARDATA(outputbytes),
                       VARSIZE(outputbytes) - VARHDRSZ);
      }

      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 2a1af63..62f4c18 100644
*** a/src/backend/bootstrap/bootstrap.c
--- b/src/backend/bootstrap/bootstrap.c
*************** InsertOneValue(char *value, int i)
*** 835,841 ****
      Oid            typioparam;
      Oid            typinput;
      Oid            typoutput;
-     char       *prt;

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

--- 835,840 ----
*************** InsertOneValue(char *value, int i)
*** 849,857 ****
                            &typinput, &typoutput);

      values[i] = OidInputFunctionCall(typinput, value, typioparam, -1);
!     prt = OidOutputFunctionCall(typoutput, values[i]);
!     elog(DEBUG4, "inserted -> %s", prt);
!     pfree(prt);
  }

  /* ----------------
--- 848,861 ----
                            &typinput, &typoutput);

      values[i] = OidInputFunctionCall(typinput, value, typioparam, -1);
!
!     /*
!      * 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 d91a663..dd35212 100644
*** a/src/backend/executor/spi.c
--- b/src/backend/executor/spi.c
*************** SPI_fname(TupleDesc tupdesc, int fnumber
*** 869,877 ****
  char *
  SPI_getvalue(HeapTuple tuple, TupleDesc tupdesc, int fnumber)
  {
!     char       *result;
!     Datum        origval,
!                 val;
      bool        isnull;
      Oid            typoid,
                  foutoid;
--- 869,875 ----
  char *
  SPI_getvalue(HeapTuple tuple, TupleDesc tupdesc, int fnumber)
  {
!     Datum        val;
      bool        isnull;
      Oid            typoid,
                  foutoid;
*************** SPI_getvalue(HeapTuple tuple, TupleDesc
*** 886,892 ****
          return NULL;
      }

!     origval = heap_getattr(tuple, fnumber, tupdesc, &isnull);
      if (isnull)
          return NULL;

--- 884,890 ----
          return NULL;
      }

!     val = heap_getattr(tuple, fnumber, tupdesc, &isnull);
      if (isnull)
          return NULL;

*************** SPI_getvalue(HeapTuple tuple, TupleDesc
*** 897,918 ****

      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;
  }

  Datum
--- 895,901 ----

      getTypeOutputInfo(typoid, &foutoid, &typisvarlena);

!     return OidOutputFunctionCall(foutoid, val);
  }

  Datum
diff --git a/src/backend/utils/adt/rowtypes.c b/src/backend/utils/adt/rowtypes.c
index cb04a72..33647f7 100644
*** a/src/backend/utils/adt/rowtypes.c
--- b/src/backend/utils/adt/rowtypes.c
*************** record_out(PG_FUNCTION_ARGS)
*** 398,412 ****
              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];
!
          value = OutputFunctionCall(&column_info->proc, attr);

          /* Detect whether we need double quotes for this value */
--- 398,404 ----
              column_info->column_type = column_type;
          }

!         attr = values[i];
          value = OutputFunctionCall(&column_info->proc, attr);

          /* Detect whether we need double quotes for this value */
*************** record_out(PG_FUNCTION_ARGS)
*** 437,448 ****
          }
          if (nq)
              appendStringInfoCharMacro(&buf, '"');
-
-         pfree(value);
-
-         /* Clean up detoasted copy, if any */
-         if (DatumGetPointer(attr) != DatumGetPointer(values[i]))
-             pfree(DatumGetPointer(attr));
      }

      appendStringInfoChar(&buf, ')');
--- 429,434 ----
*************** record_send(PG_FUNCTION_ARGS)
*** 759,785 ****
              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];
!
          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);
--- 745,755 ----
              column_info->column_type = column_type;
          }

!         attr = values[i];
          outputbytes = SendFunctionCall(&column_info->proc, attr);
          pq_sendint(&buf, VARSIZE(outputbytes) - VARHDRSZ, 4);
          pq_sendbytes(&buf, VARDATA(outputbytes),
                       VARSIZE(outputbytes) - VARHDRSZ);
      }

      pfree(values);