Re: Aggregate-function space leakage

I wrote:
> Anyway, I'll go take a look at exactly what would be involved in the
> first choice.

Actually, it seems this way results in a net *savings* of code, because
we can simply remove the code that was responsible for retail pfree'ing
of the transition values.  I suppose that code must have predated the
introduction of MemoryContextReset, or it would have occurred to us to
do it like this to begin with.

I think that WindowAgg does not need any changes because it already does
MemoryContextResetAndDeleteChildren(winstate->wincontext) at partition
boundaries.  Hitoshi, do you agree?

            regards, tom lane

Index: src/backend/executor/nodeAgg.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/executor/nodeAgg.c,v
retrieving revision 1.167
diff -c -r1.167 nodeAgg.c
*** src/backend/executor/nodeAgg.c    17 Jun 2009 16:05:34 -0000    1.167
--- src/backend/executor/nodeAgg.c    23 Jul 2009 19:19:33 -0000
***************
*** 55,60 ****
--- 55,62 ----
   *      in either case its value need not be preserved.  See int8inc() for an
   *      example.    Notice that advance_transition_function() is coded to avoid a
   *      data copy step when the previous transition value pointer is returned.
+  *      Also, some transition functions make use of the aggcontext to store
+  *      working state.
   *
   *
   * Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group
***************
*** 273,290 ****
          }

          /*
-          * If we are reinitializing after a group boundary, we have to free
-          * any prior transValue to avoid memory leakage.  We must check not
-          * only the isnull flag but whether the pointer is NULL; since
-          * pergroupstate is initialized with palloc0, the initial condition
-          * has isnull = 0 and null pointer.
-          */
-         if (!peraggstate->transtypeByVal &&
-             !pergroupstate->transValueIsNull &&
-             DatumGetPointer(pergroupstate->transValue) != NULL)
-             pfree(DatumGetPointer(pergroupstate->transValue));
-
-         /*
           * (Re)set transValue to the initial value.
           *
           * Note that when the initial value is pass-by-ref, we must copy it
--- 275,280 ----
***************
*** 911,920 ****
          }

          /*
!          * Clear the per-output-tuple context for each group
           */
          ResetExprContext(econtext);

          /*
           * Initialize working state for a new input tuple group
           */
--- 901,915 ----
          }

          /*
!          * Clear the per-output-tuple context for each group, as well as
!          * aggcontext (which contains any pass-by-ref transvalues of the
!          * old group).  We also clear any child contexts of the aggcontext;
!          * some aggregate functions store working state in such contexts.
           */
          ResetExprContext(econtext);

+         MemoryContextResetAndDeleteChildren(aggstate->aggcontext);
+
          /*
           * Initialize working state for a new input tuple group
           */
***************
*** 1234,1240 ****
       * structures and transition values.  NOTE: the details of what is stored
       * in aggcontext and what is stored in the regular per-query memory
       * context are driven by a simple decision: we want to reset the
!      * aggcontext in ExecReScanAgg to recover no-longer-wanted space.
       */
      aggstate->aggcontext =
          AllocSetContextCreate(CurrentMemoryContext,
--- 1229,1236 ----
       * structures and transition values.  NOTE: the details of what is stored
       * in aggcontext and what is stored in the regular per-query memory
       * context are driven by a simple decision: we want to reset the
!      * aggcontext at group boundaries (if not hashing) and in ExecReScanAgg
!      * to recover no-longer-wanted space.
       */
      aggstate->aggcontext =
          AllocSetContextCreate(CurrentMemoryContext,
Index: src/backend/utils/adt/array_userfuncs.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/array_userfuncs.c,v
retrieving revision 1.31
diff -c -r1.31 array_userfuncs.c
*** src/backend/utils/adt/array_userfuncs.c    20 Jun 2009 18:45:28 -0000    1.31
--- src/backend/utils/adt/array_userfuncs.c    23 Jul 2009 19:19:33 -0000
***************
*** 539,545 ****

      /*
       * Make the result.  We cannot release the ArrayBuildState because
!      * sometimes aggregate final functions are re-executed.
       */
      result = makeMdArrayResult(state, 1, dims, lbs,
                                 CurrentMemoryContext,
--- 539,547 ----

      /*
       * Make the result.  We cannot release the ArrayBuildState because
!      * sometimes aggregate final functions are re-executed.  Rather, it
!      * is nodeAgg.c's responsibility to reset the aggcontext when it's
!      * safe to do so.
       */
      result = makeMdArrayResult(state, 1, dims, lbs,
                                 CurrentMemoryContext,