Thread: Properly handling aggregate in nested function call

Properly handling aggregate in nested function call

From

Matt Magoffin

Date:

14 December 2021, 00:12:31

I am working on a C aggregate function that returns a numeric[] result. If I execute the aggregate and return the results directly in my SQL, I get the expected results. For example:

SELECT vec_agg_mean(nums) FROM measurements;

NOTICE: avg 0 = 1.23000000000000000000

NOTICE: avg 1 = 1.9700000000000000

NOTICE: avg 2 = 3.7000000000000000

vec_agg_mean

----------------------------------------------------------------

{1.23000000000000000000,1.9700000000000000,3.7000000000000000}

(1 row)

The NOTICE logs are are there to help me verify the computed result in the aggregate final function, and they essentially do

DatumGetCString(DirectFunctionCall1(numeric_out, datum))

However if I nest the aggregate inside something, such as unnest(), I get what appear to be just memory addresses:

SELECT unnest(vec_agg_mean(nums)) FROM measurements;

NOTICE: avg 0 = 1.23000000000000000000

NOTICE: avg 1 = 1.9700000000000000

NOTICE: avg 2 = 3.7000000000000000

unnest

----------------

94674302945040

94674302945052

94674302945064

(3 rows)

You can see the NOTICE logs are still the same. Passing the aggregate result to any other function seems to expose the problem, for example:

SELECT ARRAY[vec_agg_mean(nums)]::numeric[] FROM measurements;

NOTICE: avg 0 = 1.23000000000000000000

NOTICE: avg 1 = 1.9700000000000000

NOTICE: avg 2 = 3.7000000000000000

array

--------------------------------------------------

(1 row)

Any ideas what I’m doing wrong here? The source is available here:

https://github.com/SolarNetwork/aggs_for_vecs/blob/9e742cdc32a113268fd3c1f928c8ac724acec9f5/vec_agg_mean.c

Cheers,

Matt Magoffin

Re: Properly handling aggregate in nested function call

From

Tom Lane

Date:

15 December 2021, 01:51:18

Matt Magoffin <postgresql.org@msqr.us> writes:
> Any ideas what I’m doing wrong here? The source is available here:

> https://github.com/SolarNetwork/aggs_for_vecs/blob/9e742cdc32a113268fd3c1f928c8ac724acec9f5/vec_agg_mean.c

Hmm, I think you're abusing the ArrayBuildState API.  In particular,
what guarantees that the astate->dvalues and astate->dnulls arrays
are long enough for what you're stuffing into them?  You should
probably palloc temp arrays right here and then use construct_md_array
directly instead of dealing with an ArrayBuildState.

Also, I wonder what happens when state->vec_counts[i] is zero.
That's seemingly not your problem right now, since the ereport(NOTICE)
is being reached; but it sure looks like trouble waiting to happen.

            regards, tom lane

Re: Properly handling aggregate in nested function call

From

Matt Magoffin

Date:

16 December 2021, 01:14:51

On 15/12/2021, at 11:51 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Hmm, I think you're abusing the ArrayBuildState API. In particular,
what guarantees that the astate->dvalues and astate->dnulls arrays
are long enough for what you're stuffing into them?

The length is defined in the aggregate transition function (i.e. the length of the first input array) and enforced there [1] so I can stuff away here.

You should
probably palloc temp arrays right here and then use construct_md_array
directly instead of dealing with an ArrayBuildState.

OK, I can try that out. I did have another branch where I create the ArrayBuildState in this final function [2] but I still got the same result. I’ll try using construct_md_array instead as you suggest, thank you.

Also, I wonder what happens when state->vec_counts[i] is zero.
That's seemingly not your problem right now, since the ereport(NOTICE)
is being reached; but it sure looks like trouble waiting to happen.

It can happen, but the ArrayBuildState is initialised with all astate->dnulls[i] as true at the start [3], so the

if (state->vec_counts[i]) {

}

test just skips those elements and leaves them as NULL.

— m@

[1] https://github.com/SolarNetwork/aggs_for_vecs/blob/9e742cdc32a113268fd3c1f928c8ac724acec9f5/vec_stat_accum.c#L47-L49

[2] https://github.com/SolarNetwork/aggs_for_vecs/blob/ab5bea1039f865c26b899f99de866f172d125dc2/vec_agg_mean.c#L23

[3] https://github.com/SolarNetwork/aggs_for_vecs/blob/ab5bea1039f865c26b899f99de866f172d125dc2/util.c#L94-L96

Re: Properly handling aggregate in nested function call

From

Matt Magoffin

Date:

16 December 2021, 01:54:33

On 15/12/2021, at 11:51 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

You should
probably palloc temp arrays right here and then use construct_md_array
directly instead of dealing with an ArrayBuildState.

OK, I gave that a go [1] this time in a vec_agg_sum() aggregate, that operates much the same as the vec_agg_mean() one and is behaving in the same way. For example this shows the correct values:

SELECT vec_agg_sum(nums) FROM measurements;

NOTICE: sum 0 = 4.92

NOTICE: sum 1 = 5.91

NOTICE: sum 2 = 14.80

vec_agg_sum

-------------------

{4.92,5.91,14.80}

(1 row)

But adding unnest() goes back to seemingly memory address values:

SELECT unnest(vec_agg_sum(nums)) FROM measurements;

NOTICE: sum 0 = 4.92

NOTICE: sum 1 = 5.91

NOTICE: sum 2 = 14.80

unnest

----------------

94069010663032

94069010663044

94069010663056

(3 rows)

Any other ideas I could look into?

— m@

[1] https://github.com/SolarNetwork/aggs_for_vecs/blob/df8b950c73d9b32fa6f016b44dd8859e9341fe0f/vec_agg_sum.c#L50-L52

Re: Properly handling aggregate in nested function call

From

Tom Lane

Date:

16 December 2021, 04:43:21

Matt Magoffin <postgresql.org@msqr.us> writes:
> Any other ideas I could look into?

Per the old saw, when you can't see the problem, it usually means
you're looking in the wrong place.  I looked at the SQL declaration
of the function [1], and saw:

CREATE OR REPLACE FUNCTION
vec_agg_mean_finalfn(internal)
RETURNS bigint[]
AS 'aggs_for_vecs', 'vec_agg_mean_finalfn'
LANGUAGE c;

Of course what this function is actually returning is numeric[].
There is some code such as array_out that will look at the
element type OID embedded in the array value, and do the right
thing.  But other code will believe the function's declared
result type, and that sort of code will go wrong.

I see from your C code that you're hoping to make this stuff
somewhat polymorphic.  The simplest way to do that, and maybe
the best, is to make multiple SQL aliases for the same
C function, and then multiple aggregate declarations on top
of that.  Alternatively you can declare one set of polymorphic
functions and aggregates, in which case you'll need to closely
study the stuff about the finalfunc_extra option [2].

            regards, tom lane

[1]
https://github.com/SolarNetwork/aggs_for_vecs/blob/9e742cdc32a113268fd3c1f928c8ac724acec9f5/aggs_for_vecs--1.3.0.sql#L1342
[2] https://www.postgresql.org/docs/current/xaggr.html#XAGGR-POLYMORPHIC-AGGREGATES

Re: Properly handling aggregate in nested function call

From

Matt Magoffin

Date:

16 December 2021, 05:11:12

On 16/12/2021, at 2:43 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Of course what this function is actually returning is numeric[].
> There is some code such as array_out that will look at the
> element type OID embedded in the array value, and do the right
> thing.  But other code will believe the function's declared
> result type, and that sort of code will go wrong.

(Hand slap to face!) Oh my, thank you for spotting that, this has been driving me to distraction, thinking I was doing
somethingwrong with memory contexts or anything other than that copy/paste error. 

> Alternatively you can declare one set of polymorphic
> functions and aggregates, in which case you'll need to closely
> study the stuff about the finalfunc_extra option [2].

Thanks for the tips, I have been eyeing that documentation.

— m@