* Neil Conway ([EMAIL PROTECTED]) 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

Attachment: signature.asc
Description: Digital signature

Reply via email to