Re: array_accum aggregate - Mailing list pgsql-patches

* Neil Conway (neilc@samurai.com) wrote:
> On Wed, 2006-10-11 at 00:51 -0400, Stephen Frost wrote:
> >      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.

Ok.  Either way is fine with me, honestly.

> > *** 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.

Ah, yeah, you know I noticed that when I added memutils but wasn't
thinking when I went back and added execnodes (I had been trying to
minimize the number of includes by adding ones I needed one at a time
and seeing if any more were necessary).

> > + /* 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.

Indeed. :/

> > +     /* 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).

Ah, ok, guess I should go read those.

> > + 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.

Huh, alright.  I'll probably just change it to PG_RETURN_NULL().

    Thanks!

        Stephen