Fix things so that array_agg_finalfn does not modify or free its input

ArrayBuildState, per trouble report from Merlin Moncure.  By adopting
this fix, we are essentially deciding that aggregate final-functions
should not modify their inputs ever.  Adjust documentation and comments
to match that conclusion.

diff --git a/doc/src/sgml/xaggr.sgml b/doc/src/sgml/xaggr.sgml
index b223888f9ed92c70475b1f3499ea4fd555433dc9..1175b3640d70977fcc892c0737ae39bcd0c35473 100644 (file)
--- a/doc/src/sgml/xaggr.sgml
+++ b/doc/src/sgml/xaggr.sgml
@@ -1,4 +1,4 @@
-<!-- $PostgreSQL: pgsql/doc/src/sgml/xaggr.sgml,v 1.37 2008/12/28 18:53:54 tgl Exp $ -->
+<!-- $PostgreSQL: pgsql/doc/src/sgml/xaggr.sgml,v 1.38 2009/06/20 18:45:28 tgl Exp $ -->
 
  <sect1 id="xaggr">
   <title>User-Defined Aggregates</title>
@@ -175,10 +175,14 @@ SELECT attrelid::regclass, array_accum(atttypid::regtype)
             (IsA(fcinfo-&gt;context, AggState) ||
              IsA(fcinfo-&gt;context, WindowAggState)))
 </programlisting>
-   One reason for checking this is that when it is true, the first input
+   One reason for checking this is that when it is true for a transition
+   function, the first input
    must be a temporary transition value and can therefore safely be modified
    in-place rather than allocating a new copy.  (This is the <emphasis>only</>
-   case where it is safe for a function to modify a pass-by-reference input.)
+   case where it is safe for a function to modify a pass-by-reference input.
+   In particular, aggregate final functions should not modify their inputs in
+   any case, because in some cases they will be re-executed on the same
+   final transition value.)
    See <literal>int8inc()</> for an example.
   </para>
 

diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index 0d9eb21a919e5690e19e67d043087be3d982f647..7343cb3752ecec1a7246d51851bf80090230bcfb 100644 (file)
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -27,7 +27,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/executor/nodeWindowAgg.c,v 1.5 2009/06/11 14:48:57 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/executor/nodeWindowAgg.c,v 1.6 2009/06/20 18:45:28 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -410,9 +410,8 @@ eval_windowaggregates(WindowAggState *winstate)
         * need the current aggregate value.  This is considerably more efficient
         * than the naive approach of re-running the entire aggregate calculation
         * for each current row.  It does assume that the final function doesn't
-        * damage the running transition value.  (Some C-coded aggregates do that
-        * for efficiency's sake --- but they are supposed to do so only when
-        * their fcinfo->context is an AggState, not a WindowAggState.)
+        * damage the running transition value, but we have the same assumption
+        * in nodeAgg.c too (when it rescans an existing hash table).
         *
         * In many common cases, multiple rows share the same frame and hence the
         * same aggregate value. (In particular, if there's no ORDER BY in a RANGE

diff --git a/src/backend/utils/adt/array_userfuncs.c b/src/backend/utils/adt/array_userfuncs.c
index c30d6b42b07cb959247921e2ff2cd5494fe0fac3..a1f48d8784c5ddbef0e0584b8018731300d177ef 100644 (file)
--- a/src/backend/utils/adt/array_userfuncs.c
+++ b/src/backend/utils/adt/array_userfuncs.c
@@ -6,7 +6,7 @@
  * Copyright (c) 2003-2009, PostgreSQL Global Development Group
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/utils/adt/array_userfuncs.c,v 1.30 2009/06/11 14:49:03 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/utils/adt/array_userfuncs.c,v 1.31 2009/06/20 18:45:28 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -537,10 +537,13 @@ array_agg_finalfn(PG_FUNCTION_ARGS)
        dims[0] = state->nelems;
        lbs[0] = 1;
 
-       /* Release working state if regular aggregate, but not if window agg */
+       /*
+        * Make the result.  We cannot release the ArrayBuildState because
+        * sometimes aggregate final functions are re-executed.
+        */
        result = makeMdArrayResult(state, 1, dims, lbs,
                                                           CurrentMemoryContext,
-                                                          IsA(fcinfo->context, AggState));
+                                                          false);
 
        PG_RETURN_DATUM(result);
 }

diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index b3973fbccb97379a45376d3b6735a3b294096e6b..54c489a896911668b777ab05a571e966ec1eb3bd 100644 (file)
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/utils/adt/arrayfuncs.c,v 1.157 2009/06/11 14:49:03 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/utils/adt/arrayfuncs.c,v 1.158 2009/06/20 18:45:28 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -4179,9 +4179,21 @@ accumArrayResult(ArrayBuildState *astate,
                }
        }
 
-       /* Use datumCopy to ensure pass-by-ref stuff is copied into mcontext */
+       /*
+        * Ensure pass-by-ref stuff is copied into mcontext; and detoast it too
+        * if it's varlena.  (You might think that detoasting is not needed here
+        * because construct_md_array can detoast the array elements later.
+        * However, we must not let construct_md_array modify the ArrayBuildState
+        * because that would mean array_agg_finalfn damages its input, which
+        * is verboten.  Also, this way frequently saves one copying step.)
+        */
        if (!disnull && !astate->typbyval)
-               dvalue = datumCopy(dvalue, astate->typbyval, astate->typlen);
+       {
+               if (astate->typlen == -1)
+                       dvalue = PointerGetDatum(PG_DETOAST_DATUM_COPY(dvalue));
+               else
+                       dvalue = datumCopy(dvalue, astate->typbyval, astate->typlen);
+       }
 
        astate->dvalues[astate->nelems] = dvalue;
        astate->dnulls[astate->nelems] = disnull;