Re: array_accum aggregate

On Wed, 2006-10-11 at 00:51 -0400, Stephen Frost wrote:
> An array_accum aggregate has existed in the documentation for quite
> some time using the inefficient (for larger arrays) array_append
> routine.

My vague recollection is that array_accum() is only defined in the
documentation because there was some debate about how it ought to behave
in some corner cases. I can't recall the details, but it was discussed
when array_accum() was originally proposed by Joe. Does anyone remember?

Minor comments on the patch:

> *** 132,138 ****
> </programlisting>
>
> Here, the actual state type for any aggregate call is the array type
> ! having the actual input type as elements.
> </para>
>
> <para>
> --- 132,141 ----
> </programlisting>
>
> Here, the actual state type for any aggregate call is the array type
> ! having the actual input type as elements. Note: array_accum() is now
> ! a built-in aggregate which uses a much more efficient mechanism than
> ! that which is provided by array_append, prior users of array_accum()
> ! may be pleasantly suprised at the marked improvment for larger arrays.
> </para>

Not worth documenting, I think.

> *** 15,20 ****
> --- 15,22 ----
> #include "utils/array.h"
> #include "utils/builtins.h"
> #include "utils/lsyscache.h"
> + #include "utils/memutils.h"
> + #include "nodes/execnodes.h"

Include directives should be sorted roughly alphabetically, with the
exception of postgres.h and system headers.

> + /* Structure, used by aaccum_sfunc and aaccum_ffunc to
> + * implement the array_accum() aggregate, for storing
> + * pointers to the ArrayBuildState for the array we are
> + * building and the MemoryContext in which it is being
> + * built. Note that this structure is
> + * considered an 'anyarray' externally, which is a
> + * variable-length datatype, and therefore
> + * must open with an int32 defining the length. */

Gross.

> + /* Make sure we are being called in an aggregate. */
> + if (!fcinfo->context || !IsA(fcinfo->context, AggState))
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("Can not call aaccum_sfunc as a non-aggregate"),
> + errhint("Use the array_accum aggregate")));

Per the error message style guidelines, errmsg() should begin with a
lowercase letter and errhint() should be a complete sentence (so it
needs punctuation, for example).

> + Datum
> + aaccum_ffunc(PG_FUNCTION_ARGS)
> + {
> + aaccum_info *ainfo;
> +
> + /* Check if we are passed in a NULL */
> + if (PG_ARGISNULL(0)) PG_RETURN_ARRAYTYPE_P(NULL);

There is no guarantee why SQL NULL and PG_RETURN_XYZ(NULL) refer to the
same thing -- use PG_RETURN_NULL() to return a SQL NULL value, or just
make the function strict.

-Neil