From 82480e28f5744582dba78320824e3569ed76e74a Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 20 Jun 2009 18:45:28 +0000 Subject: [PATCH] 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. --- doc/src/sgml/xaggr.sgml | 10 +++++++--- src/backend/executor/nodeWindowAgg.c | 7 +++---- src/backend/utils/adt/array_userfuncs.c | 9 ++++++--- src/backend/utils/adt/arrayfuncs.c | 18 +++++++++++++++--- 4 files changed, 31 insertions(+), 13 deletions(-) diff --git a/doc/src/sgml/xaggr.sgml b/doc/src/sgml/xaggr.sgml index b223888f9e..1175b3640d 100644 --- a/doc/src/sgml/xaggr.sgml +++ b/doc/src/sgml/xaggr.sgml @@ -1,4 +1,4 @@ - + User-Defined Aggregates @@ -175,10 +175,14 @@ SELECT attrelid::regclass, array_accum(atttypid::regtype) (IsA(fcinfo->context, AggState) || IsA(fcinfo->context, WindowAggState))) - 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 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 int8inc() for an example. diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c index 0d9eb21a91..7343cb3752 100644 --- 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 c30d6b42b0..a1f48d8784 100644 --- 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 b3973fbccb..54c489a896 100644 --- 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;