Re: array_accum aggregate - Mailing list pgsql-patches

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