rdelaage
/
postgresql
mirror of https://git.postgresql.org/git/postgresql.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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.
This commit is contained in:
Tom Lane 2009-06-20 18:45:28 +00:00
parent 87698ffa8e
commit 82480e28f5
4 changed files with 31 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
doc/src/sgml/xaggr.sgml
View File

@ -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>

7
src/backend/executor/nodeWindowAgg.c
View File

@ -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

9
src/backend/utils/adt/array_userfuncs.c
View File

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

18
src/backend/utils/adt/arrayfuncs.c
View File

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