On Mon, Nov 23, 2015 at 6:29 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On 21 November 2015 at 03:54, Marko Tiikkaja <marko@joh.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.
There is a review, but no input from the author for more than 1 month,
hence patch has been marked as "returned with feedback". Feel free to
move it to next CF if you want to post a new version.
--
Michael