array_agg versus repeated execution of aggregate final functions

I've identified the cause of Merlin Moncure's crash report here:
http://archives.postgresql.org/pgsql-hackers/2009-06/msg01171.php
(Thanks to Merlin for making some proprietary stuff available to me
for testing it.)

The problem query generates a plan in which a HashAggregate node is on
the inside of a NestLoop, where it can be (and is) executed more than
once. ExecReScanAgg decides that it can rescan the existing hash table
instead of forcing it to be rebuilt. What that means is that the
aggregate final-functions will be re-executed again on the latest
transition values. And array_agg_finalfn has got a couple of issues
with that. The obvious one is that it thinks it can release the
ArrayBuildState when called by an Agg node. The less obvious one
is that if any of the original input values were toasted,
construct_md_array does this:
if (elmlen == -1)
elems[i] = PointerGetDatum(PG_DETOAST_DATUM(elems[i]));
which means it's clobbering an element of the ArrayBuildState's elems
array with a pointer to short-lived data. So on the re-execution it
may find elems[] pointing to bogus data.

There are basically two ways that we could respond to this:

(1) Decide that the bug is array_agg_finalfn's, and make the necessary
fixes so that it doesn't modify or free its argument ArrayBuildState.
This would amount to legislating that aggregate final functions cannot
scribble on their inputs *ever*, rather than the compromise position
we thought we had adopted last fall, namely that they can do so when
called by an Agg node but not when called by WindowAgg.

(2) Decide that we want to allow aggregate final functions to scribble
on their inputs, which would mean dropping the optimization in
ExecReScanAgg to allow rescanning a completed hash table.

On the whole I think #1 is the right answer. If we decide that #2
is correct, then this is a bug of long standing --- ever since hash
aggregation was introduced in 7.4. The fact that we've not gotten
complaints previously suggests that having aggregate final functions
modify their inputs isn't really being done in practice.

On the other hand one could argue that #2 is a safer fix because it
makes fewer assumptions about aggregate behavior. (I'm not entirely
convinced though --- any existing aggregate code that does that is
a time bomb anyway, unless it gets fixed to know about WindowAgg.)

Comments?

regards, tom lane