From: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
---|---|
To: | Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> |
Cc: | Marko Tiikkaja <marko(at)joh(dot)to>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: onlyvalue aggregate (was: First Aggregate Funtion?) |
Date: | 2015-12-24 02:49:05 |
Message-ID: | CAB7nPqQ=xma_0vuGK5rBP0YmSGCBVK__vqQ4ZMJJdUA=q+gVYA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, Nov 23, 2015 at 6:29 PM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> 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.
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
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2015-12-24 02:50:47 | Re: Speed up Clog Access by increasing CLOG buffers |
Previous Message | Amit Langote | 2015-12-24 02:46:09 | Re: A Typo in regress/sql/privileges.sql |