On 21 November 2015 at 03:54, Marko Tiikkaja <ma...@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. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers