Re: onlyvalue aggregate (was: First Aggregate Funtion?)

On 21 November 2015 at 03:54, Marko Tiikkaja <marko(at)joh(dot)to> wrote:
> Here's v2 of the patch. How's this look?
>

Here are some initial review comments:

* My first thought on reading this patch is that it is somewhat
under-commented. For example, I would expect at least a block comment
at the top of the new code added by this patch. Also the fields of the
new structure could use some comments -- it might be obvious what
datum and isnull are for, but fcinfo is less obvious until you read
the code that uses it. Likewise the transfn is quite long, with almost
no explanatory comments.

* There's a clear bug in the null handling in the second branch of the
transfn -- when the new value is null and the previous value wasn't.
For example:

SELECT single_value(x) FROM (VALUES ('x'), (null)) t(x);
server closed the connection unexpectedly

* In the finalfn, I think calling AggCheckCallContext() should be the
first thing it does. Compare for example array_agg_array_finalfn().

* There's another less obvious bug in the way these functions handle
complex types. For example:

SELECT single_value(t) FROM (VALUES (1,'One'), (1,'One')) t(x,y);
ERROR: cache lookup failed for type 2139062143

You might want to look at how array_agg() handles that. Also the code
behind array_position() has some elements in common with this patch.

* Consider collation handling, as illustrated by array_position().

So I'm afraid there's still some work to do, but there are plenty of
examples in existing code to borrow from.

Regards,
Dean