Re: Memory error in user-defined aggregation function - Mailing list pgsql-general

Hi,

Finally got this running under the debugger and figured out what is
going on. I had been under the impression that

         if (PG_ARGISNULL(0))
                 PG_RETURN_NULL();

         state = (quartile_state *) PG_GETARG_POINTER(0);

would ensure that state was never a null pointer. However this is not
the case, and an additional check for state==0x0 solved the problem.
Somewhat unexpected, I have to say.

I would still be interested in any ways in which this implementation
could be improved. It would be good if there were some model
implementations for this type of thing - without orafce to guide me I
would have had a hard time figuring any of this out from the docs. I'd
gladly make the quartile implementation available for this purpose if
there is interest.

Adriaan


On 7 August 2012 15:04, Adriaan Joubert <adriaan.joubert@gmail.com> wrote:
> Hi,
>
> I've implemented an aggregation function to compute quartiles in C
> borrowing liberally from orafce code. I uses this code in a windowing
> context and it worked fine until today - and I'm not sure what
> changed. This is on 9.1.2 and I have also tried it on 9.1.4.
>
> What I have determined so far (by sprinkling a lot of elog's
> throughout the code) is that it does not seem to be data specific,
> although it seems to depend on the number of aggregations I do (up to
> about 1250 seems to be fine, beyond that it chokes). I also
> established that there does not seem to be a problem with the transfer
> function, and the data is accumulated without any issues. The error I
> see is in the call to first_quartile_final (listed below). The pointer
> to the transfer data structure is not null, but accessing the field
> mstate->nelems causes a segflt. So the transfer data structure pointer
> is bogus.
>
> I've recompiled postgres with debugging enabled and have connected to
> the backend with gdb, but haven't had any joy in persuading gdb to
> actually stop in the correct file so that I can step through. I'll
> keep on trying to make some headway with that.
>
> In the meantime I would appreciate any comments as to whether the
> approach taken is the right one, and whether additional checks can be
> inserted to avoid this segmentation faults.
>
> Many thanks,
>
> Adriaan
>
>
> My transfer data structure is
>
> typedef struct
> {
>   int len; /* allocated length */
>   int nextlen; /* next allocated length */
>   int nelems; /* number of valid entries */
>   float8  *values;
> } quartile_state;
>
> On the first call to the aggregate function this data structure is
> allocated as follows:
>
> static quartile_state *
> quartile_accummulate(quartile_state *mstate, float8 value,
> MemoryContext aggcontext)
> {
>         MemoryContext oldcontext;
>
>         if (mstate == NULL)
>         {
>                 /* First call - initialize */
>                 oldcontext = MemoryContextSwitchTo(aggcontext);
>                 mstate = palloc(sizeof(quartile_state));
>                 mstate->len = 512;
>                 mstate->nextlen = 2 * 512;
>                 mstate->nelems = 0;
>                 mstate->values = palloc(mstate->len * sizeof(float8));
>                 MemoryContextSwitchTo(oldcontext);
>         }
>         else
>         {
>                 if (mstate->nelems >= mstate->len)
>                 {
>                         int newlen = mstate->nextlen;
>
>                         oldcontext = MemoryContextSwitchTo(aggcontext);
>                         mstate->nextlen += mstate->len;
>                         mstate->len = newlen;
>                         mstate->values = repalloc(mstate->values, mstate->len * sizeof(float8));
>                         MemoryContextSwitchTo(oldcontext);
>                 }
>         }
>
>         mstate->values[mstate->nelems++] = value;
>
>         return mstate;
> }
>
>
> And the transfer function itself is
>
> PG_FUNCTION_INFO_V1(quartile_transfer);
> Datum
> quartile_transfer(PG_FUNCTION_ARGS) {
>         MemoryContext   aggcontext;
>         quartile_state *state = NULL;
>         float8 elem;
>
>         if (!AggCheckCallContext(fcinfo, &aggcontext))
>         {
>                 elog(ERROR, "quartile_transform called in non-aggregate context");
>         }
>
>         state = PG_ARGISNULL(0) ? NULL : (quartile_state *) PG_GETARG_POINTER(0);
>         if (PG_ARGISNULL(1))
>                 PG_RETURN_POINTER(state);
>
>         elem = PG_GETARG_FLOAT8(1);
>
>         state = quartile_accummulate(state, elem, aggcontext);
>
>         PG_RETURN_POINTER(state);
> }
>
> The final function for the computation of the first quartile is
>
> PG_FUNCTION_INFO_V1(first_quartile_final);
> Datum
> first_quartile_final(PG_FUNCTION_ARGS) {
>         quartile_state *state = NULL;
>         float8 result;
>
>         if (PG_ARGISNULL(0))
>                 PG_RETURN_NULL();
>
>         state = (quartile_state *) PG_GETARG_POINTER(0);
>
>         /** HERE state->nelems causes a segflt */
>         if (state->nelems<4)
>                 PG_RETURN_NULL();
>
>         result = quartile_result(state, 0.25);
>
>         PG_RETURN_FLOAT8(result);
> }